| Message ID | 20240703-iio-pac1921-v1-1-54c47d9180b6@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: adc: add support for pac1921 | expand |
Hey, On Wed, Jul 03, 2024 at 03:34:35PM +0200, Matteo Martelli wrote: > Add binging for Microchip PAC1921 Power/Current monitor > > Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com> > --- > .../bindings/iio/adc/microchip,pac1921.yaml | 79 ++++++++++++++++++++++ > 1 file changed, 79 insertions(+) > > diff --git a/Documentation/devicetree/bindings/iio/adc/microchip,pac1921.yaml b/Documentation/devicetree/bindings/iio/adc/microchip,pac1921.yaml > new file mode 100644 > index 000000000000..ec08228dcc16 > --- /dev/null > +++ b/Documentation/devicetree/bindings/iio/adc/microchip,pac1921.yaml > @@ -0,0 +1,79 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/iio/adc/microchip,pac1921.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Microchip PAC1921 High-Side Power/Current Monitor with Anaog Output > + > +maintainers: > + - Matteo Martelli <matteomartelli3@gmail.com> > + > +description: | > + The PAC1921 is a power/current monitoring device with an analog output > + and I2C/SMBus interface. > + > + Datasheet can be found here: > + https://ww1.microchip.com/downloads/en/DeviceDoc/PAC1921-Data-Sheet-DS20005293E.pdf > + > +properties: > + compatible: > + const: microchip,pac1921 > + > + reg: > + maxItems: 1 > + > + "#io-channel-cells": > + const: 1 > + > + shunt-resistor-micro-ohms: > + description: > + Value in micro Ohms of the shunt resistor connected between > + the SENSE+ and SENSE- inputs, across which the current is measured. > + Value is needed to compute the scaling of the measured current. > + > + label: > + description: Unique name to identify which device this is. > + > + 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. > + 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? > + > +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 for your patch, Conor. > + shunt-resistor-micro-ohms = <10000>; > + microchip,dv-gain = <4>; > + microchip,di-gain = <32>; > + }; > + }; > +... > > -- > 2.45.1 >
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
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.
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
diff --git a/Documentation/devicetree/bindings/iio/adc/microchip,pac1921.yaml b/Documentation/devicetree/bindings/iio/adc/microchip,pac1921.yaml new file mode 100644 index 000000000000..ec08228dcc16 --- /dev/null +++ b/Documentation/devicetree/bindings/iio/adc/microchip,pac1921.yaml @@ -0,0 +1,79 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/adc/microchip,pac1921.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Microchip PAC1921 High-Side Power/Current Monitor with Anaog Output + +maintainers: + - Matteo Martelli <matteomartelli3@gmail.com> + +description: | + The PAC1921 is a power/current monitoring device with an analog output + and I2C/SMBus interface. + + Datasheet can be found here: + https://ww1.microchip.com/downloads/en/DeviceDoc/PAC1921-Data-Sheet-DS20005293E.pdf + +properties: + compatible: + const: microchip,pac1921 + + reg: + maxItems: 1 + + "#io-channel-cells": + const: 1 + + shunt-resistor-micro-ohms: + description: + Value in micro Ohms of the shunt resistor connected between + the SENSE+ and SENSE- inputs, across which the current is measured. + Value is needed to compute the scaling of the measured current. + + label: + description: Unique name to identify which device this is. + + 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: + 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 + +additionalProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + adc@4c { + compatible = "microchip,pac1921"; + #io-channel-cells = <1>; + label = "vbat"; + reg = <0x4c>; + shunt-resistor-micro-ohms = <10000>; + microchip,dv-gain = <4>; + microchip,di-gain = <32>; + }; + }; +...
Add binging for Microchip PAC1921 Power/Current monitor Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com> --- .../bindings/iio/adc/microchip,pac1921.yaml | 79 ++++++++++++++++++++++ 1 file changed, 79 insertions(+)