diff mbox series

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

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

Commit Message

Matteo Martelli July 3, 2024, 1:34 p.m. UTC
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(+)

Comments

Conor Dooley July 3, 2024, 3:10 p.m. UTC | #1
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
>
Matteo Martelli July 4, 2024, 10:06 a.m. UTC | #2
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
Conor Dooley July 4, 2024, 10:37 a.m. UTC | #3
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.
Matteo Martelli July 4, 2024, 5:50 p.m. UTC | #4
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 mbox series

Patch

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>;
+        };
+    };
+...