On Sun, Nov 29, 2020 at 11:57 PM Linus Walleij <linus.walleij@xxxxxxxxxx> wrote: > > This adds an IIO magnetometer driver for the Yamaha > YAS530 family of magnetometer/compass chips YAS530, > YAS532 and YAS533. > > 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 > patching this one will probably be reasonable. > > The source code for Samsung Galaxy A7's YAS539 is not > that significantly different from the YAS530 in the that is significantly > 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. May I comment? :-) Okay my (mostly minor) comments below. ... > +#include <linux/module.h> > +#include <linux/mod_devicetable.h> > +#include <linux/i2c.h> > +#include <linux/err.h> > +#include <linux/mutex.h> > +#include <linux/delay.h> > +#include <linux/bitops.h> > +#include <linux/random.h> > +#include <linux/regmap.h> > +#include <linux/unaligned/be_byteshift.h> > +#include <linux/bitfield.h> > +#include <linux/regulator/consumer.h> > +#include <linux/gpio/consumer.h> > +#include <linux/pm_runtime.h> Perhaps sorted? > +#include <linux/iio/iio.h> > +#include <linux/iio/buffer.h> > +#include <linux/iio/trigger_consumer.h> > +#include <linux/iio/triggered_buffer.h> Ditto? ... > +#define YAS5XX_OFFSET_X 0x85 /* +/-31 */ > +#define YAS5XX_OFFSET_Y1 0x86 /* +/-31 */ > +#define YAS5XX_OFFSET_Y2 0x87 /* +/-31 */ What comments mean? Range of valid values? Perhaps [-31..31] I explain why I'm commenting on this. My confusion becomes from my first thought was that this is about register offsets. But then I have read the name and assumed above. And still not sure that it is a right assumption. ... > +#define YAS530_DATA_CENTER 2048 > +#define YAS530_DATA_OVERFLOW 4095 Here... > +#define YAS532_DATA_CENTER 4096 > +#define YAS532_DATA_OVERFLOW 8191 ...and here sound like values are depend to the register bits, so, I would perhaps right them in a such way (or make a comment) ... > +/* Turn off device regulators etc after 5 seconds of inactivity */ > +#define YAS5XX_AUTOSUSPEND_DELAY 5000 _MS ... > + > + One blank line is usually enough. ... > + val = get_unaligned_be16(&data[0]); > + val = get_unaligned_be16(&data[0]); Briefly looking I don't see other calls, but here no need to use get_unaligned_*(), rather be16_to_cpu{p}/cpu_to_be16{p}. ... > +static int yas5xx_busy_wait(struct yas5xx *yas5xx) > +{ > + int maxloops = 1000; > + unsigned int val; > + int ret; > + > + while (maxloops) { > + ret = regmap_read(yas5xx->map, YAS5XX_MEASURE_DATA, &val); > + if (ret) > + return ret; > + > + if (!(val & YAS5XX_MEASURE_DATA_BUSY)) > + return 0; > + > + maxloops--; > + } > + > + return -ETIMEDOUT; > +} Reimplementation of regmap_read_poll_timeout() ? ... > +static const struct iio_chan_spec_ext_info yas5xx_ext_info[] = { > + IIO_MOUNT_MATRIX(IIO_SHARED_BY_DIR, yas5xx_get_mount_matrix), > + { }, No need for comma. > +}; ... > +#define YAS5XX_AXIS_CHANNEL(axis, index) \ > + { \ > + .type = IIO_MAGN, \ > + .modified = 1, \ > + .channel2 = IIO_MOD_##axis, \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ > + BIT(IIO_CHAN_INFO_SCALE), \ > + .ext_info = yas5xx_ext_info, \ > + .address = index, \ > + .scan_index = index, \ > + .scan_type = { \ > + .sign = 's', \ > + .realbits = 32, \ > + .storagebits = 32, \ > + .endianness = IIO_CPU \ ...but here I would rather leave it. > + }, \ > + } ... > + .endianness = IIO_CPU Ditto. ... > +static const unsigned long yas5xx_scan_masks[] = { 0xf, 0 }; GENMASK()? Or is it plain value? ... > +static const struct regmap_config yas5xx_regmap_config = { > + .reg_bits = 8, > + .val_bits = 8, > + .max_register = 0xff, > + .volatile_reg = yas5xx_volatile_reg, > +}; You have mutex in the code and another lock by regmap core. Do you need both? ... > + /* Sanity check */ > + for (i = 0; i < 13; i++) { > + if (data[i] != 0) > + break; > + } memchr_inv() ... > + if (i == 13 && !(data[13] & BIT(7))) > + dev_err(yas5xx->dev, "calibration is blank!\n"); And we continue... Shouldn't we bail out or decrease level to warning? ... > + yas5xx->version = data[13] & 0x01; BIT(0) ? ... > + indio_dev = devm_iio_device_alloc(dev, sizeof(*yas5xx)); > + if (indio_dev == NULL) !indio_dev ? > + return -ENOMEM; ... > + if (yas5xx->reset) If it's optional we don't need this check, otherwise we don't need it either. Or I missed something? > + gpiod_set_value_cansleep(yas5xx->reset, 1); ... > + if (yas5xx->reset) Ditto. > + gpiod_set_value_cansleep(yas5xx->reset, 1); ... > + if (yas5xx->reset) > + gpiod_set_value_cansleep(yas5xx->reset, 0); Ditto. ... > + if (yas5xx->reset) > + gpiod_set_value_cansleep(yas5xx->reset, 1); Ditto. -- With Best Regards, Andy Shevchenko
