[PATCH v3 0/2] iio: accel: sca3300: Accelerometer support and binding docs

[PATCH v3 0/2] iio: accel: sca3300: Accelerometer support and binding docs

[Tomas Melin]

Patch series adds driver and dt-bindings for Murata SCA3300 accelerometer.

Device is a 3-axis Accelerometer with digital SPI interface.

Patches are tested and based on linux-next.

Changes V3:
Addressed comments from Jonathan Cameron and Andy Shevchenko
- Grouped include statements
- Revised SCA* define naming and grouped statements
- Changed to GENMASK() for masks
- Indentation fix for ACCEL_CHANNEL define
- Remove field declarations using default values
- Change to if(ret) style for checking return values
- Add defined mask value SCA3300_MASK_STATUS for return status
- Remove redundant type casts
- Clarify comment wording
- Refactor to remove variable idx and redundant else branches
- Unify sleep in device init, separate sleeps not needed since operation mode change dropped
- Drop redundant memory allocation error message in probe
- dev.parent is set by iio core, removed from driver probe
- Drop of_match_ptr()

Changes V2:
Addressed comments from Jonathan Cameron
- Add manufacturer name to dt-binding file
- Update spdx license statement for dt-bindings
- Remove murata,opmode devicetree property in favor of driver scale + frequency properties
- Add copyright year for driver
- Remove X_READ and X_WRITE defines
- Add _available sysfs attributes
- Fix errors in documentation formatting
- Use ARRAY_SIZE where applicable
- Use get/put_unaligned_be16() helpers where applicable
- Factor out error handling to separate function
- Return only negative values from transfer function
- Fix INFO_SCALE to return multiplier instead of inverted value
- Change INFO_SCALE values to be actual scale instead of mode value
- Do not provide INFO_PROCESSED values for user space
- Add error message for failed data read in irq handler
- Move trigger handler timestamping as part of pushing data to buffers
- Document startup sequence, providing reference to data sheet section
- Convert iio_triggered_buffer_setup to devm_iio_triggered_buffer_setup,
making remove() obsolete

Tomas Melin (2):
      dt-bindings: iio: accel: Add SCA3300 documentation
      iio: accel: Add driver for Murata SCA3300 accelerometer

 Documentation/devicetree/bindings/iio/accel/murata,sca3300.yaml |  44 ++++++++++++++++++
 drivers/iio/accel/Kconfig                                       |  13 ++++++
 drivers/iio/accel/Makefile                                      |   1 +
 drivers/iio/accel/sca3300.c                                     | 462 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 520 insertions(+)

[PATCH v3 1/2] dt-bindings: iio: accel: Add SCA3300 documentation

[Tomas Melin]

initial DT bindings for Murata SCA3300 Accelerometer.

Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
---
 .../bindings/iio/accel/murata,sca3300.yaml    | 44 +++++++++++++++++++
 1 file changed, 44 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/accel/murata,sca3300.yaml

diff --git a/Documentation/devicetree/bindings/iio/accel/murata,sca3300.yaml b/Documentation/devicetree/bindings/iio/accel/murata,sca3300.yaml
new file mode 100644
index 000000000000..55fd3548e3b6
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/accel/murata,sca3300.yaml
@@ -0,0 +1,44 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/accel/murata,sca3300.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Murata SCA3300 Accelerometer
+
+description: |
+  3-axis industrial accelerometer with digital SPI interface
+  https://www.murata.com/en-global/products/sensor/accel/sca3300
+
+maintainers:
+  - Tomas Melin <tomas.melin@vaisala.com>
+
+properties:
+  compatible:
+    enum:
+      - murata,sca3300
+
+  reg:
+    maxItems: 1
+
+  spi-max-frequency:
+    maximum: 8000000
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        accelerometer@0 {
+            compatible = "murata,sca3300";
+            reg = <0x0>;
+            spi-max-frequency = <4000000>;
+        };
+    };
+...
-- 
2.21.3

[PATCH v3 2/2] iio: accel: Add driver for Murata SCA3300 accelerometer

[Tomas Melin]

Add initial support for Murata SCA3300 3-axis industrial
accelerometer with digital SPI interface. This device also
provides a temperature measurement.

Datasheet: https://www.murata.com/en-global/products/sensor/accel/sca3300

Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
---
 drivers/iio/accel/Kconfig   |  13 +
 drivers/iio/accel/Makefile  |   1 +
 drivers/iio/accel/sca3300.c | 462 ++++++++++++++++++++++++++++++++++++
 3 files changed, 476 insertions(+)
 create mode 100644 drivers/iio/accel/sca3300.c

diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
index cceda3cecbcf..0dbf7b648e8a 100644
--- a/drivers/iio/accel/Kconfig
+++ b/drivers/iio/accel/Kconfig
@@ -450,6 +450,19 @@ config SCA3000
 	  To compile this driver as a module, say M here: the module will be
 	  called sca3000.
 
