Re: [PATCH v2 1/2] dt-bindings: iio: adc: add support for PAC1711

[David Lechner]

On 8/12/26 8:34 AM, Ariana.Lazar@xxxxxxxxxxxxx wrote:
> Hi David,
> 
> Thank you for the review. Please see my comments below.
> 
>>> +
>>> +  microchip,accumulation-mode:
>>> +    $ref: /schemas/types.yaml#/definitions/string
>>> +    description: |
>>> +      The Hardware Accumulator may be used to accumulate VPOWER or
>>> VSENSE values
>>> +      for any channel. By setting the accumulator for a channel to
>>> accumulate
>>> +      the VPOWER values gives a measure of accumulated power over
>>> a time period,
>>> +      which is equivalent to energy. Setting the accumulator for a
>>> channel to
>>> +      accumulate VSENSE values gives a measure of accumulated
>>> current, which is
>>> +      equivalent to charge.
>>> +
>>> +      The Hardware Accumulator could be configured as:
>>> +       "vpower" - Accumulator accumulates VPOWER (energy)
>>> +       "vsense" - Accumulator accumulates VSENSE (Coulomb Counter)
>>> +    enum: [vpower, vsense]
>>> +    default: vpower
>>
>> Why does this one have to be a DT property? Can it not be switched
>> at runtime to accumulate one or the other at different times?
> 
> This property aims to specify what kind of hardware is intended to be
> used/available for the user.
> 
> There are two main cases here:
> - the user wants to measure also the current/power consumed before the
> driver insertion (e.g. from the boot to user control) and if this is a
> runtime setting, the hardware accumulator will be reset by the default
> configuration the driver starts with.
> - the driver does not know what type of hardware it's dealing with. In
> case the part is monitoring the charge/discharge current it does not
> make sense in user-space to change the accumulator to calculate energy.
> Same if the hardware is intended to calculate energy it does not make
> sense in user-space to change to Coulomb counter. Changing the setting
> from one mode to another will reset the hardware accumulator inside the
> chip. 

These are good reasons. I would put more of this explanation in the
binding description.

I also wonder if we should try to make the naming a bit more generic
so it can be used with similar devices, like make the enum energy
and charge.

And maybe call the property microchip,accumulator-source. (Unless
mode is describing an input pin, it sounds like configuration rather
than hardware description, which raises eyebrows, while "source" describes
how things are wired or how signals should be routed.)

> 
>>
>> Also datahseet says it can accumulate vbus measurements.
>>
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - vdd-supply
>>> +  - shunt-resistor-micro-ohms
>>> +
> 
> In the previous version of this patch series it was recommended to drop
> VBUS accumulation option from the supported functionalities because it
> has no practical usecase (other then maybe long term average) as
> Jonathan suggested in the review for version 1:
> https://lore.kernel.org/all/20251015-pac1711-v1-2-976949e36367@xxxxxxxxxxxxx/
> 
> I will readd it if it is needed.

No need. The reasoning makes sense to me.

> 
> Best regards,
> Ariana
>