Re: [PATCH] [RFC] hwmon: nct6775: Register fan PWMs as PWM chip

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

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

Hello Richard,

just a very quick look:

On Wed, Feb 25, 2026 at 01:51:59PM +0100, Richard Weinberger wrote:
> @@ -3501,6 +3592,131 @@ static int add_temp_sensors(struct nct6775_data *data, const u16 *regp,
>  	return 0;
>  }
>  
> +static int nct6775_pwm_round_waveform_fromhw(struct pwm_chip *chip,
> +					     struct pwm_device *pwm,
> +					     const void *_wfhw,
> +					     struct pwm_waveform *wf)
> +{
> +	struct nct6775_data *data = pwmchip_get_drvdata(chip);
> +	const u8 *wfhw = _wfhw;
> +
> +	if (get_pwm_period(data, pwm->hwpwm, &wf->period_length_ns))
> +		return 1;

That looks wrong. In principle nct6775_pwm_round_waveform_fromhw()
doesn't depend on hardware state. It's supposed to just convert the
settings stored in _wfhw to wf. If you know that some things are
constant during the lifetime of the PWM and you read those from
hardware, return a proper error code, not 1.

> +	wf->duty_length_ns = mul_u64_u64_div_u64(*wfhw, wf->period_length_ns, 255);
> +	wf->duty_offset_ns = 0;
> +
> +	return 0;
> +}
> +
> +static int nct6775_pwm_round_waveform_tohw(struct pwm_chip *chip,
> +					   struct pwm_device *pwm,
> +					   const struct pwm_waveform *wf,
> +					   void *_wfhw)
> +{
> +	struct nct6775_data *data = pwmchip_get_drvdata(chip);
> +	u8 *wfhw = _wfhw;
> +	u64 cur_period;
> +
> +	if (wf->period_length_ns == 0) {
> +		*wfhw = 0;
> +		return 0;
> +	}
> +
> +	if (get_pwm_period(data, pwm->hwpwm, &cur_period))
> +		return 1;
> +
> +	if (wf->duty_length_ns >= cur_period)
> +		*wfhw = 255;
> +	else
> +		*wfhw = mul_u64_u64_div_u64(wf->duty_length_ns, 255, wf->period_length_ns);
> +
> +	if (wf->period_length_ns != cur_period)
> +		return 1;

Rounding down wf->period_length_ns is fine, so this must be:

	if (wf->period_length_ns < cur_period)
		return 1;

> +
> +	return 0;
> +}
> +
> +
> +static int nct6775_pwm_write_waveform(struct pwm_chip *chip,
> +				      struct pwm_device *pwm,
> +				      const void *_wfhw)
> +{
> +	struct nct6775_data *data = pwmchip_get_drvdata(chip);
> +	const u8 *wfhw = _wfhw;
> +
> +	return write_pwm(data, pwm->hwpwm, 0, *wfhw);
> +}
> +
> +static int nct6775_pwm_read_waveform(struct pwm_chip *chip,
> +				     struct pwm_device *pwm,
> +				     void *_wfhw)
> +{
> +	struct nct6775_data *data = nct6775_update_device(pwmchip_parent(chip));
> +	u8 *wfhw = _wfhw;
> +	int val;
> +
> +	val = read_pwm(data, pwm->hwpwm, 0);
> +	if (val < 0)
> +		return val;
> +
> +	*wfhw = (u8)val;
> +
> +	return 0;
> +}
> +
> +static int nct6775_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct nct6775_data *data = pwmchip_get_drvdata(chip);
> +
> +	if (data->pwm_enable[pwm->hwpwm] > manual)
> +		return -EBUSY;
> +
> +	data->pwm_exported[pwm->hwpwm] = true;
> +
> +	return 0;
> +}
> +
> +static void nct6775_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct nct6775_data *data = pwmchip_get_drvdata(chip);
> +
> +	data->pwm_exported[pwm->hwpwm] = false;
> +}
> +
> +static const struct pwm_ops nct6775_pwm_ops = {
> +	.sizeof_wfhw = sizeof(u8),
> +	.request = nct6775_pwm_request,
> +	.free = nct6775_pwm_free,
> +	.round_waveform_fromhw = nct6775_pwm_round_waveform_fromhw,
> +	.round_waveform_tohw = nct6775_pwm_round_waveform_tohw,
> +	.write_waveform = nct6775_pwm_write_waveform,
> +	.read_waveform = nct6775_pwm_read_waveform,
> +};
> +
> +static int nct6775_register_pwm_chip(struct device *dev, struct nct6775_data *data)
> +{
> +	struct pwm_chip *chip;
> +	int ret;
> +
> +	if (data->pwm_num < 1)
> +		return 0;
> +
> +	chip = devm_pwmchip_alloc(dev, data->pwm_num, 0);
> +	if (IS_ERR(chip))
> +		return PTR_ERR(chip);
> +
> +	chip->ops = &nct6775_pwm_ops;
> +	pwmchip_set_drvdata(chip, data);
> +
> +	ret = devm_pwmchip_add(dev, chip);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Could not add PWM chip\n");
> +
> +

s/\n\n/\n/

> +	return 0;
> +}
> +
>  int nct6775_probe(struct device *dev, struct nct6775_data *data,
>  		  const struct regmap_config *regmapcfg)
>  {

Best regards
Uwe

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