linux-hwmon.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 1/2] dt-bindings: hwmon: Add max6639
@ 2023-04-20 11:17 Naresh Solanki
  2023-04-20 11:17 ` [PATCH v2 2/2] hwmon: (max6639) Add compatible string Naresh Solanki
  2023-04-21  7:37 ` [PATCH v2 1/2] dt-bindings: hwmon: Add max6639 Krzysztof Kozlowski
  0 siblings, 2 replies; 14+ messages in thread
From: Naresh Solanki @ 2023-04-20 11:17 UTC (permalink / raw)
  To: Jean Delvare, Guenter Roeck, Rob Herring, Krzysztof Kozlowski,
	Naresh Solanki
  Cc: Marcello Sylvester Bauer, linux-hwmon, devicetree, linux-kernel

From: Marcello Sylvester Bauer <sylv@sylv.io>

Add Devicetree binding documentation for Maxim MAX6639 temperature
monitor with PWM fan-speed controller.

Signed-off-by: Marcello Sylvester Bauer <sylv@sylv.io>
Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
...
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
+
+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
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      fan-controller@10 {
+        compatible = "maxim,max6639";
+        reg = <0x10>;
+      };
+    };
+...

base-commit: b8610e0fe08f1476895f141be60f8dd842f0adac
-- 
2.39.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v2 2/2] hwmon: (max6639) Add compatible string
  2023-04-20 11:17 [PATCH v2 1/2] dt-bindings: hwmon: Add max6639 Naresh Solanki
@ 2023-04-20 11:17 ` Naresh Solanki
  2023-04-21  7:37 ` [PATCH v2 1/2] dt-bindings: hwmon: Add max6639 Krzysztof Kozlowski
  1 sibling, 0 replies; 14+ messages in thread
From: Naresh Solanki @ 2023-04-20 11:17 UTC (permalink / raw)
  To: Jean Delvare, Guenter Roeck; +Cc: Naresh Solanki, linux-hwmon, linux-kernel

Use maxim,max6639 as compatible string for the driver.

Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
...
Changes in V2:
- None, Updated DT patch
---
 drivers/hwmon/max6639.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/hwmon/max6639.c b/drivers/hwmon/max6639.c
index 9b895402c80d..ada410bfb7b6 100644
--- a/drivers/hwmon/max6639.c
+++ b/drivers/hwmon/max6639.c
@@ -618,11 +618,17 @@ MODULE_DEVICE_TABLE(i2c, max6639_id);
 
 static DEFINE_SIMPLE_DEV_PM_OPS(max6639_pm_ops, max6639_suspend, max6639_resume);
 
+static const struct of_device_id max6639_of_match[] = {
+	{ .compatible = "maxim,max6639", },
+	{ },
+};
+
 static struct i2c_driver max6639_driver = {
 	.class = I2C_CLASS_HWMON,
 	.driver = {
 		   .name = "max6639",
 		   .pm = pm_sleep_ptr(&max6639_pm_ops),
+		   .of_match_table = max6639_of_match,
 		   },
 	.probe_new = max6639_probe,
 	.id_table = max6639_id,
-- 
2.39.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 1/2] dt-bindings: hwmon: Add max6639
  2023-04-20 11:17 [PATCH v2 1/2] dt-bindings: hwmon: Add max6639 Naresh Solanki
  2023-04-20 11:17 ` [PATCH v2 2/2] hwmon: (max6639) Add compatible string Naresh Solanki
@ 2023-04-21  7:37 ` Krzysztof Kozlowski
  2023-04-24  9:18   ` Naresh Solanki
  1 sibling, 1 reply; 14+ messages in thread
From: Krzysztof Kozlowski @ 2023-04-21  7:37 UTC (permalink / raw)
  To: Naresh Solanki, Jean Delvare, Guenter Roeck, Rob Herring,
	Krzysztof Kozlowski
  Cc: Marcello Sylvester Bauer, linux-hwmon, devicetree, linux-kernel

On 20/04/2023 13:17, Naresh Solanki wrote:
> From: Marcello Sylvester Bauer <sylv@sylv.io>
> 
> Add Devicetree binding documentation for Maxim MAX6639 temperature
> monitor with PWM fan-speed controller.
> 
> Signed-off-by: Marcello Sylvester Bauer <sylv@sylv.io>
> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
> ...

Wrong syntax. '...' do not mean anything, it's ---

> 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?

> +
> +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?

Anyway, the binding looks incomplete. Where are the supplies? Interrupts?

> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      fan-controller@10 {
> +        compatible = "maxim,max6639";
> +        reg = <0x10>;
> +      };
> +    };
> +...
> 
> base-commit: b8610e0fe08f1476895f141be60f8dd842f0adac

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 1/2] dt-bindings: hwmon: Add max6639
  2023-04-21  7:37 ` [PATCH v2 1/2] dt-bindings: hwmon: Add max6639 Krzysztof Kozlowski
