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

Chanh Nguyen chanh at amperemail.onmicrosoft.com
Sat Jun 8 02:47:42 AEST 2024



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?

Thanks!

Chanh Ng


> Cheers,
> Conor.
> 
>>
>> The sysfs is generated by the max31790 driver are shown below. We can see
>> the PWM5 and PWM6 are not visible, and the fan11 and fan12 are visible. And
>> all FAN devices are on my system, which worked as expected.
>>
>> root at my-platform:/sys/class/hwmon/hwmon14# ls
>> device       fan12_input  fan1_target  fan2_target  fan3_target fan4_target
>> fan6_enable  of_node      pwm2         pwm4
>> fan11_fault  fan1_enable  fan2_enable  fan3_enable  fan4_enable fan5_enable
>> fan6_fault   power        pwm2_enable  pwm4_enable
>> fan11_input  fan1_fault   fan2_fault   fan3_fault   fan4_fault fan5_fault
>> fan6_input   pwm1         pwm3         subsystem
>> fan12_fault  fan1_input   fan2_input   fan3_input   fan4_input fan5_input
>> name         pwm1_enable  pwm3_enable  uevent
>>
>> Please share your comments!
>>
>>> Hope this helps,
>>> Guenter
>>>


More information about the openbmc mailing list