Re: [hwmon-next PATCH v5 3/4] hwmon: (adt7470) Expose fan control via PWM framework

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The ADT7470 driver implements and registers secondary functionality (PWM) directly in the hwmon driver, violating subsystem layering guidelines.
--

--- Patch [3]: [hwmon-next PATCH v5 3/4] hwmon: (adt7470) Expose fan control via PWM framework ---
commit 8476b17bbf277543f8b1034721091c420371204b
Author: Luiz Angelo Daros de Luca <luizluca@gmail.com>

hwmon: (adt7470) Expose fan control via PWM framework

The ADT7470 features four PWM outputs designed to control fans. This commit
exposes the ADT7470 fan control lines through the generic PWM framework,
allowing generic PWM consumers described in Device Tree to manage the fans.

> diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
> index 87ddd7b8ddaea..eeec3db0da88d 100644
> --- a/drivers/hwmon/adt7470.c
> +++ b/drivers/hwmon/adt7470.c
[ ... ]
> @@ -1328,6 +1497,29 @@ static int adt7470_probe(struct i2c_client *client)
>  
>  	data->pwm_freq = (u32)freq_val;
>  
> +	data->use_pwm_framework = false;
> +
> +	if (device_property_present(dev, "#pwm-cells")) {
> +		if (IS_REACHABLE(CONFIG_PWM)) {
> +			struct pwm_chip *chip;
> +
> +			chip = devm_pwmchip_alloc(dev, ADT7470_PWM_COUNT, 0);
> +			if (IS_ERR(chip))
> +				return PTR_ERR(chip);
> +
> +			chip->ops = &adt7470_pwm_ops;
> +			pwmchip_set_drvdata(chip, data);
> +
> +			err = devm_pwmchip_add(dev, chip);

[Severity: Medium]
Does this violate the hardware monitoring subsystem guidelines?

The guidelines state that if a hardware monitoring device supports secondary
functionality (such as PWM in this case), the secondary functionality should
be implemented as an auxiliary device, with the driver residing in the
appropriate subsystem directory. 

Could this PWM functionality be split out into a separate driver under
drivers/pwm/ using the auxiliary bus, rather than implementing the PWM
operations and registration directly in the hwmon driver?

> +			if (err)
> +				return dev_err_probe(dev, err, "failed to register PWM chip\n");
> +
> +			data->use_pwm_framework = true;
> +		} else {
> +			dev_warn(dev, "#pwm-cells present but CONFIG_PWM disabled. HWMON PWM attributes not hidden.\n");
> +		}
> +	}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730-adt7470_thermalzone-v5-0-42995a6bb8c5@gmail.com?part=3