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

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 28 Nov 2020 22:04:56 +0100
Linus Walleij <linus.walleij@linaro.org> wrote:

> 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.

Sorry - totally trivial one I should have expressed better.
Seems to be 2 blank lines after this block when one will do fine from
a readability point of view.

> 
> > > + * 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?

I'd just name it after a specific part.

> 
> > > +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.

Could do, but to a degree what makes sense here is dependent on what
aligns well with the datasheet as that's what people will check against.
Is there a public datasheet with this in?


> 
> > 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.

Cool.

> 
> Yours,
> Linus Walleij