diff mbox series

[hwmon-next,v4,2/3] dt-bindings: hwmon: add Microchip EMC2305 fan controller.

Message ID 20220623165217.59252-3-michaelsh@nvidia.com (mailing list archive)
State New
Headers show
Series Add support for EMC2305 Fan Speed Controller. | expand

Commit Message

Michael Shych June 23, 2022, 4:52 p.m. UTC
From: Michael Shych <michaelsh@nvidia.com>

Add basic description of emc2305 driver device tree binding.

Signed-off-by: Michael Shych <michaelsh@nvidia.com>
Reviewed-by: Vadim Pasternak <vadimp@nvidia.com>
---
v2->v3
Changes pointed out by Rob Herring and Guenter Roeck:
- Describe separate channels of fan-controller;
- Remove pwm_max property;
- Fix compatible property.
Changes added by Michael Shych:
- Fix dt binding check warnings.
v1->v2
- Fix dt binding check errors;
- Add descriptions;
- Add missing fields;
- Change the patch subject name;
- Separate pwm-min, pwm-max per PWM channel.
---
 .../bindings/hwmon/microchip,emc2305.yaml          | 106 +++++++++++++++++++++
 1 file changed, 106 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/microchip,emc2305.yaml

Comments

Rob Herring June 30, 2022, 10:11 p.m. UTC | #1
On Thu, Jun 23, 2022 at 07:52:16PM +0300, michaelsh@nvidia.com wrote:
> From: Michael Shych <michaelsh@nvidia.com>
> 
> Add basic description of emc2305 driver device tree binding.
> 
> Signed-off-by: Michael Shych <michaelsh@nvidia.com>
> Reviewed-by: Vadim Pasternak <vadimp@nvidia.com>
> ---
> v2->v3
> Changes pointed out by Rob Herring and Guenter Roeck:
> - Describe separate channels of fan-controller;
> - Remove pwm_max property;
> - Fix compatible property.
> Changes added by Michael Shych:
> - Fix dt binding check warnings.
> v1->v2
> - Fix dt binding check errors;
> - Add descriptions;
> - Add missing fields;
> - Change the patch subject name;
> - Separate pwm-min, pwm-max per PWM channel.
> ---
>  .../bindings/hwmon/microchip,emc2305.yaml          | 106 +++++++++++++++++++++
>  1 file changed, 106 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/microchip,emc2305.yaml
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/microchip,emc2305.yaml b/Documentation/devicetree/bindings/hwmon/microchip,emc2305.yaml
> new file mode 100644
> index 000000000000..d054ba46ae23
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/microchip,emc2305.yaml
> @@ -0,0 +1,106 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +
> +$id: http://devicetree.org/schemas/hwmon/microchip,emc2305.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip EMC2305 RPM-based PWM Fan Speed Controller

RPM-based? So there is a tach signal too? Don't those need the number of 
pulses per revolution that the fan provides.

To repeat what I say for every fan controller binding now, until there's 
a common binding to describe fan controllers, fans and their 
relationship to each other, I'm not signing off on any fan binding 
doing its own thing.

Rob
Michael Shych July 18, 2022, 12:38 p.m. UTC | #2
Hi,

Sorry for long delay in getting back to you.
Please see below.

