Received-SPF: pass (google.com: domain of linux-kernel+bounces-206508-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <e15695d6-b1b1-472a-8288-dcdfba5d619d@amperemail.onmicrosoft.com>
Date: Fri, 7 Jun 2024 23:47:42 +0700
User-Agent: Mozilla Thunderbird
From: Chanh Nguyen <chanh@amperemail.onmicrosoft.com>
Subject: Re: [PATCH v2 3/3] dt-bindings: hwmon: max31790: Add
 maxim,pwmout-pin-as-tach-input property
To: Conor Dooley <conor@kernel.org>
Cc: Guenter Roeck <linux@roeck-us.net>,
 Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
 Chanh Nguyen <chanh@os.amperecomputing.com>, Jean Delvare
 <jdelvare@suse.com>, Rob Herring <robh+dt@kernel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
 Conor Dooley <conor+dt@kernel.org>, Justin Ledford
 <justinledford@google.com>, devicetree@vger.kernel.org,
 linux-hwmon@vger.kernel.org, linux-kernel@vger.kernel.org,
 OpenBMC Maillist <openbmc@lists.ozlabs.org>,
 Open Source Submission <patches@amperecomputing.com>,
 Phong Vo <phong@os.amperecomputing.com>,
 Thang Nguyen <thang@os.amperecomputing.com>,
 Quan Nguyen <quan@os.amperecomputing.com>
References: <20240414042246.8681-4-chanh@os.amperecomputing.com>
 <13b195e6-cbbd-4f74-a6fa-d874cb4aaa45@linaro.org>
 <065243cc-09cf-4087-8842-bd4394fb324f@amperemail.onmicrosoft.com>
 <d549cf2b-a7fa-4644-8fcb-3c420503ee01@amperemail.onmicrosoft.com>
 <20240423-gallantly-slurp-24adbfbd6f09@spud>
 <ab5cfd8c-0e88-4194-a77e-5ffbb6890319@amperemail.onmicrosoft.com>
 <396b47f5-9604-44ab-881f-94d0664bcab8@roeck-us.net>
 <0dcc8788-604a-49c1-8c6b-fdbfa9192039@amperemail.onmicrosoft.com>
 <da94fde6-3286-44eb-a543-c2ac4d11cd32@roeck-us.net>
 <8fb38eb3-bb94-49cc-b5bc-80989d7876b9@amperemail.onmicrosoft.com>
 <20240508-onward-sedation-621cc48fa83f@spud>
Content-Language: en-US
In-Reply-To: <20240508-onward-sedation-621cc48fa83f@spud>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?NTgxaTV6RU1DV3FkS1hyR0tsbDBiTjc1bFV2NXJQdWNHWVNQbDc4RFBIaUVT?=
 =?utf-8?B?dzRDcHN0Z1dZaWt1UzlXR09ZcEJNb0FZeDVyQ3FqN2MyVWJQMC81eG9aNEtz?=
 =?utf-8?B?L3R2UW4xQXF3VHo0WXNEbEcyTXp4U3lFM3VwMGV5TlZQZDRrL1NFeDV2MmVn?=
 =?utf-8?B?ZFdlcTYvNXUvbGRvYkVPYW9TOWxVOE0wREpQNHNLZzl1c0FLajBZNkdhMHRJ?=
 =?utf-8?B?VExZbzZLTnRqTlUvQkZ6RTcxWjhKMGZ0enV3NUdoKzlRbmF0cXRldEJ1Q2I3?=
 =?utf-8?B?dDNNcHNwalVGZzZOWjB2TjdlSlhGczVrTDFCZnRKdWMzRitGL3lqNnovMVBa?=
 =?utf-8?B?QkxhNHlJc1ZXaU43U3IrNnJmaDQ1UmFvenc3RWI5V0lLeDBXTW9IL2ZTOHJB?=
 =?utf-8?B?bGQvUFIxckRsSFRzNzNpaGhOMUlwVEpEdnpLbDQvd0tOM2dDMkhHeEI0c0lo?=
 =?utf-8?B?SUp2K2wxMnQwUjQ4UGRBdmROT0kvcU16MnhLR2hJdllSRnQvUEJnZnJwTUVW?=
 =?utf-8?B?ZVlCLzNpVFVGZDJJZFI1NFRacGhGSzVQM3lxdEtyYUdKTGg0YUZzc2lXdWpU?=
 =?utf-8?B?NTg0TmhDTjF2Yy92RTgxWlBTdTd0ZDZQNEppdUQxajZzeVVKUUpDN1J4UGVF?=
 =?utf-8?B?K0N0VTJrcmRpTk0wZjh3bjhEVkN6aWhBVHg0Y3MvU3ZYV1JXeVhlRG02b3pJ?=
 =?utf-8?B?eEZvc0dlMFUzZVl6S1NabmhwZGN0cFpzVWMrZVRvdG9EUnV6Sk1PWHFqS2pB?=
 =?utf-8?B?Z2lxRHY1WWt3cUowdkNIMGV3UGJaTXhtQUZqSlY2OXhhbjB3dnJPZ3lpMm8w?=
 =?utf-8?B?QXk3NGRnUXlXM3cycGxZUXBqaEEzRDJ4UjZXeVJ1U0JOSjJENEFpeGo0OGRF?=
 =?utf-8?B?RjVYaCtzSGNtdUFQMlFaVHFnaXdIRlMvNmJjNEY1N2lRRGdnY2tQNkZ6MnVq?=
 =?utf-8?B?cVVadFF6OHhUUGdlNm1HQTFPemM5bVAwNUFsODVjV3RkSUsvKzZjQXd3eG15?=
 =?utf-8?B?dWR4VjcvZkJWT0VLa1N4ay9seWpZSm55RmdoSk5SN254SlFLeVBXQldBMENF?=
 =?utf-8?B?elJnSGVzNjUvT1hvOHJURmFtQkRkNGlDei9xNVgwK2Jwa3crQVZFL1hheXNu?=
 =?utf-8?B?L3NNK0hWWjRyY3VTQy9PU2EzYm40NWlRNXBrSDV0dUNVNDd3NFk0dmR5azla?=
 =?utf-8?B?NzIvakFFVnNmaTdXamZseHhGNEdndEROMjdYSExqQnBjeWFpU3FqOFlaQmZM?=
 =?utf-8?B?bFZEdjllMWhMWUp2ejQwZjJKQTdvd0Rob3RONm1lZlFtT1RuQzFOc1VreFlu?=
 =?utf-8?B?SHRkUkpGNk1PMUtVYkVpMmcwbnNTakx0cjY1dkZZcjB3UWs2V0VsQnhtaXla?=
 =?utf-8?B?YXBDTW52NFg1TnF0RytYcSs1Q2FSeFZmWlJvQjZteUVpZzczUmtYUDZUMmEy?=
 =?utf-8?B?ejlFa0YxeEFSWm94YW8yRUhncmt3L1hhTmd3UzlWTGlUUzlEVG1lZjRFZmFF?=
 =?utf-8?B?RUJCOGZzUzNGem9FS2twcWY2alF1YmQxYlJ1OWsxUHowdWNRTDRyS3NyTkls?=
 =?utf-8?B?TjVqVWR1RXJiMkFGc09wUnN2aFlFL09yOGVyVHZVYTRyYi91S0pZWURTRm9u?=
 =?utf-8?B?QWpKTnZFeXIwb2NQeXNMQ1JzRmRtYitLblFFbXNpYWFhMi8yakluWlpJaHNT?=
 =?utf-8?B?bTFod3VoNkJ5eUhjbkhqUXBRcnpRTVBhU0RJOGNHdHZrK3g2MkFHRzZBbTE5?=
 =?utf-8?B?L0ZQemJmc0l1ME96aklUL3BmYjRGSkczYVZtZE9iejAwbFE5ZkZFckFRd1VP?=
 =?utf-8?B?UVY1MzFoN1dxSm9nbDZPZmRvdm8zR1pzSWhXR1ppTGlGWE9hZTgyd0ZCbjRB?=
 =?utf-8?B?cCtvcVVGenJhUnVCZkY2MXlmUGdXOXg1aHZBeExoNG9Hcm5MN1BaOTlzd2p1?=
 =?utf-8?B?NlFMb1ZCVXdhUEZBbUQyQ0xpbEExQ0htcnprbWFzS01iVXB5VEhjcXpSOXAw?=
 =?utf-8?B?NW13Vi9ObEp5eTJMNUlWRzVKczFDN285blZDTlRML1FVakRPYlZNMmM4Wjll?=
 =?utf-8?B?U2hDekZQTEZTZHViUWRVN09LbG9CL2tKZ1RCQjMzRkZOcG5ObjFrSlBOWWk3?=
 =?utf-8?B?QjFVcWd5OUdnakRROUNhVnVaUTVCdUdwcVhrcHBhUVZlWEZaR0YxejJyYnl4?=
 =?utf-8?Q?ldicyhm0Fveks1QwUkpYh9s=3D?=
X-OriginatorOrg: amperemail.onmicrosoft.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 9c30221b-62e4-4224-7fa7-08dc87119206
X-MS-Exchange-CrossTenant-AuthSource: DM6PR01MB5947.prod.exchangelabs.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Jun 2024 16:47:51.7019
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3bc2b170-fd94-476d-b0ce-4229bdc904a7
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: fDIKC3S6b5jiQJrHO4PI65tRRNmvPMcGlX4pDbBYwB+3N9fTjNrAp0iwHbAL6X1JVV/wCVM92IUDcwoU4URNjKnw2N6UrRDU+m+q0ziFTVOIb6HMjNk5J1bNW92PdnsP
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR01MB7051


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