Re: [PATCH v2 4/4] iio: magnetometer: ak8975: modernize polling loops with iopoll() macros

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Tue, Apr 21, 2026 at 10:07:22AM +0000, Joshua Crofts wrote:
> The driver currently uses while loops and msleep() for polling during
> conversion waits.
> 
> Replace the custom polling loops with readx_poll_timeout() and
> read_poll_timeout() macros from <linux/iopoll.h>. This reduces
> boilerplate, standardizes timeout handling and improves overall code
> readability, keeping the original timing and error behaviour.

...

>  static int wait_conversion_complete_gpio(struct ak8975_data *data)
>  {
>  	struct i2c_client *client = data->client;
> -	u32 timeout_ms = AK8975_MAX_CONVERSION_TIMEOUT;
>  	int ret;
> +	int val;
>  
>  	/* Wait for the conversion to complete. */
> -	while (timeout_ms) {
> -		msleep(AK8975_CONVERSION_DONE_POLL_TIME);
> -		if (gpiod_get_value(data->eoc_gpiod))
> -			break;
> -		timeout_ms -= AK8975_CONVERSION_DONE_POLL_TIME;
> -	}
> -	if (!timeout_ms) {
> +	ret = readx_poll_timeout(gpiod_get_value, data->eoc_gpiod, val,
> +				 val != 0,
> +				 AK8975_CONVERSION_DONE_POLL_TIME * USEC_PER_MSEC,
> +				 AK8975_MAX_CONVERSION_TIMEOUT * USEC_PER_MSEC);
> +	if (ret) {
>  		dev_err(&client->dev, "Conversion timeout happened\n");
>  		return -EINVAL;

Side note: I would drop this message and return the correct error code
(in a separate patch):
	if (ret)
		return ret;

>  	}

...

>  static int wait_conversion_complete_polled(struct ak8975_data *data)
>  {
>  	struct i2c_client *client = data->client;
> -	u8 read_status;
> -	u32 timeout_ms = AK8975_MAX_CONVERSION_TIMEOUT;
>  	int ret;
> +	int val;
>  
>  	/* Wait for the conversion to complete. */
> -	while (timeout_ms) {
> -		msleep(AK8975_CONVERSION_DONE_POLL_TIME);
> -		ret = i2c_smbus_read_byte_data(client,
> -					       data->def->ctrl_regs[ST1]);
> -		if (ret < 0) {
> -			dev_err(&client->dev, "Error in reading ST1\n");
> -			return ret;
> -		}
> -		read_status = ret;
> -		if (read_status)
> -			break;
> -		timeout_ms -= AK8975_CONVERSION_DONE_POLL_TIME;
> +	ret = read_poll_timeout(i2c_smbus_read_byte_data, val, val != 0,
> +				AK8975_CONVERSION_DONE_POLL_TIME * USEC_PER_MSEC,
> +				AK8975_MAX_CONVERSION_TIMEOUT * USEC_PER_MSEC,
> +				true,
> +				client, data->def->ctrl_regs[ST1]);
> +	if (val < 0) {
> +		dev_err(&client->dev, "Error in reading ST1\n");
> +		return val;
>  	}
> -	if (!timeout_ms) {

> +

This blank line is not needed as the check coupled with the call
(yes, I understand the need to check the val first).

> +	if (ret == -ETIMEDOUT) {
>  		dev_err(&client->dev, "Conversion timeout happened\n");
>  		return -EINVAL;
>  	}

This is ugly error code shadowing. See above. Preparatory patch, please,
to avoid need of this piece of code altogether.

> -	return read_status;
> +	return val;
>  }
> 

-- 
With Best Regards,
Andy Shevchenko