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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] iio: frequency: ad9910: ad9910_probe() fails when optional drctl PWM is absent
--

commit 5605b6a5e6a1ded9bd90cf8a046c13f843a2865d
Author: Rodrigo Alencar <rodrigo.alencar@analog.com>

iio: frequency: ad9910: support drctl control

This commit adds optional PWM control for the AD9910 digital ramp
generator DRCTL pin and exposes the integration_time attribute on the DRG
ramp up and ramp down channels using the PWM waveform API.

> diff --git a/drivers/iio/frequency/ad9910.c b/drivers/iio/frequency/ad9910.c
> index dc13986dff3db..1de02017f78c6 100644
> --- a/drivers/iio/frequency/ad9910.c
> +++ b/drivers/iio/frequency/ad9910.c

[ ... ]

> @@ -2249,6 +2365,21 @@ static int ad9910_probe(struct spi_device *spi)
>  
>  	fsleep(AD9910_WAKEUP_DELAY_us);
>  
> +	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;
> +	} else {

[Severity: High]
Will ad9910_probe() fail on Device Tree systems when the optional "drctl"
PWM is omitted from the device node?

On Device Tree platforms, the call chain is:

  ad9910_probe()
    -> devm_pwm_get(dev, "drctl")
      -> pwm_get(dev, "drctl")
        -> of_pwm_get(dev, np, "drctl")
          -> of_property_match_string(np, "pwm-names", "drctl")

Because a non-NULL con_id ("drctl") is passed, of_pwm_get() in
drivers/pwm/core.c resolves the name before looking up the "pwms"
property:

	if (con_id) {
		index = of_property_match_string(np, "pwm-names", con_id);
		if (index < 0)
			return ERR_PTR(index);
	}

In drivers/of/property.c, of_property_match_string() returns -EINVAL when
the "pwm-names" property is absent, or -ENODATA when the string is not
found:

	const struct property *prop = of_find_property(np, propname, NULL);
	...
	if (!prop)
		return -EINVAL;
	if (!prop->value)
		return -ENODATA;

Since ad9910_probe() only ignores -ENOENT and -ENODEV, won't
ERR_PTR(-EINVAL) or ERR_PTR(-ENODATA) cause probe to fail instead of
treating the "drctl" PWM as absent?

> +		ret = devm_add_action_or_reset(dev, ad9910_drctl_disable_action,
> +					       st->drctl_pwm);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "failed to add drctl disable action\n");
> +	}
> +
>  	ret = ad9910_parse_fw(st);
>  	if (ret)
>  		return ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-ad9910-backend-support-v2-0-0ff7f0bff326@analog.com?part=7