+config SCA3300
+	tristate "Murata SCA3300 3-Axis Accelerometer Driver"
+	depends on SPI
+	select CRC8
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
+	help
+	  Say yes here to build support for Murata SCA3300 3-Axis
+	  accelerometer.
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called sca3300.
+
 config STK8312
 	tristate "Sensortek STK8312 3-Axis Accelerometer Driver"
 	depends on I2C
diff --git a/drivers/iio/accel/Makefile b/drivers/iio/accel/Makefile
index 32cd1342a31a..4b56527a2b97 100644
--- a/drivers/iio/accel/Makefile
+++ b/drivers/iio/accel/Makefile
@@ -50,6 +50,7 @@ obj-$(CONFIG_MXC4005)		+= mxc4005.o
 obj-$(CONFIG_MXC6255)		+= mxc6255.o
 
 obj-$(CONFIG_SCA3000)		+= sca3000.o
+obj-$(CONFIG_SCA3300)		+= sca3300.o
 
 obj-$(CONFIG_STK8312)		+= stk8312.o
 obj-$(CONFIG_STK8BA50)		+= stk8ba50.o
diff --git a/drivers/iio/accel/sca3300.c b/drivers/iio/accel/sca3300.c
new file mode 100644
index 000000000000..bd291ee6e2a4
--- /dev/null
+++ b/drivers/iio/accel/sca3300.c
@@ -0,0 +1,462 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2021 Vaisala Oyj. All rights reserved.
+ */
+
+#include <linux/bitops.h>
+#include <linux/crc8.h>
+#include <linux/delay.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/spi/spi.h>
+
+#include <asm/unaligned.h>
+
+#include <linux/iio/buffer.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+
+#define SCA3300_ALIAS "sca3300"
+
+#define SCA3300_CRC8_POLYNOMIAL 0x1d
+
+#define SCA3300_MASK_STATUS	GENMASK(8, 0)
+#define SCA3300_MASK_RS_STATUS	GENMASK(1, 0)
+
+#define SCA3300_REG_MODE	0xd
+#define SCA3300_REG_SELBANK	0x1f
+#define SCA3300_REG_STATUS	0x6
+#define SCA3300_REG_WHOAMI	0x10
+
+#define SCA3300_VALUE_DEVICE_ID	0x51
+#define SCA3300_VALUE_RS_ERROR	0x3
+#define SCA3300_VALUE_SW_RESET	0x20
+
+enum sca3300_scan_indexes {
+	SCA3300_ACC_X = 0,
+	SCA3300_ACC_Y,
+	SCA3300_ACC_Z,
+	SCA3300_TEMP,
+	SCA3300_TIMESTAMP,
+};
+
+#define SCA3300_ACCEL_CHANNEL(index, reg, axis) {			\
+	.type = IIO_ACCEL,						\
+	.address = reg,							\
+	.modified = 1,							\
+	.channel2 = IIO_MOD_##axis,					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
+	.info_mask_shared_by_type =					\
+	BIT(IIO_CHAN_INFO_SCALE) |					\
+	BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),		\
+	.info_mask_shared_by_type_available =				\
+	BIT(IIO_CHAN_INFO_SCALE) |					\
+	BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),		\
+	.scan_index = index,						\
+	.scan_type = {							\
+			.sign = 's',					\
+			.realbits = 16,					\
+			.storagebits = 16,				\
+			.endianness = IIO_CPU,				\
+	},								\
+}
+
+static const struct iio_chan_spec sca3300_channels[] = {
+	SCA3300_ACCEL_CHANNEL(SCA3300_ACC_X, 0x1, X),
+	SCA3300_ACCEL_CHANNEL(SCA3300_ACC_Y, 0x2, Y),
+	SCA3300_ACCEL_CHANNEL(SCA3300_ACC_Z, 0x3, Z),
+	{
+		.type = IIO_TEMP,
+		.address = 0x5,
+		.scan_index = SCA3300_TEMP,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.scan_type = {
+			.sign = 's',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_CPU,
+		},
+	},
+	IIO_CHAN_SOFT_TIMESTAMP(4),
+};
+
+static const int sca3300_lp_freq[] = {70, 70, 70, 10};
+static const int sca3300_accel_scale[][2] = {{0, 370}, {0, 741}, {0, 185}, {0, 185}};
+
+static const unsigned long sca3300_scan_masks[] = {
+	BIT(SCA3300_ACC_X) | BIT(SCA3300_ACC_Y) | BIT(SCA3300_ACC_Z) |
+	BIT(SCA3300_TEMP),
+	0
+};
+
+/**
+ * struct sca3300_data - device data
+ * @spi: SPI device structure
+ * @lock: Data buffer lock
+ * @txbuf: Transmit buffer
+ * @rxbuf: Receive buffer
+ * @scan: Triggered buffer. Four channel 16-bit data + 64-bit timestamp
+ */
+struct sca3300_data {
+	struct spi_device *spi;
+	struct mutex lock;
+	u8 txbuf[4];
+	u8 rxbuf[4];
+	struct {
+		s16 channels[4];
+		s64 ts __aligned(sizeof(s64));
+	} scan;
+};
+
+DECLARE_CRC8_TABLE(sca3300_crc_table);
+
+static int sca3300_transfer(struct sca3300_data *sca_data, int *val)
+{
+	struct spi_delay delay = {.value = 10, .unit = SPI_DELAY_UNIT_USECS};
+	int32_t ret;
+	int rs;
+	u8 crc;
+	struct spi_transfer xfers[2] = {
+		{
+			.tx_buf = sca_data->txbuf,
+			.len = ARRAY_SIZE(sca_data->txbuf),
+			.delay = delay,
+			.cs_change = 1,
+		},
+		{
+			.rx_buf = sca_data->rxbuf,
+			.len = ARRAY_SIZE(sca_data->rxbuf),
+			.delay = delay,
+		}
+	};
+
+	/* inverted crc value as described in device data sheet */
+	crc = ~crc8(sca3300_crc_table, &sca_data->txbuf[0], 3, CRC8_INIT_VALUE);
+	sca_data->txbuf[3] = crc;
+
+	ret = spi_sync_transfer(sca_data->spi, xfers, ARRAY_SIZE(xfers));
+	if (ret) {
+		dev_err(&sca_data->spi->dev,
+			"transfer error, error: %d\n", ret);
+		return -EIO;
+	}
+
+	crc = ~crc8(sca3300_crc_table, &sca_data->rxbuf[0], 3, CRC8_INIT_VALUE);
+	if (sca_data->rxbuf[3] != crc) {
+		dev_err(&sca_data->spi->dev, "CRC checksum mismatch");
+		return -EIO;
+	}
+
+	/* get return status */
+	rs = sca_data->rxbuf[0] & SCA3300_MASK_RS_STATUS;
+	if (rs == SCA3300_VALUE_RS_ERROR)
+		ret = -EINVAL;
+
+	*val = sign_extend32(get_unaligned_be16(&sca_data->rxbuf[1]), 15);
+
+	return ret;
+}
+
+static int sca3300_error_handler(struct sca3300_data *sca_data)
+{
+	int ret;
+	int val;
+
+	mutex_lock(&sca_data->lock);
+	sca_data->txbuf[0] = 0x0 | (SCA3300_REG_STATUS << 2);
+	ret = sca3300_transfer(sca_data, &val);
+	mutex_unlock(&sca_data->lock);
+	/*
+	 * return status error is cleared after reading status register once,
+	 * expect EINVAL here
+	 */
+	if (ret != -EINVAL) {
+		dev_err(&sca_data->spi->dev,
+			"error reading device status: %d\n", ret);
+		return ret;
+	}
+
+	dev_err(&sca_data->spi->dev, "device status: 0x%lx\n",
+		(val & SCA3300_MASK_STATUS));
+
+	return 0;
+}
+
+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);
+	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);
+}
+
+static int sca3300_write_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int val, int val2, long mask)
+{
+	struct sca3300_data *data = iio_priv(indio_dev);
+	int reg_val;
+	int ret;
+	int i;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		if (val)
+			return -EINVAL;
+
+		for (i = 0; i < ARRAY_SIZE(sca3300_accel_scale); i++) {
+			if (val2 == sca3300_accel_scale[i][1])
+				return sca3300_write_reg(data, SCA3300_REG_MODE, i);
+		}
+		return -EINVAL;
+
+	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
+		ret = sca3300_read_reg(data, SCA3300_REG_MODE, &reg_val);
+		if (ret)
+			return ret;
+		/* freq. change is possible only for mode 3 and 4 */
+		if (reg_val == 2 && val == sca3300_lp_freq[3])
+			return sca3300_write_reg(data, SCA3300_REG_MODE, 3);
+		if (reg_val == 3 && val == sca3300_lp_freq[2])
+			return sca3300_write_reg(data, SCA3300_REG_MODE, 2);
+		return -EINVAL;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int sca3300_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int *val, int *val2, long mask)
+{
+	struct sca3300_data *data = iio_priv(indio_dev);
+	int ret;
+	int reg_val;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = sca3300_read_reg(data, chan->address, val);
+		if (ret)
+			return ret;
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		ret = sca3300_read_reg(data, SCA3300_REG_MODE, &reg_val);
+		if (ret)
+			return ret;
+		*val = 0;
+		*val2 = sca3300_accel_scale[reg_val][1];
+		return IIO_VAL_INT_PLUS_MICRO;
+	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
+		ret = sca3300_read_reg(data, SCA3300_REG_MODE, &reg_val);
+		if (ret)
+			return ret;
+		*val = sca3300_lp_freq[reg_val];
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static irqreturn_t sca3300_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct sca3300_data *data = iio_priv(indio_dev);
+	int bit, ret, val, i = 0;
+
+	for_each_set_bit(bit, indio_dev->active_scan_mask,
+			 indio_dev->masklength) {
+		ret = sca3300_read_reg(data, sca3300_channels[bit].address,
+				       &val);
+		if (ret) {
+			dev_err(&data->spi->dev,
+				"failed to read register, error: %d\n", ret);
+			goto out;
+		}
+		data->scan.channels[i++] = val;
+	}
+
+	iio_push_to_buffers_with_timestamp(indio_dev, &data->scan,
+					   iio_get_time_ns(indio_dev));
+out:
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
+/*
+ * sca3300_init - Device init sequence. See datasheet rev 2 section
+ * 4.2 Start-Up Sequence for details.
+ */
+static int sca3300_init(struct sca3300_data *sca_data,
+			struct iio_dev *indio_dev)
+{
+	int ret;
+	int value = 0;
+
+	ret = sca3300_write_reg(sca_data, SCA3300_REG_MODE,
+				SCA3300_VALUE_SW_RESET);
+	if (ret)
+		return ret;
+
+	/*
+	 * wait 1ms after SW-reset command
+	 * wait 15ms for settling of signal paths
+	 */
+	usleep_range(16e3, 50e3);
+	ret = sca3300_read_reg(sca_data, SCA3300_REG_WHOAMI, &value);
+	if (ret)
+		return ret;
+
+	if (value != SCA3300_VALUE_DEVICE_ID) {
+		dev_err(&sca_data->spi->dev,
+			"device id not expected value, %d != %u\n",
+			value, SCA3300_VALUE_DEVICE_ID);
+		return -ENODEV;
+	}
+	return 0;
+}
+
+static int sca3300_debugfs_reg_access(struct iio_dev *indio_dev,
+				      unsigned int reg, unsigned int writeval,
+				      unsigned int *readval)
+{
+	struct sca3300_data *data = iio_priv(indio_dev);
+	int value;
+	int ret;
+
+	if (reg > SCA3300_REG_SELBANK)
+		return -EINVAL;
+
+	if (!readval)
+		return sca3300_write_reg(data, reg, writeval);
+
+	ret = sca3300_read_reg(data, reg, &value);
+	if (ret)
+		return ret;
+
+	*readval = value;
+
+	return 0;
+}
+
+static int sca3300_read_avail(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan,
+			      const int **vals, int *type, int *length,
+			      long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		*vals = (const int *)sca3300_accel_scale;
+		*length = ARRAY_SIZE(sca3300_accel_scale) * 2 - 2;
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		return IIO_AVAIL_LIST;
+	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
+		*vals = &sca3300_lp_freq[2];
+		*length = 2;
+		*type = IIO_VAL_INT;
+		return IIO_AVAIL_LIST;
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info sca3300_info = {
+	.read_raw = sca3300_read_raw,
+	.write_raw = sca3300_write_raw,
+	.debugfs_reg_access = &sca3300_debugfs_reg_access,
+	.read_avail = sca3300_read_avail,
+};
+
+static int sca3300_probe(struct spi_device *spi)
+{
+	struct sca3300_data *sca_data;
+	struct iio_dev *indio_dev;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*sca_data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	sca_data = iio_priv(indio_dev);
+	mutex_init(&sca_data->lock);
+	sca_data->spi = spi;
+
+	crc8_populate_msb(sca3300_crc_table, SCA3300_CRC8_POLYNOMIAL);
+
+	indio_dev->info = &sca3300_info;
+	indio_dev->name = SCA3300_ALIAS;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = sca3300_channels;
+	indio_dev->num_channels = ARRAY_SIZE(sca3300_channels);
+	indio_dev->available_scan_masks = sca3300_scan_masks;
+
+	ret = sca3300_init(sca_data, indio_dev);
+	if (ret) {
+		dev_err(&spi->dev, "failed to init device, error: %d\n", ret);
+		return ret;
+	}
+
+	ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
+					      iio_pollfunc_store_time,
+					      sca3300_trigger_handler, NULL);
+	if (ret) {
+		dev_err(&spi->dev,
+			"iio triggered buffer setup failed, error: %d\n", ret);
+		return ret;
+	}
+
+	ret = devm_iio_device_register(&spi->dev, indio_dev);
+	if (ret) {
+		dev_err(&spi->dev, "iio device register failed, error: %d\n",
+			ret);
+		return ret;
+	}
+
+	return ret;
+}
+
+static const struct of_device_id sca3300_dt_ids[] = {
+	{ .compatible = "murata,sca3300"},
+	{}
+};
+MODULE_DEVICE_TABLE(of, sca3300_dt_ids);
+
+static struct spi_driver sca3300_driver = {
+	.driver = {
+		.name		= SCA3300_ALIAS,
+		.of_match_table = sca3300_dt_ids,
+	},
+
+	.probe	= sca3300_probe,
+};
+
+module_spi_driver(sca3300_driver);
+
+MODULE_AUTHOR("Tomas Melin <tomas.melin@vaisala.com>");
+MODULE_DESCRIPTION("Murata SCA3300 SPI Accelerometer");
+MODULE_LICENSE("GPL v2");
-- 
2.21.3

Re: [PATCH v3 2/2] iio: accel: Add driver for Murata SCA3300 accelerometer

[Andy Shevchenko]

On Mon, Apr 19, 2021 at 4:26 PM Tomas Melin <tomas.melin@vaisala.com> wrote:

Thanks for an update, it's getting better! My comments below.

> Add initial support for Murata SCA3300 3-axis industrial
> accelerometer with digital SPI interface. This device also
> provides a temperature measurement.

First of all, you forgot Cc reviewer(s).

> Datasheet: https://www.murata.com/en-global/products/sensor/accel/sca3300

>

No blank line in the tag block.

> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>


...

> +/*
> + * Copyright (c) 2021 Vaisala Oyj. All rights reserved.
> + */

One line.

...

> +#define SCA3300_MASK_STATUS    GENMASK(8, 0)
> +#define SCA3300_MASK_RS_STATUS GENMASK(1, 0)

This feels like an orphan. Shouldn't you move it closer to the group
of corresponding register / etc definition?

> +#define SCA3300_REG_MODE       0xd
> +#define SCA3300_REG_SELBANK    0x1f
> +#define SCA3300_REG_STATUS     0x6
> +#define SCA3300_REG_WHOAMI     0x10
> +
> +#define SCA3300_VALUE_DEVICE_ID        0x51
> +#define SCA3300_VALUE_RS_ERROR 0x3
> +#define SCA3300_VALUE_SW_RESET 0x20

As above it doesn't shed a light for the relationship between
registers and these fields (?). I.o.w the names w/o properly grouped
(and probably commented) are confusing.

...

> +/**
> + * struct sca3300_data - device data
> + * @spi: SPI device structure
> + * @lock: Data buffer lock

> + * @txbuf: Transmit buffer
> + * @rxbuf: Receive buffer

Are the buffers subject to DMA? Shouldn't they have the proper alignment?

> + * @scan: Triggered buffer. Four channel 16-bit data + 64-bit timestamp
> + */
> +struct sca3300_data {
> +       struct spi_device *spi;
> +       struct mutex lock;
> +       u8 txbuf[4];
> +       u8 rxbuf[4];
> +       struct {
> +               s16 channels[4];
> +               s64 ts __aligned(sizeof(s64));
> +       } scan;
> +};

...

> +       struct spi_delay delay = {.value = 10, .unit = SPI_DELAY_UNIT_USECS};

Missed space.

...

> +       sca_data->txbuf[0] = 0x0 | (SCA3300_REG_STATUS << 2);

Seems you ignored my comment. What is this 0x0? What is the meaning of it?
Same for all the rest magic numbers in the code.

> +       /*
> +        * return status error is cleared after reading status register once,
> +        * expect EINVAL here

/*
 * Fix the style of all your multi-line comments.
 * You may follow this example.
 */

> +        */
> +       if (ret != -EINVAL) {
> +               dev_err(&sca_data->spi->dev,
> +                       "error reading device status: %d\n", ret);
> +               return ret;
> +       }
> +
> +       dev_err(&sca_data->spi->dev, "device status: 0x%lx\n",
> +               (val & SCA3300_MASK_STATUS));

Too many parentheses.

> +       return 0;
> +}

