diff mbox series

iio:gyro:mpu3050 Treat otp value as a __le64 and use FIELD_GET() to break up

Message ID 20201128185156.428327-1-jic23@kernel.org (mailing list archive)
State New, archived
Headers show
Series iio:gyro:mpu3050 Treat otp value as a __le64 and use FIELD_GET() to break up | expand

Commit Message

Jonathan Cameron Nov. 28, 2020, 6:51 p.m. UTC 
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Inspired by Andy Shevchenko's proposal to use get_unaligned_leXX().

The whole one time programable memory is treated as a single 64bit
little endian value.  Thus we can avoid a lot of messy handling
of fields overlapping byte boundaries by just loading and manipulating
it as an __le64 converted to a u64.  That lets us just use FIELD_GET()
and GENMASK() to extract the values desired.

Note only build tested.

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

Comments

Andy Shevchenko Nov. 28, 2020, 11:18 p.m. UTC | #1 
On Sun, Nov 29, 2020 at 12:22 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Sat, Nov 28, 2020 at 7:54 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >
> > Inspired by Andy Shevchenko's proposal to use get_unaligned_leXX().
> >
> > The whole one time programable memory is treated as a single 64bit
> > little endian value.  Thus we can avoid a lot of messy handling
> > of fields overlapping byte boundaries by just loading and manipulating
> > it as an __le64 converted to a u64.  That lets us just use FIELD_GET()
> > and GENMASK() to extract the values desired.
> >
> > Note only build tested.
> >
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Cc: Linus Walleij <linus.walleij@linaro.org>
>
> Are there any specific prerequisites? linux-next?
>
> When I apply this and try to compile for an ARMv7 target I get
> a lot of noise and an error:
>
> In file included from <command-line>:0:0:
> ../drivers/iio/gyro/mpu3050-core.c: In function ‘mpu3050_hw_init’:
> ../include/linux/bits.h:36:11: warning: right shift count is negative
> [-Wshift-count-negative]
>    (~UL(0) >> (BITS_PER_LONG - 1 - (h))))

I think GENMASK_ULL() has to be used.
Linus Walleij Nov. 29, 2020, 1:11 a.m. UTC | #2 
On Sun, Nov 29, 2020 at 12:18 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Sun, Nov 29, 2020 at 12:22 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Sat, Nov 28, 2020 at 7:54 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > >
> > > Inspired by Andy Shevchenko's proposal to use get_unaligned_leXX().
> > >
> > > The whole one time programable memory is treated as a single 64bit
> > > little endian value.  Thus we can avoid a lot of messy handling
> > > of fields overlapping byte boundaries by just loading and manipulating
> > > it as an __le64 converted to a u64.  That lets us just use FIELD_GET()
> > > and GENMASK() to extract the values desired.
> > >
> > > Note only build tested.
> > >
> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > Cc: Linus Walleij <linus.walleij@linaro.org>
> >
> > Are there any specific prerequisites? linux-next?
> >
> > When I apply this and try to compile for an ARMv7 target I get
> > a lot of noise and an error:
> >
> > In file included from <command-line>:0:0:
> > ../drivers/iio/gyro/mpu3050-core.c: In function ‘mpu3050_hw_init’:
> > ../include/linux/bits.h:36:11: warning: right shift count is negative
> > [-Wshift-count-negative]
> >    (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
>
> I think GENMASK_ULL() has to be used.

Oh indeed. It works as long as one just try to test-compile it on a 64bit
machine of course :D

Yours,
Linus Walleij
Jonathan Cameron Nov. 29, 2020, 11:28 a.m. UTC | #3 
On Sun, 29 Nov 2020 02:11:01 +0100
Linus Walleij <linus.walleij@linaro.org> wrote:

> On Sun, Nov 29, 2020 at 12:18 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Sun, Nov 29, 2020 at 12:22 AM Linus Walleij <linus.walleij@linaro.org> wrote:  
> > > On Sat, Nov 28, 2020 at 7:54 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > >  
> > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > >
> > > > Inspired by Andy Shevchenko's proposal to use get_unaligned_leXX().
> > > >
> > > > The whole one time programable memory is treated as a single 64bit
> > > > little endian value.  Thus we can avoid a lot of messy handling
> > > > of fields overlapping byte boundaries by just loading and manipulating
> > > > it as an __le64 converted to a u64.  That lets us just use FIELD_GET()
> > > > and GENMASK() to extract the values desired.
> > > >
> > > > Note only build tested.
> > > >
> > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > Cc: Linus Walleij <linus.walleij@linaro.org>  
> > >
> > > Are there any specific prerequisites? linux-next?
> > >
> > > When I apply this and try to compile for an ARMv7 target I get
> > > a lot of noise and an error:
> > >
> > > In file included from <command-line>:0:0:
> > > ../drivers/iio/gyro/mpu3050-core.c: In function ‘mpu3050_hw_init’:
> > > ../include/linux/bits.h:36:11: warning: right shift count is negative
> > > [-Wshift-count-negative]
> > >    (~UL(0) >> (BITS_PER_LONG - 1 - (h))))  
> >
> > I think GENMASK_ULL() has to be used.  
> 
> Oh indeed. It works as long as one just try to test-compile it on a 64bit
> machine of course :D

