linux-kernel - Re: [PATCH v7 1/3] dt-bindings: hwmon: Add adt7475 fan/pwm properties

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4858ce06-2081-4335-af09-f118872317ea@alliedtelesis.co.nz>
Date: Wed, 28 May 2025 21:18:37 +0000
From: Chris Packham <Chris.Packham@...iedtelesis.co.nz>
To: Uwe Kleine-König <ukleinek@...nel.org>
CC: "jdelvare@...e.com" <jdelvare@...e.com>, "linux@...ck-us.net"
	<linux@...ck-us.net>, "robh@...nel.org" <robh@...nel.org>,
	"krzk+dt@...nel.org" <krzk+dt@...nel.org>, "conor+dt@...nel.org"
	<conor+dt@...nel.org>, "linux-hwmon@...r.kernel.org"
	<linux-hwmon@...r.kernel.org>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "linux-pwm@...r.kernel.org"
	<linux-pwm@...r.kernel.org>
Subject: Re: [PATCH v7 1/3] dt-bindings: hwmon: Add adt7475 fan/pwm properties

Hi Uwe,

On 28/05/2025 18:10, Uwe Kleine-König wrote:
> Hello Chris,
>
> On Tue, May 27, 2025 at 08:24:56PM +0000, Chris Packham wrote:
>> On 28/05/2025 04:12, Uwe Kleine-König wrote:
>>> Hello,
>>>
>>> On Tue, Jul 23, 2024 at 10:17:35AM +1200, Chris Packham wrote:
>>>> Add fan child nodes that allow describing the connections for the
>>>> ADT7475 to the fans it controls. This also allows setting some
>>>> initial values for the pwm duty cycle and frequency.
>>>>
>>>> Signed-off-by: Chris Packham <chris.packham@...iedtelesis.co.nz>
>>>> Reviewed-by: Rob Herring (Arm) <robh@...nel.org>
>>>> ---
>>>>
>>>> Notes:
>>>>       Changes in v7:
>>>>       - None
>>>>       Changes in v6:
>>>>       - Collect r-by from Rob
>>>>       Changes in v5:
>>>>       - Use nanoseconds for PWM frequency and duty cycle as per existing
>>>>         conventions for PWMs
>>>>       - Set flags to 0 in example to match adi,pwm-active-state setting
>>>>       Changes in v4:
>>>>       - 0 is not a valid frequency value
>>>>       Changes in v3:
>>>>       - Use the pwm provider/consumer bindings
>>>>       Changes in v2:
>>>>       - Document 0 as a valid value (leaves hardware as-is)
>>>>
>>>>    .../devicetree/bindings/hwmon/adt7475.yaml    | 35 ++++++++++++++++++-
>>>>    1 file changed, 34 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/hwmon/adt7475.yaml b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
>>>> index 051c976ab711..df2b5b889e4d 100644
>>>> --- a/Documentation/devicetree/bindings/hwmon/adt7475.yaml
>>>> +++ b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
>>>> @@ -51,6 +51,24 @@ properties:
>>>>          enum: [0, 1]
>>>>          default: 1
>>>>    
>>>> +  "#pwm-cells":
>>>> +    const: 4
>>> I asked to add support for #pwm-cells = <4> to the pwm core in reply to
>>> v4 (see
>>> https://lore.kernel.org/linux-pwm/drqvaon5lb2ei3jqofutbr6demibyfdhbmr24sva27gzpqdnon@fxa7rpl33iih/).
>>>
>>> I'm unhappy to see this merged anyhow in combination with ad-hoc parsing
>>> of the pwm properties in the driver :-\
>> As I mentioned at the time the adt7475 is not currently pwm_chip so I
>> need the ad-hoc parsing in that driver. I'd be happy to take you
>> prototype patch for pwm/core.c and polish it although I don't really
>> have a good way of testing it.
> It's more the deviation of the default binding for PWMs that I don't
> like than the ad-hoc parsing. Ideally the adt7475 would provide a
> pwmchip (as the binding suggests) and the fan would be formalized as a
> pwm-fan. With the binding that was chosen here that option becomes more
> ugly than necessary to implement.
>
> If I understand correctly you need the default value for duty to
> statically setup (or only initialize?) a fan, right?
Correct.
> I'm not sure I like
> extending #pwm-cells for a default duty value. Thinking about that a
> while I'd prefer a binding that looks more like the clock configuration
> stuff because actually having the period and flags as part of the
> reference to the PWM to be used is also a bit strange. So I imagine
> something like:
>
> 	mypwm: pwm {
> 		compatible = "...."
> 		#pwm-cells = <1>;
> 	};
>
> 	fan {
> 		compatible = "pwm-fan";
> 		pwms = <&mypwm 1>;
> 		assigned-pwms = <&mypwm>;
> 		assigned-pwm-default-period-lengths-ns = <40000>;
> 		assigned-pwm-default-flags = <PWM_POLARITY_INVERTED>;
> 	};
>
> Then specifying a period (or later a duty cycle length) would be
> optional and could be provided iff the device needs that for operation.

The frequency and flags were already part of the standard #pwm-cells 
which I think is why I was encouraged to use them. I was also trying to 
get something that would work as an ACPI overlay which turned out to be 
really hard.

> My mail was just me being frustrated about another special case that I'd
> have to handle if I go into that direction. I should have been more
> attentive to that development before it entered the mainline.
I'd be happy to deprecate the 4 cell thing and replace it with 3 cell + 
vendor property for the default period if that helps.
>
> Best regards
> Uwe