| Message ID | 20231020-topic-chipcap2-v3-0-5b3bb50a5f0b@gmail.com |
|---|---|
| Headers | show |
| Series | hwmon: Add support for Amphenol ChipCap 2 | expand |
On Thu, Dec 07, 2023 at 08:44:55PM +0100, Javier Carrasco wrote: > + if (regulator_is_enabled(data->regulator)) { > + ret = regulator_disable(data->regulator); > + if (ret < 0) > + return ret; > + > + msleep(CC2_POWER_CYCLE_MS); /* ensure a clean power cycle */ > + } > + > + ret = regulator_enable(data->regulator); > + if (ret < 0) > + return ret; This is very buggy. A consumer should only disable a regulator if it itself enabled that regulator (or it *requires* an exclusive regulator which isn't a good fit here), and there's no guarantee that disabling a regulator will actually result in a power off. Either the board might not physically or through constraints permit the state to change or another user may have enabled the regulator. The driver needs to keep track of if it enabled the regulator and only disable it as many times as it enabled it. For this usage with trying to bounce the power of the regulator you can keep track of the actual power state of the supply by listening to notifications, and should possibly just keep the regulator disabled when it's not actively in use (if no alarm is active or measurement in progress?). > + data->regulator = devm_regulator_get_optional(dev, "vdd"); Does the device *really* work without power? The datasheet appears to suggest that the device has a non-optional supply Vdd https://f.hubspotusercontent40.net/hubfs/9035299/Documents/AAS-916-127J-Telaire-ChipCap2-022118-web.pdf (there's also a Vcore pin but that appears to be for connecting a decoupling capacitor rather than a supply). In general _optional() should only be used for supplies which may be physically absent.
On 07.12.23 21:05, Mark Brown wrote: > On Thu, Dec 07, 2023 at 08:44:55PM +0100, Javier Carrasco wrote: > >> + if (regulator_is_enabled(data->regulator)) { >> + ret = regulator_disable(data->regulator); >> + if (ret < 0) >> + return ret; >> + >> + msleep(CC2_POWER_CYCLE_MS); /* ensure a clean power cycle */ >> + } >> + >> + ret = regulator_enable(data->regulator); >> + if (ret < 0) >> + return ret; > > This is very buggy. A consumer should only disable a regulator if it > itself enabled that regulator (or it *requires* an exclusive regulator > which isn't a good fit here), and there's no guarantee that disabling a > regulator will actually result in a power off. Either the board might > not physically or through constraints permit the state to change or > another user may have enabled the regulator. The driver needs to keep > track of if it enabled the regulator and only disable it as many times > as it enabled it. The idea is actually that if alarms are required, an exclusive regulator will be necessary to trigger power cycles and enter the command mode. In summary there would be two options: either a regulator is defined and can be controlled to trigger the command mode or no regulator was defined for this device and therefore no command mode is available i.e. interrupts cannot be configured. That would be the case for example when the supply is always on. > > For this usage with trying to bounce the power of the regulator you can > keep track of the actual power state of the supply by listening to > notifications, and should possibly just keep the regulator disabled when > it's not actively in use (if no alarm is active or measurement in > progress?). > >> + data->regulator = devm_regulator_get_optional(dev, "vdd"); > > Does the device *really* work without power? The datasheet appears to > suggest that the device has a non-optional supply Vdd > > https://f.hubspotusercontent40.net/hubfs/9035299/Documents/AAS-916-127J-Telaire-ChipCap2-022118-web.pdf > The vdd is a non-optional supply and the device cannot run without it. What is optional is the controllable regulator to enter the command window. Hence why I used _optional() functions. I could ban any use without a controllable regulator to make things simpler, but if no alarms are required, the device should work fine without command mode. Either way the driver must have some control over a regulator for the mentioned command window. > (there's also a Vcore pin but that appears to be for connecting a > decoupling capacitor rather than a supply). > > In general _optional() should only be used for supplies which may be > physically absentIn the end I need a way to control a regulator to enter in command mode, but still offering measurement functionality if there is no regulator to control. The current approach is that there must be a supply, but the regulator might be controllable or not. Thank you for your feedback and best regards, Javier Carrasco
On 07.12.23 21:44, Mark Brown wrote: > There is a specific API for exclusive regulators which the driver is not > using, and it's unconditionally doing the disable/enable cycle here. > That is right, I will call regulator_get_exclusive() instead. > The driver needs to be explicitly configured for this and have separate > code paths for normal operation and operation where the supply can be > bounced like this. In neither code path should the supply be optional. > Right now we don't have a mechanism for discovering optionally exclusive > and enable/disablable supplies which is what the device needs, we could > potentially add that since this does seem like a viable use case and we > already have enough information in the DT to say if the supply matches > the constraints. Probably the two properties queryable separately. If > that API were added then the driver would do a normal regulator_get() > then check if it has the capabilities it needs and either keep the > supply on all the time (or possibly just during measurements?) or enable > the alarm functionality. In that case I will split the driver development into two steps. First I will stick to the existing API and implement only the code path where an exclusive regulator is required i.e. not optional, which will simplify the review process considerably. When the driver makes it through and all other issues are also solved, I will work on the optional exclusive regulator. This is probably an edge case and it will increase complexity to actually use half of the device capabilities to save the exclusive regulator, but at some point I would like to offer that as well. Thank you again and best regards, Javier Carrasco
This series adds support and documentation for the Amphenol ChipCap 2 humidity and temperature sensor in its digital version. This I2C device provides 14-bit humidity and temperature measurements as well as low (minimum) and high (maximum) humidity alarms. A ready signal is also available to reduce delays while fetching data. The proposed driver implements the logic to perform measurements with and without the ready signal, EEPROM configuration and alarm signaling. The features this driver does not support (I2C address and command window length modification) have been documented in the "Known Issues" section. The complete supported functionality has been tested with a CC2D33S sensor (a 'sleep' device) connected to a Raspberry Pi Zero 2 w. Different device tree node definitions (with and without regulator, ready and/or alarm signals) have been positively tested. The non-sleep measurement mechanism has been inferred from the first measurement, which is carried out automatically and it is common for all part numbers. Any testing or improvements with a non-sleep device is more than welcome. The tests have also covered the properties added to the hwmon core to account for minimum and maximum humidity alarms. Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> --- Changes in v3: - ABI: sysfs-class-hwmon: documented humidity min/max alarms. - General: reorder patches (bindings first to remove checkpatch warnings). - General: remove part number wildcards and use real part numbers. - chipcap2.c: improve error path in probe function. - chipcap2.c: fix error handling if regulator could not be registered. - chipcap2.c: use absolute values for hysteresis (for both ABI compatibility and simplicity). - chipcap2.c: minor code-style fixes and variable renaming. - Link to v2: https://lore.kernel.org/r/20231020-topic-chipcap2-v2-0-f5c325966fdb@gmail.com Changes in v2: - vendor-prefixes: full company name in the vendor description (Krzystof Kozlowski) - chipcap2.c: proper i2c_device_id table, coding style fixes, cleaner error path in the probe function (Krzystof Kozlowski) - dt-bindings: per-item description and lowercase names (Krzystof Kozlowski) - MAINTAINERS: fix manufacturer name (Krzystof Kozlowski) - Link to v1: https://lore.kernel.org/r/20231020-topic-chipcap2-v1-0-087e21d4b1ed@gmail.com --- Javier Carrasco (5): dt-bindings: vendor-prefixes: add Amphenol hwmon: (core) Add support for humidity min/max alarm ABI: sysfs-class-hwmon: add descriptions for humidity min/max alarms dt-bindings: hwmon: Add Amphenol ChipCap 2 hwmon: Add support for Amphenol ChipCap 2 Documentation/ABI/testing/sysfs-class-hwmon | 18 + .../bindings/hwmon/amphenol,chipcap2.yaml | 74 ++ .../devicetree/bindings/vendor-prefixes.yaml | 2 + Documentation/hwmon/chipcap2.rst | 73 ++ Documentation/hwmon/index.rst | 1 + MAINTAINERS | 8 + drivers/hwmon/Kconfig | 10 + drivers/hwmon/Makefile | 1 + drivers/hwmon/chipcap2.c | 1040 ++++++++++++++++++++ drivers/hwmon/hwmon.c | 2 + include/linux/hwmon.h | 4 + 11 files changed, 1233 insertions(+) --- base-commit: 33cc938e65a98f1d29d0a18403dbbee050dcad9a change-id: 20231020-topic-chipcap2-e2d8985430c2 Best regards,