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

[Krzysztof Kozlowski]

On 04/08/2023 18:10, 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.
> 
> 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.

Yeah, this also would work.

Best regards,
Krzysztof