| Message ID | 20201026175340.19570-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] iio: gyro: mpu3050: Use get_unaligned_leXX() | expand |
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
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); >
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.
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
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.
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);
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(-)