Thanks,
   Michael.

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Friday, July 1, 2022 1:12 AM
> To: Michael Shych <michaelsh@nvidia.com>
> Cc: linux@roeck-us.net; linux-hwmon@vger.kernel.org;
> devicetree@vger.kernel.org; Vadim Pasternak <vadimp@nvidia.com>
> Subject: Re: [PATCH hwmon-next v4 2/3] dt-bindings: hwmon: add Microchip
> EMC2305 fan controller.
> 
> On Thu, Jun 23, 2022 at 07:52:16PM +0300, michaelsh@nvidia.com wrote:
> > From: Michael Shych <michaelsh@nvidia.com>
> >
> > Add basic description of emc2305 driver device tree binding.
> >
> > Signed-off-by: Michael Shych <michaelsh@nvidia.com>
> > Reviewed-by: Vadim Pasternak <vadimp@nvidia.com>
> > ---
> > v2->v3
> > Changes pointed out by Rob Herring and Guenter Roeck:
> > - Describe separate channels of fan-controller;
> > - Remove pwm_max property;
> > - Fix compatible property.
> > Changes added by Michael Shych:
> > - Fix dt binding check warnings.
> > v1->v2
> > - Fix dt binding check errors;
> > - Add descriptions;
> > - Add missing fields;
> > - Change the patch subject name;
> > - Separate pwm-min, pwm-max per PWM channel.
> > ---
> >  .../bindings/hwmon/microchip,emc2305.yaml          | 106
> +++++++++++++++++++++
> >  1 file changed, 106 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/hwmon/microchip,emc2305.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/hwmon/microchip,emc2305.yaml
> > b/Documentation/devicetree/bindings/hwmon/microchip,emc2305.yaml
> > new file mode 100644
> > index 000000000000..d054ba46ae23
> > --- /dev/null
> > +++
> b/Documentation/devicetree/bindings/hwmon/microchip,emc2305.yaml
> > @@ -0,0 +1,106 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
> > +---
> > +
> > +$id: http://devicetree.org/schemas/hwmon/microchip,emc2305.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Microchip EMC2305 RPM-based PWM Fan Speed Controller
> 
> RPM-based? So there is a tach signal too? Don't those need the number of
> pulses per revolution that the fan provides.
> 
It's not relevant. The driver implements Direct setting mode according 
to the Datasheet: https://www.microchip.com/en-us/product/EMC2305
I can add this note to the documentation patch.

> To repeat what I say for every fan controller binding now, until there's a
> common binding to describe fan controllers, fans and their relationship to
> each other, I'm not signing off on any fan binding doing its own thing.
> 
I'm confused here as I thought that I already changed to common fan-controller with advice of 
Gunter in patch series V3.
Do you mean that we should use some common FAN/PWM/ TACHO etc. generic binding
mechanism that fits all drivers?
Could you advise if it's already existed and reference to example?
If it doesn't exist, it'll be too complicated to provide such a new generic description within 
the submission of a driver for EMC2305 device.
We can just completely remove OF interface and pass the necessary configuration through
the platform data.

> Rob
Rob Herring Aug. 4, 2022, 5:45 p.m. UTC | #3
On Mon, Jul 18, 2022 at 12:38:58PM +0000, Michael Shych wrote:
> Hi,
> 
> Sorry for long delay in getting back to you.
> Please see below.
> 
> Thanks,
>    Michael.
> 
> > -----Original Message-----
> > From: Rob Herring <robh@kernel.org>
> > Sent: Friday, July 1, 2022 1:12 AM
> > To: Michael Shych <michaelsh@nvidia.com>
> > Cc: linux@roeck-us.net; linux-hwmon@vger.kernel.org;
> > devicetree@vger.kernel.org; Vadim Pasternak <vadimp@nvidia.com>
> > Subject: Re: [PATCH hwmon-next v4 2/3] dt-bindings: hwmon: add Microchip
> > EMC2305 fan controller.
> > 
> > On Thu, Jun 23, 2022 at 07:52:16PM +0300, michaelsh@nvidia.com wrote:
> > > From: Michael Shych <michaelsh@nvidia.com>
> > >
> > > Add basic description of emc2305 driver device tree binding.
> > >
> > > Signed-off-by: Michael Shych <michaelsh@nvidia.com>
> > > Reviewed-by: Vadim Pasternak <vadimp@nvidia.com>
> > > ---
> > > v2->v3
> > > Changes pointed out by Rob Herring and Guenter Roeck:
> > > - Describe separate channels of fan-controller;
> > > - Remove pwm_max property;
> > > - Fix compatible property.
> > > Changes added by Michael Shych:
> > > - Fix dt binding check warnings.
> > > v1->v2
> > > - Fix dt binding check errors;
> > > - Add descriptions;
> > > - Add missing fields;
> > > - Change the patch subject name;
> > > - Separate pwm-min, pwm-max per PWM channel.
> > > ---
> > >  .../bindings/hwmon/microchip,emc2305.yaml          | 106
> > +++++++++++++++++++++
> > >  1 file changed, 106 insertions(+)
> > >  create mode 100644
> > > Documentation/devicetree/bindings/hwmon/microchip,emc2305.yaml
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/hwmon/microchip,emc2305.yaml
> > > b/Documentation/devicetree/bindings/hwmon/microchip,emc2305.yaml
> > > new file mode 100644
> > > index 000000000000..d054ba46ae23
> > > --- /dev/null
> > > +++
> > b/Documentation/devicetree/bindings/hwmon/microchip,emc2305.yaml
> > > @@ -0,0 +1,106 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
> > > +---
> > > +
> > > +$id: http://devicetree.org/schemas/hwmon/microchip,emc2305.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Microchip EMC2305 RPM-based PWM Fan Speed Controller
> > 
> > RPM-based? So there is a tach signal too? Don't those need the number of
> > pulses per revolution that the fan provides.
> > 
> It's not relevant. The driver implements Direct setting mode according 
> to the Datasheet: https://www.microchip.com/en-us/product/EMC2305
> I can add this note to the documentation patch.

