Add ST lsm6dso i3c support

PATCH Superseded Vitor Soares 3 participants 9 messages Jul 10, 2019 – Jul 11, 2019

Subsystems (5)

IIO Driver core Register map abstraction I3C +1 more subsystems

lore

Share & tools

lore

Vitor Soares

author

2019-07-10 14:56 UTC #0 lore raw

v3 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 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 | 61 +++++++++++++++++++++++++++++
 include/linux/i3c/device.h                  |  4 ++
 include/linux/regmap.h                      | 20 ++++++++++
 10 files changed, 205 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-10 14:56 UTC #1 lore raw

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

Mark Brown

maintainer

2019-07-10 15:02 UTC #2 lore raw

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

Quoted 3 lines · Vitor Soares

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

Please do not submit new versions of already applied patches, please submit incremental updates to the existing code. Modifying existing commits creates problems for other users building on top of those commits so it's best practice to only change pubished git commits if absolutely essential.

Vitor Soares

author

2019-07-10 14:56 UTC #3 lore raw

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

Vitor Soares

author

2019-07-10 14:56 UTC #4 lore raw

PATCH v3 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 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 | 61 +++++++++++++++++++++++++++++
 3 files changed, 69 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 +61 -0

61 lines not on this page · raw message

Signature · 1 line

2.7.4

Lorenzo Bianconi

maintainer

2019-07-10 19:44 UTC #5 lore raw

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

Quoted 5 lines

For today the st_lsm6dsx driver support LSM6DSO and LSM6DSR sensor only in spi and i2c mode.
The LSM6DSO and LSM6DSR are also i3c capable so lets give i3c support to them.

Hi Vitor,

just few comments inline.

Regards,
Lorenzo

Quoted 18 lines

Signed-off-by Vitor Soares <[email protected]>
Acked-by Lorenzo Bianconi <[email protected]>
---
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 | 61 +++++++++++++++++++++++++++++
 3 files changed, 69 insertions(+), 1 deletion(-)
 create mode 100644 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c

[...]

Quoted 15 lines

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

new file mode 100644
index 0000000..f683754
@@ -0,0 +1,61 @@
+// 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>

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

Quoted 10 lines

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

+#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"
+
+static const struct i3c_device_id st_lsm6dsx_i3c_ids[];
+

why do we need this? I guess you can just move st_lsm6dsx_i3c_ids definition here

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

+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_device_match_id(i3cdev,
+							    st_lsm6dsx_i3c_ids);

i3c_device_match_id can theoretically fail so is it better to check return value here? (maybe I am too paranoid :))

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

Quoted 2 lines

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +2 -0
+	struct regmap *regmap;
+	int hw_id = (int)id->data;

I guess we do not need this since we use it just in st_lsm6dsx_probe(), we can just do:

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

Quoted 34 lines

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +31 -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_id, regmap);
+}
+
+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),
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(i3c, st_lsm6dsx_i3c_ids);
+
+static struct i3c_driver st_lsm6dsx_driver = {
+	.driver = {
+		.name = "st_lsm6dsx_i3c",
+		.pm = &st_lsm6dsx_pm_ops,
+	},
+	.probe = st_lsm6dsx_i3c_probe,
+	.id_table = st_lsm6dsx_i3c_ids,
+};
+module_i3c_driver(st_lsm6dsx_driver);
+
+MODULE_AUTHOR("Vitor Soares <[email protected]>");
+MODULE_DESCRIPTION("STMicroelectronics st_lsm6dsx i3c driver");
+MODULE_LICENSE("GPL v2");

Signature · 1 line

2.7.4

Vitor Soares

author

2019-07-11 10:12 UTC #6 lore raw

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

Hi Lorenzo,

From: Lorenzo Bianconi <[email protected]>
Date: Wed, Jul 10, 2019 at 20:44:05

Quoted 108 lines

Quoted 5 lines
For today the st_lsm6dsx driver support LSM6DSO and LSM6DSR sensor only in spi and i2c mode.
The LSM6DSO and LSM6DSR are also i3c capable so lets give i3c support to them.
Hi Vitor,
just few comments inline.
Regards,
Lorenzo
Quoted 18 lines
Signed-off-by Vitor Soares <[email protected]>
Acked-by Lorenzo Bianconi <[email protected]>
---
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 | 61 +++++++++++++++++++++++++++++
 3 files changed, 69 insertions(+), 1 deletion(-)
 create mode 100644 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c
