Re: [PATCH v2 1/2] iio: gyro: Add driver support for ADXRS290

[Nishant Malpani <nish.malpani25@gmail.com>]

Hello Andy,

Thanks for the review. Comments inline...

On 22/07/20 1:16 am, Andy Shevchenko wrote:
> On Tue, Jul 21, 2020 at 9:20 PM Nishant Malpani
> <nish.malpani25@gmail.com> 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: ...
> 
Okay. Will fix in v3.

>> Signed-off-by: Nishant Malpani <nish.malpani25@gmail.com>
> 
> ...
> 
>> +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 ?
> 
Sure, makes it very precise. Will address in v3.

>> +};
>> +
>> +/*
>> + * 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.
> 
Yes.

>> +};
> 
> ...
> 
>> +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?
>
'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).

>> +       u16 temp;
>> +
>> +       mutex_lock(&st->lock);
>> +       temp = spi_w8r16(st->spi, cmd);
> 
>> +       if (temp < 0) {
> 
> How can this ever happen?
> 
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.

>> +               ret = temp;
>> +               goto err_unlock;
>> +       }
>> +
>> +       *val = temp;
>> +
>> +err_unlock:
>> +       mutex_unlock(&st->lock);
>> +       return ret;
>> +}
> 
> Ditto for the rest of the similar cases.
>
Sure.

> ...
> 
>> +       case IIO_CHAN_INFO_SCALE:
>> +               switch (chan->type) {
>> +               case IIO_ANGL_VEL:
>> +                       *val = 0;
> 
> 
>> +                       *val2 = 87266;
> 
> Magic!
> 
Haha, will add comments in v3!

>> +                       return IIO_VAL_INT_PLUS_NANO;
>> +               case IIO_TEMP:
> 
>> +                       *val = 100;
> 
> Magic!
> 
Will add comments in v3.

>> +                       return IIO_VAL_INT;
>> +               default:
>> +                       return -EINVAL;
>> +               }
> 
> ...
> 
>> +               *vals = (const int *)adxrs290_lpf_3db_freq_tbl;
> 
> Why casting?
> 
adxrs290_lpf_3db_freq_tbl is of type (int *)[2], right? Without the 
casting, an incompatible-pointer-type error is thrown.

> ...
> 
>> +               *vals = (const int *)adxrs290_hpf_3db_freq_tbl;
> 
> Ditto.
> 
See above comment.

> ...
> 
> 
>> +       struct iio_dev *indio_dev;
>> +       struct adxrs290_state *st;
> 
>> +       int ret;
>> +       u8 val, val2;
> 
> Swap them to have in reversed spruce tree order.
> 
Okay, will do so in v3.

> ...
> 
>> +       indio_dev->dev.parent = &spi->dev;
> 
> Do you need this?
Oh, right (I'm aware of Alexandru's recent patch on this). Will address 
in v3.

> 
>> +       /* max transition time to measurement mode */
>> +       msleep_interruptible(ADXRS290_MAX_TRANSITION_TIME_MS);
> 
> I'm not sure what the point of interruptible variant here?
> 
I referred Documentation/timers/timers-howto.rst for this.
My reasoning was shaped to use the interruptible variant because the 
transition settles in a time *less than* 100ms and since 100ms is quite 
a huge time to sleep, it should be interrupted in case a signal arrives.

> ...
> 
>> +static const struct of_device_id adxrs290_of_match[] = {
>> +       { .compatible = "adi,adxrs290" },
> 
>> +       { },
> 
> No comma here!
> 
Okay. Will remove in v3.

With regards,
Nishant Malpani

>> +};
>