...

> +static irqreturn_t sca3300_trigger_handler(int irq, void *p)
> +{
> +       struct iio_poll_func *pf = p;
> +       struct iio_dev *indio_dev = pf->indio_dev;
> +       struct sca3300_data *data = iio_priv(indio_dev);
> +       int bit, ret, val, i = 0;
> +
> +       for_each_set_bit(bit, indio_dev->active_scan_mask,
> +                        indio_dev->masklength) {
> +               ret = sca3300_read_reg(data, sca3300_channels[bit].address,
> +                                      &val);
> +               if (ret) {
> +                       dev_err(&data->spi->dev,
> +                               "failed to read register, error: %d\n", ret);

> +                       goto out;

Does it mean interrupt is handled in this case?
Perhaps a comment why it's okay to consider so?

> +               }
> +               data->scan.channels[i++] = val;
> +       }
> +
> +       iio_push_to_buffers_with_timestamp(indio_dev, &data->scan,
> +                                          iio_get_time_ns(indio_dev));
> +out:
> +       iio_trigger_notify_done(indio_dev->trig);
> +
> +       return IRQ_HANDLED;
> +}

...

> +       /*
> +        * wait 1ms after SW-reset command
> +        * wait 15ms for settling of signal paths
> +        */
> +       usleep_range(16e3, 50e3);

