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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 4 May 2023 15:17:26 +0530
From:   Naresh Solanki <naresh.solanki@...ements.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        Jean Delvare <jdelvare@...e.com>,
        Guenter Roeck <linux@...ck-us.net>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>
Cc:     Marcello Sylvester Bauer <sylv@...v.io>,
        linux-hwmon@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/2] dt-bindings: hwmon: Add max6639

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@...ements.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?

Best Regards,
~Naresh

> 
> Best regards,
> Krzysztof
>