| 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
| ||
|
Message-ID: <530ef711-a32a-fbf7-7bd0-2ddb4cb070f3@linaro.org>
Date: Thu, 4 May 2023 13:44:47 +0200
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Naresh Solanki <naresh.solanki@...ements.com>,
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
On 04/05/2023 13:36, Naresh Solanki wrote:
> 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@...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?
>>
>> 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.
I don't care about driver and did not comment about it. I don't
understand why do you bring it here all'n over.
Best regards,
Krzysztof
Powered by blists - more mailing lists