@ 2023-04-24  9:18   ` Naresh Solanki
  2023-04-24  9:42     ` Krzysztof Kozlowski
  0 siblings, 1 reply; 14+ messages in thread
From: Naresh Solanki @ 2023-04-24  9:18 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Jean Delvare, Guenter Roeck, Rob Herring,
	Krzysztof Kozlowski
  Cc: Marcello Sylvester Bauer, linux-hwmon, devicetree, linux-kernel

Hi Krzysztof,


On 21-04-2023 01:07 pm, Krzysztof Kozlowski wrote:
> On 20/04/2023 13:17, Naresh Solanki wrote:
>> From: Marcello Sylvester Bauer <sylv@sylv.io>
>>
>> Add Devicetree binding documentation for Maxim MAX6639 temperature
>> monitor with PWM fan-speed controller.
>>
>> Signed-off-by: Marcello Sylvester Bauer <sylv@sylv.io>
>> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
>> ...
> 
> Wrong syntax. '...' do not mean anything, it's ---
Ack
> 
>> 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" ?

> 
>> +
>> +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.
> 
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    i2c {
>> +      #address-cells = <1>;
>> +      #size-cells = <0>;
>> +
>> +      fan-controller@10 {
>> +        compatible = "maxim,max6639";
>> +        reg = <0x10>;
>> +      };
>> +    };
>> +...
>>
>> base-commit: b8610e0fe08f1476895f141be60f8dd842f0adac
> 
> Best regards,
> Krzysztof
> 
Regards,
Naresh

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 1/2] dt-bindings: hwmon: Add max6639
  2023-04-24  9:18   ` Naresh Solanki
@ 2023-04-24  9:42     ` Krzysztof Kozlowski
  2023-04-24  9:53       ` Naresh Solanki
  0 siblings, 1 reply; 14+ messages in thread
From: Krzysztof Kozlowski @ 2023-04-24  9:42 UTC (permalink / raw)
  To: Naresh Solanki, Jean Delvare, Guenter Roeck, Rob Herring,
	Krzysztof Kozlowski
  Cc: Marcello Sylvester Bauer, linux-hwmon, devicetree, linux-kernel

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


> 
>>
>>> +
>>> +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.


Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 1/2] dt-bindings: hwmon: Add max6639
  2023-04-24  9:42     ` Krzysztof Kozlowski
@ 2023-04-24  9:53       ` Naresh Solanki
  2023-05-03  8:26         ` Naresh Solanki
  0 siblings, 1 reply; 14+ messages in thread
From: Naresh Solanki @ 2023-04-24  9:53 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Jean Delvare, Guenter Roeck, Rob Herring,
	Krzysztof Kozlowski
  Cc: Marcello Sylvester Bauer, linux-hwmon, devicetree, linux-kernel

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.
> 
> 
> Best regards,
> Krzysztof
> 

