[PATCH v1] iio: gyro: mpu3050: Use get_unaligned_leXX()

[PATCH v1] iio: gyro: mpu3050: Use get_unaligned_leXX()

[Andy Shevchenko]

This makes the driver code slightly easier to read.

Cc: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/iio/gyro/mpu3050-core.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/gyro/mpu3050-core.c b/drivers/iio/gyro/mpu3050-core.c
index 00e58060968c..42b6ccb13568 100644
--- a/drivers/iio/gyro/mpu3050-core.c
+++ b/drivers/iio/gyro/mpu3050-core.c
@@ -827,15 +827,15 @@ static int mpu3050_hw_init(struct mpu3050 *mpu3050)
 		 "die ID: %04X, wafer ID: %02X, A lot ID: %04X, "
 		 "W lot ID: %03X, WP ID: %01X, rev ID: %02X\n",
 		 /* Die ID, bits 0-12 */
-		 (otp[1] << 8 | otp[0]) & 0x1fff,
+		 get_unaligned_le16(&otp[0]) & GENMASK(12, 0),
 		 /* Wafer ID, bits 13-17 */
-		 ((otp[2] << 8 | otp[1]) & 0x03e0) >> 5,
+		 (get_unaligned_le16(&otp[1]) & GENMASK(9, 5)) >> 5,
 		 /* A lot ID, bits 18-33 */
-		 ((otp[4] << 16 | otp[3] << 8 | otp[2]) & 0x3fffc) >> 2,
+		 (get_unaligned_le24(&otp[2]) & GENMASK(17, 2)) >> 2,
 		 /* W lot ID, bits 34-45 */
-		 ((otp[5] << 8 | otp[4]) & 0x3ffc) >> 2,
+		 (get_unaligned_le16(&otp[4]) & GENMASK(13, 2)) >> 2,
 		 /* WP ID, bits 47-49 */
-		 ((otp[6] << 8 | otp[5]) & 0x0380) >> 7,
+		 (get_unaligned_le16(&otp[5]) & GENMASK(9, 7)) >> 7,
 		 /* rev ID, bits 50-55 */
 		 otp[6] >> 2);
 
-- 
2.28.0

Re: [PATCH v1] iio: gyro: mpu3050: Use get_unaligned_leXX()

[Linus Walleij]

On Mon, Oct 26, 2020 at 6:53 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> This makes the driver code slightly easier to read.
>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Oh, neat bitting!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v1] iio: gyro: mpu3050: Use get_unaligned_leXX()

[Jonathan Cameron]

On Mon, 26 Oct 2020 19:53:40 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> This makes the driver code slightly easier to read.
> 
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

What you have is good, but could we do better?

Why not use an __le64 to grab the whole thing, convert it to a local
cpu endian u64 then use FIELD_GET or similar to pull out the right bits.
Would be fairly self documenting as well which is always nice.

Or am I missing something?

Jonathan


> ---
>  drivers/iio/gyro/mpu3050-core.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/gyro/mpu3050-core.c b/drivers/iio/gyro/mpu3050-core.c
> index 00e58060968c..42b6ccb13568 100644
> --- a/drivers/iio/gyro/mpu3050-core.c
> +++ b/drivers/iio/gyro/mpu3050-core.c
> @@ -827,15 +827,15 @@ static int mpu3050_hw_init(struct mpu3050 *mpu3050)
>  		 "die ID: %04X, wafer ID: %02X, A lot ID: %04X, "
>  		 "W lot ID: %03X, WP ID: %01X, rev ID: %02X\n",
>  		 /* Die ID, bits 0-12 */
> -		 (otp[1] << 8 | otp[0]) & 0x1fff,
> +		 get_unaligned_le16(&otp[0]) & GENMASK(12, 0),
>  		 /* Wafer ID, bits 13-17 */
> -		 ((otp[2] << 8 | otp[1]) & 0x03e0) >> 5,
> +		 (get_unaligned_le16(&otp[1]) & GENMASK(9, 5)) >> 5,
>  		 /* A lot ID, bits 18-33 */
> -		 ((otp[4] << 16 | otp[3] << 8 | otp[2]) & 0x3fffc) >> 2,
> +		 (get_unaligned_le24(&otp[2]) & GENMASK(17, 2)) >> 2,
>  		 /* W lot ID, bits 34-45 */
> -		 ((otp[5] << 8 | otp[4]) & 0x3ffc) >> 2,
> +		 (get_unaligned_le16(&otp[4]) & GENMASK(13, 2)) >> 2,
>  		 /* WP ID, bits 47-49 */
> -		 ((otp[6] << 8 | otp[5]) & 0x0380) >> 7,
> +		 (get_unaligned_le16(&otp[5]) & GENMASK(9, 7)) >> 7,
>  		 /* rev ID, bits 50-55 */
>  		 otp[6] >> 2);
>  

