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

From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
To: Naresh Solanki <naresh.solanki@9elements.com>,
	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
Subject: Re: [PATCH v2 1/2] dt-bindings: hwmon: Add max6639
Date: Wed, 3 May 2023 18:18:31 +0200	[thread overview]
Message-ID: <20eb1d0e-0aa2-9d41-7ba5-2feb148748d0@linaro.org> (raw)
In-Reply-To: <bd45ea5d-e6e4-403a-e855-376e0f647f91@9elements.com>

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?

Best regards,
Krzysztof