Re: [PATCH v2 1/2] dt-bindings: hwmon: Add max6639

[Naresh Solanki <naresh.solanki@9elements.com>]

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
>