Re: [PATCH v4 2/2] iio: accel: Add driver for Murata SCA3300 accelerometer

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 23 Apr 2021 19:06:30 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

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

It's not a totally clear cut case of regmap making sense for this particular
device. I think you'd have do a custom regmap to support
the cs_change = 1, needed for transfers and once you are going down that
route the advantages of regmap have to be balanced against needing
custom callbacks anyway.

Without actually implementing it I'm not certain whether it would be
a good thing here or not. It should be fairly easy to try though if
Tomas wants to.

Jonathan

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