Re: [PATCH v2 1/6] iio: imu: adis: Add optional self_test callback and fix custom reset dispatch

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] iio: imu: adis: adis_reset() API bypasses custom reset operations
--

commit 94d1ab88cd933c9c52f4aca4a47015c8c3d32cd3
Author: Radu Sabau <radu.sabau@analog.com>

    iio: imu: adis: Add optional self_test callback and fix custom reset dispatch
    
    Add an optional self_test callback to struct adis_data, for devices
    whose self-test doesn't fit the generic mask-write/sleep/check-status
    sequence. Purely additive: defaults to NULL, no change for existing
    drivers.
    
    Also fix __adis_initial_startup()'s no-reset-gpio path: it called
    __adis_reset() directly instead of adis->ops->reset(), silently
    ignoring any driver's custom reset implementation. Invisible until now
    since adis_default_ops.reset is __adis_reset() itself. Dispatch
    through adis->ops->reset() instead, adding the settle-time sleep at
    the call site for custom ops (documented not to sleep internally)
    while skipping it for the default op (which already sleeps
    internally).

> diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
> index a2bc1d14ed91d..306777526a118 100644
> --- a/drivers/iio/imu/adis.c
> +++ b/drivers/iio/imu/adis.c

[ ... ]

> @@ -413,7 +422,20 @@ int __adis_initial_startup(struct adis *adis)
>  		gpiod_set_value_cansleep(gpio, 0);
>  		msleep(timeouts->reset_ms);
>  	} else {
> -		ret = __adis_reset(adis);
> +		ret = adis->ops->reset(adis);

[Severity: Low]
While this correctly fixes the dispatch here, does the public adis_reset()
API function in include/linux/iio/imu/adis.h also need to be updated?

It currently still bypasses the custom operations by hardcoding a direct call
to __adis_reset(adis):

include/linux/iio/imu/adis.h:adis_reset() {
	guard(mutex)(&adis->state_lock);
	return __adis_reset(adis);
}

If a driver utilizing a custom reset callback attempts to use adis_reset()
(e.g., during error recovery), it would still bypass the custom logic and
write directly to the default global command register.

For a device with a custom reset, like adis16550, this issues an 8-bit SPI
write (ADIS_GLOB_CMD_SW_RESET) which would fail because its custom SPI
transfer handler expects 16-bit or 32-bit transfers.

While it appears no in-tree drivers with custom operations currently invoke
this wrapper, it seems leaving it this way breaks the API contract established
by this patch.

> +		if (ret)
> +			return ret;
> +		/*
> +		 * The default reset op (__adis_reset()) already sleeps the
> +		 * settle time internally. Custom reset ops are documented
> +		 * (struct adis_ops::reset) not to, so sleep here for them.
> +		 */
> +		if (adis->ops != &adis_default_ops)
> +			msleep(timeouts->sw_reset_ms);
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008-adis16607-v2-0-7c94df80878a@analog.com?part=1