Re: [PATCH v2 5/6] hwmon: (adt7470) Register as a PWM provider

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] Calculating the PWM duty cycle using `DIV_ROUND_CLOSEST_ULL` with a 64-bit divisor causes 32-bit truncation and a divide-by-zero kernel panic on 32-bit architectures.
- [High] Early return in PWM apply relies on unsynchronized, zero-initialized cache values, potentially ignoring the first 0% duty cycle request.
- [Medium] Registering a generic PWM provider directly from a hardware monitoring driver violates subsystem layering guidelines.
- [Medium] The driver silently ignores the requested PWM signal polarity, applying normal polarity even when inverted is requested.

Pre-existing issues:
- [High] The PWM automatic mode mask calculation swaps the masks for PWM channels 3 and 4, causing writes to one channel to affect the configuration of the other.
--

--- Patch [5]: [PATCH v2 5/6] hwmon: (adt7470) Register as a PWM provider ---
commit e11bee28efc81066c4940f5c07591cd6181c6453
Author: Luiz Angelo Daros de Luca <luizluca@gmail.com>

hwmon: (adt7470) Register as a PWM provider

The patch registers the ADT7470 hardware monitoring driver as a generic PWM
provider, allowing standard Device Tree consumers like "pwm-fan" to use it.
It converts the PWM framework's requested duty cycle to the 0-255 scale
used by the hardware registers. The legacy sysfs interface operates in
parallel and remains unaffected.

> diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
> index d881609c99aa3..aaae14a421cd8 100644
> --- a/drivers/hwmon/adt7470.c
> +++ b/drivers/hwmon/adt7470.c

[ ... ]

> @@ -864,6 +865,57 @@ static int adt7470_pwm_write(struct device *dev, u32 attr, int channel, long val
>  	return err;
>  }
>  
> +static int adt7470_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			     const struct pwm_state *state)
> +{
> +	struct adt7470_data *data = pwmchip_get_drvdata(chip);
> +	unsigned int pwm_auto_reg_mask;
> +	int err;
> +	u8 val;
> +
> +	if (pwm->hwpwm % 2)
> +		pwm_auto_reg_mask = ADT7470_PWM2_AUTO_MASK;
> +	else
> +		pwm_auto_reg_mask = ADT7470_PWM1_AUTO_MASK;

[Severity: High]
This is a pre-existing issue, but does this logic incorrectly swap the masks
for PWM channels 3 and 4?

The logic uses hwpwm % 2 to select between ADT7470_PWM1_AUTO_MASK and
ADT7470_PWM2_AUTO_MASK. For channel 3 (hwpwm = 2) and channel 4 (hwpwm = 3),
does this inadvertently select 0x80 for PWM3 and 0x40 for PWM4?

If the datasheet and macro definitions dictate 0x40 for PWM3 and 0x80 for
PWM4, could writing to one of these channels corrupt the configuration
of the other?

This issue seems to be copied from the preexisting adt7470_pwm_write() sysfs
handler.

> +
> +	if (state->enabled && state->period > 0)

[Severity: Medium]
Does this apply callback silently ignore requested PWM polarities?

If a consumer specifies PWM_POLARITY_INVERSED in the PWM state, the hardware
only supports normal polarity.

Since there is no check to see if state->polarity != PWM_POLARITY_NORMAL,
will this cause the driver to quietly apply normal polarity instead of
rejecting the unsupported request?

> +		val = DIV_ROUND_CLOSEST_ULL(state->duty_cycle * 255, state->period);

[Severity: High]
Does this calculation cause a divide-by-zero on 32-bit architectures?

When a consumer requests a PWM period that is a multiple of 2^32 ns,
do_div() implicitly casts the divisor to a 32-bit integer on 32-bit systems,
meaning a value like 1ULL << 32 becomes 0.

Since the truncation happens during the division after the state->period > 0
check, can this lead to a kernel panic?

> +	else
> +		val = 0;
> +
> +	mutex_lock(&data->lock);
> +
> +	if (data->pwm[pwm->hwpwm] == val &&
> +	    data->pwm_automatic[pwm->hwpwm] == 0) {
> +		mutex_unlock(&data->lock);
> +		return 0;
> +	}

[Severity: High]
Can this early return falsely ignore the first 0% duty cycle request?

Because the driver data is allocated with devm_kzalloc(), data->pwm is
zero-initialized.

If a consumer requests a 0% duty cycle (val = 0) before the
adt7470_update_thread has completed its first hardware state synchronization,
will this condition pass and return early without sending the 0% duty cycle
to the hardware?

[ ... ]

> @@ -1299,6 +1351,21 @@ static int adt7470_probe(struct i2c_client *client)
>  	if (IS_ERR(hwmon_dev))
>  		return PTR_ERR(hwmon_dev);
>  
> +	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);
> +		if (err)
> +			return dev_warn_probe(dev, err, "failed to register PWM chip\n");
> +	}

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

The guidelines indicate that secondary functionalities, such as PWM or GPIO,
should be implemented as auxiliary devices rather than registering directly
from drivers/hwmon/.

By using devm_pwmchip_alloc() and devm_pwmchip_add() directly in the hwmon
probe function, does this bypass the separation into an auxiliary device and
cross-subsystem review?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717-adt7470_thermalzone-v2-0-a55147958fad@gmail.com?part=5