diff mbox series

iio/inv_icm42600: add inv_icm42600 id_table

Message ID 20240825063938.56319-1-jasonliu10041728@gmail.com (mailing list archive)
State Changes Requested
Headers show
Series iio/inv_icm42600: add inv_icm42600 id_table | expand

Commit Message

jason liu Aug. 25, 2024, 6:39 a.m. UTC
Add the id_table of inv_icm42600, so the device can probe correctly.

Signed-off-by: Jason Liu <jasonliu10041728@gmail.com>
---
 drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c | 15 +++++++++++++++
 drivers/iio/imu/inv_icm42600/inv_icm42600_spi.c | 15 +++++++++++++++
 2 files changed, 30 insertions(+)

Comments

Jean-Baptiste Maneyrol Aug. 26, 2024, 8:22 a.m. UTC | #1
Hello,

I was believing that id tables weren't required anymore when using of tables.

Jonathan,
can you help on this subject?

If we have to add id tables, then we need to add all supported chips (missing here icm42686 and icm42688).

Thanks,
JB
Jonathan Cameron Aug. 26, 2024, 10:20 a.m. UTC | #2
On Mon, 26 Aug 2024 08:22:11 +0000
Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com> wrote:

> Hello,
> 
> I was believing that id tables weren't required anymore when using of tables.
> 
> Jonathan,
> can you help on this subject?
> 
> If we have to add id tables, then we need to add all supported chips (missing here icm42686 and icm42688).

There were some oddities around autoloading for some busses a while
back but I can't find the reference.

+CC Mark + Wolfram for input.
Do we currently need i2c_device_id and spi_device_id tables for
autoprobing on DT only platforms?

A few minor comments inline.




