On Tue, Jul 21, 2020 at 9:20 PM Nishant Malpani <nish.malpani25@xxxxxxxxx> wrote: > > ADXRS290 is a high performance MEMS pitch and roll (dual-axis in-plane) > angular rate sensor (gyroscope) designed for use in stabilization > applications. It also features an internal temperature sensor and > programmable high-pass and low-pass filters. > > Add support for ADXRS290 in direct-access mode for now. > Datasheet: > Link: https://www.analog.com/media/en/technical-documentation/data-sheets/ADXRS290.pdf Drop that 'Link:' part and followed blank line, so get something like Datasheet: https://... Signed-off-by: ... > Signed-off-by: Nishant Malpani <nish.malpani25@xxxxxxxxx> ... > +config ADXRS290 > + tristate "Analog Devices ADXRS290 Dual-Axis MEMS Gyroscope SPI driver" > + depends on SPI > + help > + Say yes here to build support for Analog Devices ADXRS290 programmable > + digital output gyroscope. > + > + This driver can also be built as a module. If so, the module will be > + called adxrs290. > +enum adxrs290_mode { > + STANDBY, > + MEASUREMENT, > +}; > +struct adxrs290_state { > + struct spi_device *spi; > + /* Serialize reads and their subsequent processing */ > + struct mutex lock; > + enum adxrs290_mode mode; > + unsigned int lpf_3db_freq_idx; > + unsigned int hpf_3db_freq_idx; > +}; ... > +/* > + * Available cut-off frequencies of the low pass filter in Hz. > + * The integer part and fractional part are represented separately. > + */ > +static const int adxrs290_lpf_3db_freq_tbl[][2] = { What about adxrs290_lpf_3db_freq_hz_table ? > +}; > + > +/* > + * Available cut-off frequencies of the high pass filter in Hz. > + * The integer part and fractional part are represented separately. > + */ > +static const int adxrs290_hpf_3db_freq_tbl[][2] = { Ditto. > +}; ... > +static int adxrs290_get_rate_data(struct iio_dev *indio_dev, const u8 cmd, > + unsigned int *val) > +{ > + struct adxrs290_state *st = iio_priv(indio_dev); > + int ret = 0; Purpose of this? > + u16 temp; > + > + mutex_lock(&st->lock); > + temp = spi_w8r16(st->spi, cmd); > + if (temp < 0) { How can this ever happen? > + ret = temp; > + goto err_unlock; > + } > + > + *val = temp; > + > +err_unlock: > + mutex_unlock(&st->lock); > + return ret; > +} Ditto for the rest of the similar cases. ... > + case IIO_CHAN_INFO_SCALE: > + switch (chan->type) { > + case IIO_ANGL_VEL: > + *val = 0; > + *val2 = 87266; Magic! > + return IIO_VAL_INT_PLUS_NANO; > + case IIO_TEMP: > + *val = 100; Magic! > + return IIO_VAL_INT; > + default: > + return -EINVAL; > + } ... > + *vals = (const int *)adxrs290_lpf_3db_freq_tbl; Why casting? ... > + *vals = (const int *)adxrs290_hpf_3db_freq_tbl; Ditto. ... > + struct iio_dev *indio_dev; > + struct adxrs290_state *st; > + int ret; > + u8 val, val2; Swap them to have in reversed spruce tree order. ... > + indio_dev->dev.parent = &spi->dev; Do you need this? > + /* max transition time to measurement mode */ > + msleep_interruptible(ADXRS290_MAX_TRANSITION_TIME_MS); I'm not sure what the point of interruptible variant here? ... > +static const struct of_device_id adxrs290_of_match[] = { > + { .compatible = "adi,adxrs290" }, > + { }, No comma here! > +}; -- With Best Regards, Andy Shevchenko
