Add ST lsm6dso i3c support

This patch series add i3c support for STM LSM6DSO and LSM6DSR sensors.

It is also introduced i3c support on regmap api. Due the lack of i3c devices HDR capables on the market the support for now is only for i3c sdr mode by using i3c_device_do_priv_xfers() method.

The i3c regmap api is already available in the Git repository at:
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git
tags/regmap-i3c

Change in v4:
remover hw_id variable from st_lsm6dsx_i3c_probe()

Change in v3:
Update st_lsm6dsx_probe() call
Remove i3c_get_device_id() and use i3c_device_match_id()

Changes in v2:
Change i3c_get_device_id() to drivers/i3c/device.c Add support for LSM6DSR

Vitor Soares (3):
regmap: add i3c bus support
i3c: add i3c_get_device_id helper
iio: imu: st_lsm6dsx: add i3c basic support for LSM6DSO and LSM6DSR

 drivers/base/regmap/Kconfig                 |  6 ++-
 drivers/base/regmap/Makefile                |  1 +
 drivers/base/regmap/regmap-i3c.c            | 60 +++++++++++++++++++++++++++++
 drivers/i3c/device.c                        | 46 ++++++++++++++++++++++
 drivers/i3c/master.c                        | 45 ----------------------
 drivers/iio/imu/st_lsm6dsx/Kconfig          |  8 +++-
 drivers/iio/imu/st_lsm6dsx/Makefile         |  1 +
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c | 58 ++++++++++++++++++++++++++++
 include/linux/i3c/device.h                  |  4 ++
 include/linux/regmap.h                      | 20 ++++++++++
 10 files changed, 202 insertions(+), 47 deletions(-)
 create mode 100644 drivers/base/regmap/regmap-i3c.c
 create mode 100644 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c

Add basic support for i3c bus.
This is a simple implementation that only give support for SDR Read and Write commands.

Signed-off-by Vitor Soares <[email protected]>

---
 drivers/base/regmap/Kconfig      |  6 +++-
 drivers/base/regmap/Makefile     |  1 +
 drivers/base/regmap/regmap-i3c.c | 60 ++++++++++++++++++++++++++++++++++++++++
 include/linux/regmap.h           | 20 ++++++++++++++
 4 files changed, 86 insertions(+), 1 deletion(-)
 create mode 100644 drivers/base/regmap/regmap-i3c.c

drivers/base/regmap/Kconfig +5 -1

index 6ad5ef4..c8bbf53 100644
@@ -4,7 +4,7 @@
 # subsystems should select the appropriate symbols.
 
 config REGMAP
-	default y if (REGMAP_I2C || REGMAP_SPI || REGMAP_SPMI || REGMAP_W1 || REGMAP_AC97 || REGMAP_MMIO || REGMAP_IRQ)
+	default y if (REGMAP_I2C || REGMAP_SPI || REGMAP_SPMI || REGMAP_W1 || REGMAP_AC97 || REGMAP_MMIO || REGMAP_IRQ || REGMAP_I3C)
 	select IRQ_DOMAIN if REGMAP_IRQ
 	bool
 

@@ -49,3 +49,7 @@ config REGMAP_SOUNDWIRE
 config REGMAP_SCCB
 	tristate
 	depends on I2C
+
+config REGMAP_I3C
+	tristate
+	depends on I3C

drivers/base/regmap/Makefile +1 -0

index f5b4e88..ff6c7d8 100644
@@ -16,3 +16,4 @@ obj-$(CONFIG_REGMAP_IRQ) += regmap-irq.o
 obj-$(CONFIG_REGMAP_W1) += regmap-w1.o
 obj-$(CONFIG_REGMAP_SOUNDWIRE) += regmap-sdw.o
 obj-$(CONFIG_REGMAP_SCCB) += regmap-sccb.o
+obj-$(CONFIG_REGMAP_I3C) += regmap-i3c.o

drivers/base/regmap/regmap-i3c.c new file +60 -0

new file mode 100644
index 0000000..1578fb5
@@ -0,0 +1,60 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2018 Synopsys, Inc. and/or its affiliates.
+
+#include <linux/regmap.h>
+#include <linux/i3c/device.h>
+#include <linux/i3c/master.h>
+#include <linux/module.h>
+
+static int regmap_i3c_write(void *context, const void *data, size_t count)
+{
+	struct device *dev = context;
+	struct i3c_device *i3c = dev_to_i3cdev(dev);
+	struct i3c_priv_xfer xfers[] = {
+		{
+			.rnw = false,
+			.len = count,
+			.data.out = data,
+		},
+	};
+
+	return i3c_device_do_priv_xfers(i3c, xfers, 1);
+}
+
+static int regmap_i3c_read(void *context,
+			   const void *reg, size_t reg_size,
+			   void *val, size_t val_size)
+{
+	struct device *dev = context;
+	struct i3c_device *i3c = dev_to_i3cdev(dev);
+	struct i3c_priv_xfer xfers[2];
+
+	xfers[0].rnw = false;
+	xfers[0].len = reg_size;
+	xfers[0].data.out = reg;
+
+	xfers[1].rnw = true;
+	xfers[1].len = val_size;
+	xfers[1].data.in = val;
+
+	return i3c_device_do_priv_xfers(i3c, xfers, 2);
+}
+
+static struct regmap_bus regmap_i3c = {
+	.write = regmap_i3c_write,
+	.read = regmap_i3c_read,
+};
+
+struct regmap *__devm_regmap_init_i3c(struct i3c_device *i3c,
+				      const struct regmap_config *config,
+				      struct lock_class_key *lock_key,
+				      const char *lock_name)
+{
+	return __devm_regmap_init(&i3c->dev, &regmap_i3c, &i3c->dev, config,
+				  lock_key, lock_name);
+}
+EXPORT_SYMBOL_GPL(__devm_regmap_init_i3c);
+
+MODULE_AUTHOR("Vitor Soares <[email protected]>");
+MODULE_DESCRIPTION("Regmap I3C Module");
+MODULE_LICENSE("GPL v2");

include/linux/regmap.h +20 -0

index daeec7d..f65984d 100644
@@ -25,6 +25,7 @@ struct module;
 struct clk;
 struct device;
 struct i2c_client;
+struct i3c_device;
 struct irq_domain;
 struct slim_device;
 struct spi_device;

@@ -624,6 +625,10 @@ struct regmap *__devm_regmap_init_slimbus(struct slim_device *slimbus,
 				 const struct regmap_config *config,
 				 struct lock_class_key *lock_key,
 				 const char *lock_name);
+struct regmap *__devm_regmap_init_i3c(struct i3c_device *i3c,
+				 const struct regmap_config *config,
+				 struct lock_class_key *lock_key,
+				 const char *lock_name);
 /*
  * Wrapper for regmap_init macros to include a unique lockdep key and name
  * for each call. No-op if CONFIG_LOCKDEP is not set.

@@ -982,6 +987,21 @@ bool regmap_ac97_default_volatile(struct device *dev, unsigned int reg);
 #define devm_regmap_init_slimbus(slimbus, config)			\
 	__regmap_lockdep_wrapper(__devm_regmap_init_slimbus, #config,	\
 				slimbus, config)
+
+/**
+ * devm_regmap_init_i3c() - Initialise managed register map
+ *
+ * @i3c: Device that will be interacted with
+ * @config: Configuration for register map
+ *
+ * The return value will be an ERR_PTR() on error or a valid pointer
+ * to a struct regmap.  The regmap will be automatically freed by the
+ * device management code.
+ */
+#define devm_regmap_init_i3c(i3c, config)				\
+	__regmap_lockdep_wrapper(__devm_regmap_init_i3c, #config,	\
+				i3c, config)
+
 int regmap_mmio_attach_clk(struct regmap *map, struct clk *clk);
 void regmap_mmio_detach_clk(struct regmap *map);
 void regmap_exit(struct regmap *map);

