Add ST lsm6dso i3c support

PATCH Superseded Vitor Soares 2 participants 17 messages Jul 12, 2019 – Jul 16, 2019

Subsystems (5)

IIO Driver core Register map abstraction I3C +1 more subsystems

lore

Share & tools

lore

Vitor Soares

author

2019-07-12 11:53 UTC #0 lore raw

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

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

Signature · 1 line

2.7.4

Vitor Soares

author

2019-07-12 11:53 UTC #1 lore raw

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

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

Boris Brezillon

2019-07-12 15:59 UTC #2 lore raw

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

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 150 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);

Vitor Soares

author

2019-07-12 16:15 UTC #3 lore raw

RE: [PATCH v4 1/3] regmap: add i3c bus support

Hi Boris,

From: Boris Brezillon <[email protected]>
Date: Fri, Jul 12, 2019 at 16:59:15

Quoted 10 lines

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.

I mention that in the cover letter and I kept it for reference.

Next time I will drop it.

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);

Vitor Soares

author

2019-07-12 11:53 UTC #4 lore raw

PATCH v4 2/3 i3c: move i3c_device_match_id to device.c and export it

In reply to Vitor Soares

The i3c device driver needs the i3c_device_id table. Lets move it 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 +46 -0

index 69cc040..383df3b 100644
@@ -200,6 +200,52 @@ struct i3c_device *dev_to_i3cdev(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(dev_to_i3cdev);
 
+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~~

51 lines not on this page · raw message

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)
 {

Signature · 1 line

2.7.4

Boris Brezillon

2019-07-12 16:03 UTC #5 lore raw

Re: [PATCH v4 2/3] i3c: move i3c_device_match_id to device.c and export it

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 24 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 +0 -0
index 69cc040..383df3b 100644

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

Quoted 3 lines

drivers/i3c/device.c +0 -0

@@ -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 120 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 +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)
 {

Vitor Soares

author

2019-07-12 16:22 UTC #6 lore raw

RE: [PATCH v4 2/3] i3c: move i3c_device_match_id to device.c and export it

Hi Boris,

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

Quoted 5 lines

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.

Quoted 8 lines

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

Fix in next drop.

Quoted 19 lines

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(-)

Quoted 10 lines

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.

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

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);

Quoted 13 lines

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=

Thanks for your comments.

Best regards,
Vitor Soares

Boris Brezillon

2019-07-12 16:34 UTC #7 lore raw

Re: [PATCH v4 2/3] i3c: move i3c_device_match_id to device.c and export it

Quoted 5 lines · On Fri, 12 Jul 2019 16:21:49 +0000 Vitor Soares <[email protected]>…

Quoted 2 lines
You're missing a kerneldoc here.
I will do that. Can you clarify why we need that?

So the function is properly documented here [1].

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

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);
+

[1]https://www.kernel.org/doc/html/latest/driver-api/i3c/device-driver-api.html

Vitor Soares

author

2019-07-12 11:53 UTC #8 lore raw

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

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 +58 -0

58 lines not on this page · raw message

Signature · 7 lines

2.7.4


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

Boris Brezillon

2019-07-12 16:14 UTC #9 lore raw

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

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 8 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 15 lines

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");

Vitor Soares

author

2019-07-12 16:28 UTC #10 lore raw

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

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 12 lines

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

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 21 lines

	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 15 lines

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");

Best regards,
Vitor Soares

Boris Brezillon

2019-07-12 16:43 UTC #11 lore raw

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

Quoted 97 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 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 7 lines

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?

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 13 lines

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 8 lines

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?

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.

Vitor Soares

author

2019-07-12 18:43 UTC #12 lore raw

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

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

Quoted 103 lines

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 8 lines

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]

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

Quoted 20 lines

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 6 lines

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.

Thanks,
Vitor Soares

Boris Brezillon

2019-07-12 20:03 UTC #13 lore raw

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

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);
      |

Vitor Soares

author

2019-07-16 13:22 UTC #14 lore raw

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

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);

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?

Best regards,
Vitor Soares

Boris Brezillon

2019-07-16 13:40 UTC #15 lore raw

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

Quoted 137 lines · On Tue, 16 Jul 2019 13:22:25 +0000 Vitor Soares <[email protected]>…

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);
      |       

Quoted 7 lines

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);

The (int) cast is implicit, no need to add it here.

Quoted 6 lines

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?

I think we're good, we would have a problem if you were defining ST_LSM6DSO_ID as an ULL (unsigned long long, AKA u64) and trying to cast that value to a void pointer on a 32-bit arch.

Boris Brezillon

2019-07-12 15:58 UTC #16 lore raw

In reply to Vitor Soares

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

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

Quoted 6 lines

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()

Did not receive this v3...

Quoted 24 lines

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