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