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

[Guenter Roeck]

On 6/7/24 09:47, Chanh Nguyen wrote:
> 
> 
> On 08/05/2024 23:47, Conor Dooley wrote:
>> On Wed, May 08, 2024 at 10:44:34AM +0700, Chanh Nguyen wrote:
>>> On 05/05/2024 22:40, Guenter Roeck wrote:
>>>> On 5/5/24 03:08, Chanh Nguyen wrote:
>>>>> On 25/04/2024 21:05, Guenter Roeck wrote:
>>>>>> On 4/25/24 03:33, Chanh Nguyen wrote:
>>>>>>
>>>>>> pwm outputs on MAX31790 are always tied to the matching
>>>>>> tachometer inputs
>>>>>> (pwm1 <--> tach1 etc) and can not be reconfigured, meaning tach-ch for
>>>>>> channel X would always be X.
>>>>>>
>>>>>>> I would like to open a discussion about whether we should
>>>>>>> use the tach-ch property on the fan-common.yaml
>>>>>>>
>>>>>>> I'm looking forward to hearing comments from everyone. For
>>>>>>> me, both tach-ch and vendor property are good.
>>>>>>>
>>>>>>
>>>>>> 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@os.amperecomputing.com/)
>>>
>>> My device tree is configured as below, I would like to configure PWMOUT pins
>>> 5 and 6 to become the tachometer input pins.
>>>
>>>         fan-controller at 20 {
>>>           compatible = "maxim,max31790";
>>>           reg = <0x20>;
>>>           maxim,pwmout-pin-as-tach-input = /bits/ 8 <0 0 0 0 1 1>;
>>>         };
>>
>> Why are you still operating off a binding that looks like this? I
>> thought that both I and Krzysztof told you to go and take a look at how
>> the aspeed,g6-pwm-tach.yaml binding looped and do something similar
>> here. You'd end up with something like:
>>
>>          fan-controller at 20 {
>>            compatible = "maxim,max31790";
>>            reg = <0x20>;
>>
>>            fan-0 {
>>              pwms = <&pwm-provider ...>;
>>              tach-ch = 6;
>>          };
>>
>>            fan-1 {
>>              pwms = <&pwm-provider ...>;
>>              tach-ch = 7;
>>          };
>> };
>>
>> You can, as tach-ch or pwms do not need to be unique, set multiple
>> channels up as using the same tachs and/or pwms.
>> In the case of this particular fan controller, I think that the max31790
>> is actually the pwm provider so you'd modify it something like:
>>
>>          pwm-provider: fan-controller at 20 {
>>            compatible = "maxim,max31790";
>>            reg = <0x20>;
>>       #pwm-cells = <N>;
>>
>>            fan-0 {
>>              pwms = <&pwm-provider ...>;
>>              tach-ch = <6>;
>>          };
>>
>>            fan-1 {
>>              pwms = <&pwm-provider ...>;
>>              tach-ch = <7>;
>>          };
>> };
>>
>> I just wrote this in my mail client's editor, so it may not be not
>> valid, but it is how the fan bindings expect you to represent this kind
>> of scenario.
>>
> 
> My apologies for the late reply.
> 
> Thank you, Conor, for your recommendation!
> 
> I spend more time checking the aspeed,g6-pwm-tach.yaml . Finally, I'll support the child nodes by having different tach-ch values. My system is designed similar to Figure 6 (8 Tach Monitors, 4PMWs).
> 
> I'm going to push the patch series v3 soon.
> 
> 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 at 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>;
>          };
>        };
>      };
> 
> 
> As your example, I saw the #pwm-cells = <N> . Please let me know, what's the purpose of this property?
> 

It is the number of fields in "pwms" after the provider reference.
In your case it would be 1 (the index). If the pwm has additional
configuration parameters such as the frequency or polarity there
would be additional entries.

Guenter