[PATCH v2 0/2] iio: imu: smi240: cover-letter

[PATCH v2 0/2] iio: imu: smi240: cover-letter

[Jianping.Shen]

From: "Shen Jianping (ME-SE/EAD2)" <Jianping.Shen@de.bosch.com>

This patchset adds the iio driver and the dt-binding for bosch imu smi240.
The smi240 is a combined three axis angular rate and three axis acceleration
sensor module with a measurement range of +/-300°/s and up to 16g.
smi240 does not support interrupt.

dt-bindings: v1 -> v2
    - Add more detail in description
    - Add maintainer
    - Add vdd and vddio power supply
    - Use generic node name
    - Order the properties according to DTS coding style
    
imu driver v1 -> v2
    - Use regmap for register access
    - Redefine channel for each singel axis
    - Provide triggered buffer
    - Fix findings in Kconfig
    - Remove unimportant functions

Shen Jianping (ME-SE/EAD2) (2):
  dt-bindings: iio: imu: smi240: devicetree binding
  iio: imu: smi240: imu driver

 .../bindings/iio/imu/bosch,smi240.yaml        |  51 +++
 drivers/iio/imu/Kconfig                       |   1 +
 drivers/iio/imu/Makefile                      |   1 +
 drivers/iio/imu/smi240/Kconfig                |  12 +
 drivers/iio/imu/smi240/Makefile               |   7 +
 drivers/iio/imu/smi240/smi240.h               |  30 ++
 drivers/iio/imu/smi240/smi240_core.c          | 392 ++++++++++++++++++
 drivers/iio/imu/smi240/smi240_spi.c           | 173 ++++++++
 8 files changed, 667 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/imu/bosch,smi240.yaml
 create mode 100644 drivers/iio/imu/smi240/Kconfig
 create mode 100644 drivers/iio/imu/smi240/Makefile
 create mode 100644 drivers/iio/imu/smi240/smi240.h
 create mode 100644 drivers/iio/imu/smi240/smi240_core.c
 create mode 100644 drivers/iio/imu/smi240/smi240_spi.c

-- 
2.34.1

[PATCH v2 1/2] dt-bindings: iio: imu: smi240: devicetree binding

[Jianping.Shen]

From: "Shen Jianping (ME-SE/EAD2)" <Jianping.Shen@de.bosch.com>

dt-bindings: iio: imu: smi240: dt-binding improvements
Signed-off-by: Shen Jianping (ME-SE/EAD2) <Jianping.Shen@de.bosch.com>
---

Notes:
    v1 -> v2
    - Add more detail in description
    - Add maintainer
    - Add vdd and vddio power supply
    - Use generic node name
    - Order the properties according to DTS coding style

 .../bindings/iio/imu/bosch,smi240.yaml        | 51 +++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/imu/bosch,smi240.yaml

diff --git a/Documentation/devicetree/bindings/iio/imu/bosch,smi240.yaml b/Documentation/devicetree/bindings/iio/imu/bosch,smi240.yaml
new file mode 100644
index 00000000000..0529e67de26
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/imu/bosch,smi240.yaml
@@ -0,0 +1,51 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/imu/bosch,smi240.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Bosch smi240 imu
+
+maintainers:
+  - Jianping Shen <Jianping.Shen@de.bosch.com>
+
+description:
+  The smi240 is a combined three axis angular rate and three 
+  axis acceleration sensor module with a measurement range of 
+  +/-300°/s and up to 16g. smi240 does not support interrupt.
+  https://www.bosch-semiconductors.com/mems-sensors/highly-automated-driving/smi240/
+
+properties:
+  compatible:
+    const: bosch,smi240
+
+  reg:
+    maxItems: 1
+
+  vdd-supply:
+    description: provide VDD power to the sensor.
+
+  vddio-supply:
+    description: provide VDD IO power to the sensor.
+
+required:
+  - compatible
+  - reg
+
+allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        imu@0 {
+            compatible = "bosch,smi240";            
+            reg = <0>;
+            spi-max-frequency = <10000000>;
+        };
+    };
-- 
2.34.1

Re: [PATCH v2 1/2] dt-bindings: iio: imu: smi240: devicetree binding

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3081 bytes --]

On Fri, Aug 09, 2024 at 01:16:34PM +0200, Jianping.Shen@de.bosch.com wrote:
> From: "Shen Jianping (ME-SE/EAD2)" <Jianping.Shen@de.bosch.com>
> 
> dt-bindings: iio: imu: smi240: dt-binding improvements

The commit message here doesn't make much sese, you're adding a new
file. Something like the description in the binding below would be a
better fit than what you have.

> Signed-off-by: Shen Jianping (ME-SE/EAD2) <Jianping.Shen@de.bosch.com>
                               ^^^^^^^^^^^^
If this is not part of your name, or a client of Bosch that you want to
attribute authorship to, remove it.

> ---
> 
> Notes:
>     v1 -> v2
>     - Add more detail in description
>     - Add maintainer
>     - Add vdd and vddio power supply
>     - Use generic node name
>     - Order the properties according to DTS coding style
> 
>  .../bindings/iio/imu/bosch,smi240.yaml        | 51 +++++++++++++++++++
>  1 file changed, 51 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/imu/bosch,smi240.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/imu/bosch,smi240.yaml b/Documentation/devicetree/bindings/iio/imu/bosch,smi240.yaml
> new file mode 100644
> index 00000000000..0529e67de26
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/imu/bosch,smi240.yaml
> @@ -0,0 +1,51 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/imu/bosch,smi240.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Bosch smi240 imu
> +
> +maintainers:
> +  - Jianping Shen <Jianping.Shen@de.bosch.com>
> +
> +description:
> +  The smi240 is a combined three axis angular rate and three 
> +  axis acceleration sensor module with a measurement range of 
> +  +/-300°/s and up to 16g.

> smi240 does not support interrupt.

You can, drop this, don't think it's worth mentioning in the
description.

> +  https://www.bosch-semiconductors.com/mems-sensors/highly-automated-driving/smi240/
> +
> +properties:
> +  compatible:
> +    const: bosch,smi240
> +
> +  reg:
> +    maxItems: 1
> +
> +  vdd-supply:
> +    description: provide VDD power to the sensor.
> +
> +  vddio-supply:
> +    description: provide VDD IO power to the sensor.

I think the descriptions for these are not really worth having and can
be replaced with "vddio-supply: true" etc.

> +
> +required:
> +  - compatible
> +  - reg

However, the supplies likely are required to function and should be
added to the required section.

> +
> +allOf:
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    spi {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        imu@0 {
> +            compatible = "bosch,smi240";            

You have trailing whitespace after this line.

Cheers,
Conor.

> +            reg = <0>;
> +            spi-max-frequency = <10000000>;
> +        };
> +    };
> -- 
> 2.34.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH v2 2/2] iio: imu: smi240: imu driver

[Jianping.Shen]

From: "Shen Jianping (ME-SE/EAD2)" <Jianping.Shen@de.bosch.com>

iio: imu: smi240: driver improvements
Signed-off-by: Shen Jianping (ME-SE/EAD2) <Jianping.Shen@de.bosch.com>
---

Notes:
    v1 -> v2
    - Use regmap for register access
    - Redefine channel for each singel axis
    - Provide triggered buffer
    - Fix findings in Kconfig
    - Remove unimportant functions

 drivers/iio/imu/Kconfig              |   1 +
 drivers/iio/imu/Makefile             |   1 +
 drivers/iio/imu/smi240/Kconfig       |  12 +
 drivers/iio/imu/smi240/Makefile      |   7 +
 drivers/iio/imu/smi240/smi240.h      |  30 ++
 drivers/iio/imu/smi240/smi240_core.c | 392 +++++++++++++++++++++++++++
 drivers/iio/imu/smi240/smi240_spi.c  | 173 ++++++++++++
 7 files changed, 616 insertions(+)
 create mode 100644 drivers/iio/imu/smi240/Kconfig
 create mode 100644 drivers/iio/imu/smi240/Makefile
 create mode 100644 drivers/iio/imu/smi240/smi240.h
 create mode 100644 drivers/iio/imu/smi240/smi240_core.c
 create mode 100644 drivers/iio/imu/smi240/smi240_spi.c

