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;
Message-ID: <45236017-22d2-f9f8-0069-77195e49221d@9elements.com>
Date:   Thu, 4 May 2023 17:06:48 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.1
Subject: Re: [PATCH v2 1/2] dt-bindings: hwmon: Add max6639
Content-Language: en-US
To:     Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
        Jean Delvare <jdelvare@suse.com>,
        Guenter Roeck <linux@roeck-us.net>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
Cc:     Marcello Sylvester Bauer <sylv@sylv.io>,
        linux-hwmon@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20230420111759.2687001-1-Naresh.Solanki@9elements.com>
 <76e57634-75dd-01e8-9c56-36ed7de17812@linaro.org>
 <c8d1b5db-318e-3401-0834-b89769831eca@9elements.com>
 <be129c4f-3ad7-c54b-936e-08b142608ebc@linaro.org>
 <88f9a008-2861-284c-76c4-7d416c107fbb@9elements.com>
 <bd45ea5d-e6e4-403a-e855-376e0f647f91@9elements.com>
 <20eb1d0e-0aa2-9d41-7ba5-2feb148748d0@linaro.org>
 <9d989c4c-7c9e-9e95-133f-03741d07198b@9elements.com>
 <80b60de0-dcb5-303f-8d13-f4b1cf7d8521@linaro.org>
From:   Naresh Solanki <naresh.solanki@9elements.com>
In-Reply-To: <80b60de0-dcb5-303f-8d13-f4b1cf7d8521@linaro.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi Krzysztof,

On 04-05-2023 04:17 pm, Krzysztof Kozlowski wrote:
> On 04/05/2023 11:47, Naresh Solanki wrote:
>> Hi Krzysztof,
>>
>> On 03-05-2023 09:48 pm, Krzysztof Kozlowski wrote:
>>> On 03/05/2023 10:26, Naresh Solanki wrote:
>>>> Hi Krzysztof,
>>>>
>>>> On 24-04-2023 03:23 pm, Naresh Solanki wrote:
>>>>> Hi Krzysztof,
>>>>>
>>>>> On 24-04-2023 03:12 pm, Krzysztof Kozlowski wrote:
>>>>>> On 24/04/2023 11:18, Naresh Solanki wrote:
>>>>>>
>>>>>>>>> Changes in V2:
>>>>>>>>> - Update subject
>>>>>>>>> - Drop blank lines
>>>>>>>>> ---
>>>>>>>>>      .../bindings/hwmon/maxim,max6639.yaml         | 52
>>>>>>>>> +++++++++++++++++++
>>>>>>>>>      1 file changed, 52 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..1aaedfd7cee0
>>>>>>>>> --- /dev/null
>>>>>>>>> +++ b/Documentation/devicetree/bindings/hwmon/maxim,max6639.yaml
>>>>>>>>> @@ -0,0 +1,52 @@
>>>>>>>>> +# 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
>>>>>>>>
>>>>>>>> What is this device? fan controller?
>>>>>>> Yes Fan controller.
>>>>>>> Do you want me to update the title here as:
>>>>>>> "Maxim MAC6639 2 channel fan controller & temperature monitor" ?
>>>>>>
>>>>>> Enough would be:
>>>>>> Maxim MAX6639 Fan Controller
>>>>> Ack
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>> +
>>>>>>>>> +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.
>>>>>>>>> +
>>>>>>>>> +  Datasheets:
>>>>>>>>> +    https://datasheets.maximintegrated.com/en/ds/MAX6639-MAX6639F.pdf
>>>>>>>>> +
>>>>>>>>> +properties:
>>>>>>>>> +  compatible:
>>>>>>>>> +    enum:
>>>>>>>>> +      - maxim,max6639
>>>>>>>>> +
>>>>>>>>> +  reg:
>>>>>>>>> +    maxItems: 1
>>>>>>>>> +
>>>>>>>>> +  '#address-cells':
>>>>>>>>> +    const: 1
>>>>>>>>> +
>>>>>>>>> +  '#size-cells':
>>>>>>>>> +    const: 0
>>>>>>>>
>>>>>>>> Why do you need these two properties?
>>>>>>> Ack. Will remove them.
>>>>>>>>
>>>>>>>> Anyway, the binding looks incomplete. Where are the supplies?
>>>>>>>> Interrupts?
>>>>>>> This patch just adds basic support to the existing platform driver.
>>>>>>> Intention is to be able to call the driver from DT with basic
>>>>>>> initialization from driver the existing driver.
>>>>>>
>>>>>> Bindings should be rather complete. Here the datasheet is accessible and
>>>>>> few properties quite obvious, so I don't see a reason to skip them.
>>>>> I agree with you. But currently the driver which is already merged
>>>>> doesn't has it implemented.
>>>>> And will be working on separate patch to include this later.
>>>> Please let me know if this approach is acceptable, or if there are any
>>>> other suggestions or concerns that you have.
>>>
>>> You are adding new bindings, so what does the driver has to do with it?
>> The reason for adding these new bindings is to enable the use of the
>> driver on my machine. Without the compatible string, it would not be
>> possible to use the driver.
>>
>> Currently, the driver initializes the device with defaults, which is
>> good enough for my application. Also, as you previously pointed out, it
>> uses the optional 'fan-supply' which will be included in the next patch
>> revision.
>>
>> I hope this clarifies my reasoning. Could you kindly confirm if we can
>> proceed with this approach?
> 
> No, we cannot, because we asked you to fix things there. Your entire
> explanation about compatible and driver is not related to the comment
> you received: bindings should be complete. You argue that bindings do
> not have to be complete, because of something with driver. This is not
> related. Bindings are not for driver.

I understand that complete bindings are important, but as the driver is 
already merged and functional, my immediate goal is to enable its use on 
my machine. I will work on a separate patch to include the interrupts in 
both binding & driver.

> 
> Best regards,
> Krzysztof
>