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

[Matteo Martelli <matteomartelli3@gmail.com>]

Conor Dooley wrote:
> On Thu, Jul 04, 2024 at 12:06:31PM +0200, Matteo Martelli wrote:
> > 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.
> 
> Usually gain values are left out of DT entirely, unless the gain is
> something set by the board, for example, whether or not some input pins
> are tied high or low.
> 
> > > > +    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?
> 
> Yes.
> 
> > 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.
> 
> Nah, the regulator should be marked required in the binding, since
> without power the device cannot function, right? The regulator core will
> create a dummy register if one is not provided in the device tree, so
> you don't need to add any conditional logic around regulator actions.
> 
> > 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.
> 
> We should fully describe devices, where possible, even if the driver for
> the device doesn't use it.
> 
> Cheers,
> Conor.
> 

Thanks Conor, I sent a patch v2 addressing these points.

Link to v2: https://lore.kernel.org/linux-iio/20240704-iio-pac1921-v2-0-0deb95a48409@gmail.com

Matteo