[PATCH 0/3] Add ST lsm6dso i3c suppport

[PATCH 0/3] Add ST lsm6dso i3c suppport

[Vitor Soares]

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

-- 
2.7.4


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

[Vitor Soares]

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 <vitor.soares@synopsys.com>
---
 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

diff --git a/drivers/base/regmap/Kconfig b/drivers/base/regmap/Kconfig
index 6ad5ef4..c8bbf53 100644
--- a/drivers/base/regmap/Kconfig
+++ b/drivers/base/regmap/Kconfig
@@ -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
diff --git a/drivers/base/regmap/Makefile b/drivers/base/regmap/Makefile
index f5b4e88..ff6c7d8 100644
--- a/drivers/base/regmap/Makefile
+++ b/drivers/base/regmap/Makefile
@@ -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
diff --git a/drivers/base/regmap/regmap-i3c.c b/drivers/base/regmap/regmap-i3c.c
new file mode 100644
index 0000000..565997b
--- /dev/null
+++ b/drivers/base/regmap/regmap-i3c.c
@@ -0,0 +1,62 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018 Synopsys, Inc. and/or its affiliates.
+ *
+ * Author: Vitor Soares <vitor.soares@synopsys.com>
+ */
+
+#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");
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index daeec7d..f65984d 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -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);
-- 
2.7.4


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

[Boris Brezillon]

Typo in the subject: s/remap/regmap/

On Mon, 15 Apr 2019 21:19:39 +0200
Vitor Soares <vitor.soares@synopsys.com> wrote:

> 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 <vitor.soares@synopsys.com>
> ---
>  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
> 
> diff --git a/drivers/base/regmap/Kconfig b/drivers/base/regmap/Kconfig
> index 6ad5ef4..c8bbf53 100644
> --- a/drivers/base/regmap/Kconfig
> +++ b/drivers/base/regmap/Kconfig
> @@ -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
> diff --git a/drivers/base/regmap/Makefile b/drivers/base/regmap/Makefile
> index f5b4e88..ff6c7d8 100644
> --- a/drivers/base/regmap/Makefile
> +++ b/drivers/base/regmap/Makefile
> @@ -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
> diff --git a/drivers/base/regmap/regmap-i3c.c b/drivers/base/regmap/regmap-i3c.c
> new file mode 100644
> index 0000000..565997b
> --- /dev/null
> +++ b/drivers/base/regmap/regmap-i3c.c
> @@ -0,0 +1,62 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018 Synopsys, Inc. and/or its affiliates.
> + *
> + * Author: Vitor Soares <vitor.soares@synopsys.com>
> + */
> +
> +#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");
> diff --git a/include/linux/regmap.h b/include/linux/regmap.h
> index daeec7d..f65984d 100644
> --- a/include/linux/regmap.h
> +++ b/include/linux/regmap.h
> @@ -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

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


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


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

[Vitor Soares]

Hi Boris,

From: Boris Brezillon <boris.brezillon@collabora.com>
Date: Tue, Apr 16, 2019 at 07:15:51

> Typo in the subject: s/remap/regmap/
> 
> On Mon, 15 Apr 2019 21:19:39 +0200
> Vitor Soares <vitor.soares@synopsys.com> wrote:
> 
> > 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 <vitor.soares@synopsys.com>
> > ---
> >  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
> > 
> > diff --git a/drivers/base/regmap/Kconfig b/drivers/base/regmap/Kconfig
> > index 6ad5ef4..c8bbf53 100644
> > --- a/drivers/base/regmap/Kconfig
> > +++ b/drivers/base/regmap/Kconfig
> > @@ -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
> > diff --git a/drivers/base/regmap/Makefile b/drivers/base/regmap/Makefile
> > index f5b4e88..ff6c7d8 100644
> > --- a/drivers/base/regmap/Makefile
> > +++ b/drivers/base/regmap/Makefile
> > @@ -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
> > diff --git a/drivers/base/regmap/regmap-i3c.c b/drivers/base/regmap/regmap-i3c.c
> > new file mode 100644
> > index 0000000..565997b
> > --- /dev/null
> > +++ b/drivers/base/regmap/regmap-i3c.c
> > @@ -0,0 +1,62 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2018 Synopsys, Inc. and/or its affiliates.
> > + *
> > + * Author: Vitor Soares <vitor.soares@synopsys.com>
> > + */
> > +
> > +#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");
> > diff --git a/include/linux/regmap.h b/include/linux/regmap.h
> > index daeec7d..f65984d 100644
> > --- a/include/linux/regmap.h
> > +++ b/include/linux/regmap.h
> > @@ -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
> 
> 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.

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


