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

From: "Michał Mirosław" <mirq-linux@rere.qmqm.pl>
To: Linus Walleij <linus.walleij@linaro.org>
Cc: Jonathan Cameron <jic23@kernel.org>,
	linux-iio@vger.kernel.org, Hartmut Knaack <knaack.h@gmx.de>,
	Lars-Peter Clausen <lars@metafoo.de>,
	Peter Meerwald-Stadler <pmeerw@pmeerw.net>,
	linux-input@vger.kernel.org,
	Nick Reitemeyer <nick.reitemeyer@web.de>,
	Stephan Gerhold <stephan@gerhold.net>
Subject: Re: [PATCH 2/3 v3] iio: magnetometer: ak8974: Break out measurement
Date: Fri, 17 Apr 2020 16:02:46 +0200	[thread overview]
Message-ID: <20200417140246.GA18337@qmqm.qmqm.pl> (raw)
In-Reply-To: <20200417114020.31291-2-linus.walleij@linaro.org>

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>