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