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