Messages in this thread | | | Subject | Re: [PATCH v4 2/2] iio: accel: Add driver for Murata SCA3300 accelerometer | From | Tomas Melin <> | Date | Mon, 26 Apr 2021 06:53:38 +0300 |
| |
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, ®_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&data=04%7C01%7Ctomas.melin%40vaisala.com%7C3b7eaa3e7fd748ed89f408d9071771d9%7C6d7393e041f54c2e9b124c2be5da5c57%7C0%7C0%7C637548619567203976%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=2aiEKH6Ua6NI2dIv9TfraxaiCbSP4FY%2BKEaPv4bsfwY%3D&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. >>
| |