Doh.  That's me being lazy :(

Will flip them all over to GENMASK_ULL and do a 32 bit build test.
I checked that the FIELD_GET would be fine, but forgot to look at
GENMASK.

oops.

Jonathan

> 
> Yours,
> Linus Walleij
diff mbox series

Patch

diff --git a/drivers/iio/gyro/mpu3050-core.c b/drivers/iio/gyro/mpu3050-core.c
index 00e58060968c..6380fc1bdee3 100644
--- a/drivers/iio/gyro/mpu3050-core.c
+++ b/drivers/iio/gyro/mpu3050-core.c
@@ -13,6 +13,7 @@ 
  * TODO: add support for setting up the low pass 3dB frequency.
  */
 
+#include <linux/bitfield.h>
 #include <linux/bitops.h>
 #include <linux/delay.h>
 #include <linux/err.h>
@@ -784,7 +785,8 @@  static int mpu3050_read_mem(struct mpu3050 *mpu3050,
 static int mpu3050_hw_init(struct mpu3050 *mpu3050)
 {
 	int ret;
-	u8 otp[8];
+	__le64 otp_le;
+	u64 otp;
 
 	/* Reset */
 	ret = regmap_update_bits(mpu3050->map,
@@ -815,29 +817,31 @@  static int mpu3050_hw_init(struct mpu3050 *mpu3050)
 				MPU3050_MEM_USER_BANK |
 				MPU3050_MEM_OTP_BANK_0),
 			       0,
-			       sizeof(otp),
-			       otp);
+			       sizeof(otp_le),
+			       (u8 *)&otp_le);
 	if (ret)
 		return ret;
 
 	/* This is device-unique data so it goes into the entropy pool */
-	add_device_randomness(otp, sizeof(otp));
+	add_device_randomness(&otp_le, sizeof(otp_le));
+
+	otp = le64_to_cpu(otp_le);
 
 	dev_info(mpu3050->dev,
-		 "die ID: %04X, wafer ID: %02X, A lot ID: %04X, "
-		 "W lot ID: %03X, WP ID: %01X, rev ID: %02X\n",
+		 "die ID: %04lX, wafer ID: %02lX, A lot ID: %04lX, "
+		 "W lot ID: %03lX, WP ID: %01lX, rev ID: %02lX\n",
 		 /* Die ID, bits 0-12 */
-		 (otp[1] << 8 | otp[0]) & 0x1fff,
+		 FIELD_GET(GENMASK(12, 0), otp),
 		 /* Wafer ID, bits 13-17 */
-		 ((otp[2] << 8 | otp[1]) & 0x03e0) >> 5,
+		 FIELD_GET(GENMASK(17, 13), otp),
 		 /* A lot ID, bits 18-33 */
-		 ((otp[4] << 16 | otp[3] << 8 | otp[2]) & 0x3fffc) >> 2,
+		 FIELD_GET(GENMASK(33, 18), otp),
 		 /* W lot ID, bits 34-45 */
-		 ((otp[5] << 8 | otp[4]) & 0x3ffc) >> 2,
+		 FIELD_GET(GENMASK(45, 34), otp),
 		 /* WP ID, bits 47-49 */
-		 ((otp[6] << 8 | otp[5]) & 0x0380) >> 7,
+		 FIELD_GET(GENMASK(49, 47), otp),
 		 /* rev ID, bits 50-55 */
-		 otp[6] >> 2);
+		 FIELD_GET(GENMASK(55, 50), otp));
 
 	return 0;
 }