On Tue, Jul 21, 2020 at 9:20 PM Nishant MalpaniOkay. Will fix in v3.
<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: ...
Sure, makes it very precise. Will address in v3.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 ?
Yes.+};
+
+/*
+ * 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.
'ret' will not be initialized if a successful spi_w8r16() takes place i.e if it doesn't go into the 'if' block. (Doesn't make sense to have it now since the below block of code is erroneous, but will need this in v3).+};
...
+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?
Oops, you're right. Even though spi_w8r16() returns a negative error code on failure, 'temp' is declared unsigned. Thanks for pointing out. Will fix in v3.+ u16 temp;
+
+ mutex_lock(&st->lock);
+ temp = spi_w8r16(st->spi, cmd);
+ if (temp < 0) {
How can this ever happen?
Sure.+ ret = temp;
+ goto err_unlock;
+ }
+
+ *val = temp;
+
+err_unlock:
+ mutex_unlock(&st->lock);
+ return ret;
+}
Ditto for the rest of the similar cases.
...Haha, will add comments in v3!
+ case IIO_CHAN_INFO_SCALE:
+ switch (chan->type) {
+ case IIO_ANGL_VEL:
+ *val = 0;
+ *val2 = 87266;
Magic!
Will add comments in v3.+ return IIO_VAL_INT_PLUS_NANO;
+ case IIO_TEMP:
+ *val = 100;
Magic!
adxrs290_lpf_3db_freq_tbl is of type (int *)[2], right? Without the casting, an incompatible-pointer-type error is thrown.+ return IIO_VAL_INT;
+ default:
+ return -EINVAL;
+ }
...
+ *vals = (const int *)adxrs290_lpf_3db_freq_tbl;
Why casting?
...See above comment.
+ *vals = (const int *)adxrs290_hpf_3db_freq_tbl;
Ditto.
...Okay, will do so in v3.
+ struct iio_dev *indio_dev;
+ struct adxrs290_state *st;
+ int ret;
+ u8 val, val2;
Swap them to have in reversed spruce tree order.
...Oh, right (I'm aware of Alexandru's recent patch on this). Will address in v3.
+ indio_dev->dev.parent = &spi->dev;
Do you need this?
I referred Documentation/timers/timers-howto.rst for 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?
...Okay. Will remove in v3.
+static const struct of_device_id adxrs290_of_match[] = {
+ { .compatible = "adi,adxrs290" },
+ { },
No comma here!
+};