Re: [PATCH 07/10] iio: frequency: ad9910: support drctl control

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] iio: ad9910: missing error rollback for integration_time on hardware failure
- [Medium] iio: ad9910: integration_time write fails if PWM lacks hardware readback
- [Medium] iio: ad9910: missing PWM disable on driver unbind causing resource leak
--

commit 3bdef9985085248f9ab75c013e273ef9d7e62a3f
Author: Rodrigo Alencar <rodrigo.alencar@analog.com>

iio: frequency: ad9910: support drctl control

This commit adds support for controlling the AD9910 digital ramp generator
slope direction using a PWM signal applied to the DRCTL pin. It exposes
integration_time attributes to set the time intervals for ramp up and ramp
down sweeps.

> diff --git a/drivers/iio/frequency/ad9910.c b/drivers/iio/frequency/ad9910.c
> index dc13986dff3db..3d517879dee39 100644
> --- a/drivers/iio/frequency/ad9910.c
> +++ b/drivers/iio/frequency/ad9910.c
[ ... ]
> @@ -609,6 +614,66 @@ static int ad9910_sw_powerdown_set(struct ad9910_state *st, bool enable)
[ ... ]
> +static int ad9910_drctl_sync(struct ad9910_state *st)
> +{
[ ... ]
> +	state.enabled = true;
> +	ret = pwm_apply_might_sleep(st->drctl_pwm, &state);
> +	if (ret)
> +		return ret;
> +
> +	ret = pwm_get_state_hw(st->drctl_pwm, &state);
> +	if (ret)
> +		return ret;

[Severity: Medium]
Will this cause spurious write failures on systems where the PWM driver
lacks hardware state readback support? If the underlying PWM driver does
not implement .get_state or .read_waveform, pwm_get_state_hw() returns
-EOPNOTSUPP. This forces ad9910_drctl_sync() to abort and return an
error, despite the PWM configuration having been successfully applied
via pwm_apply_might_sleep() right before this.

[ ... ]
> @@ -1637,6 +1726,26 @@ static int ad9910_write_raw(struct iio_dev *indio_dev,
[ ... ]
> +	case IIO_CHAN_INFO_INT_TIME: {
> +		s64 val64 = iio_val_s64_compose(val, val2);
> +
> +		if (!st->drctl_pwm)
> +			return -EOPNOTSUPP;
> +		if (val64 < 0)
> +			return -EINVAL;
> +
> +		switch (chan->channel) {
> +		case AD9910_CHANNEL_DRG_RAMP_UP:
> +			st->ramp_up_time_ns = val64;
> +			break;
> +		case AD9910_CHANNEL_DRG_RAMP_DOWN:
> +			st->ramp_down_time_ns = val64;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		return ad9910_drctl_sync(st);
> +	}

[Severity: Medium]
Is there a risk of state corruption here if the hardware sync fails? The
driver updates the cached software state (ramp_up_time_ns or
ramp_down_time_ns) immediately with the user-provided value before applying
it to the hardware via ad9910_drctl_sync().

If the underlying PWM provider rejects the configuration or lacks state
readback, ad9910_drctl_sync() returns an error. This error is propagated
to userspace, but the old integration time value is never restored in the
software cache.

[ ... ]
> @@ -2249,6 +2360,15 @@ static int ad9910_probe(struct spi_device *spi)
[ ... ]
>  
> +	st->drctl_pwm = devm_pwm_get(dev, "drctl");
> +	if (IS_ERR(st->drctl_pwm)) {
> +		ret = PTR_ERR(st->drctl_pwm);
> +		if (ret != -ENOENT && ret != -ENODEV)
> +			return dev_err_probe(dev, PTR_ERR(st->drctl_pwm),
> +					     "failed to get drctl pwm\n");
> +		st->drctl_pwm = NULL;
> +	}
> +

[Severity: Medium]
Does this leak the active PWM hardware state on driver unbind? The PWM is
acquired here using devm_pwm_get(), and enabled during integration_time
writes via pwm_apply_might_sleep() inside ad9910_drctl_sync().

However, there doesn't appear to be any corresponding devm_add_action()
registered, or a .remove callback, to ensure pwm_disable() is called.
When the driver unbinds, devres will automatically release the PWM reference
via pwm_put(), but pwm_put() does not disable active PWM hardware. This
might leave the PWM signal toggling indefinitely after the AD9910 is
powered down.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-ad9910-backend-support-v1-0-3c21648c8e26@analog.com?part=7