The i3c device driver needs the i3c_device_id table.

Lets move to device.c and export it to be used.

Signed-off-by Vitor Soares <[email protected]>

---
Changes in v4:
None

Changes in v3:
Remove i3c_get_device_id
Move i3c_device_match_id from drivers/i3c/master.c to drivers/i3c/device.c Export i3c_device_match_id

Changes in v2:
move this function to drivers/i3c/device.c

 drivers/i3c/device.c       | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/i3c/master.c       | 45 ---------------------------------------------
 include/linux/i3c/device.h |  4 ++++
 3 files changed, 50 insertions(+), 45 deletions(-)

drivers/i3c/device.c +0 -0

index 69cc040..383df3b 100644

drivers/i3c/device.c +46 -0

+const struct i3c_device_id *
+i3c_device_match_id(struct i3c_device *i3cdev,
+		    const struct i3c_device_id *id_table)
+{
+	struct i3c_device_info devinfo;
+	const struct i3c_device_id *id;
+
+	i3c_device_get_info(i3cdev, &devinfo);
+
+	/*
+	 * The lower 32bits of the provisional ID is just filled with a random
+	 * value, try to match using DCR info.
+	 */
+	if (!I3C_PID_RND_LOWER_32BITS(devinfo.pid)) {
+		u16 manuf = I3C_PID_MANUF_ID(devinfo.pid);
+		u16 part = I3C_PID_PART_ID(devinfo.pid);
+		u16 ext_info = I3C_PID_EXTRA_INFO(devinfo.pid);
+
+		/* First try to match by manufacturer/part ID. */
+		for (id = id_table; id->match_flags != 0; id++) {
+			if ((id->match_flags & I3C_MATCH_MANUF_AND_PART) !=
+			    I3C_MATCH_MANUF_AND_PART)
+				continue;
+
+			if (manuf != id->manuf_id || part != id->part_id)
+				continue;
+
+			if ((id->match_flags & I3C_MATCH_EXTRA_INFO) &&
+			    ext_info != id->extra_info)
+				continue;
+
+			return id;
+		}
+	}
+
+	/* Fallback to DCR match. */
+	for (id = id_table; id->match_flags != 0; id++) {
+		if ((id->match_flags & I3C_MATCH_DCR) &&
+		    id->dcr == devinfo.dcr)
+			return id;
+	}
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(i3c_device_match_id);
+
 /**
  * i3c_driver_register_with_owner() - register an I3C device driver
  *

drivers/i3c/master.c +0 -45

index 5f4bd52..7667f84 100644
@@ -270,51 +270,6 @@ static const struct device_type i3c_device_type = {
 	.uevent = i3c_device_uevent,
 };
 
-static const struct i3c_device_id *
-i3c_device_match_id(struct i3c_device *i3cdev,
-		    const struct i3c_device_id *id_table)
-{
-	struct i3c_device_info devinfo;
-	const struct i3c_device_id *id;
-
-	i3c_device_get_info(i3cdev, &devinfo);
-
-	/*
-	 * The lower 32bits of the provisional ID is just filled with a random
-	 * value, try to match using DCR info.
-	 */
-	if (!I3C_PID_RND_LOWER_32BITS(devinfo.pid)) {
-		u16 manuf = I3C_PID_MANUF_ID(devinfo.pid);
-		u16 part = I3C_PID_PART_ID(devinfo.pid);
-		u16 ext_info = I3C_PID_EXTRA_INFO(devinfo.pid);
-
-		/* First try to match by manufacturer/part ID. */
-		for (id = id_table; id->match_flags != 0; id++) {
-			if ((id->match_flags & I3C_MATCH_MANUF_AND_PART) !=
-			    I3C_MATCH_MANUF_AND_PART)
-				continue;
-
-			if (manuf != id->manuf_id || part != id->part_id)
-				continue;
-
-			if ((id->match_flags & I3C_MATCH_EXTRA_INFO) &&
-			    ext_info != id->extra_info)
-				continue;
-
-			return id;
-		}
-	}
-
-	/* Fallback to DCR match. */
-	for (id = id_table; id->match_flags != 0; id++) {
-		if ((id->match_flags & I3C_MATCH_DCR) &&
-		    id->dcr == devinfo.dcr)
-			return id;
-	}
-
-	return NULL;
-}
-
 static int i3c_device_match(struct device *dev, struct device_driver *drv)
 {
 	struct i3c_device *i3cdev;

include/linux/i3c/device.h +4 -0

index 5ecb055..de102e4 100644
@@ -188,6 +188,10 @@ static inline struct i3c_driver *drv_to_i3cdrv(struct device_driver *drv)
 struct device *i3cdev_to_dev(struct i3c_device *i3cdev);
 struct i3c_device *dev_to_i3cdev(struct device *dev);
 
+const struct i3c_device_id *
+i3c_device_match_id(struct i3c_device *i3cdev,
+		    const struct i3c_device_id *id_table);
+
 static inline void i3cdev_set_drvdata(struct i3c_device *i3cdev,
 				      void *data)
 {

For today the st_lsm6dsx driver support LSM6DSO and LSM6DSR sensor only in spi and i2c mode.

The LSM6DSO and LSM6DSR are also i3c capable so lets give i3c support to them.

Signed-off-by Vitor Soares <[email protected]>

Acked-by Lorenzo Bianconi <[email protected]>

---
Changes in v4:
Remove hw_id variable

Changes in v3:
Remove unnecessary st_lsm6dsx_i3c_data table used to hold device name Use st_lsm6dsx_probe new form

Changes in v2:
Add support for LSM6DSR
Set pm_ops to st_lsm6dsx_pm_ops

 drivers/iio/imu/st_lsm6dsx/Kconfig          |  8 +++-
 drivers/iio/imu/st_lsm6dsx/Makefile         |  1 +
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c | 58 +++++++++++++++++++++++++++++
 3 files changed, 66 insertions(+), 1 deletion(-)
 create mode 100644 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c

drivers/iio/imu/st_lsm6dsx/Kconfig +7 -1

index 9e59297..6b5a73c 100644
@@ -1,11 +1,12 @@
 
 config IIO_ST_LSM6DSX
 	tristate "ST_LSM6DSx driver for STM 6-axis IMU MEMS sensors"
-	depends on (I2C || SPI)
+	depends on (I2C || SPI || I3C)
 	select IIO_BUFFER
 	select IIO_KFIFO_BUF
 	select IIO_ST_LSM6DSX_I2C if (I2C)
 	select IIO_ST_LSM6DSX_SPI if (SPI_MASTER)
+	select IIO_ST_LSM6DSX_I3C if (I3C)
 	help
 	  Say yes here to build support for STMicroelectronics LSM6DSx imu
 	  sensor. Supported devices: lsm6ds3, lsm6ds3h, lsm6dsl, lsm6dsm,

@@ -23,3 +24,8 @@ config IIO_ST_LSM6DSX_SPI
 	tristate
 	depends on IIO_ST_LSM6DSX
 	select REGMAP_SPI
+
+config IIO_ST_LSM6DSX_I3C
+	tristate
+	depends on IIO_ST_LSM6DSX
+	select REGMAP_I3C

drivers/iio/imu/st_lsm6dsx/Makefile +1 -0

index e5f733c..c676965 100644
@@ -4,3 +4,4 @@ st_lsm6dsx-y := st_lsm6dsx_core.o st_lsm6dsx_buffer.o \
 obj-$(CONFIG_IIO_ST_LSM6DSX) += st_lsm6dsx.o
 obj-$(CONFIG_IIO_ST_LSM6DSX_I2C) += st_lsm6dsx_i2c.o
 obj-$(CONFIG_IIO_ST_LSM6DSX_SPI) += st_lsm6dsx_spi.o
+obj-$(CONFIG_IIO_ST_LSM6DSX_I3C) += st_lsm6dsx_i3c.o

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +10 -0

new file mode 100644
index 0000000..2e89524
@@ -0,0 +1,58 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018 Synopsys, Inc. and/or its affiliates.
+ *
+ * Author: Vitor Soares <[email protected]>
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/i3c/device.h>

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +15 -0

+}
+
+static struct i3c_driver st_lsm6dsx_driver = {
+	.driver = {
+		.name = "st_lsm6dsx_i3c",
+		.pm = &st_lsm6dsx_pm_ops,
+	},
+	.probe = st_lsm6dsx_i3c_probe,
+	.id_table = st_lsm6dsx_i3c_ids,
+};
+module_i3c_driver(st_lsm6dsx_driver);
+
+MODULE_AUTHOR("Vitor Soares <[email protected]>");
+MODULE_DESCRIPTION("STMicroelectronics st_lsm6dsx i3c driver");
+MODULE_LICENSE("GPL v2");

Quoted 4 lines · On Fri, 12 Jul 2019 13:53:28 +0200 Vitor Soares <[email protected]>…

Add basic support for i3c bus.
This is a simple implementation that only give support for SDR Read and Write commands.

This patch has been applied by Mark already. Please make sure to drop already applied patches when submitting a new version.

Quoted 151 lines

Signed-off-by Vitor Soares <[email protected]>

---
 drivers/base/regmap/Kconfig      |  6 +++-
 drivers/base/regmap/Makefile     |  1 +
 drivers/base/regmap/regmap-i3c.c | 60 ++++++++++++++++++++++++++++++++++++++++
 include/linux/regmap.h           | 20 ++++++++++++++
 4 files changed, 86 insertions(+), 1 deletion(-)
 create mode 100644 drivers/base/regmap/regmap-i3c.c

drivers/base/regmap/Kconfig +5 -1

index 6ad5ef4..c8bbf53 100644
@@ -4,7 +4,7 @@
 # subsystems should select the appropriate symbols.
 
 config REGMAP
-	default y if (REGMAP_I2C || REGMAP_SPI || REGMAP_SPMI || REGMAP_W1 || REGMAP_AC97 || REGMAP_MMIO || REGMAP_IRQ)
+	default y if (REGMAP_I2C || REGMAP_SPI || REGMAP_SPMI || REGMAP_W1 || REGMAP_AC97 || REGMAP_MMIO || REGMAP_IRQ || REGMAP_I3C)
 	select IRQ_DOMAIN if REGMAP_IRQ
 	bool
 

@@ -49,3 +49,7 @@ config REGMAP_SOUNDWIRE
 config REGMAP_SCCB
 	tristate
 	depends on I2C
+
+config REGMAP_I3C
+	tristate
+	depends on I3C

drivers/base/regmap/Makefile +1 -0

index f5b4e88..ff6c7d8 100644
@@ -16,3 +16,4 @@ obj-$(CONFIG_REGMAP_IRQ) += regmap-irq.o
 obj-$(CONFIG_REGMAP_W1) += regmap-w1.o
 obj-$(CONFIG_REGMAP_SOUNDWIRE) += regmap-sdw.o
 obj-$(CONFIG_REGMAP_SCCB) += regmap-sccb.o
+obj-$(CONFIG_REGMAP_I3C) += regmap-i3c.o

drivers/base/regmap/regmap-i3c.c new file +60 -0

new file mode 100644
index 0000000..1578fb5
@@ -0,0 +1,60 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2018 Synopsys, Inc. and/or its affiliates.
+
+#include <linux/regmap.h>
+#include <linux/i3c/device.h>
+#include <linux/i3c/master.h>
+#include <linux/module.h>
+
+static int regmap_i3c_write(void *context, const void *data, size_t count)
+{
+	struct device *dev = context;
+	struct i3c_device *i3c = dev_to_i3cdev(dev);
+	struct i3c_priv_xfer xfers[] = {
+		{
+			.rnw = false,
+			.len = count,
+			.data.out = data,
+		},
+	};
+
+	return i3c_device_do_priv_xfers(i3c, xfers, 1);
+}
+
+static int regmap_i3c_read(void *context,
+			   const void *reg, size_t reg_size,
+			   void *val, size_t val_size)
+{
+	struct device *dev = context;
+	struct i3c_device *i3c = dev_to_i3cdev(dev);
+	struct i3c_priv_xfer xfers[2];
+
+	xfers[0].rnw = false;
+	xfers[0].len = reg_size;
+	xfers[0].data.out = reg;
+
+	xfers[1].rnw = true;
+	xfers[1].len = val_size;
+	xfers[1].data.in = val;
+
+	return i3c_device_do_priv_xfers(i3c, xfers, 2);
+}
+
+static struct regmap_bus regmap_i3c = {
+	.write = regmap_i3c_write,
+	.read = regmap_i3c_read,
+};
+
+struct regmap *__devm_regmap_init_i3c(struct i3c_device *i3c,
+				      const struct regmap_config *config,
+				      struct lock_class_key *lock_key,
+				      const char *lock_name)
+{
+	return __devm_regmap_init(&i3c->dev, &regmap_i3c, &i3c->dev, config,
+				  lock_key, lock_name);
+}
+EXPORT_SYMBOL_GPL(__devm_regmap_init_i3c);
+
+MODULE_AUTHOR("Vitor Soares <[email protected]>");
+MODULE_DESCRIPTION("Regmap I3C Module");
+MODULE_LICENSE("GPL v2");

include/linux/regmap.h +20 -0

index daeec7d..f65984d 100644
@@ -25,6 +25,7 @@ struct module;
 struct clk;
 struct device;
 struct i2c_client;
+struct i3c_device;
 struct irq_domain;
 struct slim_device;
 struct spi_device;

@@ -624,6 +625,10 @@ struct regmap *__devm_regmap_init_slimbus(struct slim_device *slimbus,
 				 const struct regmap_config *config,
 				 struct lock_class_key *lock_key,
 				 const char *lock_name);
+struct regmap *__devm_regmap_init_i3c(struct i3c_device *i3c,
+				 const struct regmap_config *config,
+				 struct lock_class_key *lock_key,
+				 const char *lock_name);
 /*
  * Wrapper for regmap_init macros to include a unique lockdep key and name
  * for each call. No-op if CONFIG_LOCKDEP is not set.

@@ -982,6 +987,21 @@ bool regmap_ac97_default_volatile(struct device *dev, unsigned int reg);
 #define devm_regmap_init_slimbus(slimbus, config)			\
 	__regmap_lockdep_wrapper(__devm_regmap_init_slimbus, #config,	\
 				slimbus, config)
+
+/**
+ * devm_regmap_init_i3c() - Initialise managed register map
+ *
+ * @i3c: Device that will be interacted with
+ * @config: Configuration for register map
+ *
+ * The return value will be an ERR_PTR() on error or a valid pointer
+ * to a struct regmap.  The regmap will be automatically freed by the
+ * device management code.
+ */
+#define devm_regmap_init_i3c(i3c, config)				\
+	__regmap_lockdep_wrapper(__devm_regmap_init_i3c, #config,	\
+				i3c, config)
+
 int regmap_mmio_attach_clk(struct regmap *map, struct clk *clk);
 void regmap_mmio_detach_clk(struct regmap *map);
 void regmap_exit(struct regmap *map);

Quoted 1 line · On Fri, 12 Jul 2019 13:53:29 +0200 Vitor Soares <[email protected]>…

The i3c device driver needs the i3c_device_id table.

"Some I3C device drivers need to know which entry matches the i3c_device object passed to the probe function"

Quoted 1 line

Lets move to device.c and export it to be used.

"Let's move i3c_device_match_id() to device.c and export it so it can be used by drivers."

Quoted 19 lines

Signed-off-by Vitor Soares <[email protected]>

---
Changes in v4:
None

Changes in v3:
Remove i3c_get_device_id
Move i3c_device_match_id from drivers/i3c/master.c to drivers/i3c/device.c Export i3c_device_match_id

Changes in v2:
move this function to drivers/i3c/device.c

 drivers/i3c/device.c       | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/i3c/master.c       | 45 ---------------------------------------------
 include/linux/i3c/device.h |  4 ++++
 3 files changed, 50 insertions(+), 45 deletions(-)

drivers/i3c/device.c struct i3c_device *dev_to_i3cdev(struct device *dev) · line 202

Quoted 8 lines

drivers/i3c/device.c +0 -0

index 69cc040..383df3b 100644
@@ -200,6 +200,52 @@ struct i3c_device *dev_to_i3cdev(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(dev_to_i3cdev);
 

You're missing a kerneldoc here.

Quoted 114 lines

drivers/i3c/device.c +46 -0

+const struct i3c_device_id *
+i3c_device_match_id(struct i3c_device *i3cdev,
+		    const struct i3c_device_id *id_table)
+{
+	struct i3c_device_info devinfo;
+	const struct i3c_device_id *id;
+
+	i3c_device_get_info(i3cdev, &devinfo);
+
+	/*
+	 * The lower 32bits of the provisional ID is just filled with a random
+	 * value, try to match using DCR info.
+	 */
+	if (!I3C_PID_RND_LOWER_32BITS(devinfo.pid)) {
+		u16 manuf = I3C_PID_MANUF_ID(devinfo.pid);
+		u16 part = I3C_PID_PART_ID(devinfo.pid);
+		u16 ext_info = I3C_PID_EXTRA_INFO(devinfo.pid);
+
+		/* First try to match by manufacturer/part ID. */
+		for (id = id_table; id->match_flags != 0; id++) {
+			if ((id->match_flags & I3C_MATCH_MANUF_AND_PART) !=
+			    I3C_MATCH_MANUF_AND_PART)
+				continue;
+
+			if (manuf != id->manuf_id || part != id->part_id)
+				continue;
+
+			if ((id->match_flags & I3C_MATCH_EXTRA_INFO) &&
+			    ext_info != id->extra_info)
+				continue;
+
+			return id;
+		}
+	}
+
+	/* Fallback to DCR match. */
+	for (id = id_table; id->match_flags != 0; id++) {
+		if ((id->match_flags & I3C_MATCH_DCR) &&
+		    id->dcr == devinfo.dcr)
+			return id;
+	}
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(i3c_device_match_id);
+
 /**
  * i3c_driver_register_with_owner() - register an I3C device driver
  *

drivers/i3c/master.c +0 -45

index 5f4bd52..7667f84 100644
@@ -270,51 +270,6 @@ static const struct device_type i3c_device_type = {
 	.uevent = i3c_device_uevent,
 };
 
-static const struct i3c_device_id *
-i3c_device_match_id(struct i3c_device *i3cdev,
-		    const struct i3c_device_id *id_table)
-{
-	struct i3c_device_info devinfo;
-	const struct i3c_device_id *id;
-
-	i3c_device_get_info(i3cdev, &devinfo);
-
-	/*
-	 * The lower 32bits of the provisional ID is just filled with a random
-	 * value, try to match using DCR info.
-	 */
-	if (!I3C_PID_RND_LOWER_32BITS(devinfo.pid)) {
-		u16 manuf = I3C_PID_MANUF_ID(devinfo.pid);
-		u16 part = I3C_PID_PART_ID(devinfo.pid);
-		u16 ext_info = I3C_PID_EXTRA_INFO(devinfo.pid);
-
-		/* First try to match by manufacturer/part ID. */
-		for (id = id_table; id->match_flags != 0; id++) {
-			if ((id->match_flags & I3C_MATCH_MANUF_AND_PART) !=
-			    I3C_MATCH_MANUF_AND_PART)
-				continue;
-
-			if (manuf != id->manuf_id || part != id->part_id)
-				continue;
-
-			if ((id->match_flags & I3C_MATCH_EXTRA_INFO) &&
-			    ext_info != id->extra_info)
-				continue;
-
-			return id;
-		}
-	}
-
-	/* Fallback to DCR match. */
-	for (id = id_table; id->match_flags != 0; id++) {
-		if ((id->match_flags & I3C_MATCH_DCR) &&
-		    id->dcr == devinfo.dcr)
-			return id;
-	}
-
-	return NULL;
-}
-
 static int i3c_device_match(struct device *dev, struct device_driver *drv)
 {
 	struct i3c_device *i3cdev;

include/linux/i3c/device.h +0 -0

index 5ecb055..de102e4 100644
@@ -188,6 +188,10 @@ static inline struct i3c_driver *drv_to_i3cdrv(struct device_driver *drv)
 struct device *i3cdev_to_dev(struct i3c_device *i3cdev);
 struct i3c_device *dev_to_i3cdev(struct device *dev);
 

include/linux/i3c/device.h static inline struct i3c_driver *drv_to_i3cdrv(struct device_driver *drv) · line 197

Quoted 7 lines

include/linux/i3c/device.h +4 -0

+const struct i3c_device_id *
+i3c_device_match_id(struct i3c_device *i3cdev,
+		    const struct i3c_device_id *id_table);
+
 static inline void i3cdev_set_drvdata(struct i3c_device *i3cdev,
 				      void *data)
 {

_______________________________________________
linux-i3c mailing list
[email protected]
https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.infradead.org_mailman_listinfo_linux-2Di3c&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=3BEF5uxfJXeA-aip7adilYENAWYp6xNoJXvidZZZpY4&s=JcBzwEcWAIoOCufxAYFd4YjCZppBQDA2-nVqfV1Cj5g&e=

Quoted 78 lines · On Fri, 12 Jul 2019 13:53:30 +0200 Vitor Soares <[email protected]>…

For today the st_lsm6dsx driver support LSM6DSO and LSM6DSR sensor only in spi and i2c mode.

The LSM6DSO and LSM6DSR are also i3c capable so lets give i3c support to them.

Signed-off-by Vitor Soares <[email protected]>

Acked-by Lorenzo Bianconi <[email protected]>

---
Changes in v4:
Remove hw_id variable

Changes in v3:
Remove unnecessary st_lsm6dsx_i3c_data table used to hold device name Use st_lsm6dsx_probe new form

Changes in v2:
Add support for LSM6DSR
Set pm_ops to st_lsm6dsx_pm_ops

 drivers/iio/imu/st_lsm6dsx/Kconfig          |  8 +++-
 drivers/iio/imu/st_lsm6dsx/Makefile         |  1 +
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c | 58 +++++++++++++++++++++++++++++
 3 files changed, 66 insertions(+), 1 deletion(-)
 create mode 100644 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c

drivers/iio/imu/st_lsm6dsx/Kconfig +7 -1

index 9e59297..6b5a73c 100644
@@ -1,11 +1,12 @@
 
 config IIO_ST_LSM6DSX
 	tristate "ST_LSM6DSx driver for STM 6-axis IMU MEMS sensors"
-	depends on (I2C || SPI)
+	depends on (I2C || SPI || I3C)
 	select IIO_BUFFER
 	select IIO_KFIFO_BUF
 	select IIO_ST_LSM6DSX_I2C if (I2C)
 	select IIO_ST_LSM6DSX_SPI if (SPI_MASTER)
+	select IIO_ST_LSM6DSX_I3C if (I3C)
 	help
 	  Say yes here to build support for STMicroelectronics LSM6DSx imu
 	  sensor. Supported devices: lsm6ds3, lsm6ds3h, lsm6dsl, lsm6dsm,

@@ -23,3 +24,8 @@ config IIO_ST_LSM6DSX_SPI
 	tristate
 	depends on IIO_ST_LSM6DSX
 	select REGMAP_SPI
+
+config IIO_ST_LSM6DSX_I3C
+	tristate
+	depends on IIO_ST_LSM6DSX
+	select REGMAP_I3C

drivers/iio/imu/st_lsm6dsx/Makefile +1 -0

index e5f733c..c676965 100644
@@ -4,3 +4,4 @@ st_lsm6dsx-y := st_lsm6dsx_core.o st_lsm6dsx_buffer.o \
 obj-$(CONFIG_IIO_ST_LSM6DSX) += st_lsm6dsx.o
 obj-$(CONFIG_IIO_ST_LSM6DSX_I2C) += st_lsm6dsx_i2c.o
 obj-$(CONFIG_IIO_ST_LSM6DSX_SPI) += st_lsm6dsx_spi.o
+obj-$(CONFIG_IIO_ST_LSM6DSX_I3C) += st_lsm6dsx_i3c.o

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +10 -0

new file mode 100644
index 0000000..2e89524
@@ -0,0 +1,58 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018 Synopsys, Inc. and/or its affiliates.
+ *
+ * Author: Vitor Soares <[email protected]>
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/i3c/device.h>

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c line 20

Quoted 10 lines

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +10 -0

+#include <linux/i3c/master.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+
+#include "st_lsm6dsx.h"
+
+static const struct i3c_device_id st_lsm6dsx_i3c_ids[] = {
+	I3C_DEVICE(0x0104, 0x006C, (void *)ST_LSM6DSO_ID),
+	I3C_DEVICE(0x0104, 0x006B, (void *)ST_LSM6DSR_ID),

I think you need an uintptr_t cast here:

	I3C_DEVICE(0x0104, 0x006C, (void *)(uintptr_t)ST_LSM6DSO_ID),
	I3C_DEVICE(0x0104, 0x006B, (void *)(uintptr_t)ST_LSM6DSR_ID),

otherwise gcc might complain that the integer and pointer do not have the same size (on 64-bit architectures).

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c line 28

Quoted 9 lines

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +8 -0

+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(i3c, st_lsm6dsx_i3c_ids);
+
+static const struct regmap_config st_lsm6dsx_i3c_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+};

This can be moved ...

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c line 31

Quoted 3 lines

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +3 -0

+
+static int st_lsm6dsx_i3c_probe(struct i3c_device *i3cdev)
+{

... here without the static and const qualifiers:

	struct regmap_config regmap_config = {
		.reg_bits = 8,
		.val_bits = 8,
	};

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c line 43

Quoted 12 lines

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +12 -0

+	const struct i3c_device_id *id = i3c_device_match_id(i3cdev,
+							    st_lsm6dsx_i3c_ids);
+	struct regmap *regmap;
+
+	regmap = devm_regmap_init_i3c(i3cdev, &st_lsm6dsx_i3c_regmap_config);
+	if (IS_ERR(regmap)) {
+		dev_err(&i3cdev->dev, "Failed to register i3c regmap %d\n",
+			(int)PTR_ERR(regmap));
+		return PTR_ERR(regmap);
+	}
+
+	return st_lsm6dsx_probe(&i3cdev->dev, 0, (int)id->data, regmap);

uintptr_t cast here.

Quoted 5 lines

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +5 -0

+}
+
+static struct i3c_driver st_lsm6dsx_driver = {
+	.driver = {
+		.name = "st_lsm6dsx_i3c",

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c line 58

Quoted 10 lines

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +10 -0

+		.pm = &st_lsm6dsx_pm_ops,
+	},
+	.probe = st_lsm6dsx_i3c_probe,
+	.id_table = st_lsm6dsx_i3c_ids,
+};
+module_i3c_driver(st_lsm6dsx_driver);
+
+MODULE_AUTHOR("Vitor Soares <[email protected]>");
+MODULE_DESCRIPTION("STMicroelectronics st_lsm6dsx i3c driver");
+MODULE_LICENSE("GPL v2");

Quoted 2 lines

You're missing a kerneldoc here.

I will do that. Can you clarify why we need that?

Quoted 39 lines

Quoted 39 lines

drivers/i3c/device.c +38 -0

+const struct i3c_device_id *
+i3c_device_match_id(struct i3c_device *i3cdev,
+		    const struct i3c_device_id *id_table)
+{
+	struct i3c_device_info devinfo;
+	const struct i3c_device_id *id;
+
+	i3c_device_get_info(i3cdev, &devinfo);
+
+	/*
+	 * The lower 32bits of the provisional ID is just filled with a random
+	 * value, try to match using DCR info.
+	 */
+	if (!I3C_PID_RND_LOWER_32BITS(devinfo.pid)) {
+		u16 manuf = I3C_PID_MANUF_ID(devinfo.pid);
+		u16 part = I3C_PID_PART_ID(devinfo.pid);
+		u16 ext_info = I3C_PID_EXTRA_INFO(devinfo.pid);
+
+		/* First try to match by manufacturer/part ID. */
+		for (id = id_table; id->match_flags != 0; id++) {
+			if ((id->match_flags & I3C_MATCH_MANUF_AND_PART) !=
+			    I3C_MATCH_MANUF_AND_PART)
+				continue;
+
+			if (manuf != id->manuf_id || part != id->part_id)
+				continue;
+
+			if ((id->match_flags & I3C_MATCH_EXTRA_INFO) &&
+			    ext_info != id->extra_info)
+				continue;
+
+			return id;
+		}
+	}
+
+	/* Fallback to DCR match. */
+	for (id = id_table; id->match_flags != 0; id++) {
+		if ((id->match_flags & I3C_MATCH_DCR) &&

Quoted 8 lines

drivers/i3c/device.c struct i3c_device *dev_to_i3cdev(struct device *dev) · line 248

Quoted 8 lines

drivers/i3c/device.c +8 -0

+		    id->dcr == devinfo.dcr)
+			return id;
+	}
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(i3c_device_match_id);
+

From: Boris Brezillon <[email protected]>
Date: Fri, Jul 12, 2019 at 17:14:29

Quoted 94 lines

Quoted 78 lines · On Fri, 12 Jul 2019 13:53:30 +0200 Vitor Soares <[email protected]>…

For today the st_lsm6dsx driver support LSM6DSO and LSM6DSR sensor only in spi and i2c mode.

The LSM6DSO and LSM6DSR are also i3c capable so lets give i3c support to them.

Signed-off-by Vitor Soares <[email protected]>

Acked-by Lorenzo Bianconi <[email protected]>

---
Changes in v4:
Remove hw_id variable

Changes in v3:
Remove unnecessary st_lsm6dsx_i3c_data table used to hold device name Use st_lsm6dsx_probe new form

Changes in v2:
Add support for LSM6DSR
Set pm_ops to st_lsm6dsx_pm_ops

 drivers/iio/imu/st_lsm6dsx/Kconfig          |  8 +++-
 drivers/iio/imu/st_lsm6dsx/Makefile         |  1 +
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c | 58 +++++++++++++++++++++++++++++
 3 files changed, 66 insertions(+), 1 deletion(-)
 create mode 100644 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c

drivers/iio/imu/st_lsm6dsx/Kconfig +7 -1

index 9e59297..6b5a73c 100644
@@ -1,11 +1,12 @@
 
 config IIO_ST_LSM6DSX
 	tristate "ST_LSM6DSx driver for STM 6-axis IMU MEMS sensors"
-	depends on (I2C || SPI)
+	depends on (I2C || SPI || I3C)
 	select IIO_BUFFER
 	select IIO_KFIFO_BUF
 	select IIO_ST_LSM6DSX_I2C if (I2C)
 	select IIO_ST_LSM6DSX_SPI if (SPI_MASTER)
+	select IIO_ST_LSM6DSX_I3C if (I3C)
 	help
 	  Say yes here to build support for STMicroelectronics LSM6DSx imu
 	  sensor. Supported devices: lsm6ds3, lsm6ds3h, lsm6dsl, lsm6dsm,

@@ -23,3 +24,8 @@ config IIO_ST_LSM6DSX_SPI
 	tristate
 	depends on IIO_ST_LSM6DSX
 	select REGMAP_SPI
+
+config IIO_ST_LSM6DSX_I3C
+	tristate
+	depends on IIO_ST_LSM6DSX
+	select REGMAP_I3C

drivers/iio/imu/st_lsm6dsx/Makefile +1 -0

index e5f733c..c676965 100644
@@ -4,3 +4,4 @@ st_lsm6dsx-y := st_lsm6dsx_core.o st_lsm6dsx_buffer.o \
 obj-$(CONFIG_IIO_ST_LSM6DSX) += st_lsm6dsx.o
 obj-$(CONFIG_IIO_ST_LSM6DSX_I2C) += st_lsm6dsx_i2c.o
 obj-$(CONFIG_IIO_ST_LSM6DSX_SPI) += st_lsm6dsx_spi.o
+obj-$(CONFIG_IIO_ST_LSM6DSX_I3C) += st_lsm6dsx_i3c.o

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +10 -0

new file mode 100644
index 0000000..2e89524
@@ -0,0 +1,58 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018 Synopsys, Inc. and/or its affiliates.
+ *
+ * Author: Vitor Soares <[email protected]>
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/i3c/device.h>

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c line 20

Quoted 10 lines

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +10 -0

+#include <linux/i3c/master.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+
+#include "st_lsm6dsx.h"
+
+static const struct i3c_device_id st_lsm6dsx_i3c_ids[] = {
+	I3C_DEVICE(0x0104, 0x006C, (void *)ST_LSM6DSO_ID),
+	I3C_DEVICE(0x0104, 0x006B, (void *)ST_LSM6DSR_ID),  

I think you need an uintptr_t cast here:

Quoted 5 lines

	I3C_DEVICE(0x0104, 0x006C, (void *)(uintptr_t)ST_LSM6DSO_ID),
	I3C_DEVICE(0x0104, 0x006B, (void *)(uintptr_t)ST_LSM6DSR_ID),

otherwise gcc might complain that the integer and pointer do not have the same size (on 64-bit architectures).

I don't understand this part. Can you provide or point some background?

Quoted 13 lines

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c line 28

Quoted 10 lines

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +8 -0

+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(i3c, st_lsm6dsx_i3c_ids);
+
+static const struct regmap_config st_lsm6dsx_i3c_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+};  

This can be moved ...

Quoted 5 lines

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c line 31

Quoted 3 lines

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +3 -0

+
+static int st_lsm6dsx_i3c_probe(struct i3c_device *i3cdev)
+{  

... here without the static and const qualifiers:

I understand that can be move to here, but I don't understand the advantages. Can you explain?

Quoted 100 lines · On Fri, 12 Jul 2019 16:28:02 +0000 Vitor Soares <[email protected]>…

From: Boris Brezillon <[email protected]>
Date: Fri, Jul 12, 2019 at 17:14:29

Quoted 97 lines

Quoted 78 lines · On Fri, 12 Jul 2019 13:53:30 +0200 Vitor Soares <[email protected]>…

For today the st_lsm6dsx driver support LSM6DSO and LSM6DSR sensor only in spi and i2c mode.

The LSM6DSO and LSM6DSR are also i3c capable so lets give i3c support to them.

Signed-off-by Vitor Soares <[email protected]>

Acked-by Lorenzo Bianconi <[email protected]>

---
Changes in v4:
Remove hw_id variable

Changes in v3:
Remove unnecessary st_lsm6dsx_i3c_data table used to hold device name Use st_lsm6dsx_probe new form

Changes in v2:
Add support for LSM6DSR
Set pm_ops to st_lsm6dsx_pm_ops

 drivers/iio/imu/st_lsm6dsx/Kconfig          |  8 +++-
 drivers/iio/imu/st_lsm6dsx/Makefile         |  1 +
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c | 58 +++++++++++++++++++++++++++++
 3 files changed, 66 insertions(+), 1 deletion(-)
 create mode 100644 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c

drivers/iio/imu/st_lsm6dsx/Kconfig +7 -1

index 9e59297..6b5a73c 100644
@@ -1,11 +1,12 @@
 
 config IIO_ST_LSM6DSX
 	tristate "ST_LSM6DSx driver for STM 6-axis IMU MEMS sensors"
-	depends on (I2C || SPI)
+	depends on (I2C || SPI || I3C)
 	select IIO_BUFFER
 	select IIO_KFIFO_BUF
 	select IIO_ST_LSM6DSX_I2C if (I2C)
 	select IIO_ST_LSM6DSX_SPI if (SPI_MASTER)
+	select IIO_ST_LSM6DSX_I3C if (I3C)
 	help
 	  Say yes here to build support for STMicroelectronics LSM6DSx imu
 	  sensor. Supported devices: lsm6ds3, lsm6ds3h, lsm6dsl, lsm6dsm,

@@ -23,3 +24,8 @@ config IIO_ST_LSM6DSX_SPI
 	tristate
 	depends on IIO_ST_LSM6DSX
 	select REGMAP_SPI
+
+config IIO_ST_LSM6DSX_I3C
+	tristate
+	depends on IIO_ST_LSM6DSX
+	select REGMAP_I3C

drivers/iio/imu/st_lsm6dsx/Makefile +1 -0

index e5f733c..c676965 100644
@@ -4,3 +4,4 @@ st_lsm6dsx-y := st_lsm6dsx_core.o st_lsm6dsx_buffer.o \
 obj-$(CONFIG_IIO_ST_LSM6DSX) += st_lsm6dsx.o
 obj-$(CONFIG_IIO_ST_LSM6DSX_I2C) += st_lsm6dsx_i2c.o
 obj-$(CONFIG_IIO_ST_LSM6DSX_SPI) += st_lsm6dsx_spi.o
+obj-$(CONFIG_IIO_ST_LSM6DSX_I3C) += st_lsm6dsx_i3c.o

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +10 -0

new file mode 100644
index 0000000..2e89524
@@ -0,0 +1,58 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018 Synopsys, Inc. and/or its affiliates.
+ *
+ * Author: Vitor Soares <[email protected]>
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/i3c/device.h>

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c line 20

Quoted 10 lines

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +10 -0

+#include <linux/i3c/master.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+
+#include "st_lsm6dsx.h"
+
+static const struct i3c_device_id st_lsm6dsx_i3c_ids[] = {
+	I3C_DEVICE(0x0104, 0x006C, (void *)ST_LSM6DSO_ID),
+	I3C_DEVICE(0x0104, 0x006B, (void *)ST_LSM6DSR_ID),  

I think you need an uintptr_t cast here:

	I3C_DEVICE(0x0104, 0x006C, (void *)(uintptr_t)ST_LSM6DSO_ID),
	I3C_DEVICE(0x0104, 0x006B, (void *)(uintptr_t)ST_LSM6DSR_ID),

Quoted 4 lines

Quoted 2 lines

otherwise gcc might complain that the integer and pointer do not have the same size (on 64-bit architectures).

I don't understand this part. Can you provide or point some background?

If you don't do that you'll get the following warning:

warning: cast to 'void *' from smaller integer type 'int' [-Wint-to-void-pointer-cast]

Quoted 20 lines

Quoted 19 lines

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c line 28

Quoted 11 lines

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +8 -0

+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(i3c, st_lsm6dsx_i3c_ids);
+
+static const struct regmap_config st_lsm6dsx_i3c_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+};  

This can be moved ...

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c line 31

Quoted 3 lines

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +3 -0

+
+static int st_lsm6dsx_i3c_probe(struct i3c_device *i3cdev)
+{  

... here without the static and const qualifiers:

Quoted 2 lines

I understand that can be move to here, but I don't understand the advantages. Can you explain?

It reduces the variable scope (this variable is only needed in the probe path) and avoids consuming space in the .bss section, though the second point is not so important.

From: Boris Brezillon <[email protected]>
Date: Fri, Jul 12, 2019 at 17:43:23

Quoted 106 lines

Quoted 103 lines · On Fri, 12 Jul 2019 16:28:02 +0000 Vitor Soares <[email protected]>…

From: Boris Brezillon <[email protected]>
Date: Fri, Jul 12, 2019 at 17:14:29

Quoted 99 lines

Quoted 78 lines · On Fri, 12 Jul 2019 13:53:30 +0200 Vitor Soares <[email protected]>…

For today the st_lsm6dsx driver support LSM6DSO and LSM6DSR sensor only in spi and i2c mode.

The LSM6DSO and LSM6DSR are also i3c capable so lets give i3c support to them.

Signed-off-by Vitor Soares <[email protected]>

Acked-by Lorenzo Bianconi <[email protected]>

---
Changes in v4:
Remove hw_id variable

Changes in v3:
Remove unnecessary st_lsm6dsx_i3c_data table used to hold device name Use st_lsm6dsx_probe new form

Changes in v2:
Add support for LSM6DSR
Set pm_ops to st_lsm6dsx_pm_ops

 drivers/iio/imu/st_lsm6dsx/Kconfig          |  8 +++-
 drivers/iio/imu/st_lsm6dsx/Makefile         |  1 +
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c | 58 +++++++++++++++++++++++++++++
 3 files changed, 66 insertions(+), 1 deletion(-)
 create mode 100644 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c

drivers/iio/imu/st_lsm6dsx/Kconfig +7 -1

index 9e59297..6b5a73c 100644
@@ -1,11 +1,12 @@
 
 config IIO_ST_LSM6DSX
 	tristate "ST_LSM6DSx driver for STM 6-axis IMU MEMS sensors"
-	depends on (I2C || SPI)
+	depends on (I2C || SPI || I3C)
 	select IIO_BUFFER
 	select IIO_KFIFO_BUF
 	select IIO_ST_LSM6DSX_I2C if (I2C)
 	select IIO_ST_LSM6DSX_SPI if (SPI_MASTER)
+	select IIO_ST_LSM6DSX_I3C if (I3C)
 	help
 	  Say yes here to build support for STMicroelectronics LSM6DSx imu
 	  sensor. Supported devices: lsm6ds3, lsm6ds3h, lsm6dsl, lsm6dsm,

@@ -23,3 +24,8 @@ config IIO_ST_LSM6DSX_SPI
 	tristate
 	depends on IIO_ST_LSM6DSX
 	select REGMAP_SPI
+
+config IIO_ST_LSM6DSX_I3C
+	tristate
+	depends on IIO_ST_LSM6DSX
+	select REGMAP_I3C

drivers/iio/imu/st_lsm6dsx/Makefile +1 -0

index e5f733c..c676965 100644
@@ -4,3 +4,4 @@ st_lsm6dsx-y := st_lsm6dsx_core.o st_lsm6dsx_buffer.o \
 obj-$(CONFIG_IIO_ST_LSM6DSX) += st_lsm6dsx.o
 obj-$(CONFIG_IIO_ST_LSM6DSX_I2C) += st_lsm6dsx_i2c.o
 obj-$(CONFIG_IIO_ST_LSM6DSX_SPI) += st_lsm6dsx_spi.o
+obj-$(CONFIG_IIO_ST_LSM6DSX_I3C) += st_lsm6dsx_i3c.o

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +10 -0

new file mode 100644
index 0000000..2e89524
@@ -0,0 +1,58 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018 Synopsys, Inc. and/or its affiliates.
+ *
+ * Author: Vitor Soares <[email protected]>
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/i3c/device.h>

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c line 20

Quoted 10 lines

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +10 -0

+#include <linux/i3c/master.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+
+#include "st_lsm6dsx.h"
+
+static const struct i3c_device_id st_lsm6dsx_i3c_ids[] = {
+	I3C_DEVICE(0x0104, 0x006C, (void *)ST_LSM6DSO_ID),
+	I3C_DEVICE(0x0104, 0x006B, (void *)ST_LSM6DSR_ID),    

I think you need an uintptr_t cast here:

	I3C_DEVICE(0x0104, 0x006C, (void *)(uintptr_t)ST_LSM6DSO_ID),
	I3C_DEVICE(0x0104, 0x006B, (void *)(uintptr_t)ST_LSM6DSR_ID),

otherwise gcc might complain that the integer and pointer do not have the same size (on 64-bit architectures).

Quoted 5 lines

Quoted 1 line

I don't understand this part. Can you provide or point some background?

If you don't do that you'll get the following warning:

warning: cast to 'void *' from smaller integer type 'int' [-Wint-to-void-pointer-cast]

I don't get the warning during compilation. Is there any flag to enable or so?

Quoted 109 lines · On Fri, 12 Jul 2019 18:40:14 +0000 Vitor Soares <[email protected]>…

From: Boris Brezillon <[email protected]>
Date: Fri, Jul 12, 2019 at 17:43:23

Quoted 106 lines

Quoted 103 lines · On Fri, 12 Jul 2019 16:28:02 +0000 Vitor Soares <[email protected]>…

From: Boris Brezillon <[email protected]>
Date: Fri, Jul 12, 2019 at 17:14:29

Quoted 99 lines

Quoted 78 lines · On Fri, 12 Jul 2019 13:53:30 +0200 Vitor Soares <[email protected]>…

For today the st_lsm6dsx driver support LSM6DSO and LSM6DSR sensor only in spi and i2c mode.

The LSM6DSO and LSM6DSR are also i3c capable so lets give i3c support to them.

Signed-off-by Vitor Soares <[email protected]>

Acked-by Lorenzo Bianconi <[email protected]>

---
Changes in v4:
Remove hw_id variable

Changes in v3:
Remove unnecessary st_lsm6dsx_i3c_data table used to hold device name Use st_lsm6dsx_probe new form

Changes in v2:
Add support for LSM6DSR
Set pm_ops to st_lsm6dsx_pm_ops

 drivers/iio/imu/st_lsm6dsx/Kconfig          |  8 +++-
 drivers/iio/imu/st_lsm6dsx/Makefile         |  1 +
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c | 58 +++++++++++++++++++++++++++++
 3 files changed, 66 insertions(+), 1 deletion(-)
 create mode 100644 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c

drivers/iio/imu/st_lsm6dsx/Kconfig +7 -1

index 9e59297..6b5a73c 100644
@@ -1,11 +1,12 @@
 
 config IIO_ST_LSM6DSX
 	tristate "ST_LSM6DSx driver for STM 6-axis IMU MEMS sensors"
-	depends on (I2C || SPI)
+	depends on (I2C || SPI || I3C)
 	select IIO_BUFFER
 	select IIO_KFIFO_BUF
 	select IIO_ST_LSM6DSX_I2C if (I2C)
 	select IIO_ST_LSM6DSX_SPI if (SPI_MASTER)
+	select IIO_ST_LSM6DSX_I3C if (I3C)
 	help
 	  Say yes here to build support for STMicroelectronics LSM6DSx imu
 	  sensor. Supported devices: lsm6ds3, lsm6ds3h, lsm6dsl, lsm6dsm,

@@ -23,3 +24,8 @@ config IIO_ST_LSM6DSX_SPI
 	tristate
 	depends on IIO_ST_LSM6DSX
 	select REGMAP_SPI
+
+config IIO_ST_LSM6DSX_I3C
+	tristate
+	depends on IIO_ST_LSM6DSX
+	select REGMAP_I3C

drivers/iio/imu/st_lsm6dsx/Makefile +1 -0

index e5f733c..c676965 100644
@@ -4,3 +4,4 @@ st_lsm6dsx-y := st_lsm6dsx_core.o st_lsm6dsx_buffer.o \
 obj-$(CONFIG_IIO_ST_LSM6DSX) += st_lsm6dsx.o
 obj-$(CONFIG_IIO_ST_LSM6DSX_I2C) += st_lsm6dsx_i2c.o
 obj-$(CONFIG_IIO_ST_LSM6DSX_SPI) += st_lsm6dsx_spi.o
+obj-$(CONFIG_IIO_ST_LSM6DSX_I3C) += st_lsm6dsx_i3c.o

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +10 -0

new file mode 100644
index 0000000..2e89524
@@ -0,0 +1,58 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018 Synopsys, Inc. and/or its affiliates.
+ *
+ * Author: Vitor Soares <[email protected]>
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/i3c/device.h>

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c line 20

Quoted 10 lines

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +10 -0

+#include <linux/i3c/master.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+
+#include "st_lsm6dsx.h"
+
+static const struct i3c_device_id st_lsm6dsx_i3c_ids[] = {
+	I3C_DEVICE(0x0104, 0x006C, (void *)ST_LSM6DSO_ID),
+	I3C_DEVICE(0x0104, 0x006B, (void *)ST_LSM6DSR_ID),    

I think you need an uintptr_t cast here:

	I3C_DEVICE(0x0104, 0x006C, (void *)(uintptr_t)ST_LSM6DSO_ID),
	I3C_DEVICE(0x0104, 0x006B, (void *)(uintptr_t)ST_LSM6DSR_ID),

otherwise gcc might complain that the integer and pointer do not have the same size (on 64-bit architectures).

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

I don't understand this part. Can you provide or point some background?

If you don't do that you'll get the following warning:

warning: cast to 'void *' from smaller integer type 'int' [-Wint-to-void-pointer-cast]

I don't get the warning during compilation. Is there any flag to enable or so?

Nope, nothing specific to enable, just enable this driver on an arm64 config. Note that that gcc doesn't seem to complain about this int -> void * cast (there's probably some kind of auto-promotion to pointer size), but it does complains about the following void * -> int cast:

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c: In function ‘st_lsm6dsx_i3c_probe’:
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c:43:43: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]

   43 |  return st_lsm6dsx_probe(&i3cdev->dev, 0, (int)id->data, regmap);
      |     

Hi Boris,

From: Boris Brezillon <[email protected]>
Date: Fri, Jul 12, 2019 at 21:03:20

Quoted 127 lines

Quoted 109 lines · On Fri, 12 Jul 2019 18:40:14 +0000 Vitor Soares <[email protected]>…

From: Boris Brezillon <[email protected]>
Date: Fri, Jul 12, 2019 at 17:43:23

Quoted 106 lines

Quoted 103 lines · On Fri, 12 Jul 2019 16:28:02 +0000 Vitor Soares <[email protected]>…

From: Boris Brezillon <[email protected]>
Date: Fri, Jul 12, 2019 at 17:14:29

Quoted 99 lines

Quoted 78 lines · On Fri, 12 Jul 2019 13:53:30 +0200 Vitor Soares <[email protected]>…

For today the st_lsm6dsx driver support LSM6DSO and LSM6DSR sensor only in spi and i2c mode.

The LSM6DSO and LSM6DSR are also i3c capable so lets give i3c support to them.

Signed-off-by Vitor Soares <[email protected]>

Acked-by Lorenzo Bianconi <[email protected]>

---
Changes in v4:
Remove hw_id variable

Changes in v3:
Remove unnecessary st_lsm6dsx_i3c_data table used to hold device name Use st_lsm6dsx_probe new form

Changes in v2:
Add support for LSM6DSR
Set pm_ops to st_lsm6dsx_pm_ops

 drivers/iio/imu/st_lsm6dsx/Kconfig          |  8 +++-
 drivers/iio/imu/st_lsm6dsx/Makefile         |  1 +
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c | 58 +++++++++++++++++++++++++++++
 3 files changed, 66 insertions(+), 1 deletion(-)
 create mode 100644 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c

drivers/iio/imu/st_lsm6dsx/Kconfig +7 -1

index 9e59297..6b5a73c 100644
@@ -1,11 +1,12 @@
 
 config IIO_ST_LSM6DSX
 	tristate "ST_LSM6DSx driver for STM 6-axis IMU MEMS sensors"
-	depends on (I2C || SPI)
+	depends on (I2C || SPI || I3C)
 	select IIO_BUFFER
 	select IIO_KFIFO_BUF
 	select IIO_ST_LSM6DSX_I2C if (I2C)
 	select IIO_ST_LSM6DSX_SPI if (SPI_MASTER)
+	select IIO_ST_LSM6DSX_I3C if (I3C)
 	help
 	  Say yes here to build support for STMicroelectronics LSM6DSx imu
 	  sensor. Supported devices: lsm6ds3, lsm6ds3h, lsm6dsl, lsm6dsm,

@@ -23,3 +24,8 @@ config IIO_ST_LSM6DSX_SPI
 	tristate
 	depends on IIO_ST_LSM6DSX
 	select REGMAP_SPI
+
+config IIO_ST_LSM6DSX_I3C
+	tristate
+	depends on IIO_ST_LSM6DSX
+	select REGMAP_I3C

drivers/iio/imu/st_lsm6dsx/Makefile +1 -0

index e5f733c..c676965 100644
@@ -4,3 +4,4 @@ st_lsm6dsx-y := st_lsm6dsx_core.o st_lsm6dsx_buffer.o \
 obj-$(CONFIG_IIO_ST_LSM6DSX) += st_lsm6dsx.o
 obj-$(CONFIG_IIO_ST_LSM6DSX_I2C) += st_lsm6dsx_i2c.o
 obj-$(CONFIG_IIO_ST_LSM6DSX_SPI) += st_lsm6dsx_spi.o
+obj-$(CONFIG_IIO_ST_LSM6DSX_I3C) += st_lsm6dsx_i3c.o

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +10 -0

new file mode 100644
index 0000000..2e89524
@@ -0,0 +1,58 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018 Synopsys, Inc. and/or its affiliates.
+ *
+ * Author: Vitor Soares <[email protected]>
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/i3c/device.h>

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c line 20

Quoted 10 lines

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +10 -0

+#include <linux/i3c/master.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+
+#include "st_lsm6dsx.h"
+
+static const struct i3c_device_id st_lsm6dsx_i3c_ids[] = {
+	I3C_DEVICE(0x0104, 0x006C, (void *)ST_LSM6DSO_ID),
+	I3C_DEVICE(0x0104, 0x006B, (void *)ST_LSM6DSR_ID),      

I think you need an uintptr_t cast here:

	I3C_DEVICE(0x0104, 0x006C, (void *)(uintptr_t)ST_LSM6DSO_ID),
	I3C_DEVICE(0x0104, 0x006B, (void *)(uintptr_t)ST_LSM6DSR_ID),

otherwise gcc might complain that the integer and pointer do not have the same size (on 64-bit architectures).

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

I don't understand this part. Can you provide or point some background?

If you don't do that you'll get the following warning:

warning: cast to 'void *' from smaller integer type 'int' [-Wint-to-void-pointer-cast]

I don't get the warning during compilation. Is there any flag to enable or so?

Nope, nothing specific to enable, just enable this driver on an arm64 config. Note that that gcc doesn't seem to complain about this int -> void * cast (there's probably some kind of auto-promotion to pointer size), but it does complains about the following void * -> int cast:

Quoted 4 lines

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c: In function ‘st_lsm6dsx_i3c_probe’:
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c:43:43: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]

   43 |  return st_lsm6dsx_probe(&i3cdev->dev, 0, (int)id->data, regmap);
      |       

I fixed the warning by changing:

st_lsm6dsx_probe(&i3cdev->dev, 0, (int)id->data, regmap);

to

st_lsm6dsx_probe(&i3cdev->dev, 0, (int)(uintptr_t)id->data, regmap);

But I wonder if it isn't more save to change the following too:

	I3C_DEVICE(0x0104, 0x006C, (void *)(uintptr_t)ST_LSM6DSO_ID),
	I3C_DEVICE(0x0104, 0x006B, (void *)(uintptr_t)ST_LSM6DSR_ID),

What do you think?