Thanks for your feedback.

Best regards,
Vitor Soares


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

[Vitor Soares]

Hi Mark,

From: Mark Brown <broonie@kernel.org>
Date: Tue, Apr 16, 2019 at 16:39:48

> On Mon, Apr 15, 2019 at 09:19:39PM +0200, Vitor Soares wrote:
> 
> > +++ b/drivers/base/regmap/regmap-i3c.c
> > @@ -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.

> 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

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

[Boris Brezillon]

On Tue, 23 Apr 2019 14:58:06 +0000
Vitor Soares <Vitor.Soares@synopsys.com> wrote:

> Hi Mark,
> 
> From: Mark Brown <broonie@kernel.org>
> Date: Tue, Apr 16, 2019 at 16:39:48
> 
> > On Mon, Apr 15, 2019 at 09:19:39PM +0200, Vitor Soares wrote:
> >   
> > > +++ b/drivers/base/regmap/regmap-i3c.c
> > > @@ -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.
> 
> > 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.

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.

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

[Vitor Soares]

Hi Boris,

From: Boris Brezillon <boris.brezillon@collabora.com>
Date: Tue, Apr 23, 2019 at 16:14:16

> On Tue, 23 Apr 2019 14:58:06 +0000
> Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> 
> > Hi Mark,
> > 
> > From: Mark Brown <broonie@kernel.org>
> > Date: Tue, Apr 16, 2019 at 16:39:48
> > 
> > > On Mon, Apr 15, 2019 at 09:19:39PM +0200, Vitor Soares wrote:
> > >   
> > > > +++ b/drivers/base/regmap/regmap-i3c.c
> > > > @@ -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.
> > 
> > > 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.
> 
> 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

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

[PATCH 2/3] i3c: Add i3c_get_device_id helper

[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 <vitor.soares@synopsys.com>
---
 drivers/i3c/device.c       | 8 ++++++++
 include/linux/i3c/device.h | 1 +
 2 files changed, 9 insertions(+)

diff --git a/drivers/i3c/device.c b/drivers/i3c/device.c
index 472be99..8ab8e4c 100644
--- a/drivers/i3c/device.c
+++ b/drivers/i3c/device.c
@@ -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
  *
diff --git a/include/linux/i3c/device.h b/include/linux/i3c/device.h
index 7ee7e30..ee48886 100644
--- a/include/linux/i3c/device.h
+++ b/include/linux/i3c/device.h
@@ -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)
-- 
2.7.4


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

[Boris Brezillon]

On Mon, 15 Apr 2019 21:19:40 +0200
Vitor Soares <vitor.soares@synopsys.com> wrote:

> This helper return the i3c_device_id structure in order the client
> have access to the driver data.
> 
> Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
> ---
>  drivers/i3c/device.c       | 8 ++++++++
>  include/linux/i3c/device.h | 1 +
>  2 files changed, 9 insertions(+)
> 
> diff --git a/drivers/i3c/device.c b/drivers/i3c/device.c
> index 472be99..8ab8e4c 100644
> --- a/drivers/i3c/device.c
> +++ b/drivers/i3c/device.c
> @@ -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.