+ blank line

> +       ret = sca3300_read_reg(sca_data, SCA3300_REG_WHOAMI, &value);
> +       if (ret)
> +               return ret;

> +       ret = devm_iio_device_register(&spi->dev, indio_dev);
> +       if (ret) {
> +               dev_err(&spi->dev, "iio device register failed, error: %d\n",
> +                       ret);

> +               return ret;
> +       }
> +
> +       return ret;

Deduplicate it.

Simply leave the latter one.

> +}

...

> +

No need for this blank line.

> +       .probe  = sca3300_probe,
> +};

> +

Ditto.

> +module_spi_driver(sca3300_driver);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/2] iio: accel: Add driver for Murata SCA3300 accelerometer

[Tomas Melin]

Hi Andy,

Thanks for further comments, see some answers/questions below.

thanks,

Tomas


On 4/19/21 4:55 PM, Andy Shevchenko wrote:
> On Mon, Apr 19, 2021 at 4:26 PM Tomas Melin <tomas.melin@vaisala.com> wrote:
>
> Thanks for an update, it's getting better! My comments below.
>
>> Add initial support for Murata SCA3300 3-axis industrial
>> accelerometer with digital SPI interface. This device also
>> provides a temperature measurement.
> First of all, you forgot Cc reviewer(s).