diff --git a/drivers/iio/imu/Kconfig b/drivers/iio/imu/Kconfig
index 52a155ff325..8808074513d 100644
--- a/drivers/iio/imu/Kconfig
+++ b/drivers/iio/imu/Kconfig
@@ -96,6 +96,7 @@ config KMX61
 
 source "drivers/iio/imu/inv_icm42600/Kconfig"
 source "drivers/iio/imu/inv_mpu6050/Kconfig"
+source "drivers/iio/imu/smi240/Kconfig"
 source "drivers/iio/imu/st_lsm6dsx/Kconfig"
 source "drivers/iio/imu/st_lsm9ds0/Kconfig"
 
diff --git a/drivers/iio/imu/Makefile b/drivers/iio/imu/Makefile
index 7e2d7d5c3b7..b6f162ae4ed 100644
--- a/drivers/iio/imu/Makefile
+++ b/drivers/iio/imu/Makefile
@@ -27,5 +27,6 @@ obj-y += inv_mpu6050/
 
 obj-$(CONFIG_KMX61) += kmx61.o
 
+obj-y += smi240/
 obj-y += st_lsm6dsx/
 obj-y += st_lsm9ds0/
diff --git a/drivers/iio/imu/smi240/Kconfig b/drivers/iio/imu/smi240/Kconfig
new file mode 100644
index 00000000000..b7f3598f6c4
--- /dev/null
+++ b/drivers/iio/imu/smi240/Kconfig
@@ -0,0 +1,12 @@
+config SMI240
+	tristate "Bosch Sensor SMI240 Inertial Measurement Unit"
+	depends on SPI
+	select REGMAP_SPI
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
+	help
+	  If you say yes here you get support for SMI240 IMU on SPI with
+	  accelerometer and gyroscope.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called smi240.
diff --git a/drivers/iio/imu/smi240/Makefile b/drivers/iio/imu/smi240/Makefile
new file mode 100644
index 00000000000..0e5f7db7d78
--- /dev/null
+++ b/drivers/iio/imu/smi240/Makefile
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0
+#
+# Makefile for Bosch SMI240
+#
+obj-$(CONFIG_SMI240) += smi240.o
+smi240-objs := smi240_core.o
+smi240-objs += smi240_spi.o
diff --git a/drivers/iio/imu/smi240/smi240.h b/drivers/iio/imu/smi240/smi240.h
new file mode 100644
index 00000000000..de79b289532
--- /dev/null
+++ b/drivers/iio/imu/smi240/smi240.h
@@ -0,0 +1,30 @@
+/* SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0 */
+/*
+ * Copyright (c) 2024 Robert Bosch GmbH.
+ *
+ */
+#ifndef _SMI240_H
+#define _SMI240_H
+
+#include <linux/device.h>
+#include <linux/regmap.h>
+
+struct smi240_data {
+	struct regmap *regmap;
+	u16 accel_filter_freq;
+	u16 anglvel_filter_freq;
+	u8 bite_reps;
+	u8 capture;
+	/*
+	 * Ensure natural alignment for timestamp if present.
+	 * Channel size: 2 bytes.
+	 * Max length needed: 2 * 3 channels + temp channel + 2 bytes padding + 8 byte ts.
+	 * If fewer channels are enabled, less space may be needed, as
+	 * long as the timestamp is still aligned to 8 bytes.
+	 */
+	s16 buf[12] __aligned(8);
+};
+
+int smi240_core_probe(struct device *dev, struct regmap *regmap);
+
+#endif /* _SMI240_H */
diff --git a/drivers/iio/imu/smi240/smi240_core.c b/drivers/iio/imu/smi240/smi240_core.c
new file mode 100644
index 00000000000..4b2a4a290f3
--- /dev/null
+++ b/drivers/iio/imu/smi240/smi240_core.c
@@ -0,0 +1,392 @@
+// SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0
+/*
+ * Copyright (c) 2024 Robert Bosch GmbH.
+ *
+ */
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/delay.h>
+#include <linux/bits.h>
+#include <linux/bitfield.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/trigger.h>
+
+#include "smi240.h"
+
+enum {
+	SMI240_TEMP_OBJECT,
+	SMI240_SCAN_ACCEL_X,
+	SMI240_SCAN_ACCEL_Y,
+	SMI240_SCAN_ACCEL_Z,
+	SMI240_SCAN_GYRO_X,
+	SMI240_SCAN_GYRO_Y,
+	SMI240_SCAN_GYRO_Z,
+	SMI240_SCAN_TIMESTAMP,
+};
+
+#define SMI240_CHIP_ID 0x0024
+
+#define SMI240_SOFT_CONFIG_EOC_MASK BIT_MASK(0)
+#define SMI240_SOFT_CONFIG_GYR_BW_MASK BIT_MASK(1)
+#define SMI240_SOFT_CONFIG_ACC_BW_MASK BIT_MASK(2)
+#define SMI240_SOFT_CONFIG_BITE_AUTO_MASK BIT_MASK(3)
+#define SMI240_SOFT_CONFIG_BITE_REP_MASK GENMASK(6, 4)
+
+#define SMI240_CHIP_ID_REG 0x00
+#define SMI240_SOFT_CONFIG_REG 0x0A
+#define SMI240_TEMP_CUR_REG 0x10
+#define SMI240_ACCEL_X_CUR_REG 0x11
+#define SMI240_GYRO_X_CUR_REG 0x14
+#define SMI240_DATA_CAP_FIRST_REG 0x17
+#define SMI240_CMD_REG 0x2F
+
+#define SMI240_SOFT_RESET_CMD 0xB6
+
+#define SMI240_BITE_SEQUENCE_DELAY_US 140000
+#define SMI240_FILTER_FLUSH_DELAY_US 60000
+#define SMI240_DIGITAL_STARTUP_DELAY_US 120000
+#define SMI240_MECH_STARTUP_DELAY_US 100000
+
+#define SMI240_LOW_BANDWIDTH_HZ 50
+#define SMI240_HIGH_BANDWIDTH_HZ 400
+
+#define SMI240_DATA_CHANNEL(_type, _axis, _index)                          \
+	{                                                                  \
+		.type = _type, .modified = 1, .channel2 = IIO_MOD_##_axis, \
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),              \
+		.info_mask_shared_by_type =                                \
+			BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),  \
+		.info_mask_shared_by_type_available =                      \
+			BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),  \
+		.scan_index = _index,                                      \
+		.scan_type = {                                             \
+			.sign = 's',                                       \
+			.realbits = 16,                                    \
+			.storagebits = 16,                                 \
+			.endianness = IIO_LE,                              \
+		},                                                         \
+	}
+
+#define SMI240_TEMP_CHANNEL(_index)                           \
+	{                                                     \
+		.type = IIO_TEMP, .modified = 1,              \
+		.channel2 = IIO_MOD_TEMP_OBJECT,              \
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
+		.scan_index = _index,                         \
+		.scan_type = {                                \
+			.sign = 's',                          \
+			.realbits = 16,                       \
+			.storagebits = 16,                    \
+			.endianness = IIO_LE,                 \
+		},                                            \
+	}
+
+static const struct iio_chan_spec smi240_channels[] = {
+	SMI240_TEMP_CHANNEL(SMI240_TEMP_OBJECT),
+	SMI240_DATA_CHANNEL(IIO_ACCEL, X, SMI240_SCAN_ACCEL_X),
+	SMI240_DATA_CHANNEL(IIO_ACCEL, Y, SMI240_SCAN_ACCEL_Y),
+	SMI240_DATA_CHANNEL(IIO_ACCEL, Z, SMI240_SCAN_ACCEL_Z),
+	SMI240_DATA_CHANNEL(IIO_ANGL_VEL, X, SMI240_SCAN_GYRO_X),
+	SMI240_DATA_CHANNEL(IIO_ANGL_VEL, Y, SMI240_SCAN_GYRO_Y),
+	SMI240_DATA_CHANNEL(IIO_ANGL_VEL, Z, SMI240_SCAN_GYRO_Z),
+	IIO_CHAN_SOFT_TIMESTAMP(SMI240_SCAN_TIMESTAMP),
+};
+
+static const int smi240_low_pass_freqs[] = { SMI240_LOW_BANDWIDTH_HZ,
+					     SMI240_HIGH_BANDWIDTH_HZ };
+
+static int smi240_soft_reset(struct smi240_data *data)
+{
+	int ret;
+
+	ret = regmap_write(data->regmap, SMI240_CMD_REG, SMI240_SOFT_RESET_CMD);
+	if (ret)
+		return ret;
+	fsleep(SMI240_DIGITAL_STARTUP_DELAY_US);
+	return 0;
+}
+
+static int smi240_soft_config(struct smi240_data *data)
+{
+	int ret;
+	u8 acc_bw, gyr_bw;
+	u16 request;
+
+	switch (data->accel_filter_freq) {
+	case SMI240_LOW_BANDWIDTH_HZ:
+		acc_bw = 0x1;
+		break;
+	case SMI240_HIGH_BANDWIDTH_HZ:
+		acc_bw = 0x0;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	switch (data->anglvel_filter_freq) {
+	case SMI240_LOW_BANDWIDTH_HZ:
+		gyr_bw = 0x1;
+		break;
+	case SMI240_HIGH_BANDWIDTH_HZ:
+		gyr_bw = 0x0;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	request = FIELD_PREP(SMI240_SOFT_CONFIG_EOC_MASK, 1);
+	request |= FIELD_PREP(SMI240_SOFT_CONFIG_GYR_BW_MASK, gyr_bw);
+	request |= FIELD_PREP(SMI240_SOFT_CONFIG_ACC_BW_MASK, acc_bw);
+	request |= FIELD_PREP(SMI240_SOFT_CONFIG_BITE_AUTO_MASK, 1);
+	request |= FIELD_PREP(SMI240_SOFT_CONFIG_BITE_REP_MASK,
+			      data->bite_reps - 1);
+
+	ret = regmap_write(data->regmap, SMI240_SOFT_CONFIG_REG, request);
+	if (ret)
+		return ret;
+
+	fsleep(SMI240_MECH_STARTUP_DELAY_US +
+	       data->bite_reps * SMI240_BITE_SEQUENCE_DELAY_US +
+	       SMI240_FILTER_FLUSH_DELAY_US);
+	return 0;
+}
+
+static int smi240_get_low_pass_filter_freq(struct smi240_data *data,
+					   int chan_type, int *val)
+{
+	switch (chan_type) {
+	case IIO_ACCEL:
+		*val = data->accel_filter_freq;
+		break;
+	case IIO_ANGL_VEL:
+		*val = data->anglvel_filter_freq;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int smi240_get_data(struct smi240_data *data, int chan_type, int axis,
+			   int *val)
+{
+	u8 reg;
+	int ret, sample;
+
+	if (chan_type == IIO_TEMP)
+		reg = SMI240_TEMP_CUR_REG;
+	else if (chan_type == IIO_ACCEL)
+		reg = SMI240_ACCEL_X_CUR_REG + (axis - IIO_MOD_X);
+	else if (chan_type == IIO_ANGL_VEL)
+		reg = SMI240_GYRO_X_CUR_REG + (axis - IIO_MOD_X);
+	else
+		return -EINVAL;
+
+	ret = regmap_read(data->regmap, reg, &sample);
+	if (ret)
+		return ret;
+
+	*val = sign_extend32(sample, 15);
+
+	return 0;
+}
+
+static irqreturn_t smi240_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct smi240_data *data = iio_priv(indio_dev);
+	int ret, sample, chan, i = 0;
+
+	data->capture = 1;
+
+	for_each_set_bit(chan, indio_dev->active_scan_mask,
+			 indio_dev->masklength) {
+		ret = regmap_read(data->regmap,
+				  SMI240_DATA_CAP_FIRST_REG + chan, &sample);
+		data->capture = 0;
+		if (ret)
+			break;
+		data->buf[i++] = sample;
+	}
+
+	if (ret == 0)
+		iio_push_to_buffers_with_timestamp(indio_dev, data->buf,
+						   pf->timestamp);
+
+	iio_trigger_notify_done(indio_dev->trig);
+	return IRQ_HANDLED;
+}
+
+static int smi240_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_LOW_PASS_FILTER_3DB_FREQUENCY:
+		*vals = smi240_low_pass_freqs;
+		*length = ARRAY_SIZE(smi240_low_pass_freqs);
+		*type = IIO_VAL_INT;
+		return IIO_AVAIL_LIST;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int smi240_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan, int *val,
+			   int *val2, long mask)
+{
+	int ret = 0;
+	struct smi240_data *data = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		if (iio_buffer_enabled(indio_dev))
+			return -EBUSY;
+		ret = smi240_get_data(data, chan->type, chan->channel2, val);
+		if (ret)
+			return ret;
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
+		ret = smi240_get_low_pass_filter_freq(data, chan->type, val);
+		if (ret)
+			return ret;
+		return IIO_VAL_INT;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static int smi240_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan, int val, int val2,
+			    long mask)
+{
+	int ret, i;
+	struct smi240_data *data = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
+		for (i = 0; i < ARRAY_SIZE(smi240_low_pass_freqs); i++) {
+			if (val == smi240_low_pass_freqs[i])
+				break;
+		}
+
+		if (i == ARRAY_SIZE(smi240_low_pass_freqs))
+			return -EINVAL;
+
+		switch (chan->type) {
+		case IIO_ACCEL:
+			data->accel_filter_freq = val;
+			break;
+		case IIO_ANGL_VEL:
+			data->anglvel_filter_freq = val;
+			break;
+		default:
+			return -EINVAL;
+		}
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	// Write access to soft config is locked until hard/soft reset
+	ret = smi240_soft_reset(data);
+	if (ret)
+		return ret;
+	ret = smi240_soft_config(data);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int smi240_init(struct smi240_data *data)
+{
+	data->accel_filter_freq = SMI240_HIGH_BANDWIDTH_HZ;
+	data->anglvel_filter_freq = SMI240_HIGH_BANDWIDTH_HZ;
+	data->bite_reps = 3;
+
+	return smi240_soft_config(data);
+}
+
+static IIO_CONST_ATTR_TEMP_SCALE("1/256");
+static IIO_CONST_ATTR_TEMP_OFFSET("25");
+
+static struct attribute *smi240_attrs[] = {
+	&iio_const_attr_in_temp_scale.dev_attr.attr,
+	&iio_const_attr_in_temp_offset.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group smi240_attrs_group = {
+	.attrs = smi240_attrs,
+};
+
+static const struct iio_info smi240_info = {
+	.read_avail = smi240_read_avail,
+	.read_raw = smi240_read_raw,
+	.write_raw = smi240_write_raw,
+	.attrs = &smi240_attrs_group,
+};
+
+int smi240_core_probe(struct device *dev, struct regmap *regmap)
+{
+	struct iio_dev *indio_dev;
+	struct smi240_data *data;
+	int ret, response;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!indio_dev)
+		return dev_err_probe(dev, -ENOMEM,
+				     "Allocate iio device failed\n");
+
+	data = iio_priv(indio_dev);
+	dev_set_drvdata(dev, indio_dev);
+	data->regmap = regmap;
+	data->capture = 0;
+
+	ret = regmap_read(data->regmap, SMI240_CHIP_ID_REG, &response);
+	if (ret)
+		return dev_err_probe(dev, ret, "Read chip id failed\n");
+
+	if (response != SMI240_CHIP_ID)
+		dev_info(dev, "Unknown chip id: 0x%04x\n", response);
+
+	ret = smi240_init(data);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Device initialization failed\n");
+
+	indio_dev->channels = smi240_channels;
+	indio_dev->num_channels = ARRAY_SIZE(smi240_channels);
+	indio_dev->name = "smi240";
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->info = &smi240_info;
+
+	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
+					      iio_pollfunc_store_time,
+					      smi240_trigger_handler, NULL);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Setup triggered buffer failed\n");
+
+	ret = devm_iio_device_register(dev, indio_dev);
+	if (ret)
+		return dev_err_probe(dev, ret, "Register IIO device failed\n");
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(smi240_core_probe);
+
+MODULE_AUTHOR("Markus Lochmann <markus.lochmann@de.bosch.com>");
+MODULE_AUTHOR("Stefan Gutmann <stefan.gutmann@de.bosch.com>");
+MODULE_DESCRIPTION("Bosch SMI240 driver");
+MODULE_LICENSE("Dual BSD/GPL");
diff --git a/drivers/iio/imu/smi240/smi240_spi.c b/drivers/iio/imu/smi240/smi240_spi.c
new file mode 100644
index 00000000000..ac9e37ffa37
--- /dev/null
+++ b/drivers/iio/imu/smi240/smi240_spi.c
@@ -0,0 +1,173 @@
+// SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0
+/*
+ * Copyright (c) 2024 Robert Bosch GmbH.
+ *
+ */
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+#include <linux/spi/spi.h>
+#include <linux/bitfield.h>
+#include <linux/iio/iio.h>
+
+#include "smi240.h"
+
+#define SMI240_CRC_INIT 0x05
+#define SMI240_CRC_POLY 0x0B
+#define SMI240_BUS_ID 0x00
+
+#define SMI240_SD_BIT_MASK 0x80000000
+#define SMI240_CS_BIT_MASK 0x00000008
+
+#define SMI240_WRITE_ADDR_MASK GENMASK(29, 22)
+#define SMI240_WRITE_BIT_MASK 0x00200000
+#define SMI240_WRITE_DATA_MASK GENMASK(18, 3)
+#define SMI240_CAP_BIT_MASK 0x00100000
+#define SMI240_READ_DATA_MASK GENMASK(19, 4)
+
+static u8 smi240_crc3(u32 data, u8 init, u8 poly)
+{
+	u8 crc = init;
+	u8 do_xor;
+	s8 i = 31;
+
+	do {
+		do_xor = crc & 0x04;
+		crc <<= 1;
+		crc |= 0x01 & (data >> i);
+		if (do_xor)
+			crc ^= poly;
+
+		crc &= 0x07;
+	} while (--i >= 0);
+
+	return crc;
+}
+
+static bool smi240_sensor_data_is_valid(u32 data)
+{
+	if (smi240_crc3(data, SMI240_CRC_INIT, SMI240_CRC_POLY) != 0)
+		return false;
+
+	if (FIELD_GET(SMI240_SD_BIT_MASK, data) &
+	    FIELD_GET(SMI240_CS_BIT_MASK, data))
+		return false;
+
+	return true;
+}
+
+static int smi240_regmap_spi_read(void *context, const void *reg_buf,
+				  size_t reg_size, void *val_buf,
+				  size_t val_size)
+{
+	int ret;
+	__be32 request, response;
+	struct spi_device *spi = context;
+	struct iio_dev *indio_dev = dev_get_drvdata(&spi->dev);
+	struct smi240_data *data = iio_priv(indio_dev);
+
+	request = SMI240_BUS_ID << 30;
+	request |= FIELD_PREP(SMI240_CAP_BIT_MASK, data->capture);
+	request |= FIELD_PREP(SMI240_WRITE_ADDR_MASK, *(u8 *)reg_buf);
+	request |= smi240_crc3(request, SMI240_CRC_INIT, SMI240_CRC_POLY);
+	request = cpu_to_be32(request);
+
+	/*
+	 * SMI240 module consists of a 32Bit Out Of Frame (OOF)
+	 * SPI protocol, where the slave interface responds to
+	 * the Master request in the next frame.
+	 * CS signal must toggle (> 700 ns) between the frames.
+	 */
+	ret = spi_write(spi, &request, sizeof(request));
+	if (ret)
+		return ret;
+
+	ret = spi_read(spi, &response, sizeof(response));
+	if (ret)
+		return ret;
+
+	response = be32_to_cpu(response);
+
+	if (!smi240_sensor_data_is_valid(response))
+		return -EIO;
+
+	response = FIELD_GET(SMI240_READ_DATA_MASK, response);
+	memcpy(val_buf, &response, val_size);
+
+	return 0;
+}
+
+static int smi240_regmap_spi_write(void *context, const void *data,
+				   size_t count)
+{
+	__be32 request;
+	struct spi_device *spi = context;
+	u8 reg_addr = ((u8 *)data)[0];
+	u16 reg_data = ((u8 *)data)[2] << 8 | ((u8 *)data)[1];
+
+	request = SMI240_BUS_ID << 30;
+	request |= FIELD_PREP(SMI240_WRITE_BIT_MASK, 1);
+	request |= FIELD_PREP(SMI240_WRITE_ADDR_MASK, reg_addr);
+	request |= FIELD_PREP(SMI240_WRITE_DATA_MASK, reg_data);
+	request |= smi240_crc3(request, SMI240_CRC_INIT, SMI240_CRC_POLY);
+	request = cpu_to_be32(request);
+
+	return spi_write(spi, &request, sizeof(request));
+}
+
+static struct regmap_bus smi240_regmap_bus = {
+	.read = smi240_regmap_spi_read,
+	.write = smi240_regmap_spi_write,
+};
+
+static const struct regmap_config smi240_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 16,
+	.val_format_endian = REGMAP_ENDIAN_LITTLE,
+};
+
+static int smi240_spi_probe(struct spi_device *spi)
+{
+	struct regmap *regmap;
+
+	u32 max_frequency = 10000000;
+
+	of_property_read_u32(spi->dev.of_node, "spi-max-frequency",
+			     &max_frequency);
+
+	spi->bits_per_word = 8;
+	spi->max_speed_hz = max_frequency;
+	spi->mode = SPI_MODE_0;
+
+	regmap = devm_regmap_init(&spi->dev, &smi240_regmap_bus, &spi->dev,
+				  &smi240_regmap_config);
+	if (IS_ERR(regmap))
+		return dev_err_probe(&spi->dev, PTR_ERR(regmap),
+				     "Failed to initialize SPI Regmap\n");
+
+	return smi240_core_probe(&spi->dev, regmap);
+}
+
+static const struct spi_device_id smi240_spi_id[] = { { "smi240", 0 }, {} };
+MODULE_DEVICE_TABLE(spi, smi240_spi_id);
+
+static const struct of_device_id smi240_of_match[] = {
+	{ .compatible = "bosch,smi240" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, smi240_of_match);
+
+static struct spi_driver smi240_spi_driver = {
+	.probe = smi240_spi_probe,
+	.id_table = smi240_spi_id,
+	.driver = {
+		.of_match_table = of_match_ptr(smi240_of_match),
+		.name = "smi240",
+	},
+};
+module_spi_driver(smi240_spi_driver);
+
+MODULE_AUTHOR("Markus Lochmann <markus.lochmann@de.bosch.com>");
+MODULE_AUTHOR("Stefan Gutmann <stefan.gutmann@de.bosch.com>");
+MODULE_DESCRIPTION("Bosch SMI240 SPI driver");
+MODULE_LICENSE("Dual BSD/GPL");
-- 
2.34.1

Re: [PATCH v2 2/2] iio: imu: smi240: imu driver

[Christophe JAILLET]

Le 09/08/2024 à 13:16, 
Jianping.Shen-V5te9oGctAVWk0Htik3J/w@public.gmane.org a écrit :
> From: "Shen Jianping (ME-SE/EAD2)" <Jianping.Shen-V5te9oGctAVWk0Htik3J/w@public.gmane.org>
> 
> iio: imu: smi240: driver improvements
> Signed-off-by: Shen Jianping (ME-SE/EAD2) <Jianping.Shen-V5te9oGctAVWk0Htik3J/w@public.gmane.org>
> ---

Hi,

...

> diff --git a/drivers/iio/imu/smi240/smi240_core.c b/drivers/iio/imu/smi240/smi240_core.c
> new file mode 100644
> index 00000000000..4b2a4a290f3
> --- /dev/null
> +++ b/drivers/iio/imu/smi240/smi240_core.c

...

> +static int smi240_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan, int *val,
> +			   int *val2, long mask)
> +{
> +	int ret = 0;

No need to init (and in other places you don't)

> +	struct smi240_data *data = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		if (iio_buffer_enabled(indio_dev))
> +			return -EBUSY;
> +		ret = smi240_get_data(data, chan->type, chan->channel2, val);
> +		if (ret)
> +			return ret;
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> +		ret = smi240_get_low_pass_filter_freq(data, chan->type, val);
> +		if (ret)
> +			return ret;
> +		return IIO_VAL_INT;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int smi240_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan, int val, int val2,
> +			    long mask)
> +{
> +	int ret, i;
> +	struct smi240_data *data = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> +		for (i = 0; i < ARRAY_SIZE(smi240_low_pass_freqs); i++) {
> +			if (val == smi240_low_pass_freqs[i])
> +				break;
> +		}
> +
> +		if (i == ARRAY_SIZE(smi240_low_pass_freqs))
> +			return -EINVAL;
> +
> +		switch (chan->type) {
> +		case IIO_ACCEL:
> +			data->accel_filter_freq = val;
> +			break;
> +		case IIO_ANGL_VEL:
> +			data->anglvel_filter_freq = val;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	// Write access to soft config is locked until hard/soft reset
> +	ret = smi240_soft_reset(data);
> +	if (ret)
> +		return ret;

Nitpick: missing new line?

> +	ret = smi240_soft_config(data);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}

...

> +int smi240_core_probe(struct device *dev, struct regmap *regmap)
> +{
> +	struct iio_dev *indio_dev;
> +	struct smi240_data *data;
> +	int ret, response;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!indio_dev)
> +		return dev_err_probe(dev, -ENOMEM,
> +				     "Allocate iio device failed\n");

Usually messages related to memory allocation are not useful.
Just: return -ENOMEM?

> +
> +	data = iio_priv(indio_dev);
> +	dev_set_drvdata(dev, indio_dev);
> +	data->regmap = regmap;
> +	data->capture = 0;

No need to explicitly initialize 'capture', devm_iio_device_alloc() 
already zeroes the allocated emmory.
It doesn't hurt to be explicit, but why this field and not the other ones?

> +
> +	ret = regmap_read(data->regmap, SMI240_CHIP_ID_REG, &response);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Read chip id failed\n");
> +
> +	if (response != SMI240_CHIP_ID)
> +		dev_info(dev, "Unknown chip id: 0x%04x\n", response);

...

> diff --git a/drivers/iio/imu/smi240/smi240_spi.c b/drivers/iio/imu/smi240/smi240_spi.c
> new file mode 100644
> index 00000000000..ac9e37ffa37
> --- /dev/null
> +++ b/drivers/iio/imu/smi240/smi240_spi.c

...

> +static int smi240_regmap_spi_write(void *context, const void *data,
> +				   size_t count)
> +{
> +	__be32 request;
> +	struct spi_device *spi = context;
> +	u8 reg_addr = ((u8 *)data)[0];
> +	u16 reg_data = ((u8 *)data)[2] << 8 | ((u8 *)data)[1];
> +
> +	request = SMI240_BUS_ID << 30;
> +	request |= FIELD_PREP(SMI240_WRITE_BIT_MASK, 1);
> +	request |= FIELD_PREP(SMI240_WRITE_ADDR_MASK, reg_addr);
> +	request |= FIELD_PREP(SMI240_WRITE_DATA_MASK, reg_data);
> +	request |= smi240_crc3(request, SMI240_CRC_INIT, SMI240_CRC_POLY);
> +	request = cpu_to_be32(request);
> +
> +	return spi_write(spi, &request, sizeof(request));
> +}
> +
> +static struct regmap_bus smi240_regmap_bus = {

const?

> +	.read = smi240_regmap_spi_read,
> +	.write = smi240_regmap_spi_write,
> +};

...

CJ

RE: [PATCH v2 2/2] iio: imu: smi240: imu driver

[Shen Jianping (ME-SE/EAD2)]

Hi Christophe,

....
> +
> +	data = iio_priv(indio_dev);
> +	dev_set_drvdata(dev, indio_dev);
> +	data->regmap = regmap;
> +	data->capture = 0;

No need to explicitly initialize 'capture', devm_iio_device_alloc() already zeroes the allocated emmory.
It doesn't hurt to be explicit, but why this field and not the other ones?

-> This is the flag to enable capture mode. It is important to be disabled by default, therefore rather make this explicitly.

...

Jianping

Re: [PATCH v2 2/2] iio: imu: smi240: imu driver

[Krzysztof Kozlowski]

On 09/08/2024 14:18, Shen Jianping (ME-SE/EAD2) wrote:
> Hi Christophe,
> 
> ....
>> +
>> +	data = iio_priv(indio_dev);
>> +	dev_set_drvdata(dev, indio_dev);
>> +	data->regmap = regmap;
>> +	data->capture = 0;
> 
> No need to explicitly initialize 'capture', devm_iio_device_alloc() already zeroes the allocated emmory.
> It doesn't hurt to be explicit, but why this field and not the other ones?
> 
> -> This is the flag to enable capture mode. It is important to be disabled by default, therefore rather make this explicitly.

It's redundant.

Anyway, it's not suppose to be uint8 anyway, but bool or enum.

Best regards,
Krzysztof

RE: [PATCH v2 2/2] iio: imu: smi240: imu driver

[Shen Jianping (ME-SE/EAD2)]

> Hi Christophe,
> 
> ....
>> +
>> +	data = iio_priv(indio_dev);
>> +	dev_set_drvdata(dev, indio_dev);
>> +	data->regmap = regmap;
>> +	data->capture = 0;
> 
> No need to explicitly initialize 'capture', devm_iio_device_alloc() already zeroes the allocated emmory.
> It doesn't hurt to be explicit, but why this field and not the other ones?
> 
> -> This is the flag to enable capture mode. It is important to be disabled by default, therefore rather make this explicitly.

It's redundant.

-> We would like not to make our important sensor logic dependent on the underlying infrastructure behavior. Otherwise if the new version does not zero the allocated memory  (although this is very unlikely to happen), the driver won't work , and it is difficult to find the root cause. To explicitly set the initial value, makes the driver more robust.

Anyway, it's not suppose to be uint8 anyway, but bool or enum.

->We will choose the best datatype.


Best regards,
Krzysztof

Re: [PATCH v2 2/2] iio: imu: smi240: imu driver

[Krzysztof Kozlowski]

On 09/08/2024 13:16, Jianping.Shen@de.bosch.com wrote:
> From: "Shen Jianping (ME-SE/EAD2)" <Jianping.Shen@de.bosch.com>
> 
> iio: imu: smi240: driver improvements

?????

> Signed-off-by: Shen Jianping (ME-SE/EAD2) <Jianping.Shen@de.bosch.com>
> ---
> 


...

> +	ret = regmap_read(data->regmap, SMI240_CHIP_ID_REG, &response);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Read chip id failed\n");
> +
> +	if (response != SMI240_CHIP_ID)
> +		dev_info(dev, "Unknown chip id: 0x%04x\n", response);
> +
> +	ret = smi240_init(data);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Device initialization failed\n");
> +
> +	indio_dev->channels = smi240_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(smi240_channels);
> +	indio_dev->name = "smi240";
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->info = &smi240_info;
> +
> +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> +					      iio_pollfunc_store_time,
> +					      smi240_trigger_handler, NULL);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Setup triggered buffer failed\n");
> +
> +	ret = devm_iio_device_register(dev, indio_dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Register IIO device failed\n");
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(smi240_core_probe);
> +
> +MODULE_AUTHOR("Markus Lochmann <markus.lochmann@de.bosch.com>");
> +MODULE_AUTHOR("Stefan Gutmann <stefan.gutmann@de.bosch.com>");
> +MODULE_DESCRIPTION("Bosch SMI240 driver");
> +MODULE_LICENSE("Dual BSD/GPL");

Hm? How many modules do you have here? What are their names?


> diff --git a/drivers/iio/imu/smi240/smi240_spi.c b/drivers/iio/imu/smi240/smi240_spi.c
> new file mode 100644
> index 00000000000..ac9e37ffa37
> --- /dev/null
> +++ b/drivers/iio/imu/smi240/smi240_spi.c
> @@ -0,0 +1,173 @@
> +// SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0
> +/*
> + * Copyright (c) 2024 Robert Bosch GmbH.
> + *
> + */
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/regmap.h>
> +#include <linux/spi/spi.h>
> +#include <linux/bitfield.h>
> +#include <linux/iio/iio.h>
> +
> +#include "smi240.h"
> +
> +#define SMI240_CRC_INIT 0x05
> +#define SMI240_CRC_POLY 0x0B
> +#define SMI240_BUS_ID 0x00
> +
> +#define SMI240_SD_BIT_MASK 0x80000000
> +#define SMI240_CS_BIT_MASK 0x00000008
> +
> +#define SMI240_WRITE_ADDR_MASK GENMASK(29, 22)
> +#define SMI240_WRITE_BIT_MASK 0x00200000
> +#define SMI240_WRITE_DATA_MASK GENMASK(18, 3)
> +#define SMI240_CAP_BIT_MASK 0x00100000
> +#define SMI240_READ_DATA_MASK GENMASK(19, 4)
> +
> +static u8 smi240_crc3(u32 data, u8 init, u8 poly)
> +{
> +	u8 crc = init;
> +	u8 do_xor;
> +	s8 i = 31;
> +
> +	do {
> +		do_xor = crc & 0x04;
> +		crc <<= 1;
> +		crc |= 0x01 & (data >> i);
> +		if (do_xor)
> +			crc ^= poly;
> +
> +		crc &= 0x07;
> +	} while (--i >= 0);
> +
> +	return crc;
> +}
> +
> +static bool smi240_sensor_data_is_valid(u32 data)
> +{
> +	if (smi240_crc3(data, SMI240_CRC_INIT, SMI240_CRC_POLY) != 0)
> +		return false;
> +
> +	if (FIELD_GET(SMI240_SD_BIT_MASK, data) &
> +	    FIELD_GET(SMI240_CS_BIT_MASK, data))
> +		return false;
> +
> +	return true;
> +}
> +
> +static int smi240_regmap_spi_read(void *context, const void *reg_buf,
> +				  size_t reg_size, void *val_buf,
> +				  size_t val_size)
> +{
> +	int ret;
> +	__be32 request, response;
> +	struct spi_device *spi = context;
> +	struct iio_dev *indio_dev = dev_get_drvdata(&spi->dev);
> +	struct smi240_data *data = iio_priv(indio_dev);
> +
> +	request = SMI240_BUS_ID << 30;
> +	request |= FIELD_PREP(SMI240_CAP_BIT_MASK, data->capture);
> +	request |= FIELD_PREP(SMI240_WRITE_ADDR_MASK, *(u8 *)reg_buf);
> +	request |= smi240_crc3(request, SMI240_CRC_INIT, SMI240_CRC_POLY);
> +	request = cpu_to_be32(request);
> +
> +	/*
> +	 * SMI240 module consists of a 32Bit Out Of Frame (OOF)
> +	 * SPI protocol, where the slave interface responds to
> +	 * the Master request in the next frame.
> +	 * CS signal must toggle (> 700 ns) between the frames.
> +	 */
> +	ret = spi_write(spi, &request, sizeof(request));
> +	if (ret)
> +		return ret;
> +
> +	ret = spi_read(spi, &response, sizeof(response));
> +	if (ret)
> +		return ret;
> +
> +	response = be32_to_cpu(response);
> +
> +	if (!smi240_sensor_data_is_valid(response))
> +		return -EIO;
> +
> +	response = FIELD_GET(SMI240_READ_DATA_MASK, response);
> +	memcpy(val_buf, &response, val_size);
> +
> +	return 0;
> +}
> +
> +static int smi240_regmap_spi_write(void *context, const void *data,
> +				   size_t count)
> +{
> +	__be32 request;
> +	struct spi_device *spi = context;
> +	u8 reg_addr = ((u8 *)data)[0];
> +	u16 reg_data = ((u8 *)data)[2] << 8 | ((u8 *)data)[1];
> +
> +	request = SMI240_BUS_ID << 30;
> +	request |= FIELD_PREP(SMI240_WRITE_BIT_MASK, 1);
> +	request |= FIELD_PREP(SMI240_WRITE_ADDR_MASK, reg_addr);
> +	request |= FIELD_PREP(SMI240_WRITE_DATA_MASK, reg_data);
> +	request |= smi240_crc3(request, SMI240_CRC_INIT, SMI240_CRC_POLY);
> +	request = cpu_to_be32(request);
> +
> +	return spi_write(spi, &request, sizeof(request));
> +}
> +
> +static struct regmap_bus smi240_regmap_bus = {

Not const?

> +	.read = smi240_regmap_spi_read,
> +	.write = smi240_regmap_spi_write,
> +};
> +
> +static const struct regmap_config smi240_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 16,
> +	.val_format_endian = REGMAP_ENDIAN_LITTLE,
> +};
> +
> +static int smi240_spi_probe(struct spi_device *spi)
> +{
> +	struct regmap *regmap;
> +
> +	u32 max_frequency = 10000000;
> +
> +	of_property_read_u32(spi->dev.of_node, "spi-max-frequency",
> +			     &max_frequency);

Why?


> +
> +	spi->bits_per_word = 8;

That's  default.

> +	spi->max_speed_hz = max_frequency;

Why? Core does it.

> +	spi->mode = SPI_MODE_0;

I really wonder why you need all this code...

> +
> +	regmap = devm_regmap_init(&spi->dev, &smi240_regmap_bus, &spi->dev,
> +				  &smi240_regmap_config);
> +	if (IS_ERR(regmap))
> +		return dev_err_probe(&spi->dev, PTR_ERR(regmap),
> +				     "Failed to initialize SPI Regmap\n");
> +
> +	return smi240_core_probe(&spi->dev, regmap);
> +}
> +
> +static const struct spi_device_id smi240_spi_id[] = { { "smi240", 0 }, {} };

Don't wrap it.

> +MODULE_DEVICE_TABLE(spi, smi240_spi_id);
> +
> +static const struct of_device_id smi240_of_match[] = {
> +	{ .compatible = "bosch,smi240" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, smi240_of_match);
> +
> +static struct spi_driver smi240_spi_driver = {
> +	.probe = smi240_spi_probe,
> +	.id_table = smi240_spi_id,
> +	.driver = {
> +		.of_match_table = of_match_ptr(smi240_of_match),

Why did it appear? You introduce now warnings.


Best regards,
Krzysztof

RE: [PATCH v2 2/2] iio: imu: smi240: imu driver

[Shen Jianping (ME-SE/EAD2)]

Hi

On 09/08/2024 13:16, Jianping.Shen@de.bosch.com wrote:
> From: "Shen Jianping (ME-SE/EAD2)" <Jianping.Shen@de.bosch.com>
> 
> iio: imu: smi240: driver improvements

?????
Did not get your point, what is wrong here? how it shall be?

> Signed-off-by: Shen Jianping (ME-SE/EAD2) <Jianping.Shen@de.bosch.com>
> ---
> 


...

> +	ret = regmap_read(data->regmap, SMI240_CHIP_ID_REG, &response);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Read chip id failed\n");
> +
> +	if (response != SMI240_CHIP_ID)
> +		dev_info(dev, "Unknown chip id: 0x%04x\n", response);
> +
> +	ret = smi240_init(data);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Device initialization failed\n");
> +
> +	indio_dev->channels = smi240_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(smi240_channels);
> +	indio_dev->name = "smi240";
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->info = &smi240_info;
> +
> +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> +					      iio_pollfunc_store_time,
> +					      smi240_trigger_handler, NULL);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Setup triggered buffer failed\n");
> +
> +	ret = devm_iio_device_register(dev, indio_dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Register IIO device failed\n");
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(smi240_core_probe);
> +
> +MODULE_AUTHOR("Markus Lochmann <markus.lochmann@de.bosch.com>"); 
> +MODULE_AUTHOR("Stefan Gutmann <stefan.gutmann@de.bosch.com>"); 
> +MODULE_DESCRIPTION("Bosch SMI240 driver"); MODULE_LICENSE("Dual 
> +BSD/GPL");

Hm? How many modules do you have here? What are their names?

We have one module, named  "Bosch SMI240 driver". Any problem here?



> +
> +static const struct spi_device_id smi240_spi_id[] = { { "smi240", 0 
> +}, {} };

Don't wrap it.

We don't , git send-mail did it automatically for us. 


> +MODULE_DEVICE_TABLE(spi, smi240_spi_id);
> +
> +static const struct of_device_id smi240_of_match[] = {
> +	{ .compatible = "bosch,smi240" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, smi240_of_match);
> +
> +static struct spi_driver smi240_spi_driver = {
> +	.probe = smi240_spi_probe,
> +	.id_table = smi240_spi_id,
> +	.driver = {
> +		.of_match_table = of_match_ptr(smi240_of_match),

Why did it appear? You introduce now warnings.

Did not get your point, why we introduce now warnings here ?


Best regards,
Jianping Shen

Re: [PATCH v2 2/2] iio: imu: smi240: imu driver

[Krzysztof Kozlowski]

On 13/08/2024 11:41, Shen Jianping (ME-SE/EAD2) wrote:
> Hi
> 
> On 09/08/2024 13:16, Jianping.Shen@de.bosch.com wrote:
>> From: "Shen Jianping (ME-SE/EAD2)" <Jianping.Shen@de.bosch.com>
>>
>> iio: imu: smi240: driver improvements
> 
> ?????
> Did not get your point, what is wrong here? how it shall be?

See submitting patches. This does not match your commit at all. I do not
see any driver improvements done here. If so, please list all your
improvements against existing kernel driver.


> 
>> Signed-off-by: Shen Jianping (ME-SE/EAD2) <Jianping.Shen@de.bosch.com>
>> ---
>>
> 
> 
> ...
> 
>> +	ret = regmap_read(data->regmap, SMI240_CHIP_ID_REG, &response);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Read chip id failed\n");
>> +
>> +	if (response != SMI240_CHIP_ID)
>> +		dev_info(dev, "Unknown chip id: 0x%04x\n", response);
>> +
>> +	ret = smi240_init(data);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret,
>> +				     "Device initialization failed\n");
>> +
>> +	indio_dev->channels = smi240_channels;
>> +	indio_dev->num_channels = ARRAY_SIZE(smi240_channels);
>> +	indio_dev->name = "smi240";
>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>> +	indio_dev->info = &smi240_info;
>> +
>> +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
>> +					      iio_pollfunc_store_time,
>> +					      smi240_trigger_handler, NULL);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret,
>> +				     "Setup triggered buffer failed\n");
>> +
>> +	ret = devm_iio_device_register(dev, indio_dev);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Register IIO device failed\n");
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(smi240_core_probe);
>> +
>> +MODULE_AUTHOR("Markus Lochmann <markus.lochmann@de.bosch.com>"); 
>> +MODULE_AUTHOR("Stefan Gutmann <stefan.gutmann@de.bosch.com>"); 
>> +MODULE_DESCRIPTION("Bosch SMI240 driver"); MODULE_LICENSE("Dual 
>> +BSD/GPL");
> 
> Hm? How many modules do you have here? What are their names?
> 
> We have one module, named  "Bosch SMI240 driver". Any problem here?

Yes, you put MODULE_* to how many files? Two? Three? Why is it needed
everywhere?

> 
> 
> 
>> +
>> +static const struct spi_device_id smi240_spi_id[] = { { "smi240", 0 
>> +}, {} };
> 
> Don't wrap it.
> 
> We don't , git send-mail did it automatically for us. 
> 
> 
>> +MODULE_DEVICE_TABLE(spi, smi240_spi_id);
>> +
>> +static const struct of_device_id smi240_of_match[] = {
>> +	{ .compatible = "bosch,smi240" },
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, smi240_of_match);
>> +
>> +static struct spi_driver smi240_spi_driver = {
>> +	.probe = smi240_spi_probe,
>> +	.id_table = smi240_spi_id,
>> +	.driver = {
>> +		.of_match_table = of_match_ptr(smi240_of_match),
> 
> Why did it appear? You introduce now warnings.
> 
> Did not get your point, why we introduce now warnings here ?

Fix your quoting. It's impossible to figure out what is here my quote
and what is yours.

Why? Test your code properly... Drop the of_match_ptr.



Best regards,
Krzysztof

RE: [PATCH v2 2/2] iio: imu: smi240: imu driver

[Shen Jianping (ME-SE/EAD2)]

Hi
...
>>> +	ret = regmap_read(data->regmap, SMI240_CHIP_ID_REG, &response);
>>> +	if (ret)
>>> +		return dev_err_probe(dev, ret, "Read chip id failed\n");
>>> +
>>> +	if (response != SMI240_CHIP_ID)
>>> +		dev_info(dev, "Unknown chip id: 0x%04x\n", response);
>>> +
>>> +	ret = smi240_init(data);
>>> +	if (ret)
>>> +		return dev_err_probe(dev, ret,
>>> +				     "Device initialization failed\n");
>>> +
>>> +	indio_dev->channels = smi240_channels;
>>> +	indio_dev->num_channels = ARRAY_SIZE(smi240_channels);
>>> +	indio_dev->name = "smi240";
>>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>>> +	indio_dev->info = &smi240_info;
>>> +
>>> +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
>>> +					      iio_pollfunc_store_time,
>>> +					      smi240_trigger_handler, NULL);
>>> +	if (ret)
>>> +		return dev_err_probe(dev, ret,
>>> +				     "Setup triggered buffer failed\n");
>>> +
>>> +	ret = devm_iio_device_register(dev, indio_dev);
>>> +	if (ret)
>>> +		return dev_err_probe(dev, ret, "Register IIO device failed\n");
>>> +
>>> +	return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(smi240_core_probe);
>>> +
>>> +MODULE_AUTHOR("Markus Lochmann <markus.lochmann@de.bosch.com>");
>>> +MODULE_AUTHOR("Stefan Gutmann <stefan.gutmann@de.bosch.com>");
>>> +MODULE_DESCRIPTION("Bosch SMI240 driver"); MODULE_LICENSE("Dual
>>> +BSD/GPL");
>>
>> Hm? How many modules do you have here? What are their names?
>>
>> We have one module, named  "Bosch SMI240 driver". Any problem here?
>
>Yes, you put MODULE_* to how many files? Two? Three? Why is it needed
>everywhere?

We put MODULE_* in all the *.c , just like the other IMU drivers already in source tree. They do the same.


Best regards,
Jianping Shen

Re: [PATCH v2 2/2] iio: imu: smi240: imu driver

[Krzysztof Kozlowski]

On 13/08/2024 15:54, Shen Jianping (ME-SE/EAD2) wrote:
>>>> +
>>>> +MODULE_AUTHOR("Markus Lochmann <markus.lochmann@de.bosch.com>");
>>>> +MODULE_AUTHOR("Stefan Gutmann <stefan.gutmann@de.bosch.com>");
>>>> +MODULE_DESCRIPTION("Bosch SMI240 driver"); MODULE_LICENSE("Dual
>>>> +BSD/GPL");
>>>
>>> Hm? How many modules do you have here? What are their names?
>>>
>>> We have one module, named  "Bosch SMI240 driver". Any problem here?
>>
>> Yes, you put MODULE_* to how many files? Two? Three? Why is it needed
>> everywhere?
> 
> We put MODULE_* in all the *.c , just like the other IMU drivers already in source tree. They do the same.

That's not true. First, look at adis_buffer.c. no MODULE_XXX. Second,
maybe they have multiple modules, so the macros are expected. That's why
I asked you and you said you do not have more than one module.

Best regards,
Krzysztof

RE: [PATCH v2 2/2] iio: imu: smi240: imu driver

[Shen Jianping (ME-SE/EAD2)]

Hi


...
>>>>> +
>>>>> +MODULE_AUTHOR("Markus Lochmann
><markus.lochmann@de.bosch.com>");
>>>>> +MODULE_AUTHOR("Stefan Gutmann <stefan.gutmann@de.bosch.com>");
>>>>> +MODULE_DESCRIPTION("Bosch SMI240 driver"); MODULE_LICENSE("Dual
>>>>> +BSD/GPL");
>>>>
>>>> Hm? How many modules do you have here? What are their names?
>>>>
>>>> We have one module, named  "Bosch SMI240 driver". Any problem here?
>>>
>>> Yes, you put MODULE_* to how many files? Two? Three? Why is it needed
>>> everywhere?
>>
>> We put MODULE_* in all the *.c , just like the other IMU drivers already in source
>tree. They do the same.
>
>That's not true. First, look at adis_buffer.c. no MODULE_XXX. Second, maybe they
>have multiple modules, so the macros are expected. That's why I asked you and
>you said you do not have more than one module.
>
 
Now I get the point , what you mean for "module".  Yes we have two modules. 
One module named "Bosch SMI240 driver" therefore we put MODULE_* in smi240_core.c 
And we have the second module named " Bosch SMI240 SPI driver " and we put therefore MODULE_* in smi240_spi.c

Re: [PATCH v2 2/2] iio: imu: smi240: imu driver

[Krzysztof Kozlowski]

On 13/08/2024 16:51, Shen Jianping (ME-SE/EAD2) wrote:
> Hi
> 
> 
> ...
>>>>>> +
>>>>>> +MODULE_AUTHOR("Markus Lochmann
>> <markus.lochmann@de.bosch.com>");
>>>>>> +MODULE_AUTHOR("Stefan Gutmann <stefan.gutmann@de.bosch.com>");
>>>>>> +MODULE_DESCRIPTION("Bosch SMI240 driver"); MODULE_LICENSE("Dual
>>>>>> +BSD/GPL");
>>>>>
>>>>> Hm? How many modules do you have here? What are their names?
>>>>>
>>>>> We have one module, named  "Bosch SMI240 driver". Any problem here?
>>>>
>>>> Yes, you put MODULE_* to how many files? Two? Three? Why is it needed
>>>> everywhere?
>>>
>>> We put MODULE_* in all the *.c , just like the other IMU drivers already in source
>> tree. They do the same.
>>
>> That's not true. First, look at adis_buffer.c. no MODULE_XXX. Second, maybe they
>> have multiple modules, so the macros are expected. That's why I asked you and
>> you said you do not have more than one module.
>>
>  
> Now I get the point , what you mean for "module".  Yes we have two modules. 
> One module named "Bosch SMI240 driver" therefore we put MODULE_* in smi240_core.c 
> And we have the second module named " Bosch SMI240 SPI driver " and we put therefore MODULE_* in smi240_spi.c

I asked the question on purpose, so you will double check and do not
waste my time with whatever you wan to respond just to avoid work.
Replying something just to silence review is not welcomed.

If you claim you have two modules then please give me the names of the
modules (filenames).

Best regards,
Krzysztof

Re: [PATCH v2 2/2] iio: imu: smi240: imu driver

[Krzysztof Kozlowski]

On 13/08/2024 11:41, Shen Jianping (ME-SE/EAD2) wrote:
>> +EXPORT_SYMBOL_GPL(smi240_core_probe);
>> +
>> +MODULE_AUTHOR("Markus Lochmann <markus.lochmann@de.bosch.com>"); 
>> +MODULE_AUTHOR("Stefan Gutmann <stefan.gutmann@de.bosch.com>"); 
>> +MODULE_DESCRIPTION("Bosch SMI240 driver"); MODULE_LICENSE("Dual 
>> +BSD/GPL");
> 
> Hm? How many modules do you have here? What are their names?
> 
> We have one module, named  "Bosch SMI240 driver". Any problem here?

That's not the name of the module. That's description. What is the
module filename (filename=name!)?

Fix your quoting, because you misrepresent people's comments.

Best regards,
Krzysztof

RE: [PATCH v2 2/2] iio: imu: smi240: imu driver

[Shen Jianping (ME-SE/EAD2)]

>
>On 13/08/2024 11:41, Shen Jianping (ME-SE/EAD2) wrote:
>>> +EXPORT_SYMBOL_GPL(smi240_core_probe);
>>> +
>>> +MODULE_AUTHOR("Markus Lochmann <markus.lochmann@de.bosch.com>");
>>> +MODULE_AUTHOR("Stefan Gutmann <stefan.gutmann@de.bosch.com>");
>>> +MODULE_DESCRIPTION("Bosch SMI240 driver"); MODULE_LICENSE("Dual
>>> +BSD/GPL");
>>
>> Hm? How many modules do you have here? What are their names?
>>
>> We have one module, named  "Bosch SMI240 driver". Any problem here?
>
>That's not the name of the module. That's description. What is the module filename
>(filename=name!)?
>
>Fix your quoting, because you misrepresent people's comments.

Sorry to give you this feeling, No, we we don't want to silence you to avoid work.  Just do not understand the module name correctly.

filename you mean the ko filename correct ?  We have only smi240.ko as output. Therefore just one module and we shall put MODULE_* only in one source file right. 
Shall we put in smi240_core.c or smi240_spi.c ?

Best regards,
Jianping Shen

Re: [PATCH v2 2/2] iio: imu: smi240: imu driver

[Krzysztof Kozlowski]

On 09/08/2024 13:16, Jianping.Shen@de.bosch.com wrote:
> +
> +MODULE_AUTHOR("Markus Lochmann <markus.lochmann@de.bosch.com>");
> +MODULE_AUTHOR("Stefan Gutmann <stefan.gutmann@de.bosch.com>");
> +MODULE_DESCRIPTION("Bosch SMI240 driver");
> +MODULE_LICENSE("Dual BSD/GPL");

Let's be clear here: if you bothered to look at your module, you would
see that it is duplicated. I judged by code review and questioned it.
This should lead you to double and triple check. But instead of
double-checking, you engage in long discussion with reviewer. Sorry,
that's wasting my time. Your job is to check it, if you got comments on
this, not mine.

Please carefully read upstreaming guides/presentations and which
mistakes to avoid. Hint: wasting reviewer's time.

NAK.

Best regards,
Krzysztof