> 
> Thanks,
> JB
> 
> ________________________________________
> From: Jason Liu <jasonliu10041728@gmail.com>
> Sent: Sunday, August 25, 2024 08:39
> To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>
> Cc: jic23@kernel.org <jic23@kernel.org>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; Jason Liu <jasonliu10041728@gmail.com>
> Subject: [PATCH] iio/inv_icm42600: add inv_icm42600 id_table
>  
> This Message Is From an Untrusted Sender
> You have not previously corresponded with this sender.
>  
> Add the id_table of inv_icm42600, so the device can probe correctly.
> 
> Signed-off-by: Jason Liu <jasonliu10041728@gmail.com>
> ---
>  drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c | 15 +++++++++++++++
>  drivers/iio/imu/inv_icm42600/inv_icm42600_spi.c | 15 +++++++++++++++
>  2 files changed, 30 insertions(+)
> 
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c
> index ebb31b385881..8cc550b8cfc3 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c
> @@ -71,6 +71,20 @@ static int inv_icm42600_probe(struct i2c_client *client)
>  				       inv_icm42600_i2c_bus_setup);
>  }
>  
> +/*
> + * device id table is used to identify what device can be
> + * supported by this driver
> + */
> +static const struct i2c_device_id inv_icm42600_id[] = {
> +	{"icm42600", INV_CHIP_ICM42600},
Spaces after { and before }
> +	{"icm42602", INV_CHIP_ICM42602},
> +	{"icm42605", INV_CHIP_ICM42605},
> +	{"icm42622", INV_CHIP_ICM42622},
> +	{"icm42631", INV_CHIP_ICM42631},
> +	{}
{ }

I'm trying to standardize this in IIO.

> +};
> +MODULE_DEVICE_TABLE(i2c, inv_icm42600_id);
> +
>  static const struct of_device_id inv_icm42600_of_matches[] = {
>  	{
>  		.compatible = "invensense,icm42600",
> @@ -104,6 +118,7 @@ static struct i2c_driver inv_icm42600_driver = {
>  		.of_match_table = inv_icm42600_of_matches,
>  		.pm = pm_ptr(&inv_icm42600_pm_ops),
>  	},
> +	.id_table = inv_icm42600_id,
>  	.probe = inv_icm42600_probe,
>  };
>  module_i2c_driver(inv_icm42600_driver);
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_spi.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_spi.c
> index eae5ff7a3cc1..5fe078ddc8a1 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_spi.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_spi.c
> @@ -67,6 +67,20 @@ static int inv_icm42600_probe(struct spi_device *spi)
>  				       inv_icm42600_spi_bus_setup);
>  }
>  
> +/*
> + * device id table is used to identify what device can be
> + * supported by this driver
> + */
> +static const struct spi_device_id inv_icm42600_id[] = {
> +	{"icm42600", INV_CHIP_ICM42600},
> +	{"icm42602", INV_CHIP_ICM42602},
> +	{"icm42605", INV_CHIP_ICM42605},
> +	{"icm42622", INV_CHIP_ICM42622},
> +	{"icm42631", INV_CHIP_ICM42631},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(spi, inv_icm42600_id);
> +
>  static const struct of_device_id inv_icm42600_of_matches[] = {
>  	{
>  		.compatible = "invensense,icm42600",
> @@ -100,6 +114,7 @@ static struct spi_driver inv_icm42600_driver = {
>  		.of_match_table = inv_icm42600_of_matches,
>  		.pm = pm_ptr(&inv_icm42600_pm_ops),
>  	},
> +	.id_table = inv_icm42600_id,
>  	.probe = inv_icm42600_probe,
>  };
>  module_spi_driver(inv_icm42600_driver);
Jonathan Cameron Sept. 1, 2024, 2:23 p.m. UTC | #3
On Sun, 1 Sep 2024 13:04:59 +0800
jason liu <jasonliu10041728@gmail.com> wrote:

> Hi, I would like to know your current opinion on this patch.
> 
> Let me explain the reason for proposing this patch.
> 
> First, through __spi_register_driver, we know that when registering an SPI
> driver, if an id_table is present, it matches the id_table; otherwise, it
> matches the SPI device's driver_name.
> Then, in inv_icm_42600spi.c, driver name is "inv-icm42600-spi", but the
> compatible is compatible = "invensense,icm42600".
> 
> so I think, it's necessary to add an id_table in the inv_icm42600 driver.

The quest Jean-Baptiste was raising was whether it would match on the
of_match_id table instead if it was present.  __spi_register_driver
does that so I think perhaps what you are actually referring to is
the comment in there.

	/*
	 * For Really Good Reasons we use spi: modaliases not of:
	 * modaliases for DT so module autoloading won't work if we
	 * don't have a spi_device_id as well as a compatible string.
	 */

The actually matching is done in spi_match_device.

Anyhow, the comment suggests strongly that we do still need the
spi_device_id table. I'm not 100% sure on the i2c equivalent, but
I'm fine with adding both.  Please fix up the formatting as requested
below and send a v2.

> 
> 
> Jonathan Cameron <jic23@kernel.org> 于2024年8月26日周一 18:27写道:
> 
> > On Mon, 26 Aug 2024 08:22:11 +0000
> > Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com> wrote:
> >  
> > > Hello,
> > >
> > > I was believing that id tables weren't required anymore when using of  
> > tables.  
> > >
> > > Jonathan,
> > > can you help on this subject?
> > >
> > > If we have to add id tables, then we need to add all supported chips  
> > (missing here icm42686 and icm42688).
> >
> > There were some oddities around autoloading for some busses a while
> > back but I can't find the reference.
> >
> > +CC Mark + Wolfram for input.
> > Do we currently need i2c_device_id and spi_device_id tables for
> > autoprobing on DT only platforms?
> >
> > A few minor comments inline.
> >
> >
> >
> >  
> > >
> > > Thanks,
> > > JB
> > >
> > > ________________________________________
> > > From: Jason Liu <jasonliu10041728@gmail.com>
> > > Sent: Sunday, August 25, 2024 08:39
> > > To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>
> > > Cc: jic23@kernel.org <jic23@kernel.org>; lars@metafoo.de <
> > lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>;  
> > linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; Jason Liu <  
> > jasonliu10041728@gmail.com>
> > > Subject: [PATCH] iio/inv_icm42600: add inv_icm42600 id_table
> > >
> > > This Message Is From an Untrusted Sender
> > > You have not previously corresponded with this sender.
> > >
> > > Add the id_table of inv_icm42600, so the device can probe correctly.
> > >
> > > Signed-off-by: Jason Liu <jasonliu10041728@gmail.com>
> > > ---
> > >  drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c | 15 +++++++++++++++
> > >  drivers/iio/imu/inv_icm42600/inv_icm42600_spi.c | 15 +++++++++++++++
> > >  2 files changed, 30 insertions(+)
> > >
> > > diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c  
> > b/drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c  
> > > index ebb31b385881..8cc550b8cfc3 100644
> > > --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c
> > > +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c
> > > @@ -71,6 +71,20 @@ static int inv_icm42600_probe(struct i2c_client  
> > *client)  
> > >                                      inv_icm42600_i2c_bus_setup);
> > >  }
> > >
> > > +/*
> > > + * device id table is used to identify what device can be
> > > + * supported by this driver
> > > + */
> > > +static const struct i2c_device_id inv_icm42600_id[] = {
> > > +     {"icm42600", INV_CHIP_ICM42600},  
> > Spaces after { and before }  
> > > +     {"icm42602", INV_CHIP_ICM42602},
> > > +     {"icm42605", INV_CHIP_ICM42605},
> > > +     {"icm42622", INV_CHIP_ICM42622},
> > > +     {"icm42631", INV_CHIP_ICM42631},
> > > +     {}  
> > { }
> >
> > I'm trying to standardize this in IIO.
> >  
> > > +};
> > > +MODULE_DEVICE_TABLE(i2c, inv_icm42600_id);
> > > +
> > >  static const struct of_device_id inv_icm42600_of_matches[] = {
> > >       {
> > >               .compatible = "invensense,icm42600",
> > > @@ -104,6 +118,7 @@ static struct i2c_driver inv_icm42600_driver = {
> > >               .of_match_table = inv_icm42600_of_matches,
> > >               .pm = pm_ptr(&inv_icm42600_pm_ops),
> > >       },
> > > +     .id_table = inv_icm42600_id,
> > >       .probe = inv_icm42600_probe,
> > >  };
> > >  module_i2c_driver(inv_icm42600_driver);
> > > diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_spi.c  
> > b/drivers/iio/imu/inv_icm42600/inv_icm42600_spi.c  
> > > index eae5ff7a3cc1..5fe078ddc8a1 100644
> > > --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_spi.c
> > > +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_spi.c
> > > @@ -67,6 +67,20 @@ static int inv_icm42600_probe(struct spi_device *spi)
> > >                                      inv_icm42600_spi_bus_setup);
> > >  }
> > >
> > > +/*
> > > + * device id table is used to identify what device can be
> > > + * supported by this driver
> > > + */
> > > +static const struct spi_device_id inv_icm42600_id[] = {
> > > +     {"icm42600", INV_CHIP_ICM42600},
> > > +     {"icm42602", INV_CHIP_ICM42602},
> > > +     {"icm42605", INV_CHIP_ICM42605},
> > > +     {"icm42622", INV_CHIP_ICM42622},
> > > +     {"icm42631", INV_CHIP_ICM42631},
> > > +     {}
> > > +};
> > > +MODULE_DEVICE_TABLE(spi, inv_icm42600_id);
> > > +
> > >  static const struct of_device_id inv_icm42600_of_matches[] = {
> > >       {
> > >               .compatible = "invensense,icm42600",
> > > @@ -100,6 +114,7 @@ static struct spi_driver inv_icm42600_driver = {
> > >               .of_match_table = inv_icm42600_of_matches,
> > >               .pm = pm_ptr(&inv_icm42600_pm_ops),
> > >       },
> > > +     .id_table = inv_icm42600_id,
> > >       .probe = inv_icm42600_probe,
> > >  };
> > >  module_spi_driver(inv_icm42600_driver);  
> >
> >
diff mbox series

Patch

diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c
index ebb31b385881..8cc550b8cfc3 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c
@@ -71,6 +71,20 @@  static int inv_icm42600_probe(struct i2c_client *client)
 				       inv_icm42600_i2c_bus_setup);
 }
 
