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: <jmxmxzzfyobuheqe75lj7qcq5rlt625wddb3rlhiernunjdodu@tgxghvfef4tl>
Date: Wed, 28 May 2025 08:10:53 +0200
From: Uwe Kleine-König <ukleinek@...nel.org>
To: Chris Packham <Chris.Packham@...iedtelesis.co.nz>
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

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? 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.

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.

Best regards
Uwe

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)