> +
>  /**
>   * i3c_driver_register_with_owner() - register an I3C device driver
>   *
> diff --git a/include/linux/i3c/device.h b/include/linux/i3c/device.h
> index 7ee7e30..ee48886 100644
> --- a/include/linux/i3c/device.h
> +++ b/include/linux/i3c/device.h
> @@ -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)


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

[Vitor Soares]

Hi Boris,

From: Boris Brezillon <boris.brezillon@collabora.com>
Date: Tue, Apr 16, 2019 at 07:18:25

> On Mon, 15 Apr 2019 21:19:40 +0200
> Vitor Soares <vitor.soares@synopsys.com> wrote:
> 
> > This helper return the i3c_device_id structure in order the client
> > have access to the driver data.
> > 
> > Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
> > ---
> >  drivers/i3c/device.c       | 8 ++++++++
> >  include/linux/i3c/device.h | 1 +
> >  2 files changed, 9 insertions(+)
> > 
> > diff --git a/drivers/i3c/device.c b/drivers/i3c/device.c
> > index 472be99..8ab8e4c 100644
> > --- a/drivers/i3c/device.c
> > +++ b/drivers/i3c/device.c
> > @@ -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(). 

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

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

> 
> > +
> >  /**
> >   * i3c_driver_register_with_owner() - register an I3C device driver
> >   *
> > diff --git a/include/linux/i3c/device.h b/include/linux/i3c/device.h
> > index 7ee7e30..ee48886 100644
> > --- a/include/linux/i3c/device.h
> > +++ b/include/linux/i3c/device.h
> > @@ -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)

Best regards,
Vitor Soares

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

[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 <vitor.soares@synopsys.com>
---
 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

diff --git a/drivers/iio/imu/st_lsm6dsx/Kconfig b/drivers/iio/imu/st_lsm6dsx/Kconfig
index 094fd00..1ab9bbf 100644
--- a/drivers/iio/imu/st_lsm6dsx/Kconfig
+++ b/drivers/iio/imu/st_lsm6dsx/Kconfig
@@ -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
diff --git a/drivers/iio/imu/st_lsm6dsx/Makefile b/drivers/iio/imu/st_lsm6dsx/Makefile
index e5f733c..c676965 100644
--- a/drivers/iio/imu/st_lsm6dsx/Makefile
+++ b/drivers/iio/imu/st_lsm6dsx/Makefile
@@ -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
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c
new file mode 100644
index 0000000..2df5e70
--- /dev/null
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c
@@ -0,0 +1,67 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018 Synopsys, Inc. and/or its affiliates.
+ *
+ * Author: Vitor Soares <vitor.soares@synopsys.com>
+ */
+
+#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));
+		return PTR_ERR(regmap);
+	}
+
+	return st_lsm6dsx_probe(&i3cdev->dev, 0, hw_data->id,
+				hw_data->name, regmap);
+}
+
+
+static const struct i3c_device_id st_lsm6dsx_i3c_ids[] = {
+	I3C_DEVICE(0x0104, 0x006C, &hw_data[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 <vitor.soares@synopsys.com>");
+MODULE_DESCRIPTION("STMicroelectronics st_lsm6dsx i3c driver");
+MODULE_LICENSE("GPL v2");
+
-- 
2.7.4


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

[Lorenzo Bianconi]

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

> Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
> ---
>  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
>
> diff --git a/drivers/iio/imu/st_lsm6dsx/Kconfig b/drivers/iio/imu/st_lsm6dsx/Kconfig
> index 094fd00..1ab9bbf 100644
> --- a/drivers/iio/imu/st_lsm6dsx/Kconfig
> +++ b/drivers/iio/imu/st_lsm6dsx/Kconfig
> @@ -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
> diff --git a/drivers/iio/imu/st_lsm6dsx/Makefile b/drivers/iio/imu/st_lsm6dsx/Makefile
> index e5f733c..c676965 100644
> --- a/drivers/iio/imu/st_lsm6dsx/Makefile
> +++ b/drivers/iio/imu/st_lsm6dsx/Makefile
> @@ -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
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c
> new file mode 100644
> index 0000000..2df5e70
> --- /dev/null
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c
> @@ -0,0 +1,67 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018 Synopsys, Inc. and/or its affiliates.
> + *
> + * Author: Vitor Soares <vitor.soares@synopsys.com>
> + */
> +
> +#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));
> +               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

> +}
> +

please remove 'new line' here

> +
> +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 <vitor.soares@synopsys.com>");
> +MODULE_DESCRIPTION("STMicroelectronics st_lsm6dsx i3c driver");
> +MODULE_LICENSE("GPL v2");
> +
> --
> 2.7.4
>


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

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

[Vitor Soares]

HI Lorenzo,

From: Lorenzo Bianconi <lorenzo.bianconi83@gmail.com>
Date: Mon, Apr 15, 2019 at 23:17:50

> >
> > 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
> 
> > Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
> > ---
> >  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
> >
> > diff --git a/drivers/iio/imu/st_lsm6dsx/Kconfig b/drivers/iio/imu/st_lsm6dsx/Kconfig
> > index 094fd00..1ab9bbf 100644
> > --- a/drivers/iio/imu/st_lsm6dsx/Kconfig
> > +++ b/drivers/iio/imu/st_lsm6dsx/Kconfig
> > @@ -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
> > diff --git a/drivers/iio/imu/st_lsm6dsx/Makefile b/drivers/iio/imu/st_lsm6dsx/Makefile
> > index e5f733c..c676965 100644
> > --- a/drivers/iio/imu/st_lsm6dsx/Makefile
> > +++ b/drivers/iio/imu/st_lsm6dsx/Makefile
> > @@ -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
> > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c
> > new file mode 100644
> > index 0000000..2df5e70
> > --- /dev/null
> > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c
> > @@ -0,0 +1,67 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2018 Synopsys, Inc. and/or its affiliates.
> > + *
> > + * Author: Vitor Soares <vitor.soares@synopsys.com>
> > + */
> > +
> > +#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));
> > +               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.

> 
> > +}
> > +
> 
> please remove 'new line' here

Sure.

> 
> > +
> > +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 <vitor.soares@synopsys.com>");
> > +MODULE_DESCRIPTION("STMicroelectronics st_lsm6dsx i3c driver");
> > +MODULE_LICENSE("GPL v2");
> > +
> > --
> > 2.7.4
> >
> 
> 
> -- 
> 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

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

[Boris Brezillon]

On Mon, 15 Apr 2019 21:19:41 +0200
Vitor Soares <vitor.soares@synopsys.com> wrote:

> 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 <vitor.soares@synopsys.com>
> ---
>  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
> 
> diff --git a/drivers/iio/imu/st_lsm6dsx/Kconfig b/drivers/iio/imu/st_lsm6dsx/Kconfig
> index 094fd00..1ab9bbf 100644
> --- a/drivers/iio/imu/st_lsm6dsx/Kconfig
> +++ b/drivers/iio/imu/st_lsm6dsx/Kconfig
> @@ -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
> diff --git a/drivers/iio/imu/st_lsm6dsx/Makefile b/drivers/iio/imu/st_lsm6dsx/Makefile
> index e5f733c..c676965 100644
> --- a/drivers/iio/imu/st_lsm6dsx/Makefile
> +++ b/drivers/iio/imu/st_lsm6dsx/Makefile
> @@ -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
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c
> new file mode 100644
> index 0000000..2df5e70
> --- /dev/null
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c
> @@ -0,0 +1,67 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018 Synopsys, Inc. and/or its affiliates.
> + *
> + * Author: Vitor Soares <vitor.soares@synopsys.com>
> + */
> +
> +#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];

	const char *name;

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

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

> +	const struct st_lsm6dsx_i3c_data *hw_data = id->data;
> +	struct regmap *regmap;
> +
> +	regmap = devm_regmap_init_i3c(i3cdev, &st_lsm6dsx_i3c_regmap_config);
> +	if (IS_ERR(regmap)) {
> +		dev_err(&i3cdev->dev, "Failed to register i3c regmap %d\n",
> +			(int)PTR_ERR(regmap));
> +		return PTR_ERR(regmap);
> +	}
> +
> +	return st_lsm6dsx_probe(&i3cdev->dev, 0, hw_data->id,
> +				hw_data->name, regmap);
> +}
> +
> +
> +static const struct i3c_device_id st_lsm6dsx_i3c_ids[] = {
> +	I3C_DEVICE(0x0104, 0x006C, &hw_data[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).

> +};
> +module_i3c_driver(st_lsm6dsx_driver);
> +
> +MODULE_AUTHOR("Vitor Soares <vitor.soares@synopsys.com>");
> +MODULE_DESCRIPTION("STMicroelectronics st_lsm6dsx i3c driver");
> +MODULE_LICENSE("GPL v2");
> +

You can remove this blank line.


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

[Lorenzo Bianconi]

>
> On Mon, 15 Apr 2019 21:19:41 +0200
> Vitor Soares <vitor.soares@synopsys.com> wrote:
>
> > 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 <vitor.soares@synopsys.com>
> > ---
> >  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
> >
> > diff --git a/drivers/iio/imu/st_lsm6dsx/Kconfig b/drivers/iio/imu/st_lsm6dsx/Kconfig
> > index 094fd00..1ab9bbf 100644
> > --- a/drivers/iio/imu/st_lsm6dsx/Kconfig
> > +++ b/drivers/iio/imu/st_lsm6dsx/Kconfig
> > @@ -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
> > diff --git a/drivers/iio/imu/st_lsm6dsx/Makefile b/drivers/iio/imu/st_lsm6dsx/Makefile
> > index e5f733c..c676965 100644
> > --- a/drivers/iio/imu/st_lsm6dsx/Makefile
> > +++ b/drivers/iio/imu/st_lsm6dsx/Makefile
> > @@ -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
> > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c
> > new file mode 100644
> > index 0000000..2df5e70
> > --- /dev/null
> > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c
> > @@ -0,0 +1,67 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2018 Synopsys, Inc. and/or its affiliates.
> > + *
> > + * Author: Vitor Soares <vitor.soares@synopsys.com>
> > + */
> > +
> > +#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];
>
>         const char *name;
>
> > +     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

>
> > +     { 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);
>
> > +     const struct st_lsm6dsx_i3c_data *hw_data = id->data;
> > +     struct regmap *regmap;
> > +
> > +     regmap = devm_regmap_init_i3c(i3cdev, &st_lsm6dsx_i3c_regmap_config);
> > +     if (IS_ERR(regmap)) {
> > +             dev_err(&i3cdev->dev, "Failed to register i3c regmap %d\n",
> > +                     (int)PTR_ERR(regmap));
> > +             return PTR_ERR(regmap);
> > +     }
> > +
> > +     return st_lsm6dsx_probe(&i3cdev->dev, 0, hw_data->id,
> > +                             hw_data->name, regmap);
> > +}
> > +
> > +
> > +static const struct i3c_device_id st_lsm6dsx_i3c_ids[] = {
> > +     I3C_DEVICE(0x0104, 0x006C, &hw_data[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).
>
> > +};
> > +module_i3c_driver(st_lsm6dsx_driver);
> > +
> > +MODULE_AUTHOR("Vitor Soares <vitor.soares@synopsys.com>");
> > +MODULE_DESCRIPTION("STMicroelectronics st_lsm6dsx i3c driver");
> > +MODULE_LICENSE("GPL v2");
> > +
>
> You can remove this blank line.
>


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

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

[Boris Brezillon]

On Tue, 16 Apr 2019 09:58:33 +0200
Lorenzo Bianconi <lorenzo.bianconi83@gmail.com> wrote:

> >
> > On Mon, 15 Apr 2019 21:19:41 +0200
> > Vitor Soares <vitor.soares@synopsys.com> wrote:
> >  
> > > 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 <vitor.soares@synopsys.com>
> > > ---
> > >  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
> > >
> > > diff --git a/drivers/iio/imu/st_lsm6dsx/Kconfig b/drivers/iio/imu/st_lsm6dsx/Kconfig
> > > index 094fd00..1ab9bbf 100644
> > > --- a/drivers/iio/imu/st_lsm6dsx/Kconfig
> > > +++ b/drivers/iio/imu/st_lsm6dsx/Kconfig
> > > @@ -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
> > > diff --git a/drivers/iio/imu/st_lsm6dsx/Makefile b/drivers/iio/imu/st_lsm6dsx/Makefile
> > > index e5f733c..c676965 100644
> > > --- a/drivers/iio/imu/st_lsm6dsx/Makefile
> > > +++ b/drivers/iio/imu/st_lsm6dsx/Makefile
> > > @@ -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
> > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c
> > > new file mode 100644
> > > index 0000000..2df5e70
> > > --- /dev/null
> > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c
> > > @@ -0,0 +1,67 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright (c) 2018 Synopsys, Inc. and/or its affiliates.
> > > + *
> > > + * Author: Vitor Soares <vitor.soares@synopsys.com>
> > > + */
> > > +
> > > +#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];  
> >
> >         const char *name;
> >  
> > > +     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

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.

> 
> Regards,
> Lorenzo
> 
> >  
> > > +     { 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);
> >  
> > > +     const struct st_lsm6dsx_i3c_data *hw_data = id->data;
> > > +     struct regmap *regmap;
> > > +
> > > +     regmap = devm_regmap_init_i3c(i3cdev, &st_lsm6dsx_i3c_regmap_config);
> > > +     if (IS_ERR(regmap)) {
> > > +             dev_err(&i3cdev->dev, "Failed to register i3c regmap %d\n",
> > > +                     (int)PTR_ERR(regmap));
> > > +             return PTR_ERR(regmap);
> > > +     }
> > > +
> > > +     return st_lsm6dsx_probe(&i3cdev->dev, 0, hw_data->id,
> > > +                             hw_data->name, regmap);
> > > +}
> > > +
> > > +
> > > +static const struct i3c_device_id st_lsm6dsx_i3c_ids[] = {
> > > +     I3C_DEVICE(0x0104, 0x006C, &hw_data[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).
> >  
> > > +};
> > > +module_i3c_driver(st_lsm6dsx_driver);
> > > +
> > > +MODULE_AUTHOR("Vitor Soares <vitor.soares@synopsys.com>");
> > > +MODULE_DESCRIPTION("STMicroelectronics st_lsm6dsx i3c driver");
> > > +MODULE_LICENSE("GPL v2");
> > > +  
> >
> > You can remove this blank line.
> >  
> 
> 


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

[Vitor Soares]

Hi Boris  and Lorenzo,

From: Boris Brezillon <boris.brezillon@collabora.com>
Date: Tue, Apr 16, 2019 at 09:05:40

> On Tue, 16 Apr 2019 09:58:33 +0200
> Lorenzo Bianconi <lorenzo.bianconi83@gmail.com> wrote:
> 
> > >
> > > On Mon, 15 Apr 2019 21:19:41 +0200
> > > Vitor Soares <vitor.soares@synopsys.com> wrote:
> > >  
> > > > 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 <vitor.soares@synopsys.com>
> > > > ---
> > > >  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
> > > >
> > > > diff --git a/drivers/iio/imu/st_lsm6dsx/Kconfig b/drivers/iio/imu/st_lsm6dsx/Kconfig
> > > > index 094fd00..1ab9bbf 100644
> > > > --- a/drivers/iio/imu/st_lsm6dsx/Kconfig
> > > > +++ b/drivers/iio/imu/st_lsm6dsx/Kconfig
> > > > @@ -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
> > > > diff --git a/drivers/iio/imu/st_lsm6dsx/Makefile b/drivers/iio/imu/st_lsm6dsx/Makefile
> > > > index e5f733c..c676965 100644
> > > > --- a/drivers/iio/imu/st_lsm6dsx/Makefile
> > > > +++ b/drivers/iio/imu/st_lsm6dsx/Makefile
> > > > @@ -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
> > > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c
> > > > new file mode 100644
> > > > index 0000000..2df5e70
> > > > --- /dev/null
> > > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c
> > > > @@ -0,0 +1,67 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * Copyright (c) 2018 Synopsys, Inc. and/or its affiliates.
> > > > + *
> > > > + * Author: Vitor Soares <vitor.soares@synopsys.com>
> > > > + */
> > > > +
> > > > +#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];  
> > >
> > >         const char *name;

I will fix this next patch.

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

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

> 
> > 
> > Regards,
> > Lorenzo
> > 
> > >  
> > > > +     { 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);
> > >  
> > > > +     const struct st_lsm6dsx_i3c_data *hw_data = id->data;
> > > > +     struct regmap *regmap;
> > > > +
> > > > +     regmap = devm_regmap_init_i3c(i3cdev, &st_lsm6dsx_i3c_regmap_config);
> > > > +     if (IS_ERR(regmap)) {
> > > > +             dev_err(&i3cdev->dev, "Failed to register i3c regmap %d\n",
> > > > +                     (int)PTR_ERR(regmap));
> > > > +             return PTR_ERR(regmap);
> > > > +     }
> > > > +
> > > > +     return st_lsm6dsx_probe(&i3cdev->dev, 0, hw_data->id,
> > > > +                             hw_data->name, regmap);
> > > > +}
> > > > +
> > > > +
> > > > +static const struct i3c_device_id st_lsm6dsx_i3c_ids[] = {
> > > > +     I3C_DEVICE(0x0104, 0x006C, &hw_data[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).
> > >  
> > > > +};
> > > > +module_i3c_driver(st_lsm6dsx_driver);
> > > > +
> > > > +MODULE_AUTHOR("Vitor Soares <vitor.soares@synopsys.com>");
> > > > +MODULE_DESCRIPTION("STMicroelectronics st_lsm6dsx i3c driver");
> > > > +MODULE_LICENSE("GPL v2");
> > > > +  
> > >
> > > You can remove this blank line.
> > >  
> > 
> > 

Best regards,
Vitor Soares

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

[Vitor Soares]

Hi Boris,

From: Boris Brezillon <boris.brezillon@collabora.com>
Date: Tue, Apr 16, 2019 at 07:25:39
> > +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. 

> 
> > +};
> > +module_i3c_driver(st_lsm6dsx_driver);
> > +
> > +MODULE_AUTHOR("Vitor Soares <vitor.soares@synopsys.com>");
> > +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



_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH 0/3] Add ST lsm6dso i3c suppport

[Boris Brezillon]

Hi Vitor,

On Mon, 15 Apr 2019 21:19:38 +0200
Vitor Soares <vitor.soares@synopsys.com> 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

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


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c