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@xxxxxxxxx Matteo