Ok, thanks for pointing this out, will add you as cc next round.


>
>> Datasheet: https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.murata.com%2Fen-global%2Fproducts%2Fsensor%2Faccel%2Fsca3300&amp;data=04%7C01%7Ctomas.melin%40vaisala.com%7C5259ef3cd4b645f3a7d208d9033acdc5%7C6d7393e041f54c2e9b124c2be5da5c57%7C0%7C0%7C637544373362508656%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=BZue5RQjrHWtRzEOGZAw1Avb35QKLYu0ZOnXbyj9EE8%3D&amp;reserved=0
> No blank line in the tag block.
>
>> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
>
> ...
>
>> +/*
>> + * Copyright (c) 2021 Vaisala Oyj. All rights reserved.
>> + */
> One line.

Opted for adding Description line to this header, thus planning to keep 
as multi line.


>
> ...
>
>> +#define SCA3300_MASK_STATUS    GENMASK(8, 0)
>> +#define SCA3300_MASK_RS_STATUS GENMASK(1, 0)
> This feels like an orphan. Shouldn't you move it closer to the group
> of corresponding register / etc definition?

Tried to group these in alphabetical order, but IIUC preference would be 
towards grouping

according to how they are used? Would this be clearer and acceptable?

1)

/* Device mode register */
#define SCA3300_REG_MODE    0xd
#define SCA3300_VALUE_SW_RESET    0x20