Regards,
Naresh.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 1/2] dt-bindings: hwmon: Add max6639
  2023-04-24  9:53       ` Naresh Solanki
@ 2023-05-03  8:26         ` Naresh Solanki
  2023-05-03 16:18           ` Krzysztof Kozlowski
  0 siblings, 1 reply; 14+ messages in thread
From: Naresh Solanki @ 2023-05-03  8:26 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Jean Delvare, Guenter Roeck, Rob Herring,
	Krzysztof Kozlowski
  Cc: Marcello Sylvester Bauer, linux-hwmon, devicetree, linux-kernel

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.
Thanks,
Naresh
>>
>>
>> Best regards,
>> Krzysztof
>>
> 
> Regards,
> Naresh.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 1/2] dt-bindings: hwmon: Add max6639
  2023-05-03  8:26         ` Naresh Solanki
@ 2023-05-03 16:18           ` Krzysztof Kozlowski
  2023-05-04  9:47             ` Naresh Solanki
  0 siblings, 1 reply; 14+ messages in thread
From: Krzysztof Kozlowski @ 2023-05-03 16:18 UTC (permalink / raw)
  To: Naresh Solanki, Jean Delvare, Guenter Roeck, Rob Herring,
	Krzysztof Kozlowski
  Cc: Marcello Sylvester Bauer, linux-hwmon, devicetree, linux-kernel

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 1/2] dt-bindings: hwmon: Add max6639
  2023-05-03 16:18           ` Krzysztof Kozlowski
@ 2023-05-04  9:47             ` Naresh Solanki
  2023-05-04 10:47               ` Krzysztof Kozlowski
  0 siblings, 1 reply; 14+ messages in thread
From: Naresh Solanki @ 2023-05-04  9:47 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Jean Delvare, Guenter Roeck, Rob Herring,
	Krzysztof Kozlowski
  Cc: Marcello Sylvester Bauer, linux-hwmon, devicetree, linux-kernel

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?

Best Regards,
~Naresh

> 
> Best regards,
> Krzysztof
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 1/2] dt-bindings: hwmon: Add max6639
  2023-05-04  9:47             ` Naresh Solanki
@ 2023-05-04 10:47               ` Krzysztof Kozlowski
  2023-05-04 11:36                 ` Naresh Solanki
  0 siblings, 1 reply; 14+ messages in thread
From: Krzysztof Kozlowski @ 2023-05-04 10:47 UTC (permalink / raw)
  To: Naresh Solanki, Jean Delvare, Guenter Roeck, Rob Herring,
	Krzysztof Kozlowski
  Cc: Marcello Sylvester Bauer, linux-hwmon, devicetree, linux-kernel

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.

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 1/2] dt-bindings: hwmon: Add max6639
  2023-05-04 10:47               ` Krzysztof Kozlowski
@ 2023-05-04 11:36                 ` Naresh Solanki
  2023-05-04 11:44                   ` Krzysztof Kozlowski
  2023-07-19 17:39                   ` Guenter Roeck
  0 siblings, 2 replies; 14+ messages in thread
