Re: [PATCH v2 3/3] dt-bindings: hwmon: max31790: Add maxim,pwmout-pin-as-tach-input property

From: Chanh Nguyen
Date: Fri Jun 07 2024 - 12:47:19 EST




On 09/05/2024 15:29, Krzysztof Kozlowski wrote:
On 08/05/2024 05:44, Chanh Nguyen wrote:


I am not even sure how to define tach-ch to mean "use the pwm output pin
associated with this tachometer input channel not as pwm output
but as tachometer input". That would be a boolean, not a number.


Thank Guenter,

I reviewed again the "tach-ch" property, which is used in the
https://elixir.bootlin.com/linux/v6.9-rc6/source/Documentation/devicetree/bindings/hwmon/aspeed,g6-pwm-tach.yaml#L68 and https://elixir.bootlin.com/linux/v6.9-rc6/source/drivers/hwmon/aspeed-g6-pwm-tach.c#L434

That is something completely different from my purpose.


Based on its definition, tach-ch is associated with fans, and it looks
like the .yaml file groups multiple sets of fans into a single
fan node.

In the simple case that would be
    tach-ch = <1>
...
    tach-ch = <12>

or, if all fans are controlled by a single pwm
    tach-ch = <1 2 3 4 5 6 8 9 10 11 12>

The existence of tachometer channel 7..12 implies that pwm channel
(tachometer
channel - 6) is used as tachometer channel. That should be sufficient to
program
the chip for that channel. All you'd have to do is to ensure that pwm
channel
"X" is not listed as tachometer channel "X + 6", and program pwm channel
"X - 6"
for tachometer channels 7..12 as tachometer channels.


Hi Guenter,

I applied the patch [2/3] in my patch series
(https://lore.kernel.org/lkml/20240414042246.8681-3-chanh@xxxxxxxxxxxxxxxxxxxxxx/)

My device tree is configured as below, I would like to configure PWMOUT
pins 5 and 6 to become the tachometer input pins.


And what is wrong in described common tach-ch property? I think we
explained it three times and you did not provide any arguments, what's
missing. Instead you say "I want something like this in DTS" which is
not an argument and does not help discussion.


Hi Krzysztof,

Apologies for any inconvenience caused by the delayed response.

I'll to support the child nodes by having different tach-ch values. I suggest the child nodes and the "tach-ch" is optional, it will not be added to "required:". Do you have any comments? Please help me share!

This is a brief binding
...
properties:
compatible:
const: maxim,max31790

reg:
maxItems: 1

clocks:
maxItems: 1

resets:
maxItems: 1

patternProperties:
"^fan-[0-9]+$":
$ref: fan-common.yaml#
unevaluatedProperties: false

required:
- compatible
- reg

additionalProperties: false

examples:
- |
    i2c {
      #address-cells = <1>;
      #size-cells = <0>;

      pwm_provider: fan-controller@20 {
        compatible = "maxim,max31790";
        reg = <0x20>;
        clocks = <&sys_clk>;
        resets = <&reset 0>;

        fan-0 {
          pwms = <&pwm_provider 1>;
          tach-ch = <1 2>;
        };

        fan-1 {
          pwms = <&pwm_provider 2>;
          tach-ch = <7 8>;
        };
      };
    };


If it's OK, I'm going to push the patch series v3 soon.

Thanks,
Chanh Ng

Best regards,
Krzysztof