/* Last register in map */
#define SCA3300_REG_SELBANK    0x1f

/* Device status and related mask */
#define SCA3300_REG_STATUS    0x6
#define SCA3300_MASK_STATUS    GENMASK(8, 0)

/* Device ID */
#define SCA3300_REG_WHOAMI    0x10
#define SCA3300_VALUE_DEVICE_ID    0x51

/* Device return status and mask */
#define SCA3300_VALUE_RS_ERROR    0x3
#define SCA3300_MASK_RS_STATUS    GENMASK(1, 0)

or then only adjusting current sorting with comments, like:

2)

/* Register mask values */
#define SCA3300_MASK_STATUS    GENMASK(8, 0)
#define SCA3300_MASK_RS_STATUS    GENMASK(1, 0)

/* Register index values */
#define SCA3300_REG_MODE    0xd
#define SCA3300_REG_SELBANK    0x1f
#define SCA3300_REG_STATUS    0x6
#define SCA3300_REG_WHOAMI    0x10

/* Register read/write values */
#define SCA3300_VALUE_DEVICE_ID    0x51
#define SCA3300_VALUE_RS_ERROR    0x3
#define SCA3300_VALUE_SW_RESET    0x20


>
>> +#define SCA3300_REG_MODE       0xd
>> +#define SCA3300_REG_SELBANK    0x1f
>> +#define SCA3300_REG_STATUS     0x6
>> +#define SCA3300_REG_WHOAMI     0x10
>> +
>> +#define SCA3300_VALUE_DEVICE_ID        0x51
>> +#define SCA3300_VALUE_RS_ERROR 0x3
>> +#define SCA3300_VALUE_SW_RESET 0x20
> As above it doesn't shed a light for the relationship between
> registers and these fields (?). I.o.w the names w/o properly grouped
> (and probably commented) are confusing.
>
> ...
>
>> +/**
>> + * struct sca3300_data - device data
>> + * @spi: SPI device structure
>> + * @lock: Data buffer lock
>> + * @txbuf: Transmit buffer
>> + * @rxbuf: Receive buffer
> Are the buffers subject to DMA? Shouldn't they have the proper alignment?
Good point, I will add alignment.
>
>> + * @scan: Triggered buffer. Four channel 16-bit data + 64-bit timestamp
>> + */
>> +struct sca3300_data {
>> +       struct spi_device *spi;
>> +       struct mutex lock;
>> +       u8 txbuf[4];
>> +       u8 rxbuf[4];
>> +       struct {
>> +               s16 channels[4];
>> +               s64 ts __aligned(sizeof(s64));
>> +       } scan;
>> +};
> ...
>
>> +       struct spi_delay delay = {.value = 10, .unit = SPI_DELAY_UNIT_USECS};
> Missed space.
>
> ...
>
>> +       sca_data->txbuf[0] = 0x0 | (SCA3300_REG_STATUS << 2);
> Seems you ignored my comment. What is this 0x0? What is the meaning of it?
> Same for all the rest magic numbers in the code.

Sorry, not ignored but will remove this redundant 0x0 for next round.

