Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Sender: Guenter Roeck <groeck7@gmail.com>
Message-ID: <9dfe8bbb-54df-7746-3267-5f7ea8893bef@roeck-us.net>
Date:   Thu, 10 Aug 2023 16:54:15 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH v3 1/2] dt-bindings: hwmon: Add MAX6639
Content-Language: en-US
To:     Rob Herring <robh@kernel.org>
Cc:     Conor Dooley <conor@kernel.org>,
        Naresh Solanki <naresh.solanki@9elements.com>,
        krzysztof.kozlowski+dt@linaro.org,
        Jean Delvare <jdelvare@suse.com>,
        Conor Dooley <conor+dt@kernel.org>,
        Marcello Sylvester Bauer <sylv@sylv.io>,
        linux-hwmon@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20230803144401.1151065-1-Naresh.Solanki@9elements.com>
 <20230804-tamper-numbness-0117bb53a921@spud>
 <3f947ad6-eed6-59ff-e4e6-8b21d90eb803@roeck-us.net>
 <20230810231142.GA1506309-robh@kernel.org>
From:   Guenter Roeck <linux@roeck-us.net>
In-Reply-To: <20230810231142.GA1506309-robh@kernel.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 8/10/23 16:11, Rob Herring 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@sylv.io>
>>>>
>>>> Add binding documentation for Maxim MAX6639 fan-speed controller.
>>>>
>>>> Signed-off-by: Marcello Sylvester Bauer <sylv@sylv.io>
>>>> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
>>>> ---
>>>> 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@9elements.com>
>>>> +
>>>> +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.
> 
> In case it's not clear, as-is, this binding is a NAK for me.
> 

Ok, I'll drop it.

Guenter

>> 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