diff mbox series

[2/3,v3] iio: magnetometer: ak8974: Break out measurement

Message ID 20200417114020.31291-2-linus.walleij@linaro.org (mailing list archive)
State Superseded
Headers show
Series [1/3,v3] iio: magnetometer: ak8974: Correct realbits | expand

Commit Message

Linus Walleij April 17, 2020, 11:40 a.m. UTC
This breaks out the measurement code to its own function
so we can handle this without swirling it up with the
big switch() statement inside ak8974_read_raw().

Keep a local s16 helper variable for the signed value
coming out of the measurement before assigning it to the
integer *val. The local variable makes the code easier
to read and the compiler will optimize it if possible.

Cc: Nick Reitemeyer <nick.reitemeyer@web.de>
Cc: Stephan Gerhold <stephan@gerhold.net>
Cc: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v2->v3:
- Return directly from the raw read function, we
  need no goto:s as we got rid of the lock.
- Change the measurement function to return an int *
  after measurement and just pass *val
  to the function saving a local variable.
- Insert a comment explaining the explicit cast to
  (s16).
- Rename function as ak8974_measure_channel() so the
  name states exactly what is going on.
- Break out as a separate patch.
---
 drivers/iio/magnetometer/ak8974.c | 68 +++++++++++++++++++------------
 1 file changed, 42 insertions(+), 26 deletions(-)

Comments

Andy Shevchenko April 17, 2020, 1:17 p.m. UTC | #1
On Fri, Apr 17, 2020 at 2:42 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> This breaks out the measurement code to its own function
> so we can handle this without swirling it up with the
> big switch() statement inside ak8974_read_raw().
>
> Keep a local s16 helper variable for the signed value
> coming out of the measurement before assigning it to the
> integer *val. The local variable makes the code easier
> to read and the compiler will optimize it if possible.

> +       /*
> +        * This explicit cast to (s16) is necessary as the measurement
> +        * is done in 2's complement with positive and negative values.
> +        * The follwing assignment to *val will then convert the signed
> +        * s16 value to a signed int value.
> +        */
> +       outval = (s16)le16_to_cpu(hw_values[address]);
> +       *val = outval;

I'm wondering if you may use sign_extend32() here.
Michał Mirosław April 17, 2020, 2:02 p.m. UTC | #2
On Fri, Apr 17, 2020 at 01:40:19PM +0200, Linus Walleij wrote:
> This breaks out the measurement code to its own function
> so we can handle this without swirling it up with the
> big switch() statement inside ak8974_read_raw().
> 
> Keep a local s16 helper variable for the signed value
> coming out of the measurement before assigning it to the
> integer *val. The local variable makes the code easier
> to read and the compiler will optimize it if possible.
[...]
> +static int ak8974_measure_channel(struct ak8974 *ak8974, unsigned long address,
> +				  int *val)
> +{
> +	__le16 hw_values[3];
> +	int ret;
> +	s16 outval;
[...]
> +	/*
> +	 * This explicit cast to (s16) is necessary as the measurement
> +	 * is done in 2's complement with positive and negative values.
> +	 * The follwing assignment to *val will then convert the signed
> +	 * s16 value to a signed int value.
> +	 */
> +	outval = (s16)le16_to_cpu(hw_values[address]);
> +	*val = outval;

The intermediate 'outval' is not needed. What you describe in the
comment is a normal C integer promotion rule, so I would leave the
comment out, too. IOW, this is equivalent to:

*val = (s16)le16_to_cpu(...);

Otherwise:
Reviewed-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
diff mbox series

Patch

diff --git a/drivers/iio/magnetometer/ak8974.c b/drivers/iio/magnetometer/ak8974.c
index f22b40ef5661..b8dbea119a67 100644
--- a/drivers/iio/magnetometer/ak8974.c
+++ b/drivers/iio/magnetometer/ak8974.c
@@ -554,47 +554,63 @@  static int ak8974_detect(struct ak8974 *ak8974)
 	return 0;
 }
 
+static int ak8974_measure_channel(struct ak8974 *ak8974, unsigned long address,
+				  int *val)
+{
+	__le16 hw_values[3];
+	int ret;
+	s16 outval;
+
+	pm_runtime_get_sync(&ak8974->i2c->dev);
+	mutex_lock(&ak8974->lock);
+
+	/*
+	 * We read all axes and discard all but one, for optimized
+	 * reading, use the triggered buffer.
+	 */
+	ret = ak8974_trigmeas(ak8974);
+	if (ret)
+		goto out_unlock;
+	ret = ak8974_getresult(ak8974, hw_values);
+	if (ret)
+		goto out_unlock;
+	/*
+	 * This explicit cast to (s16) is necessary as the measurement
+	 * is done in 2's complement with positive and negative values.
+	 * The follwing assignment to *val will then convert the signed
+	 * s16 value to a signed int value.
+	 */
+	outval = (s16)le16_to_cpu(hw_values[address]);
+	*val = outval;
+out_unlock:
+	mutex_unlock(&ak8974->lock);
+	pm_runtime_mark_last_busy(&ak8974->i2c->dev);
+	pm_runtime_put_autosuspend(&ak8974->i2c->dev);
+
+	return ret;
+}
+
 static int ak8974_read_raw(struct iio_dev *indio_dev,
 			   struct iio_chan_spec const *chan,
 			   int *val, int *val2,
 			   long mask)
 {
 	struct ak8974 *ak8974 = iio_priv(indio_dev);
-	__le16 hw_values[3];
-	int ret = -EINVAL;
-
-	pm_runtime_get_sync(&ak8974->i2c->dev);
-	mutex_lock(&ak8974->lock);
+	int ret;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
 		if (chan->address > 2) {
 			dev_err(&ak8974->i2c->dev, "faulty channel address\n");
-			ret = -EIO;
-			goto out_unlock;
+			return -EIO;
 		}
-		ret = ak8974_trigmeas(ak8974);
+		ret = ak8974_measure_channel(ak8974, chan->address, val);
 		if (ret)
-			goto out_unlock;
-		ret = ak8974_getresult(ak8974, hw_values);
-		if (ret)
-			goto out_unlock;
-
-		/*
-		 * We read all axes and discard all but one, for optimized
-		 * reading, use the triggered buffer.
-		 */
-		*val = (s16)le16_to_cpu(hw_values[chan->address]);
-
-		ret = IIO_VAL_INT;
+			return ret;
+		return IIO_VAL_INT;
 	}
 
- out_unlock:
-	mutex_unlock(&ak8974->lock);
-	pm_runtime_mark_last_busy(&ak8974->i2c->dev);
-	pm_runtime_put_autosuspend(&ak8974->i2c->dev);
-
-	return ret;
+	return -EINVAL;
 }
 
 static void ak8974_fill_buffer(struct iio_dev *indio_dev)