Re: [PATCH v3 1/2] dt-bindings: hwmon: Add MAX6639

[Naresh Solanki]

Hi Rob,


On Fri, 11 Aug 2023 at 04:41, Rob Herring <robh@xxxxxxxxxx> wrote:
>
> On Fri, Aug 04, 2023 at 09:10:37AM -0700, Guenter Roeck wrote:
> > On 8/4/23 08:48, Conor Dooley wrote:
> > > On Thu, Aug 03, 2023 at 04:43:59PM +0200, Naresh Solanki wrote:
> > > > From: Marcello Sylvester Bauer <sylv@xxxxxxx>
> > > >
> > > > Add binding documentation for Maxim MAX6639 fan-speed controller.
> > > >
> > > > Signed-off-by: Marcello Sylvester Bauer <sylv@xxxxxxx>
> > > > Signed-off-by: Naresh Solanki <Naresh.Solanki@xxxxxxxxxxxxx>
> > > > ---
> > > > Changes in V3:
> > > > - Update title
> > > > - Add pulses-per-revolution, supplies & interrupts
> > > > Changes in V2:
> > > > - Update subject
> > > > - Drop blank lines
> > > > ---
> > > >   .../bindings/hwmon/maxim,max6639.yaml         | 60 +++++++++++++++++++
> > > >   1 file changed, 60 insertions(+)
> > > >   create mode 100644 Documentation/devicetree/bindings/hwmon/maxim,max6639.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/hwmon/maxim,max6639.yaml b/Documentation/devicetree/bindings/hwmon/maxim,max6639.yaml
> > > > new file mode 100644
> > > > index 000000000000..b3292061ca58
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/hwmon/maxim,max6639.yaml
> > > > @@ -0,0 +1,60 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/hwmon/maxim,max6639.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Maxim MAX6639 Fan Controller
> > > > +
> > > > +maintainers:
> > > > +  - Naresh Solanki <Naresh.Solanki@xxxxxxxxxxxxx>
> > > > +
> > > > +description: |
> > > > +  The MAX6639 is a 2-channel temperature monitor with dual, automatic, PWM
> > > > +  fan-speed controller.  It monitors its own temperature and one external
> > > > +  diode-connected transistor or the temperatures of two external diode-connected
> > > > +  transistors, typically available in CPUs, FPGAs, or GPUs.
> > >
> > > > +  fan-supply:
> > > > +    description: Phandle to the regulator that provides power to the fan.
> > >
> > > > +  pulses-per-revolution:
> > > > +    description:
> > > > +      Define the number of pulses per fan revolution for each tachometer
> > > > +      input as an integer.
> > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > +    enum: [1, 2, 3, 4]
> > > > +    default: 2
> > >
> > > Apologies if I am digging up old wounds here, since there was quite a
> > > bit of back and forth on the last version, but these two newly added
> > > properties look to be common with the "pwm-fan" and with
> > > "adi,axi-fan-control". At what point should these live in a common
> > > schema instead?
> > >
> > > Otherwise, this looks okay to me, although I'll leave things to
> > > Krzysztof since he had a lot to say about the previous version.
> > >
> >
> > Rob has said that he won't accept any fan controller bindings without a generic
> > schema. At the same time he has said that he expects properties such as the
> > number of pulses per revolution to be attached to a 'fan' description, and he
> > wants pwm related properties of fan controllers to be modeled as pwm controllers.
> > And now we have a notion of a regulator providing power to the fan (which again
> > would be the fan controller, at least in cases where the fan controller
> > provides direct voltage to the fan). On top of that, this fan-supply property
> > should presumably, again, be part of a fan description and not be part of the
> > controller description. I don't think anyone knows how to make this all work
> > (I for sure don't), so it is very unlikely we'll see a generic fan controller
> > schema anytime soon.
>
> I thought what was done earlier in this series was somewhat close. And
> there are some bindings that already look pretty close to what a common
> binding should. But it seems no one wants to worry about more than their
> 1 device.

The DT binding for common fan properties is:
https://lore.kernel.org/lkml/20221130144626.GA2647609@xxxxxxxxxxxx/t/#m15ce07c3c43c46506acc389bf24d616646e05653

I wasn't sure on how to address properties for DC controlled fans.

Regards,
Naresh
>
> In case it's not clear, as-is, this binding is a NAK for me.
>
> > Given that neither fan-supply nor pulses-per-revolution is implemented in the
> > driver, and given that I am not aware of any fans which would have a value for
> > pulses-per-revolution other than 2, my personal suggestion would be to add the
> > chip to trivial devices and be done with it for the time being.
>
> I'm fine with that too. Just keep kicking that can...
>
> Rob