Re: [PATCH 1/2] dt-bindings: iio: adc: add binding for pac1921

[Matteo Martelli <matteomartelli3@gmail.com>]

Hi Conor, thanks for your feedback,

Conor Dooley wrote:
> > +
> > +  microchip,dv-gain:
> > +    description:
> > +      Digital multiplier to control the effective bus voltage gain. The gain
> > +      value of 1 is the setting for the full-scale range and it can be increased
> > +      when the system is designed for a lower VBUS range.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum: [1, 2, 4, 8, 16, 32]
> > +    default: 1
> > +
> > +  microchip,di-gain:
> 
> Why is this gain a fixed property in the devicetree, rather than
> something the user can control? Feels like it should be user
> controllable.

Gains are user controllable via the IIO_CHAN_INFO_HARDWAREGAIN. I also added
them as DT properties thinking that they could be pre-set depending on hardware
specifications: for instance by board design the monitored section is already
known to be in a particular voltage/current range (datasheet specifies
gains-ranges mapping at table 4-6 and table 4-7). Then, even if gains are
pre-set, the user can change them at runtime for instance by scaling them down
upon an overflow event. However, I can get rid of those gain properties if they
are out of the DT scope.

> > +    description:
> > +      Digital multiplier to control the effective current gain. The gain
> > +      value of 1 is the setting for the full-scale range and it can be
> > +      increased when the system is designed for a lower VSENSE range.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum: [1, 2, 4, 8, 16, 32, 64, 128]
> > +    default: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - shunt-resistor-micro-ohms
> 
> You're missing a vdd-supply btw and the !read/int pin isn't described
> here either. I think the latter needs a property to control it (probably
> a GPIO since it is intended for host control) and a default value for if
> the GPIO isn't provided?

The driver does not currently handle the vdd regulator nor the gpio for the
!read/int pin. Should they be added to the DT schema anyway?

I think I can add the vdd regulator handling with little effort, my guess is
that the "vdd-supply" property can be optional and defined as "vdd-supply: true"
in the DT schema. Then the driver, if the vdd-supply property is present in the
DT, would enable the regulator during device initialization and PM resume, and
disable it on driver removal and PM suspend.

Reguarding the !read/int pin, the current driver overrides it with a register
bit so it would not be considered at all by the device.

> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        adc@4c {
> > +            compatible = "microchip,pac1921";
> > +            #io-channel-cells = <1>;
> > +            label = "vbat";
> > +            reg = <0x4c>;
> 
> Order here should be compatible, reg, generic properties and then finally
> vendor ones.

Thanks, I will fix this in next patch version.

> 
> Thanks for your patch,
> Conor.
> 

Thanks again for you feedback,
Matteo