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

[Tomas Melin]

On 4/24/21 2:53 PM, Jonathan Cameron wrote:
> On Fri, 23 Apr 2021 19:06:30 +0300
> Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
>
> > On Tue, Apr 20, 2021 at 4:24 PM Tomas Melin <tomas.melin@xxxxxxxxxxx> 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.

Considered regmap, but given that it seems to require custom read/write

functions and that there is only SPI version of the device, I concluded 
that it did

not seem like a great fit for this driver.


I will implement the other changes suggested, and post a new version.

Thanks,

Tomas


> 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://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Flatest%2Fsource%2Fdrivers%2Fmedia%2Fi2c%2Fimx274.c%23L601&amp;data=04%7C01%7Ctomas.melin%40vaisala.com%7C3b7eaa3e7fd748ed89f408d9071771d9%7C6d7393e041f54c2e9b124c2be5da5c57%7C0%7C0%7C637548619567203976%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=2aiEKH6Ua6NI2dIv9TfraxaiCbSP4FY%2BKEaPv4bsfwY%3D&amp;reserved=0?
> >
> > ...
> >
> > > +       .debugfs_reg_access = &sca3300_debugfs_reg_access,
> > Reading of the registers you will get as a bonus when switching over
> > to regmap SPI API.