Skip to contentSkip to conversation

Add ST lsm6dso i3c suppport

PATCH Superseded Vitor Soares 4 participants 20 messages Apr 15, 2019 – Apr 23, 2019

Subsystems (5)
IIO Driver core Register map abstraction I3C +1 more subsystems

v1 of 6 Superseded Latest is v6 →

lore
Share & tools
lore

Vitor Soares

author
#0 lore raw

cover Add ST lsm6dso i3c suppport

Vitor Soares (3):
remap: Add i3c bus support
i3c: Add i3c_get_device_id helper
iio: imu: st_lsm6dsx: Add i3c basic support

 drivers/base/regmap/Kconfig                 |  6 ++-
 drivers/base/regmap/Makefile                |  1 +
 drivers/base/regmap/regmap-i3c.c            | 62 ++++++++++++++++++++++++++
 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 | 67 +++++++++++++++++++++++++++++
 include/linux/i3c/device.h                  |  1 +
 include/linux/regmap.h                      | 20 +++++++++
 9 files changed, 172 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 1/3 remap: Add I3C bus support

Add basic support for I3C bus.
This is a simple implementation that only give support for 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 | 62 ++++++++++++++++++++++++++++++++++++++++
 include/linux/regmap.h           | 20 +++++++++++++
 4 files changed, 88 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 +62 -0

62 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
#2 lore raw

Re: [PATCH 1/3] remap: Add I3C bus support

Typo in the subject: s/remap/regmap/

Quoted 135 lines · On Mon, 15 Apr 2019 21:19:39 +0200 Vitor Soares <[email protected]>…

Add basic support for I3C bus.
This is a simple implementation that only give support for 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 | 62 ++++++++++++++++++++++++++++++++++++++++
 include/linux/regmap.h           | 20 +++++++++++++
 4 files changed, 88 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 +62 -0
new file mode 100644
index 0000000..565997b
@@ -0,0 +1,62 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018 Synopsys, Inc. and/or its affiliates.
+ *
+ * Author: Vitor Soares <[email protected]>
+ */
+
+#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_LICENSE("GPL");
include/linux/regmap.h +5 -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.

include/linux/regmap.h bool regmap_ac97_default_volatile(struct device *dev, unsigned int reg); · line 992

Quoted 6 lines
include/linux/regmap.h +3 -0
@@ -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

Maybe we should have a more specific name here, as some might want to access registers using HDR modes (devm_regmap_init_i3c_sdr()?)

Quoted 15 lines
include/linux/regmap.h +12 -0
+ *
+ * @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);
author
#3 lore raw

RE: [PATCH 1/3] remap: Add I3C bus support

Hi Boris,

From: Boris Brezillon <[email protected]>
Date: Tue, Apr 16, 2019 at 07:15:51

Quoted 141 lines

Typo in the subject: s/remap/regmap/

Quoted 136 lines · On Mon, 15 Apr 2019 21:19:39 +0200 Vitor Soares <[email protected]>…

Add basic support for I3C bus.
This is a simple implementation that only give support for 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 | 62 ++++++++++++++++++++++++++++++++++++++++
 include/linux/regmap.h           | 20 +++++++++++++
 4 files changed, 88 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 +62 -0
new file mode 100644
index 0000000..565997b
@@ -0,0 +1,62 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018 Synopsys, Inc. and/or its affiliates.
+ *
+ * Author: Vitor Soares <[email protected]>
+ */
+
+#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_LICENSE("GPL");
include/linux/regmap.h +5 -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)			\
Quoted 8 lines

include/linux/regmap.h bool regmap_ac97_default_volatile(struct device *dev, unsigned int reg); · line 992

Quoted 5 lines
include/linux/regmap.h +3 -0
 	__regmap_lockdep_wrapper(__devm_regmap_init_slimbus, #config,	\
 				slimbus, config)
