Re: [PATCH v3 5/5] hwmon: Add support for Amphenol ChipCap 2

[Mark Brown]

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.

Attachment: signature.asc
Description: PGP signature