Re: [PATCH v4 06/17] iio: magnetometer: ak8975: modernize polling loops with iopoll() macros

Jonathan Cameron
2026-05-04T15:06:14Z

On Mon, 04 May 2026 11:48:18 +0200
Joshua Crofts via B4 Relay <devnull+joshua.crofts1.gmail.com@kernel.org> wrote:

> From: Joshua Crofts <joshua.crofts1@gmail.com>
>
> 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. Add
> <linux/time.h> for USEC_PER_MSEC macro instead of using magic numbers.
>
> Assisted-by: Gemini:3.1-Pro
> Co-developed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>

One question inline on whether it is a good idea to be paranoid about
gpiod_get_value() potentially returning < 0 to indicate an error.
Right now that is treated as success.

> ---
> drivers/iio/magnetometer/ak8975.c | 45 +++++++++++++++++----------------------
> 1 file changed, 19 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
> index 57f50c09cca539c3733f516a1617375e9134c349..2e750c151435da57926969a63ba9fe996d774e7a 100644
> --- a/drivers/iio/magnetometer/ak8975.c
> +++ b/drivers/iio/magnetometer/ak8975.c
> @@ -23,6 +23,7 @@
> #include <linux/pm_runtime.h>
> #include <linux/property.h>
> #include <linux/regulator/consumer.h>
> +#include <linux/time.h>
> #include <linux/types.h>
> #include <linux/wait.h>
>
> @@ -652,18 +653,15 @@ static int ak8975_setup(struct i2c_client *client)
> 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)
> - return -ETIMEDOUT;
> + 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)
> + return ret;

Should we check val as well? It might be negative if gpiod_get_value()
returned an error.. Obviously the original code didn't so this would be an
improvement rather than maintaining what that was doing.

>
> ret = i2c_smbus_read_byte_data(client, data->def->ctrl_regs[ST1]);
> if (ret < 0)
> @@ -675,28 +673,23 @@ static int wait_conversion_complete_gpio(struct ak8975_data *data)
> 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 (ret)
> + return ret;
> + if (val < 0) {
> + dev_err(&client->dev, "Error in reading ST1\n");
> + return val;
> }
> - if (!timeout_ms)
> - return -ETIMEDOUT;
>
> - return read_status;
> + return val;
> }
>
> /* Returns 0 if the end of conversion interrupt occurred or -ETIMEDOUT otherwise */
>