For the binding, it doesn't matter what a driver currently implements. 
That's one driver at one point in time. Bindings shouldn't be evolving 
and need to support more than 1 OS.

The binding needs to be able to describe the h/w. If there's a tach 
connection, you need to describe that and the properties of the fan's 
tach pulses. 

> 
> > To repeat what I say for every fan controller binding now, until there's a
> > common binding to describe fan controllers, fans and their relationship to
> > each other, I'm not signing off on any fan binding doing its own thing.
> > 
> I'm confused here as I thought that I already changed to common fan-controller with advice of 
> Gunter in patch series V3.
> Do you mean that we should use some common FAN/PWM/ TACHO etc. generic binding
> mechanism that fits all drivers?
> Could you advise if it's already existed and reference to example?

It doesn't exist. Probably the closest binding to something as a basis 
for something common is npcm750-pwm-fan.txt.

> If it doesn't exist, it'll be too complicated to provide such a new generic description within 
> the submission of a driver for EMC2305 device.
> We can just completely remove OF interface and pass the necessary configuration through
> the platform data.

That's your choice...

Rob
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/hwmon/microchip,emc2305.yaml b/Documentation/devicetree/bindings/hwmon/microchip,emc2305.yaml
new file mode 100644
index 000000000000..d054ba46ae23
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/microchip,emc2305.yaml
@@ -0,0 +1,106 @@ 
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+
+$id: http://devicetree.org/schemas/hwmon/microchip,emc2305.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microchip EMC2305 RPM-based PWM Fan Speed Controller
+
+maintainers:
+  - Michael Shych <michaelsh@nvidia.com>
+
+description: |
+  Microchip EMC2301/2/3/5 are RPM-based PWM Fan Controller.
+  The Fan Controller supports up to 5 independently controlled PWM fan drives.
+  Fan rotation speeds are reported in RPM.
+
+  Datasheet: https://www.microchip.com/en-us/product/EMC2305
+
+additionalProperties: false
+
+properties:
+  compatible:
+    enum:
+      - microchip,emc2305
+      - microchip,emc2303
+      - microchip,emc2302
+      - microchip,emc2301
+
+  reg:
+    maxItems: 1
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+  microchip,cooling-levels:
+    description:
+      Quantity of cooling level state.
+    $ref: /schemas/types.yaml#/definitions/uint8
+    minimum: 0
+    maximum: 255
+
+  microchip,pwm-separate:
+    description:
+      This flag indicates that separate PWM setting will be used
+      for different channels.
+    $ref: /schemas/types.yaml#/definitions/flag
+
+patternProperties:
+  "^channel@[0-4]$":
+    type: object
+
+    additionalProperties: false
+
+    properties:
+      reg:
+        items:
+          - enum:
+              - 0
+              - 1
+              - 2
+              - 3
+              - 4
+
+      pwm-min:
+        description:
+          Min pwm of emc2305 channel
+        $ref: /schemas/types.yaml#/definitions/uint8
+        minimum: 0
+        maximum: 255
+
+    required:
+      - reg
+
+required:
+  - compatible
+
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        fan-controller@4d {
+            compatible = "microchip,emc2305";
+            reg = <0x4d>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+            microchip,pwm-separate;
+            microchip,cooling-levels = /bits/ 8 <10>;
+
+            channel@0 {
+                reg = <0>;
+                pwm-min = /bits/ 8 <0>;
+            };
+            channel@1 {
+                reg = <1>;
+                pwm-min = /bits/ 8 <0>;
+            };
+        };
+    };
+