Re: [PATCH v1] iio: gyro: mpu3050: Use get_unaligned_leXX()

[Andy Shevchenko]

On Thu, Oct 29, 2020 at 5:05 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Mon, 26 Oct 2020 19:53:40 +0200
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>
> > This makes the driver code slightly easier to read.
> >
> > Cc: Linus Walleij <linus.walleij@linaro.org>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> What you have is good, but could we do better?
>
> Why not use an __le64 to grab the whole thing, convert it to a local
> cpu endian u64 then use FIELD_GET or similar to pull out the right bits.
> Would be fairly self documenting as well which is always nice.
>
> Or am I missing something?

My laziness. I shared this as a leftover from a big series of
converting a lot of drivers to get_unligned_.eXX(). If you consider
this is not good enough, just don't apply (no hard feelings :-) I will
drop it from my local branches.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1] iio: gyro: mpu3050: Use get_unaligned_leXX()

[Linus Walleij]

On Thu, Oct 29, 2020 at 6:10 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Thu, Oct 29, 2020 at 5:05 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Mon, 26 Oct 2020 19:53:40 +0200
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> >
> > > This makes the driver code slightly easier to read.
> > >
> > > Cc: Linus Walleij <linus.walleij@linaro.org>
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> >
> > What you have is good, but could we do better?
> >
> > Why not use an __le64 to grab the whole thing, convert it to a local
> > cpu endian u64 then use FIELD_GET or similar to pull out the right bits.
> > Would be fairly self documenting as well which is always nice.
> >
> > Or am I missing something?
>
> My laziness. I shared this as a leftover from a big series of
> converting a lot of drivers to get_unligned_.eXX(). If you consider
> this is not good enough, just don't apply (no hard feelings :-) I will
> drop it from my local branches.

I would suggest to apply it anyways because the kernel will look
better after than before this patch.

Possibly Jonathan could just slip in a TODO comment for this?

Yours,
Linus Walleij

Re: [PATCH v1] iio: gyro: mpu3050: Use get_unaligned_leXX()

[Andy Shevchenko]

On Thu, Nov 05, 2020 at 03:23:07PM +0100, Linus Walleij wrote:
> On Thu, Oct 29, 2020 at 6:10 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Thu, Oct 29, 2020 at 5:05 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > >
> > > On Mon, 26 Oct 2020 19:53:40 +0200
> > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > > This makes the driver code slightly easier to read.
> > > >
> > > > Cc: Linus Walleij <linus.walleij@linaro.org>
> > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > >
> > > What you have is good, but could we do better?
> > >
> > > Why not use an __le64 to grab the whole thing, convert it to a local
> > > cpu endian u64 then use FIELD_GET or similar to pull out the right bits.
> > > Would be fairly self documenting as well which is always nice.
> > >
> > > Or am I missing something?
> >
> > My laziness. I shared this as a leftover from a big series of
> > converting a lot of drivers to get_unligned_.eXX(). If you consider
> > this is not good enough, just don't apply (no hard feelings :-) I will
> > drop it from my local branches.
> 
> I would suggest to apply it anyways because the kernel will look
> better after than before this patch.
> 
> Possibly Jonathan could just slip in a TODO comment for this?

Sorry, I have already dropped, but if you convince Jonathan to apply, I agree
on whatever you propose (TODO comment as I see). I think patchwork or lore
still keeps a track of this.

-- 
With Best Regards,
Andy Shevchenko