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

[Rob Herring <robh@kernel.org>]

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