diff mbox series

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

Message ID f239834a6b8bd179094cdc19a3ac5ecaf807cee3.1562931742.git.vitor.soares@synopsys.com (mailing list archive)
State New, archived
Headers show
Series Add ST lsm6dso i3c support | expand

Commit Message

Vitor Soares July 12, 2019, 11:53 a.m. UTC
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 <vitor.soares@synopsys.com>
Acked-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
Changes in v4:
  Remove hw_id variable

Changes in v3:
  Remove unnecessary st_lsm6dsx_i3c_data table used to hold device name
  Use st_lsm6dsx_probe new form

Changes in v2:
  Add support for LSM6DSR
  Set pm_ops to st_lsm6dsx_pm_ops

 drivers/iio/imu/st_lsm6dsx/Kconfig          |  8 +++-
 drivers/iio/imu/st_lsm6dsx/Makefile         |  1 +
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c | 58 +++++++++++++++++++++++++++++
 3 files changed, 66 insertions(+), 1 deletion(-)
 create mode 100644 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c

Comments

Boris Brezillon July 12, 2019, 4:14 p.m. UTC | #1
On Fri, 12 Jul 2019 13:53:30 +0200
Vitor Soares <Vitor.Soares@synopsys.com> wrote:

> 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 <vitor.soares@synopsys.com>
> Acked-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
> Changes in v4:
>   Remove hw_id variable
> 
> Changes in v3:
>   Remove unnecessary st_lsm6dsx_i3c_data table used to hold device name
>   Use st_lsm6dsx_probe new form
> 
> Changes in v2:
>   Add support for LSM6DSR
>   Set pm_ops to st_lsm6dsx_pm_ops
> 
>  drivers/iio/imu/st_lsm6dsx/Kconfig          |  8 +++-
>  drivers/iio/imu/st_lsm6dsx/Makefile         |  1 +
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c | 58 +++++++++++++++++++++++++++++
>  3 files changed, 66 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c
> 
> diff --git a/drivers/iio/imu/st_lsm6dsx/Kconfig b/drivers/iio/imu/st_lsm6dsx/Kconfig
> index 9e59297..6b5a73c 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..2e89524
> --- /dev/null
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c
> @@ -0,0 +1,58 @@
> +// 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"
> +
> +static const struct i3c_device_id st_lsm6dsx_i3c_ids[] = {
> +	I3C_DEVICE(0x0104, 0x006C, (void *)ST_LSM6DSO_ID),
> +	I3C_DEVICE(0x0104, 0x006B, (void *)ST_LSM6DSR_ID),

I think you need an uintptr_t cast here:

	I3C_DEVICE(0x0104, 0x006C, (void *)(uintptr_t)ST_LSM6DSO_ID),
	I3C_DEVICE(0x0104, 0x006B, (void *)(uintptr_t)ST_LSM6DSR_ID),

otherwise gcc might complain that the integer and pointer do not have
the same size (on 64-bit architectures).

> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(i3c, st_lsm6dsx_i3c_ids);
> +
> +static const struct regmap_config st_lsm6dsx_i3c_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +};

This can be moved ...

> +
> +static int st_lsm6dsx_i3c_probe(struct i3c_device *i3cdev)
> +{

... here without the static and const qualifiers:

	struct regmap_config regmap_config = {
		.reg_bits = 8,
		.val_bits = 8,
	};

> +	const struct i3c_device_id *id = i3c_device_match_id(i3cdev,
> +							    st_lsm6dsx_i3c_ids);
> +	struct regmap *regmap;
> +
> +	regmap = devm_regmap_init_i3c(i3cdev, &st_lsm6dsx_i3c_regmap_config);
> +	if (IS_ERR(regmap)) {
> +		dev_err(&i3cdev->dev, "Failed to register i3c regmap %d\n",
> +			(int)PTR_ERR(regmap));
> +		return PTR_ERR(regmap);
> +	}
> +
> +	return st_lsm6dsx_probe(&i3cdev->dev, 0, (int)id->data, regmap);

uintptr_t cast here.

> +}
> +
> +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 <vitor.soares@synopsys.com>");
> +MODULE_DESCRIPTION("STMicroelectronics st_lsm6dsx i3c driver");
> +MODULE_LICENSE("GPL v2");
Vitor Soares July 12, 2019, 4:28 p.m. UTC | #2
From: Boris Brezillon <boris.brezillon@collabora.com>
Date: Fri, Jul 12, 2019 at 17:14:29

