Skip to contentSkip to conversation

Add ST lsm6dso i3c support

PATCH Superseded Vitor Soares 5 participants 16 messages Jun 6, 2019 – Jun 13, 2019

Subsystems (6)
IIO I2C subsystem host drivers Driver core Register map abstraction +2 more subsystems

v2 of 6 Superseded Latest is v6 →

lore
Share & tools
lore

Vitor Soares

author
#0 lore raw

v2 cover 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.

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                        |  8 +++
 drivers/iio/imu/st_lsm6dsx/Kconfig          |  8 ++-
 drivers/iio/imu/st_lsm6dsx/Makefile         |  1 +
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c | 76 +++++++++++++++++++++++++++++
 include/linux/i3c/device.h                  |  1 +
 include/linux/regmap.h                      | 20 ++++++++
 9 files changed, 179 insertions(+), 2 deletions(-)
 create mode 100644 drivers/base/regmap/regmap-i3c.c
 create mode 100644 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c
Signature · 1 line
2.7.4
author
#1 lore raw

PATCH v2 1/3 regmap: add i3c bus support

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]>

---
Changes in v2:
None

 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

60 lines not on this page · raw message

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);
Signature · 1 line
2.7.4
author
#4 lore raw

PATCH v2 2/3 i3c: add i3c_get_device_id helper

In reply to Vitor Soares

This helper return the i3c_device_id structure in order the client have access to the driver data.

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

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

 drivers/i3c/device.c       | 8 ++++++++
 include/linux/i3c/device.h | 1 +
 2 files changed, 9 insertions(+)
