Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200721181926.27046-1-nish.malpani25@gmail.com>
In-Reply-To: <20200721181926.27046-1-nish.malpani25@gmail.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Tue, 21 Jul 2020 22:46:34 +0300
Message-ID: <CAHp75Vdr+Uo2uw3mzYP+LMRgp-eyi+YjG=O+wGVqyYx-+MRCaw@mail.gmail.com>
Subject: Re: [PATCH v2 1/2] iio: gyro: Add driver support for ADXRS290
To:     Nishant Malpani <nish.malpani25@gmail.com>
Cc:     Jonathan Cameron <jic23@kernel.org>,
        "Bogdan, Dragos" <dragos.bogdan@analog.com>,
        darius.berghe@analog.com,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-iio <linux-iio@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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: ...

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

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