> On Fri, 12 Jul 2019 13:53:30 +0200
> Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> 
> > 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 <vitor.soares@synopsys.com>
> > Acked-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > ---
> > Changes in v4:
> >   Remove hw_id variable
> > 
> > Changes in v3:
> >   Remove unnecessary st_lsm6dsx_i3c_data table used to hold device name
> >   Use st_lsm6dsx_probe new form
> > 
> > Changes in v2:
> >   Add support for LSM6DSR
> >   Set pm_ops to st_lsm6dsx_pm_ops
> > 
> >  drivers/iio/imu/st_lsm6dsx/Kconfig          |  8 +++-
> >  drivers/iio/imu/st_lsm6dsx/Makefile         |  1 +
> >  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c | 58 +++++++++++++++++++++++++++++
> >  3 files changed, 66 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c
> > 
> > diff --git a/drivers/iio/imu/st_lsm6dsx/Kconfig b/drivers/iio/imu/st_lsm6dsx/Kconfig
> > index 9e59297..6b5a73c 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..2e89524
> > --- /dev/null
> > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c
> > @@ -0,0 +1,58 @@
> > +// 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"
> > +
> > +static const struct i3c_device_id st_lsm6dsx_i3c_ids[] = {
> > +	I3C_DEVICE(0x0104, 0x006C, (void *)ST_LSM6DSO_ID),
> > +	I3C_DEVICE(0x0104, 0x006B, (void *)ST_LSM6DSR_ID),
> 
> I think you need an uintptr_t cast here:
> 
> 	I3C_DEVICE(0x0104, 0x006C, (void *)(uintptr_t)ST_LSM6DSO_ID),
> 	I3C_DEVICE(0x0104, 0x006B, (void *)(uintptr_t)ST_LSM6DSR_ID),
> 
> otherwise gcc might complain that the integer and pointer do not have
> the same size (on 64-bit architectures).

I don't understand this part. Can you provide or point some background? 

> 
> > +	{ /* sentinel */ },
> > +};
> > +MODULE_DEVICE_TABLE(i3c, st_lsm6dsx_i3c_ids);
> > +
> > +static const struct regmap_config st_lsm6dsx_i3c_regmap_config = {
> > +	.reg_bits = 8,
> > +	.val_bits = 8,
> > +};
> 
> This can be moved ...
> 
> > +
> > +static int st_lsm6dsx_i3c_probe(struct i3c_device *i3cdev)
> > +{
> 
> ... here without the static and const qualifiers:

I understand that can be move to here, but I don't understand the 
advantages. Can you explain?

> 
> 	struct regmap_config regmap_config = {
> 		.reg_bits = 8,
> 		.val_bits = 8,
> 	};
> 
> > +	const struct i3c_device_id *id = i3c_device_match_id(i3cdev,
> > +							    st_lsm6dsx_i3c_ids);
> > +	struct regmap *regmap;
> > +
> > +	regmap = devm_regmap_init_i3c(i3cdev, &st_lsm6dsx_i3c_regmap_config);
> > +	if (IS_ERR(regmap)) {
> > +		dev_err(&i3cdev->dev, "Failed to register i3c regmap %d\n",
> > +			(int)PTR_ERR(regmap));
> > +		return PTR_ERR(regmap);
> > +	}
> > +
> > +	return st_lsm6dsx_probe(&i3cdev->dev, 0, (int)id->data, regmap);
> 
> uintptr_t cast here.
> 
> > +}
> > +
> > +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 <vitor.soares@synopsys.com>");
> > +MODULE_DESCRIPTION("STMicroelectronics st_lsm6dsx i3c driver");
> > +MODULE_LICENSE("GPL v2");

Best regards,
Vitor Soares
Boris Brezillon July 12, 2019, 4:43 p.m. UTC | #3
On Fri, 12 Jul 2019 16:28:02 +0000
Vitor Soares <Vitor.Soares@synopsys.com> wrote:

