2023-10-31 17:07:57

by Conor Dooley

[permalink] [raw]
Subject: Re: [PATCH v3 1/2] dt-bindings: hwmon: Add lltc ltc4286 driver bindings

On Tue, Oct 31, 2023 at 03:21:21PM +0800, Delphine CC Chiu wrote:
> Add a device tree bindings for ltc4286 device.
>
> Signed-off-by: Delphine CC Chiu <[email protected]>
>
> Changelog:
> v3 - Revise adi,vrange-select-25p6 to adi,vrange-low-enable
> v2 - Revise vrange_select_25p6 to adi,vrange-select-25p6
> - Add type for adi,vrange-select-25p6
> - Revise rsense-micro-ohms to shunt-resistor-micro-ohms
> ---
> .../bindings/hwmon/lltc,ltc4286.yaml | 52 +++++++++++++++++++
> MAINTAINERS | 10 ++++
> 2 files changed, 62 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
>
> diff --git a/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml b/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
> new file mode 100644
> index 000000000000..4695bca77c05
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
> @@ -0,0 +1,52 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/lltc,ltc4286.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: LTC4286 power monitors
> +
> +maintainers:
> + - Delphine CC Chiu <[email protected]>
> +
> +properties:
> + compatible:
> + enum:
> + - lltc,ltc4286
> + - lltc,ltc4287
> +
> + reg:
> + maxItems: 1
> +
> + adi,vrange-low-enable:
> + description:
> + This property is a bool parameter to represent the
> + voltage range is 25.6 volts or 102.4 volts for
> + this chip.
> + The default is 102.4 volts.

You've got weird wrapping of text here (short lines). Either this
property or the corollary work for me, depending on what Guenter
wants. With two nits below,
Reviewed-by: Conor Dooley <[email protected]>


> + type: boolean
> +
> + shunt-resistor-micro-ohms:
> + description:
> + Resistor value in micro-Ohm

micro-ohms

> +
> +required:
> + - compatible
> + - reg
> + - shunt-resistor-micro-ohms
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + power-sensor@40 {

the generic node name is "power-monitor".

Cheers,
Conor.


Attachments:
(No filename) (2.33 kB)
signature.asc (235.00 B)
Download all attachments

2023-10-31 17:11:52

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH v3 1/2] dt-bindings: hwmon: Add lltc ltc4286 driver bindings

On 10/31/23 10:06, Conor Dooley wrote:
> On Tue, Oct 31, 2023 at 03:21:21PM +0800, Delphine CC Chiu wrote:
>> Add a device tree bindings for ltc4286 device.
>>
>> Signed-off-by: Delphine CC Chiu <[email protected]>
>>
>> Changelog:
>> v3 - Revise adi,vrange-select-25p6 to adi,vrange-low-enable
>> v2 - Revise vrange_select_25p6 to adi,vrange-select-25p6
>> - Add type for adi,vrange-select-25p6
>> - Revise rsense-micro-ohms to shunt-resistor-micro-ohms
>> ---
>> .../bindings/hwmon/lltc,ltc4286.yaml | 52 +++++++++++++++++++
>> MAINTAINERS | 10 ++++
>> 2 files changed, 62 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml b/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
>> new file mode 100644
>> index 000000000000..4695bca77c05
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
>> @@ -0,0 +1,52 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/hwmon/lltc,ltc4286.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: LTC4286 power monitors
>> +
>> +maintainers:
>> + - Delphine CC Chiu <[email protected]>
>> +
>> +properties:
>> + compatible:
>> + enum:
>> + - lltc,ltc4286
>> + - lltc,ltc4287
>> +
>> + reg:
>> + maxItems: 1
>> +
>> + adi,vrange-low-enable:
>> + description:
>> + This property is a bool parameter to represent the
>> + voltage range is 25.6 volts or 102.4 volts for
>> + this chip.
>> + The default is 102.4 volts.
>
> You've got weird wrapping of text here (short lines). Either this
> property or the corollary work for me, depending on what Guenter
> wants. With two nits below,
> Reviewed-by: Conor Dooley <[email protected]>
>

Oh, I am tired of arguing, so I'll accept whatever gets a Reviewed-by:
tag from a DT maintainer (people do a pretty good job of wearing me down
lately).

Guenter

Subject: RE: [PATCH v3 1/2] dt-bindings: hwmon: Add lltc ltc4286 driver bindings

> -----Original Message-----
> From: Conor Dooley <[email protected]>
> Sent: Wednesday, November 1, 2023 1:06 AM
> To: Delphine_CC_Chiu/WYHQ/Wiwynn <[email protected]>
> Cc: [email protected]; Jean Delvare <[email protected]>; Guenter Roeck
> <[email protected]>; Rob Herring <[email protected]>; Krzysztof Kozlowski
> <[email protected]>; Conor Dooley <[email protected]>;
> Jonathan Corbet <[email protected]>; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH v3 1/2] dt-bindings: hwmon: Add lltc ltc4286 driver
> bindings
>
> On Tue, Oct 31, 2023 at 03:21:21PM +0800, Delphine CC Chiu wrote:
> > Add a device tree bindings for ltc4286 device.
> >
> > Signed-off-by: Delphine CC Chiu <[email protected]>
> >
> > Changelog:
> > v3 - Revise adi,vrange-select-25p6 to adi,vrange-low-enable
> > v2 - Revise vrange_select_25p6 to adi,vrange-select-25p6
> > - Add type for adi,vrange-select-25p6
> > - Revise rsense-micro-ohms to shunt-resistor-micro-ohms
> > ---
> > .../bindings/hwmon/lltc,ltc4286.yaml | 52
> +++++++++++++++++++
> > MAINTAINERS | 10 ++++
> > 2 files changed, 62 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
> > b/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
> > new file mode 100644
> > index 000000000000..4695bca77c05
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
> > @@ -0,0 +1,52 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/hwmon/lltc,ltc4286.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: LTC4286 power monitors
> > +
> > +maintainers:
> > + - Delphine CC Chiu <[email protected]>
> > +
> > +properties:
> > + compatible:
> > + enum:
> > + - lltc,ltc4286
> > + - lltc,ltc4287
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + adi,vrange-low-enable:
> > + description:
> > + This property is a bool parameter to represent the
> > + voltage range is 25.6 volts or 102.4 volts for
> > + this chip.
> > + The default is 102.4 volts.
>
> You've got weird wrapping of text here (short lines). Either this property or the
> corollary work for me, depending on what Guenter wants. With two nits
> below,
> Reviewed-by: Conor Dooley <[email protected]>

We will revise to
adi,vrange-low-enable:
description:
This property is a bool parameter to represent the
voltage range is 25.6 volts or 102.4 volts for this chip.

>
>
> > + type: boolean
> > +
> > + shunt-resistor-micro-ohms:
> > + description:
> > + Resistor value in micro-Ohm
>
> micro-ohms

We will revise to micro-ohms

>
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - shunt-resistor-micro-ohms
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > + i2c {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + power-sensor@40 {
>
> the generic node name is "power-monitor".


We will revise to power-monitor@40

>
> Cheers,
> Conor.