| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ee94c869-54ae-42ac-ac44-34535141293a@roeck-us.net>
Date: Fri, 7 Jun 2024 16:14:14 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Chanh Nguyen <chanh@...eremail.onmicrosoft.com>,
Conor Dooley <conor@...nel.org>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
Chanh Nguyen <chanh@...amperecomputing.com>, Jean Delvare
<jdelvare@...e.com>, Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>, Justin Ledford
<justinledford@...gle.com>, devicetree@...r.kernel.org,
linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org,
OpenBMC Maillist <openbmc@...ts.ozlabs.org>,
Open Source Submission <patches@...erecomputing.com>,
Phong Vo <phong@...amperecomputing.com>,
Thang Nguyen <thang@...amperecomputing.com>,
Quan Nguyen <quan@...amperecomputing.com>
Subject: Re: [PATCH v2 3/3] dt-bindings: hwmon: max31790: Add
maxim,pwmout-pin-as-tach-input property
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@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?
>
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
Powered by blists - more mailing lists