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, ®_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.