+/*
+ * device id table is used to identify what device can be
+ * supported by this driver
+ */
+static const struct i2c_device_id inv_icm42600_id[] = {
+	{"icm42600", INV_CHIP_ICM42600},
+	{"icm42602", INV_CHIP_ICM42602},
+	{"icm42605", INV_CHIP_ICM42605},
+	{"icm42622", INV_CHIP_ICM42622},
+	{"icm42631", INV_CHIP_ICM42631},
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, inv_icm42600_id);
+
 static const struct of_device_id inv_icm42600_of_matches[] = {
 	{
 		.compatible = "invensense,icm42600",
@@ -104,6 +118,7 @@  static struct i2c_driver inv_icm42600_driver = {
 		.of_match_table = inv_icm42600_of_matches,
 		.pm = pm_ptr(&inv_icm42600_pm_ops),
 	},
+	.id_table = inv_icm42600_id,
 	.probe = inv_icm42600_probe,
 };
 module_i2c_driver(inv_icm42600_driver);
diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_spi.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_spi.c
index eae5ff7a3cc1..5fe078ddc8a1 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_spi.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_spi.c
@@ -67,6 +67,20 @@  static int inv_icm42600_probe(struct spi_device *spi)
 				       inv_icm42600_spi_bus_setup);
 }
 
+/*
+ * device id table is used to identify what device can be
+ * supported by this driver
+ */
+static const struct spi_device_id inv_icm42600_id[] = {
+	{"icm42600", INV_CHIP_ICM42600},
+	{"icm42602", INV_CHIP_ICM42602},
+	{"icm42605", INV_CHIP_ICM42605},
+	{"icm42622", INV_CHIP_ICM42622},
+	{"icm42631", INV_CHIP_ICM42631},
+	{}
+};
+MODULE_DEVICE_TABLE(spi, inv_icm42600_id);
+
 static const struct of_device_id inv_icm42600_of_matches[] = {
 	{
 		.compatible = "invensense,icm42600",
@@ -100,6 +114,7 @@  static struct spi_driver inv_icm42600_driver = {
 		.of_match_table = inv_icm42600_of_matches,
 		.pm = pm_ptr(&inv_icm42600_pm_ops),
 	},
+	.id_table = inv_icm42600_id,
 	.probe = inv_icm42600_probe,
 };
 module_spi_driver(inv_icm42600_driver);