[...]
Quoted 15 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +9 -0
new file mode 100644
index 0000000..f683754
@@ -0,0 +1,61 @@
+// 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>
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c line 19
Quoted 10 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +10 -0
+#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"
+
+static const struct i3c_device_id st_lsm6dsx_i3c_ids[];
+
why do we need this? I guess you can just move st_lsm6dsx_i3c_ids definition here
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 +9 -0
+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_device_match_id(i3cdev,
+							    st_lsm6dsx_i3c_ids);
i3c_device_match_id can theoretically fail so is it better to check return value here? (maybe I am too paranoid :))
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c line 30
Quoted 2 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +2 -0
+	struct regmap *regmap;
+	int hw_id = (int)id->data;
I guess we do not need this since we use it just in st_lsm6dsx_probe(), we can just do:
return st_lsm6dsx_probe(&i3cdev->dev, 0, (int)id->data, regmap);
Quoted 26 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +26 -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_id, regmap);
+}
+
+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),
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(i3c, st_lsm6dsx_i3c_ids);
+
+static struct i3c_driver st_lsm6dsx_driver = {
+	.driver = {
+		.name = "st_lsm6dsx_i3c",
+		.pm = &st_lsm6dsx_pm_ops,
+	},
+	.probe = st_lsm6dsx_i3c_probe,
+	.id_table = st_lsm6dsx_i3c_ids,
+};

Quoted 8 lines

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

Thanks for your comments. I will address them and send a new version.

Best regards,
Vitor Soares

Vitor Soares

author

2019-07-11 11:00 UTC #7 lore raw

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

From: Vitor Soares <[email protected]>
Date: Thu, Jul 11, 2019 at 11:12:34

Quoted 45 lines

Hi Lorenzo,
From: Lorenzo Bianconi <[email protected]>
Date: Wed, Jul 10, 2019 at 20:44:05
Quoted 40 lines
Quoted 5 lines
For today the st_lsm6dsx driver support LSM6DSO and LSM6DSR sensor only in spi and i2c mode.
The LSM6DSO and LSM6DSR are also i3c capable so lets give i3c support to them.
Hi Vitor,
just few comments inline.
Regards,
Lorenzo
Quoted 18 lines
Signed-off-by Vitor Soares <[email protected]>
Acked-by Lorenzo Bianconi <[email protected]>
---
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 | 61 +++++++++++++++++++++++++++++
 3 files changed, 69 insertions(+), 1 deletion(-)
 create mode 100644 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c
[...]
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c line 28
Quoted 4 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +4 -0
+static int st_lsm6dsx_i3c_probe(struct i3c_device *i3cdev)
+{
+	const struct i3c_device_id *id = i3c_device_match_id(i3cdev,
+							    st_lsm6dsx_i3c_ids);
i3c_device_match_id can theoretically fail so is it better to check return value here? (maybe I am too paranoid :))

I was preparing the patch and if the i3c_device_match_id() fail it return NULL so the st_lsm6dsx_probe() will fail automatically. Checking the spi_get_device_id(), the drivers don't test the return value too.

Do you think it is really necessary to test it before the st_lsm6dsx_probe() function?

Best regards,
Vitor Soares

Lorenzo Bianconi

maintainer

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

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

Quoted 49 lines

From: Vitor Soares <[email protected]>
Date: Thu, Jul 11, 2019 at 11:12:34
Quoted 45 lines
Hi Lorenzo,
From: Lorenzo Bianconi <[email protected]>
Date: Wed, Jul 10, 2019 at 20:44:05
Quoted 40 lines
Quoted 5 lines
For today the st_lsm6dsx driver support LSM6DSO and LSM6DSR sensor only in spi and i2c mode.
The LSM6DSO and LSM6DSR are also i3c capable so lets give i3c support to them.
Hi Vitor,
just few comments inline.
Regards,
Lorenzo
Quoted 18 lines
Signed-off-by Vitor Soares <[email protected]>
Acked-by Lorenzo Bianconi <[email protected]>
---
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 | 61 +++++++++++++++++++++++++++++
 3 files changed, 69 insertions(+), 1 deletion(-)
 create mode 100644 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c
[...]
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c line 28
Quoted 4 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c new file +4 -0
+static int st_lsm6dsx_i3c_probe(struct i3c_device *i3cdev)
+{
+	const struct i3c_device_id *id = i3c_device_match_id(i3cdev,
+							    st_lsm6dsx_i3c_ids);
i3c_device_match_id can theoretically fail so is it better to check return value here? (maybe I am too paranoid :))

Quoted 4 lines

I was preparing the patch and if the i3c_device_match_id() fail it return NULL so the st_lsm6dsx_probe() will fail automatically. Checking the spi_get_device_id(), the drivers don't test the return value too.

multiple drivers deference it directly so I am fine to skip this check.

Regards,
Lorenzo

Quoted 6 lines

Do you think it is really necessary to test it before the st_lsm6dsx_probe() function?
Best regards,
Vitor Soares