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