From: Naresh Solanki @ 2023-05-04 11:36 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Jean Delvare, Guenter Roeck, Rob Herring,
	Krzysztof Kozlowski
  Cc: Marcello Sylvester Bauer, linux-hwmon, devicetree, linux-kernel

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
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 1/2] dt-bindings: hwmon: Add max6639
  2023-05-04 11:36                 ` Naresh Solanki
@ 2023-05-04 11:44                   ` Krzysztof Kozlowski
  2023-07-19 17:39                   ` Guenter Roeck
  1 sibling, 0 replies; 14+ messages in thread
From: Krzysztof Kozlowski @ 2023-05-04 11:44 UTC (permalink / raw)
  To: Naresh Solanki, Jean Delvare, Guenter Roeck, Rob Herring,
	Krzysztof Kozlowski
  Cc: Marcello Sylvester Bauer, linux-hwmon, devicetree, linux-kernel

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

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 1/2] dt-bindings: hwmon: Add max6639
  2023-05-04 11:36                 ` Naresh Solanki
  2023-05-04 11:44                   ` Krzysztof Kozlowski
@ 2023-07-19 17:39                   ` Guenter Roeck
  2023-07-20  7:51                     ` Naresh Solanki
  1 sibling, 1 reply; 14+ messages in thread
From: Guenter Roeck @ 2023-07-19 17:39 UTC (permalink / raw)
  To: Naresh Solanki, Krzysztof Kozlowski, Jean Delvare, Rob Herring,
	Krzysztof Kozlowski
  Cc: Marcello Sylvester Bauer, linux-hwmon, devicetree, linux-kernel

On 5/4/23 04:36, Naresh Solanki wrote:
> Hi Krzysztof,
> 
[ ... ]
>>
>> 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.
> 

As a follow-up, since it came up in a separate context:

Bindings and driver are independent of each other. _Bindings_
are supposed to be complete. However, the existence of a property
in the bindings description does not have to be reflected in
the driver.

FWIW, you _could_ have added the device to the list of trivial
devices. The only really mandatory property is vdd, and every
chip has that. All other properties are really about configuration
and/or fan properties, and I don't even know how to describe fan
properties (such as pulses per revolution, pwm parameters,
fan speed limits, the relationship between pwm outputs
and fan inputs, the relationship between fan speed input
and pwm output, or fan spin-up requirements) in devicetree.

Guenter


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 1/2] dt-bindings: hwmon: Add max6639
  2023-07-19 17:39                   ` Guenter Roeck
@ 2023-07-20  7:51                     ` Naresh Solanki
  0 siblings, 0 replies; 14+ messages in thread
From: Naresh Solanki @ 2023-07-20  7:51 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Krzysztof Kozlowski, Jean Delvare, Rob Herring,
	Krzysztof Kozlowski, Marcello Sylvester Bauer, linux-hwmon,
	devicetree, linux-kernel

Hi Guenter,


On Wed, 19 Jul 2023 at 23:10, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 5/4/23 04:36, Naresh Solanki wrote:
> > Hi Krzysztof,
> >
> [ ... ]
> >>
> >> 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.
> >
>
> As a follow-up, since it came up in a separate context:
>
> Bindings and driver are independent of each other. _Bindings_
> are supposed to be complete. However, the existence of a property
> in the bindings description does not have to be reflected in
> the driver.
>
> FWIW, you _could_ have added the device to the list of trivial
> devices. The only really mandatory property is vdd, and every
> chip has that. All other properties are really about configuration
> and/or fan properties, and I don't even know how to describe fan
> properties (such as pulses per revolution, pwm parameters,
> fan speed limits, the relationship between pwm outputs
> and fan inputs, the relationship between fan speed input
> and pwm output, or fan spin-up requirements) in devicetree.

Thanks for the inputs,
Will work on it & keep you posted.

Regards,
Naresh
>
> Guenter
>

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2023-07-20  7:51 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-04-20 11:17 [PATCH v2 1/2] dt-bindings: hwmon: Add max6639 Naresh Solanki
2023-04-20 11:17 ` [PATCH v2 2/2] hwmon: (max6639) Add compatible string Naresh Solanki
2023-04-21  7:37 ` [PATCH v2 1/2] dt-bindings: hwmon: Add max6639 Krzysztof Kozlowski
2023-04-24  9:18   ` Naresh Solanki
2023-04-24  9:42     ` Krzysztof Kozlowski
2023-04-24  9:53       ` Naresh Solanki
2023-05-03  8:26         ` Naresh Solanki
2023-05-03 16:18           ` Krzysztof Kozlowski
2023-05-04  9:47             ` Naresh Solanki
2023-05-04 10:47               ` Krzysztof Kozlowski
2023-05-04 11:36                 ` Naresh Solanki
2023-05-04 11:44                   ` Krzysztof Kozlowski
2023-07-19 17:39                   ` Guenter Roeck
2023-07-20  7:51                     ` Naresh Solanki

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).