linux-kernel - Re: [PATCH] iio/inv_icm42600: add inv_icm42600 id

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240901152346.45b096c3@jic23-huawei>
Date: Sun, 1 Sep 2024 15:23:46 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: jason liu <jasonliu10041728@...il.com>
Cc: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@....com>,
 "lars@...afoo.de" <lars@...afoo.de>, "linux-iio@...r.kernel.org"
 <linux-iio@...r.kernel.org>, "linux-kernel@...r.kernel.org"
 <linux-kernel@...r.kernel.org>, Wolfram Sang
 <wsa+renesas@...g-engineering.com>, Mark Brown <broonie@...nel.org>
Subject: Re: [PATCH] iio/inv_icm42600: add inv_icm42600 id_table

On Sun, 1 Sep 2024 13:04:59 +0800
jason liu <jasonliu10041728@...il.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@...nel.org> 于2024年8月26日周一 18:27写道：
> 
> > On Mon, 26 Aug 2024 08:22:11 +0000
> > Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@....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@...il.com>
> > > Sent: Sunday, August 25, 2024 08:39
> > > To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@....com>
> > > Cc: jic23@...nel.org <jic23@...nel.org>; lars@...afoo.de <
> > lars@...afoo.de>; linux-iio@...r.kernel.org <linux-iio@...r.kernel.org>;  
> > linux-kernel@...r.kernel.org <linux-kernel@...r.kernel.org>; Jason Liu <  
> > jasonliu10041728@...il.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@...il.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);  
> >
> >