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
