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

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

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

Hello,

On Fri, Jul 17, 2026 at 05:59:18PM -0300, Luiz Angelo Daros de Luca wrote:
> @@ -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;
> +
> +	if (state->enabled && state->period > 0)
> +		val = DIV_ROUND_CLOSEST_ULL(state->duty_cycle * 255, state->period);

rounding a division using closest is wrong in the context of PWM.

Also state->duty_cycle * 255 might overflow and you have to use the
actual period in the calculation of val instead of the requested value.

> +	else
> +		val = 0;
> +
> +	mutex_lock(&data->lock);

If you use guard(), the control flow simplifies and using goto becomes
unnecessary.

> +	if (data->pwm[pwm->hwpwm] == val &&
> +	    data->pwm_automatic[pwm->hwpwm] == 0) {
> +		mutex_unlock(&data->lock);
> +		return 0;
> +	}
> +
> +	/* Put the PWM channel in manual mode before updating it. */
> +	err = regmap_update_bits(data->regmap,
> +				 ADT7470_REG_PWM_CFG(pwm->hwpwm),
> +				 pwm_auto_reg_mask, 0);
> +	if (err < 0)
> +		goto out;
> +
> +	data->pwm_automatic[pwm->hwpwm] = 0;
> +
> +	err = regmap_write(data->regmap,
> +			   ADT7470_REG_PWM(pwm->hwpwm), val);
> +	if (err < 0)
> +		goto out;
> +
> +	data->pwm[pwm->hwpwm] = val;
> +out:
> +	mutex_unlock(&data->lock);
> +
> +	return err;
> +}
> +
> +static const struct pwm_ops adt7470_pwm_ops = {
> +	.apply = adt7470_pwm_apply,

For new drivers please implement .round_waveform_tohw(),
.round_waveform_fromhw(), .write_waveform() and ideally so
.read_waveform().

> +};

Best regards
Uwe

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