lkml.org 
[lkml]   [2021]   [Apr]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v4 2/2] iio: accel: Add driver for Murata SCA3300 accelerometer
From
Date

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

\
 
 \ /
  Last update: 2021-04-26 05:55    [W:0.063 / U:0.992 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site