Re: [PATCH 2/2 v2] iio: magnetometer: Add driver for Yamaha YAS5xx

[Linus Walleij <linus.walleij@linaro.org>]

Hi Jonathan!

I fixed most things, some elaboration inline:

On Sat, Nov 28, 2020 at 1:21 PM Jonathan Cameron <jic23@kernel.org> wrote:

> The 'interesting' nature of the packing in those calibration registers
> is unfortunate.  I'm not really sure how we make that code more readable
> but using overlapping unaligned big endian reads definitely doesn't make
> it easier for me to follow!

Yeah I am experimenting to find a good way. Maybe I will try to extract
u64 and chop out the bits from there to make it clearer.

I think a lot of clock drivers have this problem already.

> > +       This driver can also be compiled as a module.
> > +       To compile this driver as a module, choose M here: the module
> > +       will be called yamaha-yas.
> > +

> One line only

Please clarify what you want the end result to look like, I don't
quite get it... Looking a the latest committed drivers in linux-next
didn't give a clue either.

> > + * YAS530 MS-3E (2011 Samsung Galaxy S Advance)
> > + * YAS532 MS-3R (2011 Samsung Galaxy S4)
> > + * YAS533 MS-3F (Vivo 1633, 1707, V3, Y21L)
> > + * (YAS534 is a magnetic switch, not handled)
>
> And there is your reason not to use wild cards in the dt file name etc!

It's pretty safe as we definately account for all magnetometers
of the naming scheme YAS53x. The YAS534 would be in another
subsystem (proximity, I guess) so the path to the file gives the
right info: this covers all magnetometers named yas53x, goes
for both the bindings and the driver I think?

> > +static void yas53x_extract_calibration(u8 *data, struct yas5xx_calibration *c)
> > +{
>
> As below.  I'm not sure how you make this clearer, but right now it's
> really hard to follow.  Perhaps detailed comment on what the data packing
> in here is?

OK I try to detail a bit.

> > +     c->a2 = yas5xx_extract_bits(&data[3], 10, 15) - 32;
> > +     c->a3 = yas5xx_extract_bits(&data[3], 6, 9) - 8;
> > +     c->a4 = yas5xx_extract_bits(&data[4], 8, 13) - 32;
> > +     c->a5 = yas5xx_extract_bits(&data[5], 10, 15) + 38;
> > +     c->a6 = yas5xx_extract_bits(&data[5], 4, 9) - 32;
> > +     c->a7 = yas5xx_extract_bits(&data[6], 5, 11) - 64;
> > +     c->a8 = yas5xx_extract_bits(&data[7], 7, 12) - 32;
> > +     c->a9 = yas5xx_extract_bits(&data[8], 7, 14);
> > +     c->k = yas5xx_extract_bits(&data[9], 10, 14) + 10;

Since this segment easily fits in a u64 I thought about
extracing a u64 and then use FIELD_GET on that.

> As below, I wonder if you'd be better just constructing these directly from
> the 8 bit register pairs. The overlapping unaligned 16 bit reads are
> rather confusing.

I already tried that but it looks even worse :/

I will try to put them into a bigger word instead.

> > +     /* Extract linearization */
>
> Could you add a comment on the data layout here. It seems rather
> non obvious given the overlapping 16 bit reads you are doing.
> Whilst the maths might correspond to extracting bits from be16 registers
> that looks more like coincidence than design and only makes any
> useful difference to the bit 7,8 pair.  The bit 9-14 could just have
> been gotten from the next 8 bit register directly.

It sadly doesn't look any better if I operate on u8 chars. :/
I'll try to think of something.

> > +             snprintf(yas5xx->name, sizeof(yas5xx->name),
> > +                      "YAS530 MS-3E %s",
>
> Name tends to be lowercase. Also, not sure we've had one with spaces
> in it before.  I guess our userspace code is fine with it though
> as presumably you've tested that.

Yeah oddly it turns it into just "YAS530":

skomer:/home/linus# lsiio
Device 003: bma222
Device 001: current-sense-amplifier
Device 004: gp2ap002
Device 002: mpu3050
Device 000: ab8500-gpadc
Device 005: YAS530
Trigger 000: mpu3050-dev2

But it doesn't look nice, I will rename it lowercase.

Yours,
Linus Walleij