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: <20210420132333.99886-1-tomas.melin@vaisala.com> <20210420132333.99886-3-tomas.melin@vaisala.com>
In-Reply-To: <20210420132333.99886-3-tomas.melin@vaisala.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Fri, 23 Apr 2021 19:06:30 +0300
Message-ID: <CAHp75Ve2AEA8yPw5qN+R=K=ovaO8vX53hYU9=knjY_Z+EHDdww@mail.gmail.com>
Subject: Re: [PATCH v4 2/2] iio: accel: Add driver for Murata SCA3300 accelerometer
To:     Tomas Melin <tomas.melin@vaisala.com>
Cc:     Jonathan Cameron <jic23@kernel.org>,
        devicetree <devicetree@vger.kernel.org>,
        linux-iio <linux-iio@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Apr 20, 2021 at 4:24 PM Tomas Melin <tomas.melin@vaisala.com> wrote:
>
> Add initial support for Murata SCA3300 3-axis industrial
> accelerometer with digital SPI interface. This device also
> provides a temperature measurement.

Thanks for an update, my comments below.

They can be addressed as followups, but I think regmap API can be
considered right now.

...

> +static int sca3300_read_reg(struct sca3300_data *sca_data, u8 reg, int *val)
> +{
> +       int ret;
> +
> +       mutex_lock(&sca_data->lock);
> +       sca_data->txbuf[0] = reg << 2;
> +       ret = sca3300_transfer(sca_data, val);
> +       mutex_unlock(&sca_data->lock);
> +       if (ret != -EINVAL)
> +               return ret;
> +
> +       return sca3300_error_handler(sca_data);
> +}
> +
> +static int sca3300_write_reg(struct sca3300_data *sca_data, u8 reg, int val)
> +{
> +       int reg_val = 0;
> +       int ret;
> +
> +       mutex_lock(&sca_data->lock);
> +       /* BIT(7) for write operation */
> +       sca_data->txbuf[0] = BIT(7) | (reg << 2);
> +       put_unaligned_be16(val, &sca_data->txbuf[1]);
> +       ret = sca3300_transfer(sca_data, &reg_val);
> +       mutex_unlock(&sca_data->lock);
> +       if (ret != -EINVAL)
> +               return ret;
> +
> +       return sca3300_error_handler(sca_data);
> +}

Okay, BIT(7) for write/read is pretty much standard stuff for such
sensors. If you transform your driver to use REGMAP_SPI, you will get
it thru regmap configuration. Also, you will get a locking there, in
case you don't need to have several I/O in a row atomically.

..

> +       for_each_set_bit(bit, indio_dev->active_scan_mask,
> +                        indio_dev->masklength) {

One line?

> +               ret = sca3300_read_reg(data, sca3300_channels[bit].address,
> +                                      &val);
> +               if (ret) {
> +                       dev_err_ratelimited(&data->spi->dev,
> +                               "failed to read register, error: %d\n", ret);
> +                       /* handled, but bailing out due to errors */
> +                       goto out;
> +               }
> +               data->scan.channels[i++] = val;
> +       }

...

> +       int ret;
> +       int value = 0;

Reversed xmas tree ordering?

...

> +       /*
> +        * Wait 1ms after SW-reset command.
> +        * Wait 15ms for settling of signal paths.
> +        */
> +       usleep_range(16e3, 50e3);

Hmm... Perhaps re-use msleep_range()
https://elixir.bootlin.com/linux/latest/source/drivers/media/i2c/imx274.c#L601?

...

> +       .debugfs_reg_access = &sca3300_debugfs_reg_access,

Reading of the registers you will get as a bonus when switching over
to regmap SPI API.

-- 
With Best Regards,
Andy Shevchenko