drivers/i3c/device.c +8 -0
index 69cc040..a6d0796 100644
@@ -200,6 +200,14 @@ struct i3c_device *dev_to_i3cdev(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(dev_to_i3cdev);
 
+const struct i3c_device_id *i3c_get_device_id(struct i3c_device *i3cdev)
+{
+	const struct i3c_driver *i3cdrv = drv_to_i3cdrv(i3cdev->dev.driver);
+
+	return i3cdrv->id_table;
+}
+EXPORT_SYMBOL_GPL(i3c_get_device_id);
+
 /**
  * i3c_driver_register_with_owner() - register an I3C device driver
  *
include/linux/i3c/device.h +1 -0
index 5ecb055..e0415e1 100644
@@ -187,6 +187,7 @@ 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_get_device_id(struct i3c_device *i3cdev);
 
 static inline void i3cdev_set_drvdata(struct i3c_device *i3cdev,
 				      void *data)
Signature · 1 line
2.7.4
author
#7 lore raw

PATCH v2 3/3 iio: imu: st_lsm6dsx: add i3c basic support for LSM6DSO and LSM6DSR

In reply to Vitor Soares

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]>

---
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 | 76 +++++++++++++++++++++++++++++
 3 files changed, 84 insertions(+), 1 deletion(-)
 create mode 100644 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c
drivers/iio/imu/st_lsm6dsx/Kconfig +7 -1
index 002a423..8115936 100644
@@ -2,11 +2,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,

@@ -24,3 +25,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 28cc673..57cbcd6 100644
@@ -5,3 +5,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 +76 -0

76 lines not on this page · raw message

Signature · 1 line
2.7.4
#5 lore raw

Re: [PATCH v2 2/3] i3c: add i3c_get_device_id helper

In reply to Vitor Soares
Quoted 17 lines · On Thu, 6 Jun 2019 17:12:03 +0200 Vitor Soares <[email protected]>…

This helper return the i3c_device_id structure in order the client have access to the driver data.

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

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

 drivers/i3c/device.c       | 8 ++++++++
 include/linux/i3c/device.h | 1 +
 2 files changed, 9 insertions(+)
drivers/i3c/device.c +0 -0
index 69cc040..a6d0796 100644

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

Quoted 10 lines
drivers/i3c/device.c +7 -0
@@ -200,6 +200,14 @@ struct i3c_device *dev_to_i3cdev(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(dev_to_i3cdev);
 
+const struct i3c_device_id *i3c_get_device_id(struct i3c_device *i3cdev)
+{
+	const struct i3c_driver *i3cdrv = drv_to_i3cdrv(i3cdev->dev.driver);
+
+	return i3cdrv->id_table;
+}
+EXPORT_SYMBOL_GPL(i3c_get_device_id);

That's not what I asked. I told you to expose i3c_device_match_id() which already exists and is in master.c. What you really want is to get the device_id entry that matches your device, not the first entry in the table...

Quoted 16 lines
drivers/i3c/device.c +1 -0
+
 /**
  * i3c_driver_register_with_owner() - register an I3C device driver
  *
include/linux/i3c/device.h +1 -0
index 5ecb055..e0415e1 100644
@@ -187,6 +187,7 @@ 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_get_device_id(struct i3c_device *i3cdev);
 
 static inline void i3cdev_set_drvdata(struct i3c_device *i3cdev,
 				      void *data)
#11 lore raw
In reply to Vitor Soares
Quoted 1 line · Vitor Soares

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

Why is the I2C list on CC? Is there something relevant I missed?

author
#12 lore raw

Hi Wolfram,

I think I2C ecosystem is also part interested in I3C due the compatibility and maybe they can provide some feedback. If you think differently, sorry I will remove I2C list next time.

Regards,
Vitor Soares

From: Wolfram Sang <[email protected]>
Date: Thu, Jun 06, 2019 at 17:25:23

Quoted 4 lines · Vitor Soares
Quoted 1 line

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

Why is the I2C list on CC? Is there something relevant I missed?

maintainer Acked-by on v2 3/3
#8 lore raw

Re: [PATCH v2 3/3] iio: imu: st_lsm6dsx: add i3c basic support for LSM6DSO and LSM6DSR

In reply to Vitor Soares
Quoted 8 lines

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]>

---

Hi Vitor,

just a nit inline, but you can add my acked-by for st_lsm6dsx part in v3

Acked-by Lorenzo Bianconi <[email protected]>

Regards,
Lorenzo

Quoted 33 lines

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 | 76 +++++++++++++++++++++++++++++
 3 files changed, 84 insertions(+), 1 deletion(-)
 create mode 100644 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c
drivers/iio/imu/st_lsm6dsx/Kconfig +3 -1
index 002a423..8115936 100644
@@ -2,11 +2,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,

@@ -24,3 +25,8 @@ config IIO_ST_LSM6DSX_SPI
 	tristate
 	depends on IIO_ST_LSM6DSX
 	select REGMAP_SPI
+

[...]

Quoted 11 lines
drivers/iio/imu/st_lsm6dsx/Kconfig +11 -0
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/i3c/device.h>
+#include <linux/i3c/master.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+
+#include "st_lsm6dsx.h"
+
+#define NAME_SIZE	32

drivers/iio/imu/st_lsm6dsx/Kconfig

Quoted 10 lines
drivers/iio/imu/st_lsm6dsx/Kconfig +10 -0
+
+struct st_lsm6dsx_i3c_data {
+	const char name[NAME_SIZE];
+	enum st_lsm6dsx_hw_id id;
+};
+
+enum st_lsm6dsx_i3c_data_id {
+	ST_LSM6DSO_I3C_DATA_ID,
+	ST_LSM6DSR_I3C_DATA_ID,
+};

do we really need them? maybe just use hw_data[n] adding a comment to indicate the related sensor defining st_lsm6dsx_i3c_ids[]

Quoted 51 lines
drivers/iio/imu/st_lsm6dsx/Kconfig +48 -0
+
+static const struct st_lsm6dsx_i3c_data hw_data[] = {
+	{ ST_LSM6DSO_DEV_NAME, ST_LSM6DSO_ID },
+	{ ST_LSM6DSR_DEV_NAME, ST_LSM6DSR_ID },
+};
+
+static const struct regmap_config st_lsm6dsx_i3c_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+};
+
+static int st_lsm6dsx_i3c_probe(struct i3c_device *i3cdev)
+{
+	const struct i3c_device_id *id = i3c_get_device_id(i3cdev);
+	const struct st_lsm6dsx_i3c_data *hw_data = id->data;
+	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, hw_data->id,
+				hw_data->name, regmap);
+}
+
+static const struct i3c_device_id st_lsm6dsx_i3c_ids[] = {
+	I3C_DEVICE(0x0104, 0x006C, &hw_data[ST_LSM6DSO_I3C_DATA_ID]),
+	I3C_DEVICE(0x0104, 0x006B, &hw_data[ST_LSM6DSR_I3C_DATA_ID]),
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(i3c, st_lsm6dsx_i3c_ids);
+
+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");
Signature · 1 line
2.7.4
author
#6 lore raw

RE: [PATCH v2 2/3] i3c: add i3c_get_device_id helper

In reply to Boris Brezillon

Hi Boris,

From: Boris Brezillon <[email protected]>
Date: Thu, Jun 06, 2019 at 16:17:55

Quoted 23 lines
Quoted 20 lines · On Thu, 6 Jun 2019 17:12:03 +0200 Vitor Soares <[email protected]>…

This helper return the i3c_device_id structure in order the client have access to the driver data.

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

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

 drivers/i3c/device.c       | 8 ++++++++
 include/linux/i3c/device.h | 1 +
 2 files changed, 9 insertions(+)
drivers/i3c/device.c +0 -0
index 69cc040..a6d0796 100644
@@ -200,6 +200,14 @@ struct i3c_device *dev_to_i3cdev(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(dev_to_i3cdev);
Quoted 13 lines

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

Quoted 7 lines
drivers/i3c/device.c +7 -0
+const struct i3c_device_id *i3c_get_device_id(struct i3c_device *i3cdev)
+{
+	const struct i3c_driver *i3cdrv = drv_to_i3cdrv(i3cdev->dev.driver);
+
+	return i3cdrv->id_table;
+}
+EXPORT_SYMBOL_GPL(i3c_get_device_id);

That's not what I asked. I told you to expose i3c_device_match_id() which already exists and is in master.c. What you really want is to get the device_id entry that matches your device, not the first entry in the table...

I didn't see it has the full table.
I will change it.

Quoted 16 lines
Quoted 16 lines
drivers/i3c/device.c +1 -0
+
 /**
  * i3c_driver_register_with_owner() - register an I3C device driver
  *
include/linux/i3c/device.h +1 -0
index 5ecb055..e0415e1 100644
@@ -187,6 +187,7 @@ 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_get_device_id(struct i3c_device *i3cdev);
 
 static inline void i3cdev_set_drvdata(struct i3c_device *i3cdev,
 				      void *data)
#9 lore raw

Re: [PATCH v2 3/3] iio: imu: st_lsm6dsx: add i3c basic support for LSM6DSO and LSM6DSR

In reply to Vitor Soares
Quoted 71 lines · On Thu, 6 Jun 2019 17:12:04 +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]>

---
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 | 76 +++++++++++++++++++++++++++++
 3 files changed, 84 insertions(+), 1 deletion(-)
 create mode 100644 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c
drivers/iio/imu/st_lsm6dsx/Kconfig +7 -1
index 002a423..8115936 100644
@@ -2,11 +2,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,

@@ -24,3 +25,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 28cc673..57cbcd6 100644
@@ -5,3 +5,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 +11 -0
new file mode 100644
index 0000000..70b70d1
@@ -0,0 +1,76 @@
+// 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>
+#include <linux/i3c/master.h>

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

Quoted 10 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +10 -0
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+
+#include "st_lsm6dsx.h"
+
+#define NAME_SIZE	32
+
+struct st_lsm6dsx_i3c_data {
+	const char name[NAME_SIZE];

I think I mentioned already that you can simply have

	const char *name;
Quoted 28 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +28 -0
+	enum st_lsm6dsx_hw_id id;
+};
+
+enum st_lsm6dsx_i3c_data_id {
+	ST_LSM6DSO_I3C_DATA_ID,
+	ST_LSM6DSR_I3C_DATA_ID,
+};
+
+static const struct st_lsm6dsx_i3c_data hw_data[] = {
+	{ ST_LSM6DSO_DEV_NAME, ST_LSM6DSO_ID },
+	{ ST_LSM6DSR_DEV_NAME, ST_LSM6DSR_ID },
+};
+
+static const struct regmap_config st_lsm6dsx_i3c_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+};
+
+static int st_lsm6dsx_i3c_probe(struct i3c_device *i3cdev)
+{
+	const struct i3c_device_id *id = i3c_get_device_id(i3cdev);
+	const struct st_lsm6dsx_i3c_data *hw_data = id->data;
+	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));

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

Quoted 10 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +10 -0
+		return PTR_ERR(regmap);
+	}
+
+	return st_lsm6dsx_probe(&i3cdev->dev, 0, hw_data->id,
+				hw_data->name, regmap);
+}
+
+static const struct i3c_device_id st_lsm6dsx_i3c_ids[] = {
+	I3C_DEVICE(0x0104, 0x006C, &hw_data[ST_LSM6DSO_I3C_DATA_ID]),
+	I3C_DEVICE(0x0104, 0x006B, &hw_data[ST_LSM6DSR_I3C_DATA_ID]),

Still find that form counter-intuitive since you'd have to first go look at what's the value of ST_LSM6DSO_I3C_DATA_ID, then go check the entry in hw_data to find what's in there. Too many ways to get things wrong IMHO.

The following form would make it much more obvious/easy to follow:

static const st_lsm6dsx_i3c_data st_lsm6dso_i3c_data = {
	ST_LSM6DSO_DEV_NAME, ST_LSM6DSO_ID,
};
static const st_lsm6dsx_i3c_data st_lsm6dsr_i3c_data = {
	ST_LSM6DSR_DEV_NAME, ST_LSM6DSR_ID,
};
static const struct i3c_device_id st_lsm6dsx_i3c_ids[] = {
	I3C_DEVICE(0x0104, 0x006C, &st_lsm6dso_i3c_data),
	I3C_DEVICE(0x0104, 0x006B, &st_lsm6dsr_i3c_data),
};

Note that I don't see why we need to pass both the name and the ID to st_lsm6dsx_probe(). I'd expect the name to be easily deducible from the ID (using a name table whose index would match the ST_XXX_ID).

If you do this change you would actually get rid of the st_lsm6dsx_i3c_data struct and instead have:

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),
};
Quoted 17 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +17 -0
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(i3c, st_lsm6dsx_i3c_ids);
+
+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");
maintainer
#10 lore raw

Re: [PATCH v2 3/3] iio: imu: st_lsm6dsx: add i3c basic support for LSM6DSO and LSM6DSR

Quoted 143 lines
Quoted 71 lines · On Thu, 6 Jun 2019 17:12:04 +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]>

---
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 | 76 +++++++++++++++++++++++++++++
 3 files changed, 84 insertions(+), 1 deletion(-)
 create mode 100644 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c
drivers/iio/imu/st_lsm6dsx/Kconfig +7 -1
index 002a423..8115936 100644
@@ -2,11 +2,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,

@@ -24,3 +25,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 28cc673..57cbcd6 100644
@@ -5,3 +5,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 +11 -0
new file mode 100644
index 0000000..70b70d1
@@ -0,0 +1,76 @@
+// 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>
+#include <linux/i3c/master.h>

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

Quoted 10 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +10 -0
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+
+#include "st_lsm6dsx.h"
+
+#define NAME_SIZE	32
+
+struct st_lsm6dsx_i3c_data {
+	const char name[NAME_SIZE];

I think I mentioned already that you can simply have

	const char *name;
Quoted 28 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +28 -0
+	enum st_lsm6dsx_hw_id id;
+};
+
+enum st_lsm6dsx_i3c_data_id {
+	ST_LSM6DSO_I3C_DATA_ID,
+	ST_LSM6DSR_I3C_DATA_ID,
+};
+
+static const struct st_lsm6dsx_i3c_data hw_data[] = {
+	{ ST_LSM6DSO_DEV_NAME, ST_LSM6DSO_ID },
+	{ ST_LSM6DSR_DEV_NAME, ST_LSM6DSR_ID },
+};
+
+static const struct regmap_config st_lsm6dsx_i3c_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+};
+
+static int st_lsm6dsx_i3c_probe(struct i3c_device *i3cdev)
+{
+	const struct i3c_device_id *id = i3c_get_device_id(i3cdev);
+	const struct st_lsm6dsx_i3c_data *hw_data = id->data;
+	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));

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

Quoted 10 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +10 -0
+		return PTR_ERR(regmap);
+	}
+
+	return st_lsm6dsx_probe(&i3cdev->dev, 0, hw_data->id,
+				hw_data->name, regmap);
+}
+
+static const struct i3c_device_id st_lsm6dsx_i3c_ids[] = {
+	I3C_DEVICE(0x0104, 0x006C, &hw_data[ST_LSM6DSO_I3C_DATA_ID]),
+	I3C_DEVICE(0x0104, 0x006B, &hw_data[ST_LSM6DSR_I3C_DATA_ID]),

Still find that form counter-intuitive since you'd have to first go look at what's the value of ST_LSM6DSO_I3C_DATA_ID, then go check the entry in hw_data to find what's in there. Too many ways to get things wrong IMHO.

The following form would make it much more obvious/easy to follow:

static const st_lsm6dsx_i3c_data st_lsm6dso_i3c_data = {
	ST_LSM6DSO_DEV_NAME, ST_LSM6DSO_ID,
};
static const st_lsm6dsx_i3c_data st_lsm6dsr_i3c_data = {
	ST_LSM6DSR_DEV_NAME, ST_LSM6DSR_ID,
};
Quoted 8 lines
static const struct i3c_device_id st_lsm6dsx_i3c_ids[] = {
	I3C_DEVICE(0x0104, 0x006C, &st_lsm6dso_i3c_data),
	I3C_DEVICE(0x0104, 0x006B, &st_lsm6dsr_i3c_data),
};

Note that I don't see why we need to pass both the name and the ID to st_lsm6dsx_probe(). I'd expect the name to be easily deducible from the ID (using a name table whose index would match the ST_XXX_ID).

for spi/i2c we got it for free since spi_device_id/i2c_device_id has a name field we can use but we can probably align it to i3c case

Regards,
Lorenzo

Quoted 27 lines

If you do this change you would actually get rid of the st_lsm6dsx_i3c_data struct and instead have:

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),
};
Quoted 17 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +17 -0
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(i3c, st_lsm6dsx_i3c_ids);
+
+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");
#13 lore raw
In reply to Vitor Soares

Hi,

Quoted 3 lines

I think I2C ecosystem is also part interested in I3C due the compatibility and maybe they can provide some feedback. If you think differently, sorry I will remove I2C list next time.

No worries, but please do remove next time, for two reasons:

a) even for I2C clients, the i2c-list is usually not added if the client only uses standard I2C communication. If there is something which needw special attention, OK. But most of the time, the list is for the I2C core and bus master drivers, and not clients.

(That might be different for I3C, though...)

b) if the patch in question is "self-contained" in the I3C world and not affecting I2C, I think there is no need to add I2C. Interested parties can subscribe to the I3C list.

Yes, that was a good occasion to write this publicly.

Thanks,

Wolfram

maintainer
#2 lore raw

Re: [PATCH v2 1/3] regmap: add i3c bus support

In reply to Vitor Soares
Quoted 3 lines · Vitor Soares

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

The following changes since commit a188339ca5a396acc588e5851ed7e19f66b0ebd9:

Linux 5.2-rc1 (2019-05-19 15:47:09 -0700)

are available in the Git repository at:

https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git tags/regmap-i3c

for you to fetch changes up to 6445500b43129baac36c56d629cf1dd9e1104167:

regmap: add i3c bus support (2019-06-07 13:09:55 +0100)

----------------------------------------------------------------
regmap: Add I3C bus support

----------------------------------------------------------------
Vitor Soares (1):
regmap: add i3c bus support

 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
maintainer
#3 lore raw

Applied "regmap: add i3c bus support" to the regmap tree

In reply to Vitor Soares

The patch

regmap: add i3c bus support

has been applied to the regmap tree at

https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git for-5.3

All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying to this mail.

Thanks,
Mark

Quoted 1 line

From 6445500b43129baac36c56d629cf1dd9e1104167 Mon Sep 17 00:00:00 2001

From: Vitor Soares <[email protected]>
Date: Thu, 6 Jun 2019 17:12:02 +0200
Subject: [PATCH] regmap: add i3c bus support

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]>
Signed-off-by Mark Brown <[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 6ad5ef48b61e..c8bbf5322720 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 f5b4e8851d00..ff6c7d8ec1cd 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

60 lines not on this page · raw message

include/linux/regmap.h +20 -0
index daeec7dbd65c..f65984d98b07 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);
Signature · 1 line
2.20.1
author
#14 lore raw
In reply to Vitor Soares

Hi,

Since the regmap-i3c.c was already applied in:
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git
tags/regmap-i3c

I wonder what is clean way to submit this patch set?

And since the i3c-regmap was merge in
From: Vitor Soares <[email protected]>
Date: Thu, Jun 06, 2019 at 16:12:01

Quoted 15 lines

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.

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

Quoted 15 lines
 drivers/base/regmap/Kconfig                 |  6 ++-
 drivers/base/regmap/Makefile                |  1 +
 drivers/base/regmap/regmap-i3c.c            | 60 +++++++++++++++++++++++
 drivers/i3c/device.c                        |  8 +++
 drivers/iio/imu/st_lsm6dsx/Kconfig          |  8 ++-
 drivers/iio/imu/st_lsm6dsx/Makefile         |  1 +
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c | 76 +++++++++++++++++++++++++++++
 include/linux/i3c/device.h                  |  1 +
 include/linux/regmap.h                      | 20 ++++++++
 9 files changed, 179 insertions(+), 2 deletions(-)
 create mode 100644 drivers/base/regmap/regmap-i3c.c
 create mode 100644 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c
Signature · 1 line
2.7.4

Best regards,
Vitor Soares

maintainer
#15 lore raw
Quoted 5 lines · Vitor Soares

Since the regmap-i3c.c was already applied in:
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git
tags/regmap-i3c

I wonder what is clean way to submit this patch set?

Just mention in the cover letter that it depends on that tag, the point with the tag is to allow other trees to pull it in if they need it.

_______________________________________________
linux-i3c mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-i3c