Messages in this thread | | | From | Andy Shevchenko <> | Date | Fri, 23 Apr 2021 19:06:30 +0300 | Subject | Re: [PATCH v4 2/2] iio: accel: Add driver for Murata SCA3300 accelerometer |
| |
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.
...
> +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://elixir.bootlin.com/linux/latest/source/drivers/media/i2c/imx274.c#L601?
...
> + .debugfs_reg_access = &sca3300_debugfs_reg_access,
Reading of the registers you will get as a bonus when switching over to regmap SPI API.
-- With Best Regards, Andy Shevchenko
| |