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

[Conor Dooley <conor.dooley@microchip.com>]

[-- Attachment #1: Type: text/plain, Size: 3322 bytes --]

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.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]