+
+/**
+ * devm_regmap_init_i3c() - Initialise managed register map

Maybe we should have a more specific name here, as some might want to access registers using HDR modes (devm_regmap_init_i3c_sdr()?)

That make sense if the device with HDR capability is able to do everything in HDR mode and I'm not sure about that.
I would like to wait for a couple of commercial devices before that change.

Quoted 17 lines
Quoted 7 lines
include/linux/regmap.h +5 -0
+ *
+ * @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

include/linux/regmap.h bool regmap_ac97_default_volatile(struct device *dev, unsigned int reg); · line 1007

Quoted 10 lines
include/linux/regmap.h +7 -0
+ * 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);

Thanks for your feedback.

Best regards,
Vitor Soares

maintainer
#4 lore raw

Re: [PATCH 1/3] remap: Add I3C bus support

In reply to Vitor Soares

drivers/base/regmap/regmap-i3c.c line 3

Quoted 5 lines · Vitor Soares

+++ b/drivers/base/regmap/regmap-i3c.c

drivers/base/regmap/regmap-i3c.c new file +3 -0
@@ -0,0 +1,62 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018 Synopsys, Inc. and/or its affiliates.

Please make the entire header C++ style so it looks more consistent. Otherwise this looks good modulo Boris' comment; I'm fine with leaving extra modes for later so long as they can be introduced without disrupting existing users so the only question there would be if we should name the init function in some way that's specific to the I/O mode being used here.

author
#5 lore raw

RE: [PATCH 1/3] remap: Add I3C bus support

Hi Mark,

From: Mark Brown <[email protected]>
Date: Tue, Apr 16, 2019 at 16:39:48

Quoted 10 lines · Vitor Soares

drivers/base/regmap/regmap-i3c.c line 3

Quoted 6 lines

+++ b/drivers/base/regmap/regmap-i3c.c

drivers/base/regmap/regmap-i3c.c new file +3 -0
@@ -0,0 +1,62 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018 Synopsys, Inc. and/or its affiliates.

Please make the entire header C++ style so it looks more consistent. Otherwise this looks good modulo

I will change it next drop.

Quoted 5 lines

Boris' comment; I'm fine with leaving
extra modes for later so long as they can be introduced without disrupting existing users so the only question there would be if we should name the init function in some way that's specific to the I/O mode being used here.

My concern is that booth modes (SDR/HDR) might be needed on the device. e.g. use SDR to configure the device and use HDR to send/receive large data.

Best regards,
Vitor Soares

#6 lore raw

Re: [PATCH 1/3] remap: Add I3C bus support

Quoted 24 lines · On Tue, 23 Apr 2019 14:58:06 +0000 Vitor Soares <[email protected]>…

Hi Mark,

From: Mark Brown <[email protected]>
Date: Tue, Apr 16, 2019 at 16:39:48

Quoted 10 lines · Vitor Soares

drivers/base/regmap/regmap-i3c.c line 3

Quoted 6 lines

+++ b/drivers/base/regmap/regmap-i3c.c

drivers/base/regmap/regmap-i3c.c new file +3 -0
@@ -0,0 +1,62 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018 Synopsys, Inc. and/or its affiliates.

Please make the entire header C++ style so it looks more consistent. Otherwise this looks good modulo

I will change it next drop.

Quoted 5 lines

Boris' comment; I'm fine with leaving
extra modes for later so long as they can be introduced without disrupting existing users so the only question there would be if we should name the init function in some way that's specific to the I/O mode being used here.

Quoted 3 lines

My concern is that booth modes (SDR/HDR) might be needed on the device. e.g. use SDR to configure the device and use HDR to send/receive large data.

I'd say that we shouldn't use the regmap abstraction in this case or have a driver-specific backend implementation for it. I guess the common case is "regs are accessed in SDR mode", so let's keep the name as it is now and we'll define devm_regmap_init_i3c_hdr() if we ever need it. Please make it explicit in the kernel-doc that we're using SDR transfers here.

author
#7 lore raw

RE: [PATCH 1/3] remap: Add I3C bus support

Hi Boris,

From: Boris Brezillon <[email protected]>
Date: Tue, Apr 23, 2019 at 16:14:16

Quoted 31 lines
Quoted 24 lines · On Tue, 23 Apr 2019 14:58:06 +0000 Vitor Soares <[email protected]>…

Hi Mark,

From: Mark Brown <[email protected]>
Date: Tue, Apr 16, 2019 at 16:39:48

Quoted 10 lines · Vitor Soares

drivers/base/regmap/regmap-i3c.c line 3

Quoted 6 lines

+++ b/drivers/base/regmap/regmap-i3c.c

drivers/base/regmap/regmap-i3c.c new file +3 -0
@@ -0,0 +1,62 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018 Synopsys, Inc. and/or its affiliates.

Please make the entire header C++ style so it looks more consistent. Otherwise this looks good modulo

I will change it next drop.

Quoted 5 lines

Boris' comment; I'm fine with leaving
extra modes for later so long as they can be introduced without disrupting existing users so the only question there would be if we should name the init function in some way that's specific to the I/O mode being used here.

Quoted 3 lines

My concern is that booth modes (SDR/HDR) might be needed on the device. e.g. use SDR to configure the device and use HDR to send/receive large data.

Quoted 6 lines

I'd say that we shouldn't use the regmap abstraction in this case or have a driver-specific backend implementation for it. I guess the common case is "regs are accessed in SDR mode", so let's keep the name as it is now and we'll define devm_regmap_init_i3c_hdr() if we ever need it. Please make it explicit in the kernel-doc that we're using SDR transfers here.

Where should it be documented? Can you give an example?

Thanks,
Vitor Soares

author
#8 lore raw

PATCH 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]>
---
 drivers/i3c/device.c       | 8 ++++++++
 include/linux/i3c/device.h | 1 +
 2 files changed, 9 insertions(+)
drivers/i3c/device.c +8 -0
index 472be99..8ab8e4c 100644
@@ -235,6 +235,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 7ee7e30..ee48886 100644
@@ -211,6 +211,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
#9 lore raw

Re: [PATCH 2/3] i3c: Add i3c_get_device_id helper

Quoted 14 lines · On Mon, 15 Apr 2019 21:19:40 +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]>
---
 drivers/i3c/device.c       | 8 ++++++++
 include/linux/i3c/device.h | 1 +
 2 files changed, 9 insertions(+)
drivers/i3c/device.c +0 -0
index 472be99..8ab8e4c 100644

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

Quoted 10 lines
drivers/i3c/device.c +7 -0
@@ -235,6 +235,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);

I think what you want is i3c_device_match_id(). Just move the function to drivers/i3c/device.c, export it and define its prototype in include/i3c/device.h.

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 7ee7e30..ee48886 100644
@@ -211,6 +211,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)
author
#10 lore raw

RE: [PATCH 2/3] i3c: Add i3c_get_device_id helper

Hi Boris,

From: Boris Brezillon <[email protected]>
Date: Tue, Apr 16, 2019 at 07:18:25

Quoted 20 lines
Quoted 17 lines · On Mon, 15 Apr 2019 21:19:40 +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]>
---
 drivers/i3c/device.c       | 8 ++++++++
 include/linux/i3c/device.h | 1 +
 2 files changed, 9 insertions(+)
drivers/i3c/device.c +0 -0
index 472be99..8ab8e4c 100644
@@ -235,6 +235,14 @@ struct i3c_device *dev_to_i3cdev(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(dev_to_i3cdev);
Quoted 9 lines

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

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

I think what you want is i3c_device_match_id().

I want the i3c_device_id struct of an i3c_device. What is the name that you suggest?

Quoted 3 lines

Just move the function
to drivers/i3c/device.c, export it and define its prototype in include/i3c/device.h.

You are right, this should go for the device side.

Quoted 11 lines
Quoted 11 lines
drivers/i3c/device.c +1 -0
+
 /**
  * i3c_driver_register_with_owner() - register an I3C device driver
  *
include/linux/i3c/device.h +0 -0
index 7ee7e30..ee48886 100644
@@ -211,6 +211,7 @@ static inline struct i3c_driver *drv_to_i3cdrv(struct device_driver *drv)
Quoted 6 lines

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

Quoted 6 lines
include/linux/i3c/device.h +1 -0
 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)

Best regards,
Vitor Soares

author
#11 lore raw

PATCH 3/3 iio: imu: st_lsm6dsx: Add i3c basic support

In reply to Vitor Soares

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

The lsm6dso is also i3c capable so lets give i3c support to it.

Signed-off-by Vitor Soares <[email protected]>
---
 drivers/iio/imu/st_lsm6dsx/Kconfig          |  8 +++-
 drivers/iio/imu/st_lsm6dsx/Makefile         |  1 +
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c | 67 +++++++++++++++++++++++++++++
 3 files changed, 75 insertions(+), 1 deletion(-)
 create mode 100644 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c
drivers/iio/imu/st_lsm6dsx/Kconfig +7 -1
index 094fd00..1ab9bbf 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 +67 -0

67 lines not on this page · raw message

+

Signature · 1 line
2.7.4
#12 lore raw

Re: [PATCH 3/3] iio: imu: st_lsm6dsx: Add i3c basic support

Quoted 6 lines

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

The lsm6dso is also i3c capable so lets give i3c support to it.

Hi Vitor,

thx, this is very cool :)
Just a couple of nit inline

Regards,
Lorenzo

Quoted 88 lines
Signed-off-by Vitor Soares <[email protected]>
---
 drivers/iio/imu/st_lsm6dsx/Kconfig          |  8 +++-
 drivers/iio/imu/st_lsm6dsx/Makefile         |  1 +
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c | 67 +++++++++++++++++++++++++++++
 3 files changed, 75 insertions(+), 1 deletion(-)
 create mode 100644 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c
drivers/iio/imu/st_lsm6dsx/Kconfig +7 -1
index 094fd00..1ab9bbf 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 +38 -0
new file mode 100644
index 0000000..2df5e70
@@ -0,0 +1,67 @@
+// 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>
+#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 {
+       char name[NAME_SIZE];
+       enum st_lsm6dsx_hw_id id;
+};
+
+static const struct st_lsm6dsx_i3c_data hw_data[] = {
+       { ST_LSM6DSO_DEV_NAME, ST_LSM6DSO_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;

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

Quoted 10 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +10 -0
+
+       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);

In order to support the hw FIFO, I guess you are planning to support interrupts/IBI here

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

Quoted 2 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +2 -0
+}
+

please remove 'new line' here

Quoted 21 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +18 -1
+
+static const struct i3c_device_id st_lsm6dsx_i3c_ids[] = {
+       I3C_DEVICE(0x0104, 0x006C, &hw_data[0]),
+       { /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(i3c, st_lsm6dsx_i3c_ids);
+
+static struct i3c_driver st_lsm6dsx_driver = {
+       .driver.name = "st_lsm6dsx_i3c",
+       .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");
+
--

2.7.4

Signature · 3 lines
UNIX is Sexy: who | grep -i blonde | talk; cd ~; wine; talk; touch;
unzip; touch; strip; gasp; finger; gasp; mount; fsck; more; yes; gasp;
umount; make clean; sleep
author
#13 lore raw

RE: [PATCH 3/3] iio: imu: st_lsm6dsx: Add i3c basic support

HI Lorenzo,

From: Lorenzo Bianconi <[email protected]>
Date: Mon, Apr 15, 2019 at 23:17:50

Quoted 108 lines
Quoted 6 lines

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

The lsm6dso is also i3c capable so lets give i3c support to it.

Hi Vitor,

thx, this is very cool :)
Just a couple of nit inline

Regards,
Lorenzo

Quoted 93 lines
Signed-off-by Vitor Soares <[email protected]>
---
 drivers/iio/imu/st_lsm6dsx/Kconfig          |  8 +++-
 drivers/iio/imu/st_lsm6dsx/Makefile         |  1 +
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c | 67 +++++++++++++++++++++++++++++
 3 files changed, 75 insertions(+), 1 deletion(-)
 create mode 100644 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c
drivers/iio/imu/st_lsm6dsx/Kconfig +7 -1
index 094fd00..1ab9bbf 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 +43 -0
new file mode 100644
index 0000000..2df5e70
@@ -0,0 +1,67 @@
+// 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>
+#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 {
+       char name[NAME_SIZE];
+       enum st_lsm6dsx_hw_id id;
+};
+
+static const struct st_lsm6dsx_i3c_data hw_data[] = {
+       { ST_LSM6DSO_DEV_NAME, ST_LSM6DSO_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));
Quoted 8 lines

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

Quoted 5 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +5 -0
+               return PTR_ERR(regmap);
+       }
+
+       return st_lsm6dsx_probe(&i3cdev->dev, 0, hw_data->id,
+                               hw_data->name, regmap);

In order to support the hw FIFO, I guess you are planning to support interrupts/IBI here

My idea is to register the ibi and handler in this file.
The handler would call a buffer function passing the device and payload. On st_lsm6dsx_probe we can add a flag for IBI configuration on the sensor side.

Quoted 5 lines

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

Quoted 3 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +2 -0
+}
+

please remove 'new line' here

Sure.

Quoted 14 lines
Quoted 14 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +13 -0
+
+static const struct i3c_device_id st_lsm6dsx_i3c_ids[] = {
+       I3C_DEVICE(0x0104, 0x006C, &hw_data[0]),
+       { /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(i3c, st_lsm6dsx_i3c_ids);
+
+static struct i3c_driver st_lsm6dsx_driver = {
+       .driver.name = "st_lsm6dsx_i3c",
+       .probe = st_lsm6dsx_i3c_probe,
+       .id_table = st_lsm6dsx_i3c_ids,
+};
+module_i3c_driver(st_lsm6dsx_driver);
Quoted 14 lines
Quoted 8 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +5 -1
+
+MODULE_AUTHOR("Vitor Soares <[email protected]>");
+MODULE_DESCRIPTION("STMicroelectronics st_lsm6dsx i3c driver");
+MODULE_LICENSE("GPL v2");
+
--

2.7.4

Signature · 3 lines
UNIX is Sexy: who | grep -i blonde | talk; cd ~; wine; talk; touch;
unzip; touch; strip; gasp; finger; gasp; mount; fsck; more; yes; gasp;
umount; make clean; sleep

Thanks for your feedback.

Best regards,
Vitor Soares

#14 lore raw

Re: [PATCH 3/3] iio: imu: st_lsm6dsx: Add i3c basic support

In reply to Vitor Soares
Quoted 66 lines · On Mon, 15 Apr 2019 21:19:41 +0200 Vitor Soares <[email protected]>…

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

The lsm6dso is also i3c capable so lets give i3c support to it.

Signed-off-by Vitor Soares <[email protected]>
---
 drivers/iio/imu/st_lsm6dsx/Kconfig          |  8 +++-
 drivers/iio/imu/st_lsm6dsx/Makefile         |  1 +
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c | 67 +++++++++++++++++++++++++++++
 3 files changed, 75 insertions(+), 1 deletion(-)
 create mode 100644 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c
drivers/iio/imu/st_lsm6dsx/Kconfig +7 -1
index 094fd00..1ab9bbf 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 +11 -0
new file mode 100644
index 0000000..2df5e70
@@ -0,0 +1,67 @@
+// 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 {
+	char name[NAME_SIZE];

	const char *name;

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

Quoted 4 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +4 -0
+	enum st_lsm6dsx_hw_id id;
+};
+
+static const struct st_lsm6dsx_i3c_data hw_data[] = {

No need to make it an array, and it should probably be named st_lsm6dso_data not hw_data.

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

Quoted 11 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +11 -0
+	{ ST_LSM6DSO_DEV_NAME, ST_LSM6DSO_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);

					 i3c_device_match_id(i3cdev,
					 		     st_lsm6dsx_i3c_ids);
Quoted 15 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +15 -0
+	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);
+}
+
+

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

Quoted 10 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +10 -0
+static const struct i3c_device_id st_lsm6dsx_i3c_ids[] = {
+	I3C_DEVICE(0x0104, 0x006C, &hw_data[0]),
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(i3c, st_lsm6dsx_i3c_ids);
+
+static struct i3c_driver st_lsm6dsx_driver = {
+	.driver.name = "st_lsm6dsx_i3c",
+	.probe = st_lsm6dsx_i3c_probe,
+	.id_table = st_lsm6dsx_i3c_ids,

You should probably set the pm_ops here (st_lsm6dsx_pm_ops).

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c

Quoted 7 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +7 -0
+};
+module_i3c_driver(st_lsm6dsx_driver);
+
+MODULE_AUTHOR("Vitor Soares <[email protected]>");
+MODULE_DESCRIPTION("STMicroelectronics st_lsm6dsx i3c driver");
+MODULE_LICENSE("GPL v2");
+

You can remove this blank line.

#15 lore raw

Re: [PATCH 3/3] iio: imu: st_lsm6dsx: Add i3c basic support

Quoted 83 lines
Quoted 66 lines · On Mon, 15 Apr 2019 21:19:41 +0200 Vitor Soares <[email protected]>…

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

The lsm6dso is also i3c capable so lets give i3c support to it.

Signed-off-by Vitor Soares <[email protected]>
---
 drivers/iio/imu/st_lsm6dsx/Kconfig          |  8 +++-
 drivers/iio/imu/st_lsm6dsx/Makefile         |  1 +
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c | 67 +++++++++++++++++++++++++++++
 3 files changed, 75 insertions(+), 1 deletion(-)
 create mode 100644 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c
drivers/iio/imu/st_lsm6dsx/Kconfig +7 -1
index 094fd00..1ab9bbf 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 +11 -0
new file mode 100644
index 0000000..2df5e70
@@ -0,0 +1,67 @@
+// 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 {
+     char name[NAME_SIZE];

        const char *name;
Quoted 7 lines

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

Quoted 4 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +4 -0
+     enum st_lsm6dsx_hw_id id;
+};
+
+static const struct st_lsm6dsx_i3c_data hw_data[] = {

No need to make it an array, and it should probably be named st_lsm6dso_data not hw_data.

I guess it will be used even for future devices so I would prefer to make it an array with a 'general' name

Regards,
Lorenzo

Quoted 54 lines

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

Quoted 12 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +11 -0
+     { ST_LSM6DSO_DEV_NAME, ST_LSM6DSO_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);

                                         i3c_device_match_id(i3cdev,
                                                             st_lsm6dsx_i3c_ids);
Quoted 15 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +15 -0
+     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);
+}
+
+

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

Quoted 10 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +10 -0
+static const struct i3c_device_id st_lsm6dsx_i3c_ids[] = {
+     I3C_DEVICE(0x0104, 0x006C, &hw_data[0]),
+     { /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(i3c, st_lsm6dsx_i3c_ids);
+
+static struct i3c_driver st_lsm6dsx_driver = {
+     .driver.name = "st_lsm6dsx_i3c",
+     .probe = st_lsm6dsx_i3c_probe,
+     .id_table = st_lsm6dsx_i3c_ids,

You should probably set the pm_ops here (st_lsm6dsx_pm_ops).

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c

Quoted 7 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +7 -0
+};
+module_i3c_driver(st_lsm6dsx_driver);
+
+MODULE_AUTHOR("Vitor Soares <[email protected]>");
+MODULE_DESCRIPTION("STMicroelectronics st_lsm6dsx i3c driver");
+MODULE_LICENSE("GPL v2");
+

You can remove this blank line.

Signature · 3 lines
UNIX is Sexy: who | grep -i blonde | talk; cd ~; wine; talk; touch;
unzip; touch; strip; gasp; finger; gasp; mount; fsck; more; yes; gasp;
umount; make clean; sleep
#16 lore raw

Re: [PATCH 3/3] iio: imu: st_lsm6dsx: Add i3c basic support

Quoted 91 lines · On Tue, 16 Apr 2019 09:58:33 +0200 Lorenzo Bianconi…
Quoted 83 lines
Quoted 66 lines · On Mon, 15 Apr 2019 21:19:41 +0200 Vitor Soares <[email protected]>…

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

The lsm6dso is also i3c capable so lets give i3c support to it.

Signed-off-by Vitor Soares <[email protected]>
---
 drivers/iio/imu/st_lsm6dsx/Kconfig          |  8 +++-
 drivers/iio/imu/st_lsm6dsx/Makefile         |  1 +
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c | 67 +++++++++++++++++++++++++++++
 3 files changed, 75 insertions(+), 1 deletion(-)
 create mode 100644 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c
drivers/iio/imu/st_lsm6dsx/Kconfig +7 -1
index 094fd00..1ab9bbf 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 +11 -0
new file mode 100644
index 0000000..2df5e70
@@ -0,0 +1,67 @@
+// 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 {
+     char name[NAME_SIZE];  

        const char *name;
Quoted 7 lines

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

Quoted 4 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +4 -0
+     enum st_lsm6dsx_hw_id id;
+};
+
+static const struct st_lsm6dsx_i3c_data hw_data[] = {

No need to make it an array, and it should probably be named st_lsm6dso_data not hw_data.

Quoted 2 lines

I guess it will be used even for future devices so I would prefer to make it an array with a 'general' name

I find it more error-prone to reference an array with an opaque index number than reference an instance with a name that clearly reflects what HW it describes (see the I3C_DEVICE() entry at the end of the driver), but okay.

Quoted 60 lines

Regards,
Lorenzo

Quoted 54 lines

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

Quoted 12 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +11 -0
+     { ST_LSM6DSO_DEV_NAME, ST_LSM6DSO_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);  

                                         i3c_device_match_id(i3cdev,
                                                             st_lsm6dsx_i3c_ids);
Quoted 15 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +15 -0
+     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);
+}
+
+

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

Quoted 10 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +10 -0
+static const struct i3c_device_id st_lsm6dsx_i3c_ids[] = {
+     I3C_DEVICE(0x0104, 0x006C, &hw_data[0]),
+     { /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(i3c, st_lsm6dsx_i3c_ids);
+
+static struct i3c_driver st_lsm6dsx_driver = {
+     .driver.name = "st_lsm6dsx_i3c",
+     .probe = st_lsm6dsx_i3c_probe,
+     .id_table = st_lsm6dsx_i3c_ids,

You should probably set the pm_ops here (st_lsm6dsx_pm_ops).

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c

Quoted 7 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +7 -0
+};
+module_i3c_driver(st_lsm6dsx_driver);
+
+MODULE_AUTHOR("Vitor Soares <[email protected]>");
+MODULE_DESCRIPTION("STMicroelectronics st_lsm6dsx i3c driver");
+MODULE_LICENSE("GPL v2");
+

You can remove this blank line.

author
#17 lore raw

RE: [PATCH 3/3] iio: imu: st_lsm6dsx: Add i3c basic support

Hi Boris and Lorenzo,

From: Boris Brezillon <[email protected]>
Date: Tue, Apr 16, 2019 at 09:05:40

Quoted 77 lines
Quoted 74 lines · On Tue, 16 Apr 2019 09:58:33 +0200 Lorenzo Bianconi…
Quoted 74 lines
Quoted 70 lines · On Mon, 15 Apr 2019 21:19:41 +0200 Vitor Soares <[email protected]>…

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

The lsm6dso is also i3c capable so lets give i3c support to it.

Signed-off-by Vitor Soares <[email protected]>
---
 drivers/iio/imu/st_lsm6dsx/Kconfig          |  8 +++-
 drivers/iio/imu/st_lsm6dsx/Makefile         |  1 +
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c | 67 +++++++++++++++++++++++++++++
 3 files changed, 75 insertions(+), 1 deletion(-)
 create mode 100644 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c
drivers/iio/imu/st_lsm6dsx/Kconfig +7 -1
index 094fd00..1ab9bbf 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 +15 -0
new file mode 100644
index 0000000..2df5e70
@@ -0,0 +1,67 @@
+// 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>
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines

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

Quoted 6 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +6 -0
+#include "st_lsm6dsx.h"
+
+#define NAME_SIZE    32
+
+struct st_lsm6dsx_i3c_data {
+     char name[NAME_SIZE];  

        const char *name;

I will fix this next patch.

Quoted 11 lines
Quoted 11 lines
Quoted 8 lines

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

Quoted 5 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +4 -0
+     enum st_lsm6dsx_hw_id id;
+};
+
+static const struct st_lsm6dsx_i3c_data hw_data[] = {

No need to make it an array, and it should probably be named st_lsm6dso_data not hw_data.

I guess it will be used even for future devices so I would prefer to make it an array with a 'general' name

The idea is that one. Are you ok with hw_data name? Should I add the lsm6dsr support here or send in a different patch?

Quoted 5 lines

I find it more error-prone to reference an array with an opaque index number than reference an instance with a name that clearly reflects what HW it describes (see the I3C_DEVICE() entry at the end of the driver), but okay.

I will address this next version.

Quoted 49 lines
Quoted 49 lines

Regards,
Lorenzo

Quoted 44 lines

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

Quoted 12 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +11 -0
+     { ST_LSM6DSO_DEV_NAME, ST_LSM6DSO_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);  

                                         i3c_device_match_id(i3cdev,
                                                             st_lsm6dsx_i3c_ids);
Quoted 15 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +15 -0
+     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);
+}
+
+

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

Quoted 10 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +10 -0
+static const struct i3c_device_id st_lsm6dsx_i3c_ids[] = {
+     I3C_DEVICE(0x0104, 0x006C, &hw_data[0]),
+     { /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(i3c, st_lsm6dsx_i3c_ids);
+
+static struct i3c_driver st_lsm6dsx_driver = {
+     .driver.name = "st_lsm6dsx_i3c",
+     .probe = st_lsm6dsx_i3c_probe,
+     .id_table = st_lsm6dsx_i3c_ids,

You should probably set the pm_ops here (st_lsm6dsx_pm_ops).

Quoted 12 lines
Quoted 12 lines
Quoted 10 lines

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c

Quoted 7 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +7 -0
+};
+module_i3c_driver(st_lsm6dsx_driver);
+
+MODULE_AUTHOR("Vitor Soares <[email protected]>");
+MODULE_DESCRIPTION("STMicroelectronics st_lsm6dsx i3c driver");
+MODULE_LICENSE("GPL v2");
+

You can remove this blank line.

Best regards,
Vitor Soares

author
#18 lore raw

RE: [PATCH 3/3] iio: imu: st_lsm6dsx: Add i3c basic support

In reply to Boris Brezillon

Hi Boris,

From: Boris Brezillon <[email protected]>
Date: Tue, Apr 16, 2019 at 07:25:39

Quoted 8 lines

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

Quoted 6 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +6 -0
+MODULE_DEVICE_TABLE(i3c, st_lsm6dsx_i3c_ids);
+
+static struct i3c_driver st_lsm6dsx_driver = {
+	.driver.name = "st_lsm6dsx_i3c",
+	.probe = st_lsm6dsx_i3c_probe,
+	.id_table = st_lsm6dsx_i3c_ids,

You should probably set the pm_ops here (st_lsm6dsx_pm_ops).

Yes, I missed that.

Quoted 10 lines

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c

Quoted 8 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +7 -0
+};
+module_i3c_driver(st_lsm6dsx_driver);
+
+MODULE_AUTHOR("Vitor Soares <[email protected]>");
+MODULE_DESCRIPTION("STMicroelectronics st_lsm6dsx i3c driver");
+MODULE_LICENSE("GPL v2");
+

You can remove this blank line.

Thanks for your feedback.

Best regards,
Vitor Soares

#19 lore raw
In reply to Vitor Soares

Hi Vitor,

On Mon, 15 Apr 2019 21:19:38 +0200
Vitor Soares <[email protected]> wrote:

I like the very detailed description in your cover letter :P. Anyway, glad to see I3C support added to regmap. Also happy to have a new driver for an I3C device.

Thanks for working on that.

Boris

Quoted 18 lines

Vitor Soares (3):
remap: Add i3c bus support
i3c: Add i3c_get_device_id helper
iio: imu: st_lsm6dsx: Add i3c basic support

 drivers/base/regmap/Kconfig                 |  6 ++-
 drivers/base/regmap/Makefile                |  1 +
 drivers/base/regmap/regmap-i3c.c            | 62 ++++++++++++++++++++++++++
 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 | 67 +++++++++++++++++++++++++++++
 include/linux/i3c/device.h                  |  1 +
 include/linux/regmap.h                      | 20 +++++++++
 9 files changed, 172 insertions(+), 2 deletions(-)
 create mode 100644 drivers/base/regmap/regmap-i3c.c
 create mode 100644 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c