> From: Boris Brezillon <boris.brezillon@collabora.com>
> Date: Fri, Jul 12, 2019 at 17:14:29
> 
> > On Fri, 12 Jul 2019 13:53:30 +0200
> > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> >   
> > > 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 <vitor.soares@synopsys.com>
> > > Acked-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > ---
> > > Changes in v4:
> > >   Remove hw_id variable
> > > 
> > > Changes in v3:
> > >   Remove unnecessary st_lsm6dsx_i3c_data table used to hold device name
> > >   Use st_lsm6dsx_probe new form
> > > 
> > > Changes in v2:
> > >   Add support for LSM6DSR
> > >   Set pm_ops to st_lsm6dsx_pm_ops
> > > 
> > >  drivers/iio/imu/st_lsm6dsx/Kconfig          |  8 +++-
> > >  drivers/iio/imu/st_lsm6dsx/Makefile         |  1 +
> > >  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c | 58 +++++++++++++++++++++++++++++
> > >  3 files changed, 66 insertions(+), 1 deletion(-)
> > >  create mode 100644 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c
> > > 
> > > diff --git a/drivers/iio/imu/st_lsm6dsx/Kconfig b/drivers/iio/imu/st_lsm6dsx/Kconfig
> > > index 9e59297..6b5a73c 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..2e89524
> > > --- /dev/null
> > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c
> > > @@ -0,0 +1,58 @@
> > > +// 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"
> > > +
> > > +static const struct i3c_device_id st_lsm6dsx_i3c_ids[] = {
> > > +	I3C_DEVICE(0x0104, 0x006C, (void *)ST_LSM6DSO_ID),
> > > +	I3C_DEVICE(0x0104, 0x006B, (void *)ST_LSM6DSR_ID),  
> > 
> > I think you need an uintptr_t cast here:
> > 
> > 	I3C_DEVICE(0x0104, 0x006C, (void *)(uintptr_t)ST_LSM6DSO_ID),
> > 	I3C_DEVICE(0x0104, 0x006B, (void *)(uintptr_t)ST_LSM6DSR_ID),
> > 
> > otherwise gcc might complain that the integer and pointer do not have
> > the same size (on 64-bit architectures).  
> 
> I don't understand this part. Can you provide or point some background?

If you don't do that you'll get the following warning:

	warning: cast to 'void *' from smaller integer type 'int' [-Wint-to-void-pointer-cast]

> 
> >   
> > > +	{ /* sentinel */ },
> > > +};
> > > +MODULE_DEVICE_TABLE(i3c, st_lsm6dsx_i3c_ids);
> > > +
> > > +static const struct regmap_config st_lsm6dsx_i3c_regmap_config = {
> > > +	.reg_bits = 8,
> > > +	.val_bits = 8,
> > > +};  
> > 
> > This can be moved ...
> >   
> > > +
> > > +static int st_lsm6dsx_i3c_probe(struct i3c_device *i3cdev)
> > > +{  
> > 
> > ... here without the static and const qualifiers:  
> 
> I understand that can be move to here, but I don't understand the 
> advantages. Can you explain?

It reduces the variable scope (this variable is only needed in the
probe path) and avoids consuming space in the .bss section, though the
second point is not so important.
Vitor Soares July 12, 2019, 6:40 p.m. UTC | #4
From: Boris Brezillon <boris.brezillon@collabora.com>
Date: Fri, Jul 12, 2019 at 17:43:23

> On Fri, 12 Jul 2019 16:28:02 +0000
> Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> 
> > From: Boris Brezillon <boris.brezillon@collabora.com>
> > Date: Fri, Jul 12, 2019 at 17:14:29
> > 
> > > On Fri, 12 Jul 2019 13:53:30 +0200
> > > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> > >   
> > > > 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 <vitor.soares@synopsys.com>
> > > > Acked-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > ---
> > > > Changes in v4:
> > > >   Remove hw_id variable
> > > > 
> > > > Changes in v3:
> > > >   Remove unnecessary st_lsm6dsx_i3c_data table used to hold device name
> > > >   Use st_lsm6dsx_probe new form
> > > > 
> > > > Changes in v2:
> > > >   Add support for LSM6DSR
> > > >   Set pm_ops to st_lsm6dsx_pm_ops
> > > > 
> > > >  drivers/iio/imu/st_lsm6dsx/Kconfig          |  8 +++-
> > > >  drivers/iio/imu/st_lsm6dsx/Makefile         |  1 +
> > > >  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c | 58 +++++++++++++++++++++++++++++
> > > >  3 files changed, 66 insertions(+), 1 deletion(-)
> > > >  create mode 100644 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c
> > > > 
> > > > diff --git a/drivers/iio/imu/st_lsm6dsx/Kconfig b/drivers/iio/imu/st_lsm6dsx/Kconfig
> > > > index 9e59297..6b5a73c 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..2e89524
> > > > --- /dev/null
> > > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c
> > > > @@ -0,0 +1,58 @@
> > > > +// 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"
> > > > +
> > > > +static const struct i3c_device_id st_lsm6dsx_i3c_ids[] = {
> > > > +	I3C_DEVICE(0x0104, 0x006C, (void *)ST_LSM6DSO_ID),
> > > > +	I3C_DEVICE(0x0104, 0x006B, (void *)ST_LSM6DSR_ID),  
> > > 
> > > I think you need an uintptr_t cast here:
> > > 
> > > 	I3C_DEVICE(0x0104, 0x006C, (void *)(uintptr_t)ST_LSM6DSO_ID),
> > > 	I3C_DEVICE(0x0104, 0x006B, (void *)(uintptr_t)ST_LSM6DSR_ID),
> > > 
> > > otherwise gcc might complain that the integer and pointer do not have
> > > the same size (on 64-bit architectures).  
> > 
> > I don't understand this part. Can you provide or point some background?
> 
> If you don't do that you'll get the following warning:
> 
> 	warning: cast to 'void *' from smaller integer type 'int' [-Wint-to-void-pointer-cast]

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

> 
> > 
> > >   
> > > > +	{ /* sentinel */ },
> > > > +};
> > > > +MODULE_DEVICE_TABLE(i3c, st_lsm6dsx_i3c_ids);
> > > > +
> > > > +static const struct regmap_config st_lsm6dsx_i3c_regmap_config = {
> > > > +	.reg_bits = 8,
> > > > +	.val_bits = 8,
> > > > +};  
> > > 
> > > This can be moved ...
> > >   
> > > > +
> > > > +static int st_lsm6dsx_i3c_probe(struct i3c_device *i3cdev)
> > > > +{  
> > > 
> > > ... here without the static and const qualifiers:  
> > 
> > I understand that can be move to here, but I don't understand the 
> > advantages. Can you explain?
> 
> It reduces the variable scope (this variable is only needed in the
> probe path) and avoids consuming space in the .bss section, though the
> second point is not so important.

Thanks,
Vitor Soares
Boris Brezillon July 12, 2019, 8:03 p.m. UTC | #5
On Fri, 12 Jul 2019 18:40:14 +0000
Vitor Soares <Vitor.Soares@synopsys.com> wrote:

> From: Boris Brezillon <boris.brezillon@collabora.com>
> Date: Fri, Jul 12, 2019 at 17:43:23
> 
> > On Fri, 12 Jul 2019 16:28:02 +0000
> > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> >   
> > > From: Boris Brezillon <boris.brezillon@collabora.com>
> > > Date: Fri, Jul 12, 2019 at 17:14:29
> > >   
> > > > On Fri, 12 Jul 2019 13:53:30 +0200
> > > > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> > > >     
> > > > > 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 <vitor.soares@synopsys.com>
> > > > > Acked-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > > ---
> > > > > Changes in v4:
> > > > >   Remove hw_id variable
> > > > > 
> > > > > Changes in v3:
> > > > >   Remove unnecessary st_lsm6dsx_i3c_data table used to hold device name
> > > > >   Use st_lsm6dsx_probe new form
> > > > > 
> > > > > Changes in v2:
> > > > >   Add support for LSM6DSR
> > > > >   Set pm_ops to st_lsm6dsx_pm_ops
> > > > > 
> > > > >  drivers/iio/imu/st_lsm6dsx/Kconfig          |  8 +++-
> > > > >  drivers/iio/imu/st_lsm6dsx/Makefile         |  1 +
> > > > >  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c | 58 +++++++++++++++++++++++++++++
> > > > >  3 files changed, 66 insertions(+), 1 deletion(-)
> > > > >  create mode 100644 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c
> > > > > 
> > > > > diff --git a/drivers/iio/imu/st_lsm6dsx/Kconfig b/drivers/iio/imu/st_lsm6dsx/Kconfig
> > > > > index 9e59297..6b5a73c 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..2e89524
> > > > > --- /dev/null
> > > > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c
> > > > > @@ -0,0 +1,58 @@
> > > > > +// 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"
> > > > > +
> > > > > +static const struct i3c_device_id st_lsm6dsx_i3c_ids[] = {
> > > > > +	I3C_DEVICE(0x0104, 0x006C, (void *)ST_LSM6DSO_ID),
> > > > > +	I3C_DEVICE(0x0104, 0x006B, (void *)ST_LSM6DSR_ID),    
> > > > 
> > > > I think you need an uintptr_t cast here:
> > > > 
> > > > 	I3C_DEVICE(0x0104, 0x006C, (void *)(uintptr_t)ST_LSM6DSO_ID),
> > > > 	I3C_DEVICE(0x0104, 0x006B, (void *)(uintptr_t)ST_LSM6DSR_ID),
> > > > 
> > > > otherwise gcc might complain that the integer and pointer do not have
> > > > the same size (on 64-bit architectures).    
> > > 
> > > I don't understand this part. Can you provide or point some background?  
> > 
> > If you don't do that you'll get the following warning:
> > 
> > 	warning: cast to 'void *' from smaller integer type 'int' [-Wint-to-void-pointer-cast]  
> 
> I don't get the warning during compilation. Is there any flag to enable 
> or so?

Nope, nothing specific to enable, just enable this driver on an arm64
config. Note that that gcc doesn't seem to complain about this
int -> void * cast (there's probably some kind of auto-promotion to
pointer size), but it does complains about the following void * -> int
cast:

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c: In function ‘st_lsm6dsx_i3c_probe’:
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c:43:43: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
   43 |  return st_lsm6dsx_probe(&i3cdev->dev, 0, (int)id->data, regmap);
      |
Vitor Soares July 16, 2019, 1:22 p.m. UTC | #6
Hi Boris,

From: Boris Brezillon <boris.brezillon@collabora.com>
Date: Fri, Jul 12, 2019 at 21:03:20

> On Fri, 12 Jul 2019 18:40:14 +0000
> Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> 
> > From: Boris Brezillon <boris.brezillon@collabora.com>
> > Date: Fri, Jul 12, 2019 at 17:43:23
> > 
> > > On Fri, 12 Jul 2019 16:28:02 +0000
> > > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> > >   
> > > > From: Boris Brezillon <boris.brezillon@collabora.com>
> > > > Date: Fri, Jul 12, 2019 at 17:14:29
> > > >   
> > > > > On Fri, 12 Jul 2019 13:53:30 +0200
> > > > > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> > > > >     
> > > > > > 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 <vitor.soares@synopsys.com>
> > > > > > Acked-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > > > ---
> > > > > > Changes in v4:
> > > > > >   Remove hw_id variable
> > > > > > 
> > > > > > Changes in v3:
> > > > > >   Remove unnecessary st_lsm6dsx_i3c_data table used to hold device name
> > > > > >   Use st_lsm6dsx_probe new form
> > > > > > 
> > > > > > Changes in v2:
> > > > > >   Add support for LSM6DSR
> > > > > >   Set pm_ops to st_lsm6dsx_pm_ops
> > > > > > 
> > > > > >  drivers/iio/imu/st_lsm6dsx/Kconfig          |  8 +++-
> > > > > >  drivers/iio/imu/st_lsm6dsx/Makefile         |  1 +
> > > > > >  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c | 58 +++++++++++++++++++++++++++++
> > > > > >  3 files changed, 66 insertions(+), 1 deletion(-)
> > > > > >  create mode 100644 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c
> > > > > > 
> > > > > > diff --git a/drivers/iio/imu/st_lsm6dsx/Kconfig b/drivers/iio/imu/st_lsm6dsx/Kconfig
> > > > > > index 9e59297..6b5a73c 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..2e89524
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c
> > > > > > @@ -0,0 +1,58 @@
> > > > > > +// 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"
> > > > > > +
> > > > > > +static const struct i3c_device_id st_lsm6dsx_i3c_ids[] = {
> > > > > > +	I3C_DEVICE(0x0104, 0x006C, (void *)ST_LSM6DSO_ID),
> > > > > > +	I3C_DEVICE(0x0104, 0x006B, (void *)ST_LSM6DSR_ID),    
> > > > > 
> > > > > I think you need an uintptr_t cast here:
> > > > > 
> > > > > 	I3C_DEVICE(0x0104, 0x006C, (void *)(uintptr_t)ST_LSM6DSO_ID),
> > > > > 	I3C_DEVICE(0x0104, 0x006B, (void *)(uintptr_t)ST_LSM6DSR_ID),
> > > > > 
> > > > > otherwise gcc might complain that the integer and pointer do not have
> > > > > the same size (on 64-bit architectures).    
> > > > 
> > > > I don't understand this part. Can you provide or point some background?  
> > > 
> > > If you don't do that you'll get the following warning:
> > > 
> > > 	warning: cast to 'void *' from smaller integer type 'int' [-Wint-to-void-pointer-cast]  
> > 
> > I don't get the warning during compilation. Is there any flag to enable 
> > or so?
> 
> Nope, nothing specific to enable, just enable this driver on an arm64
> config. Note that that gcc doesn't seem to complain about this
> int -> void * cast (there's probably some kind of auto-promotion to
> pointer size), but it does complains about the following void * -> int
> cast:
> 
> drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c: In function ‘st_lsm6dsx_i3c_probe’:
> drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c:43:43: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
>    43 |  return st_lsm6dsx_probe(&i3cdev->dev, 0, (int)id->data, regmap);
>       |     

I fixed the warning by changing:

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

to

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

But I wonder if it isn't more save to change the following too:
	I3C_DEVICE(0x0104, 0x006C, (void *)(uintptr_t)ST_LSM6DSO_ID),
	I3C_DEVICE(0x0104, 0x006B, (void *)(uintptr_t)ST_LSM6DSR_ID),

What do you think?

Best regards,
Vitor Soares
Boris Brezillon July 16, 2019, 1:40 p.m. UTC | #7
On Tue, 16 Jul 2019 13:22:25 +0000
Vitor Soares <Vitor.Soares@synopsys.com> wrote:

> Hi Boris,
> 
> From: Boris Brezillon <boris.brezillon@collabora.com>
> Date: Fri, Jul 12, 2019 at 21:03:20
> 
> > On Fri, 12 Jul 2019 18:40:14 +0000
> > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> >   
> > > From: Boris Brezillon <boris.brezillon@collabora.com>
> > > Date: Fri, Jul 12, 2019 at 17:43:23
> > >   
> > > > On Fri, 12 Jul 2019 16:28:02 +0000
> > > > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> > > >     
> > > > > From: Boris Brezillon <boris.brezillon@collabora.com>
> > > > > Date: Fri, Jul 12, 2019 at 17:14:29
> > > > >     
> > > > > > On Fri, 12 Jul 2019 13:53:30 +0200
> > > > > > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> > > > > >       
> > > > > > > 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 <vitor.soares@synopsys.com>
> > > > > > > Acked-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > > > > ---
> > > > > > > Changes in v4:
> > > > > > >   Remove hw_id variable
> > > > > > > 
> > > > > > > Changes in v3:
> > > > > > >   Remove unnecessary st_lsm6dsx_i3c_data table used to hold device name
> > > > > > >   Use st_lsm6dsx_probe new form
> > > > > > > 
> > > > > > > Changes in v2:
> > > > > > >   Add support for LSM6DSR
> > > > > > >   Set pm_ops to st_lsm6dsx_pm_ops
> > > > > > > 
> > > > > > >  drivers/iio/imu/st_lsm6dsx/Kconfig          |  8 +++-
> > > > > > >  drivers/iio/imu/st_lsm6dsx/Makefile         |  1 +
> > > > > > >  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c | 58 +++++++++++++++++++++++++++++
> > > > > > >  3 files changed, 66 insertions(+), 1 deletion(-)
> > > > > > >  create mode 100644 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c
> > > > > > > 
> > > > > > > diff --git a/drivers/iio/imu/st_lsm6dsx/Kconfig b/drivers/iio/imu/st_lsm6dsx/Kconfig
> > > > > > > index 9e59297..6b5a73c 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..2e89524
> > > > > > > --- /dev/null
> > > > > > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c
> > > > > > > @@ -0,0 +1,58 @@
> > > > > > > +// 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"
> > > > > > > +
> > > > > > > +static const struct i3c_device_id st_lsm6dsx_i3c_ids[] = {
> > > > > > > +	I3C_DEVICE(0x0104, 0x006C, (void *)ST_LSM6DSO_ID),
> > > > > > > +	I3C_DEVICE(0x0104, 0x006B, (void *)ST_LSM6DSR_ID),      
> > > > > > 
> > > > > > I think you need an uintptr_t cast here:
> > > > > > 
> > > > > > 	I3C_DEVICE(0x0104, 0x006C, (void *)(uintptr_t)ST_LSM6DSO_ID),
> > > > > > 	I3C_DEVICE(0x0104, 0x006B, (void *)(uintptr_t)ST_LSM6DSR_ID),
> > > > > > 
> > > > > > otherwise gcc might complain that the integer and pointer do not have
> > > > > > the same size (on 64-bit architectures).      
> > > > > 
> > > > > I don't understand this part. Can you provide or point some background?    
> > > > 
> > > > If you don't do that you'll get the following warning:
> > > > 
> > > > 	warning: cast to 'void *' from smaller integer type 'int' [-Wint-to-void-pointer-cast]    
> > > 
> > > I don't get the warning during compilation. Is there any flag to enable 
> > > or so?  
> > 
> > Nope, nothing specific to enable, just enable this driver on an arm64
> > config. Note that that gcc doesn't seem to complain about this
> > int -> void * cast (there's probably some kind of auto-promotion to
> > pointer size), but it does complains about the following void * -> int
> > cast:
> > 
> > drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c: In function ‘st_lsm6dsx_i3c_probe’:
> > drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c:43:43: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
> >    43 |  return st_lsm6dsx_probe(&i3cdev->dev, 0, (int)id->data, regmap);
> >       |       
> 
> I fixed the warning by changing:
> 
> st_lsm6dsx_probe(&i3cdev->dev, 0, (int)id->data, regmap);
> 
> to
> 
> st_lsm6dsx_probe(&i3cdev->dev, 0, (int)(uintptr_t)id->data, regmap);

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

> 
> But I wonder if it isn't more save to change the following too:
> 	I3C_DEVICE(0x0104, 0x006C, (void *)(uintptr_t)ST_LSM6DSO_ID),
> 	I3C_DEVICE(0x0104, 0x006B, (void *)(uintptr_t)ST_LSM6DSR_ID),
> 
> What do you think?

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

Patch

diff --git a/drivers/iio/imu/st_lsm6dsx/Kconfig b/drivers/iio/imu/st_lsm6dsx/Kconfig
index 9e59297..6b5a73c 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..2e89524
--- /dev/null
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c
@@ -0,0 +1,58 @@ 
+// 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"
+
+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 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);
+	struct regmap *regmap;
+
+	regmap = devm_regmap_init_i3c(i3cdev, &st_lsm6dsx_i3c_regmap_config);
+	if (IS_ERR(regmap)) {
+		dev_err(&i3cdev->dev, "Failed to register i3c regmap %d\n",
+			(int)PTR_ERR(regmap));
+		return PTR_ERR(regmap);
+	}
+
+	return st_lsm6dsx_probe(&i3cdev->dev, 0, (int)id->data, regmap);
+}
+
+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 <vitor.soares@synopsys.com>");
+MODULE_DESCRIPTION("STMicroelectronics st_lsm6dsx i3c driver");
+MODULE_LICENSE("GPL v2");