From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.kernel.org ([198.145.29.99]:52220 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758503AbeD1RMi (ORCPT ); Sat, 28 Apr 2018 13:12:38 -0400 Date: Sat, 28 Apr 2018 18:12:31 +0100 From: Jonathan Cameron To: David Veenstra Cc: lars@metafoo.de, pmeerw@pmeerw.net, robh+dt@kernel.org, Michael.Hennerich@analog.com, knaack.h@gmx.de, linux-iio@vger.kernel.org, daniel.baluta@nxp.com, devel@driverdev.osuosl.org, devicetree@vger.kernel.org Subject: Re: [PATCH v3 2/9] staging: iio: ad2s1200: Improve readability with be16_to_cpup Message-ID: <20180428181231.1548a82e@archlinux> In-Reply-To: <8e171d819a0916d7a4870bf70cfeada8ba9ebdc6.1524432236.git.davidjulianveenstra@gmail.com> References: <8e171d819a0916d7a4870bf70cfeada8ba9ebdc6.1524432236.git.davidjulianveenstra@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: linux-iio-owner@vger.kernel.org List-Id: linux-iio@vger.kernel.org On Mon, 23 Apr 2018 00:03:03 +0200 David Veenstra wrote: > The manual states that the data is contained in the upper 12 bits > of the 16 bits read by spi. The code that extracts these 12 bits > is correct for both be and le machines, but this is not clear > from a first glance. > > To improve readability the relevant expressions are replaced > with equivalent expressions that use be16_to_cpup. > > Signed-off-by: David Veenstra Looks good to me. Applied to the togreg branch of iio.git and pushed out as testing for the autobuilders to play with it. Thanks, Jonathan > --- > Changes in v3: > - change type of rx to __be16. > - remove unneeded variable vel. > - remove unneeded type cast to s16 (patch line 79). > > drivers/staging/iio/resolver/ad2s1200.c | 11 ++++------- > 1 file changed, 4 insertions(+), 7 deletions(-) > > diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c > index 17d65cb437fd..998f458dc1bd 100644 > --- a/drivers/staging/iio/resolver/ad2s1200.c > +++ b/drivers/staging/iio/resolver/ad2s1200.c > @@ -47,7 +47,7 @@ struct ad2s1200_state { > struct spi_device *sdev; > int sample; > int rdvel; > - u8 rx[2] ____cacheline_aligned; > + __be16 rx ____cacheline_aligned; > }; > > static int ad2s1200_read_raw(struct iio_dev *indio_dev, > @@ -58,7 +58,6 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev, > { > struct ad2s1200_state *st = iio_priv(indio_dev); > int ret = 0; > - s16 vel; > > mutex_lock(&st->lock); > gpio_set_value(st->sample, 0); > @@ -68,7 +67,7 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev, > gpio_set_value(st->sample, 1); > gpio_set_value(st->rdvel, !!(chan->type == IIO_ANGL)); > > - ret = spi_read(st->sdev, st->rx, 2); > + ret = spi_read(st->sdev, &st->rx, 2); > if (ret < 0) { > mutex_unlock(&st->lock); > return ret; > @@ -76,12 +75,10 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev, > > switch (chan->type) { > case IIO_ANGL: > - *val = (((u16)(st->rx[0])) << 4) | ((st->rx[1] & 0xF0) >> 4); > + *val = be16_to_cpup(&st->rx) >> 4; > break; > case IIO_ANGL_VEL: > - vel = (((s16)(st->rx[0])) << 4) | ((st->rx[1] & 0xF0) >> 4); > - vel = sign_extend32(vel, 11); > - *val = vel; > + *val = sign_extend32(be16_to_cpup(&st->rx) >> 4, 11); > break; > default: > mutex_unlock(&st->lock);