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

[Tomas Melin <tomas.melin@vaisala.com>]

On 4/24/21 2:53 PM, Jonathan Cameron wrote:
> 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.

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