On Fri, 27 Nov 2020 22:09:22 -0600 Bjorn Andersson <bjorn.andersson@xxxxxxxxxx> wrote: > On Fri 27 Nov 18:40 CST 2020, Linus Walleij wrote: > > > This adds an IIO magnetometer driver for the Yamaha > > YAS53x magnetometer/compass chips YAS530 and YAS532. > > A quick survey of the source code released by different > > vendors reveal that we have these variants in the family > > with some deployments listed: > > > > * YAS529 MS-3C (2005 Samsung Aries) > > * 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) > > * YAS535 MS-6C > > * YAS536 MS-3W > > * YAS537 MS-3T (2015 Samsung Galaxy S6, Note 5) > > * YAS539 MS-3S (2018 Samsung Galaxy A7 SM-A750FN) > > > > The YAS529 is so significantly different from the > > YAS53x variants that it will require its own driver. > > The YAS537 and YAS539 have slightly different register > > sets but have strong similarities so a common driver > > will probably be reasonable. > > > > The source code for Samsung Galaxy A7's YAS539 is not > > that significantly different from the YAS530 in the > > Galaxy S Advance, so I believe we will only need this > > one driver with quirks to handle all of them. > > > > The YAS539 is actively announced on Yamaha's devices > > site: > > https://device.yamaha.com/en/lsi/products/e_compass/ > > > > This is a driver written from scratch using buffered > > IIO and runtime PM handling regulators and reset. > > > > Looks quite nice, just spotted some small things as I was skimming > through the patch. > > > Cc: phone-devel@xxxxxxxxxxxxxxx > > Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx> > > --- > > ChangeLog v1->v3: > > - This is posted along with the DT bindings which are > > in v3 so just number everything as v3. > > $subject still says v2... > > [..] > > diff --git a/drivers/iio/magnetometer/yamaha-yas53x.c b/drivers/iio/magnetometer/yamaha-yas53x.c > [..] > > +/* On YAS532 the x, y1 and y2 values are 13 bits */ > > +static u16 yas532_extract_axis(u8 *data) > > +{ > > + u16 val; > > + > > + /* > > + * These are the bits used in a 16bit word: > > + * 15 14 13 12 11 10 9 8 7 6 5 4 3 2 1 0 > > + * x x x x x x x x x x x x x > > + */ > > + val = get_unaligned_be16(&data[0]); > > + val >>= 2; > > + val &= GENMASK(12, 0); > > Wouldn't it be easier to follow if you GENMASK out the bits you document > above, then shift them right? Even better to use FIELD_GET which does what Bjorn suggested under the hood. Thanks, Jonathan > > > + return val; > > +} > [..] > > +/** > > + * yas5xx_measure() - Make a measure from the hardware* > > + * @yas5xx: The device state > > + * @t: the raw temperature measurement > > + * @x: the raw x axis measurement > > + * @y1: the y1 axis measurement > > + * @y2: the y2 axis measurement > > * Return: > > To complete the kerneldoc. > > > + */ > > +static int yas5xx_measure(struct yas5xx *yas5xx, u16 *t, u16 *x, u16 *y1, u16 *y2) > > +{ > [..] > > +/** > > This will result in someone feeling inclined to send a patch to fix the > incomplete kerneldoc. > > So please either fill it out, or drop the second '*'. > > > + * yas5xx_get_measure() - Measure a sample of all axis and process > > + * > > + * Returned valued are in nanotesla according to some code. > > + */ > > +static int yas5xx_get_measure(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo, s32 *zo) > > +{ > [..] > > +static int __maybe_unused yas5xx_runtime_resume(struct device *dev) > > +{ > > + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev)); > > + struct yas5xx *yas5xx = iio_priv(indio_dev); > > + int ret; > > + > > + ret = regulator_bulk_enable(ARRAY_SIZE(yas5xx->regs), yas5xx->regs); > > + if (ret) { > > + dev_err(dev, "cannot enable regulators\n"); > > regulator_bulk_enable() will log which of the regs it failed ot enable, > so you can omit this. > > Regards, > Bjorn