>
>> +       /*
>> +        * return status error is cleared after reading status register once,
>> +        * expect EINVAL here
> /*
>   * Fix the style of all your multi-line comments.
>   * You may follow this example.
>   */
Ok, will captialize sentence and add punctuation.
>> +        */
>> +       if (ret != -EINVAL) {
>> +               dev_err(&sca_data->spi->dev,
>> +                       "error reading device status: %d\n", ret);
>> +               return ret;
>> +       }
>> +
>> +       dev_err(&sca_data->spi->dev, "device status: 0x%lx\n",
>> +               (val & SCA3300_MASK_STATUS));
> Too many parentheses.
>
>> +       return 0;
>> +}
> ...
>
>> +static irqreturn_t sca3300_trigger_handler(int irq, void *p)
>> +{
>> +       struct iio_poll_func *pf = p;
>> +       struct iio_dev *indio_dev = pf->indio_dev;
>> +       struct sca3300_data *data = iio_priv(indio_dev);
>> +       int bit, ret, val, i = 0;
>> +
>> +       for_each_set_bit(bit, indio_dev->active_scan_mask,
>> +                        indio_dev->masklength) {
>> +               ret = sca3300_read_reg(data, sca3300_channels[bit].address,
>> +                                      &val);
>> +               if (ret) {
>> +                       dev_err(&data->spi->dev,
>> +                               "failed to read register, error: %d\n", ret);
>> +                       goto out;
> Does it mean interrupt is handled in this case?
> Perhaps a comment why it's okay to consider so?

IRQ_HANDLED seemed more correct than IRQ_NONE. Or did You have some 
other option in mind?

How about something like:

     /* handled with errors */

     goto out;


>
>> +               }
>> +               data->scan.channels[i++] = val;
>> +       }
>> +
>> +       iio_push_to_buffers_with_timestamp(indio_dev, &data->scan,
>> +                                          iio_get_time_ns(indio_dev));
>> +out:
>> +       iio_trigger_notify_done(indio_dev->trig);
>> +
>> +       return IRQ_HANDLED;
>> +}
>

Re: [PATCH v3 2/2] iio: accel: Add driver for Murata SCA3300 accelerometer

[Andy Shevchenko]

On Tue, Apr 20, 2021 at 11:50 AM Tomas Melin <tomas.melin@vaisala.com> wrote:
> On 4/19/21 4:55 PM, Andy Shevchenko wrote:
> > On Mon, Apr 19, 2021 at 4:26 PM Tomas Melin <tomas.melin@vaisala.com> wrote:

...

> >> +#define SCA3300_MASK_STATUS    GENMASK(8, 0)
> >> +#define SCA3300_MASK_RS_STATUS GENMASK(1, 0)
> > This feels like an orphan. Shouldn't you move it closer to the group
> > of corresponding register / etc definition?
>
> Tried to group these in alphabetical order, but IIUC preference would be
> towards grouping

Yes, alphabetical is about header block, and definition should be
understandable and HW represented.

> according to how they are used? Would this be clearer and acceptable?

1) with some amendments, see below.

> 1)
>
> /* Device mode register */
> #define SCA3300_REG_MODE    0xd
> #define SCA3300_VALUE_SW_RESET    0x20

SCA3300_MODE_SW_RESET

> /* Last register in map */
> #define SCA3300_REG_SELBANK    0x1f
>
> /* Device status and related mask */
> #define SCA3300_REG_STATUS    0x6
> #define SCA3300_MASK_STATUS    GENMASK(8, 0)

SCA3300_STATUS_MASK

and so on (I guess you got the pattern)

> /* Device ID */
> #define SCA3300_REG_WHOAMI    0x10
> #define SCA3300_VALUE_DEVICE_ID    0x51
>
> /* Device return status and mask */
> #define SCA3300_VALUE_RS_ERROR    0x3
> #define SCA3300_MASK_RS_STATUS    GENMASK(1, 0)

...

> >> + * @txbuf: Transmit buffer
> >> + * @rxbuf: Receive buffer
> > Are the buffers subject to DMA? Shouldn't they have the proper alignment?
> Good point, I will add alignment.

Move them to the end of the structure to save few bytes,

...

> >> +       sca_data->txbuf[0] = 0x0 | (SCA3300_REG_STATUS << 2);
> > Seems you ignored my comment. What is this 0x0? What is the meaning of it?
> > Same for all the rest magic numbers in the code.
>
> Sorry, not ignored but will remove this redundant 0x0 for next round.

Maybe it's not redundant after all (I noticed other magic numbers in
the same position)? Please, comment your intention case-by-case.

...

> >> +       for_each_set_bit(bit, indio_dev->active_scan_mask,
> >> +                        indio_dev->masklength) {
> >> +               ret = sca3300_read_reg(data, sca3300_channels[bit].address,
> >> +                                      &val);
> >> +               if (ret) {
> >> +                       dev_err(&data->spi->dev,
> >> +                               "failed to read register, error: %d\n", ret);
> >> +                       goto out;
> > Does it mean interrupt is handled in this case?
> > Perhaps a comment why it's okay to consider so?
>
> IRQ_HANDLED seemed more correct than IRQ_NONE.

Why? Care to explain?

>  Or did You have some
> other option in mind?
>
> How about something like:
>
>      /* handled with errors */

But what if this is the very first interrupt (bit in the loop) that
failed? What about the rest?

>      goto out;
>
> >> +               }
> >> +               data->scan.channels[i++] = val;
> >> +       }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/2] iio: accel: Add driver for Murata SCA3300 accelerometer

