Re: [hwmon-next PATCH v3 1/4] dt-bindings: hwmon: adi,adt7470

["Uwe Kleine-König" <ukleinek@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3626 bytes --]

Hello Guenter,

On Thu, Jul 30, 2026 at 02:37:01PM -0700, Guenter Roeck wrote:
> On 7/30/26 14:11, Luiz Angelo Daros de Luca wrote:
> the architecture, while I looked at the `emc2305` approach,
> > delegating the actual fan management to the time-proven `pwm-fan`
> > driver allows us to avoid reinventing the wheel (like cooling device
> > registration and state management) inside the adt7470 driver. It also
> > keeps the standard PWM consumer/provider relationship clear in the DT.
> > It still feels strange that emc2305 declares #pwm-cells and its
> > subdriver fans pwm properties and they do not work as a pwm
> > controller. Other drivers, like aspeed-g6-pwm-tach (with similar
> 
> That is your opinion. #pwm-cells and pwms exists because it is mandated
> by DT maintainers for configuring pwm properties. That does not have to
> reflect the implementation: DT property guidance explicitly states
> that properties must describe the hardware, not the implementation.
> There is no mandate how to implement a specific driver (such as a fan
> controller driver) because a specific binding exists. It is perfectly
> valid to describe pwm properties of a fan controller using #pwm-cells
> and pwms without modeling it as pwm controller (which actually mandates
> support of the pwm subsystem or no good reason other than "because").
> 
> While you may consider the emc2305 approach inferior, I happen to
> disagree. That is your personal opinion. For my part I consider it
> inferior and overly complex having to involve another subsystem (pwm)
> and another driver (pwm-fan) just to make the chip's pwm outputs
> visible to the thermal subsystem as cooling devices.

Strange how opinions can differ. I'm with Luiz here. While I agree
that mixing subsystems is slightly inconvenient, hwmon uses i2c and spi
and clk and regmap and probably more, but pwm is a problem?

What you call "inferior and overly complex" is actually nice and elegant
in my view. There is code that handles PWMs, and by not using it you add
code duplication with all the downsides that comes with it (different
behaviours, maintenance overhead, wrong expectations).

So amc6821 and nct7363 only support #pwm-cells = <2> which is
non-standard for PWMs and I don't see where the period (cell 1 for
amc6821 and cell 2 for nct7363 *sigh*) is going into the driver.

For emc2305 uses #pwm-cells = <3> but cell 2 specifies something that
actually belongs to pinctrl and not pwm, and cell 0 is a frequency
instead of a time. And interpretation of cell 1 should better be done
using:

diff --git a/drivers/hwmon/emc2305.c b/drivers/hwmon/emc2305.c
index eef3b021671b..c704fcd61356 100644
--- a/drivers/hwmon/emc2305.c
+++ b/drivers/hwmon/emc2305.c
@@ -571,12 +571,8 @@ static int emc2305_of_parse_pwm_child(struct device *dev,
 	}
 
 	if (args.args_count > 1) {
-		if (args.args[1] == PWM_POLARITY_NORMAL || args.args[1] == PWM_POLARITY_INVERSED)
-			data->pwm_polarity_mask |= args.args[1] << ch;
-		else
-			dev_err(dev, "Wrong PWM polarity config provided: %d\n", args.args[0]);
-	} else {
-		data->pwm_polarity_mask |= PWM_POLARITY_NORMAL << ch;
+		if (args.args[1] & PWM_POLARITY_INVERTED)
+			data->pwm_polarity_mask |= 1 << ch;
 	}
 
 	if (args.args_count > 2) {

(Yes, I agree that the difference between PWM_POLARITY_INVERSED and
PWM_POLARITY_INVERTED is annoying.)

And all of these seem to ignore the phandle.

So these all only somewhat look like standard pwm bindings, but actually
there are traps and surprises hidden for the device tree author.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]