| 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
| ||
|
Date: Fri, 7 Jun 2024 23:47:42 +0700
From: Chanh Nguyen <chanh@...eremail.onmicrosoft.com>
To: Conor Dooley <conor@...nel.org>
Cc: Guenter Roeck <linux@...ck-us.net>,
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 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@...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
>>>
Powered by blists - more mailing lists