[Tomas Melin]

On 4/20/21 1:47 PM, Andy Shevchenko wrote:
> On Tue, Apr 20, 2021 at 11:50 AM Tomas Melin <tomas.melin@vaisala.com> wrote:
>
>>>> +       sca_data->txbuf[0] = 0x0 | (SCA3300_REG_STATUS << 2);
>>> Seems you ignored my comment. What is this 0x0? What is the meaning of it?
>>> Same for all the rest magic numbers in the code.
>> Sorry, not ignored but will remove this redundant 0x0 for next round.
> Maybe it's not redundant after all (I noticed other magic numbers in
> the same position)? Please, comment your intention case-by-case.

0x0 is for read operation, but since it's just or'd, end result should

be the same. It was there in v1 for clarity (also #defined). Basically

thinking perhaps it's cleaner to just leave it out.

Other magics should be cleaned up now.


> ...
>
>>>> +       for_each_set_bit(bit, indio_dev->active_scan_mask,
>>>> +                        indio_dev->masklength) {
>>>> +               ret = sca3300_read_reg(data, sca3300_channels[bit].address,
>>>> +                                      &val);
>>>> +               if (ret) {
>>>> +                       dev_err(&data->spi->dev,
>>>> +                               "failed to read register, error: %d\n", ret);
>>>> +                       goto out;
>>> Does it mean interrupt is handled in this case?
>>> Perhaps a comment why it's okay to consider so?
>> IRQ_HANDLED seemed more correct than IRQ_NONE.
> Why? Care to explain?

Thinking that IRQ was for the device and it was indeed handled. There 
were errors when handling

it, but it was handled as much as possible.

>
>>   Or did You have some
>> other option in mind?
>>
>> How about something like:
>>
>>       /* handled with errors */
> But what if this is the very first interrupt (bit in the loop) that
> failed? What about the rest?

Aah, right. Other option could be to simply continue loop and set 'val' 
to e.g. 0 for

readings with errors. But perhaps it is after all better to bail out, 
and only for cases

when _all_ data is reliable, it is pushed to buffers(?)

Comes to mind that perhaps better to have error message in this irq 
handler as

dev_err_ratelimited(), to avoid possible flooding.


So to conclude, proposing:

*change to dev_err_ratelimited()

* comment goto:

     /* handled, but bailing out this round due to errors */


Would this be OK?

Thanks,

Tomas




>
>>       goto out;
>>
>>>> +               }
>>>> +               data->scan.channels[i++] = val;
>>>> +       }

Re: [PATCH v3 2/2] iio: accel: Add driver for Murata SCA3300 accelerometer

[Andy Shevchenko]

On Tue, Apr 20, 2021 at 2:36 PM Tomas Melin <tomas.melin@vaisala.com> wrote:
> On 4/20/21 1:47 PM, Andy Shevchenko wrote:
> > On Tue, Apr 20, 2021 at 11:50 AM Tomas Melin <tomas.melin@vaisala.com> wrote:

...

> >>>> +       for_each_set_bit(bit, indio_dev->active_scan_mask,
> >>>> +                        indio_dev->masklength) {
> >>>> +               ret = sca3300_read_reg(data, sca3300_channels[bit].address,
> >>>> +                                      &val);
> >>>> +               if (ret) {
> >>>> +                       dev_err(&data->spi->dev,
> >>>> +                               "failed to read register, error: %d\n", ret);
> >>>> +                       goto out;
> >>> Does it mean interrupt is handled in this case?
> >>> Perhaps a comment why it's okay to consider so?
> >> IRQ_HANDLED seemed more correct than IRQ_NONE.
> > Why? Care to explain?
>
> Thinking that IRQ was for the device and it was indeed handled. There
> were errors when handling
>
> it, but it was handled as much as possible.
>
> >
> >>   Or did You have some
> >> other option in mind?
> >>
> >> How about something like:
> >>
> >>       /* handled with errors */
> > But what if this is the very first interrupt (bit in the loop) that
> > failed? What about the rest?
>
> Aah, right. Other option could be to simply continue loop and set 'val'
> to e.g. 0 for
>
> readings with errors. But perhaps it is after all better to bail out,
> and only for cases
>
> when _all_ data is reliable, it is pushed to buffers(?)
>
> Comes to mind that perhaps better to have error message in this irq
> handler as
>
> dev_err_ratelimited(), to avoid possible flooding.
>
>
> So to conclude, proposing:
>
> *change to dev_err_ratelimited()
>
> * comment goto:
>
>      /* handled, but bailing out this round due to errors */
>
> Would this be OK?

Sounds like a plan!

> >>       goto out;
> >>
> >>>> +               }
> >>>> +               data->scan.channels[i++] = val;
> >>>> +       }

-- 
With Best Regards,
Andy Shevchenko