From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: Jonathan Cameron <jic23@kernel.org>
Cc: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>,
<linux-iio@vger.kernel.org>, <devicetree@vger.kernel.org>,
Rob Herring <robh+dt@kernel.org>,
Mark Rutland <mark.rutland@arm.com>
Subject: Re: [PATCH v2 1/6] iio: imu: inv_mpu6050: cleanup of/acpi support
Date: Mon, 8 Nov 2021 17:52:43 +0000 [thread overview]
Message-ID: <20211108175243.00005655@Huawei.com> (raw)
In-Reply-To: <20200206115155.42dca690@archlinux>
On Thu, 6 Feb 2020 11:51:55 +0000
Jonathan Cameron <jic23@kernel.org> wrote:
> On Thu, 6 Feb 2020 11:31:00 +0100
> Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> wrote:
>
> > Use device_get_match_data for using of and acpi tables.
> > In spi support add missing mpu6515 and of match table.
> > Reorganize Kconfig to display chips grouped by generations.
> >
> > Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
> Great.
>
> Applied to the togreg branch of iio.git. Clearly I took this
> one very quickly after posting, but note it's not in a non rebasing
> tree so I can still change that if anyone else has comments!
>
I happened to be looking at the code this affects for an entirely
different reason and realized there is a problem...
> Thanks
>
> Jonathan
>
> > ---
> > drivers/iio/imu/inv_mpu6050/Kconfig | 8 ++---
> > drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c | 26 +++-----------
> > drivers/iio/imu/inv_mpu6050/inv_mpu_spi.c | 44 +++++++++++++++++++++--
> > 3 files changed, 50 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/iio/imu/inv_mpu6050/Kconfig b/drivers/iio/imu/inv_mpu6050/Kconfig
> > index 017bc0fcc365..003134734646 100644
> > --- a/drivers/iio/imu/inv_mpu6050/Kconfig
> > +++ b/drivers/iio/imu/inv_mpu6050/Kconfig
> > @@ -15,8 +15,8 @@ config INV_MPU6050_I2C
> > select INV_MPU6050_IIO
> > select REGMAP_I2C
> > help
> > - This driver supports the Invensense MPU6050/6500/6515,
> > - MPU9150/9250/9255 and ICM20608/20602 motion tracking devices
> > + This driver supports the Invensense MPU6050/9150,
> > + MPU6500/6515/9250/9255, ICM20608 and ICM20602 motion tracking devices
> > over I2C.
> > This driver can be built as a module. The module will be called
> > inv-mpu6050-i2c.
> > @@ -27,8 +27,8 @@ config INV_MPU6050_SPI
> > select INV_MPU6050_IIO
> > select REGMAP_SPI
> > help
> > - This driver supports the Invensense MPU6000/6500/6515,
> > - MPU9250/9255 and ICM20608/20602 motion tracking devices
> > + This driver supports the Invensense MPU6000,
> > + MPU6500/6515/9250/9255, ICM20608 and ICM20602 motion tracking devices
> > over SPI.
> > This driver can be built as a module. The module will be called
> > inv-mpu6050-spi.
> > diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
> > index f47a28b4be23..49e9aa777ba4 100644
> > --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
> > +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
> > @@ -10,6 +10,7 @@
> > #include <linux/iio/iio.h>
> > #include <linux/module.h>
> > #include <linux/of_device.h>
> > +#include <linux/property.h>
> > #include "inv_mpu_iio.h"
> >
> > static const struct regmap_config inv_mpu_regmap_config = {
> > @@ -54,20 +55,6 @@ static int inv_mpu6050_deselect_bypass(struct i2c_mux_core *muxc, u32 chan_id)
> > return 0;
> > }
> >
> > -static const char *inv_mpu_match_acpi_device(struct device *dev,
> > - enum inv_devices *chip_id)
> > -{
> > - const struct acpi_device_id *id;
> > -
> > - id = acpi_match_device(dev->driver->acpi_match_table, dev);
> > - if (!id)
> > - return NULL;
> > -
> > - *chip_id = (int)id->driver_data;
> > -
> > - return dev_name(dev);
> > -}
> > -
> > static bool inv_mpu_i2c_aux_bus(struct device *dev)
> > {
> > struct inv_mpu6050_state *st = iio_priv(dev_get_drvdata(dev));
> > @@ -130,6 +117,7 @@ static int inv_mpu_magn_disable(struct iio_dev *indio_dev)
> > static int inv_mpu_probe(struct i2c_client *client,
> > const struct i2c_device_id *id)
> > {
> > + const void *match;
> > struct inv_mpu6050_state *st;
> > int result;
> > enum inv_devices chip_type;
> > @@ -140,18 +128,14 @@ static int inv_mpu_probe(struct i2c_client *client,
> > I2C_FUNC_SMBUS_I2C_BLOCK))
> > return -EOPNOTSUPP;
> >
> > - if (client->dev.of_node) {
> > - chip_type = (enum inv_devices)
> > - of_device_get_match_data(&client->dev);
> > + match = device_get_match_data(&client->dev);
> > + if (match) {
Unfortunately, chip_type == 0 and hence match == NULL doesn't
necessarily mean no match... It might mean the enum value was
0 which is the case for INV_MPU6050
For the of case, we probably get away with it because
id->driver_data will be set anyway but I don't think that was what
was intended.
Fixes are all a bit mess. Simplest might be to add a check of
dev_fwnode() to verify if there is a firmware node or not and
then use the match value form device_get_match_data without checking
it against 0.
Jonathan
> > + chip_type = (enum inv_devices)match;
> > name = client->name;
> > } else if (id) {
> > chip_type = (enum inv_devices)
> > id->driver_data;
> > name = id->name;
> > - } else if (ACPI_HANDLE(&client->dev)) {
> > - name = inv_mpu_match_acpi_device(&client->dev, &chip_type);
> > - if (!name)
> > - return -ENODEV;
> > } else {
> > return -ENOSYS;
> > }
> > diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_spi.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_spi.c
> > index ec102d5a5c77..834bfa8e8684 100644
> > --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_spi.c
> > +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_spi.c
> > @@ -4,6 +4,8 @@
> > */
> > #include <linux/module.h>
> > #include <linux/acpi.h>
> > +#include <linux/of.h>
> > +#include <linux/property.h>
> > #include <linux/spi/spi.h>
> > #include <linux/regmap.h>
> > #include <linux/iio/iio.h>
> > @@ -41,17 +43,18 @@ static int inv_mpu_i2c_disable(struct iio_dev *indio_dev)
> >
> > static int inv_mpu_probe(struct spi_device *spi)
> > {
> > + const void *match;
> > struct regmap *regmap;
> > const struct spi_device_id *spi_id;
> > - const struct acpi_device_id *acpi_id;
> > const char *name = NULL;
> > enum inv_devices chip_type;
> >
> > if ((spi_id = spi_get_device_id(spi))) {
> > chip_type = (enum inv_devices)spi_id->driver_data;
> > name = spi_id->name;
> > - } else if ((acpi_id = acpi_match_device(spi->dev.driver->acpi_match_table, &spi->dev))) {
> > - chip_type = (enum inv_devices)acpi_id->driver_data;
> > + } else if ((match = device_get_match_data(&spi->dev))) {
> > + chip_type = (enum inv_devices)match;
> > + name = dev_name(&spi->dev);
> > } else {
> > return -ENODEV;
> > }
> > @@ -74,6 +77,7 @@ static int inv_mpu_probe(struct spi_device *spi)
> > static const struct spi_device_id inv_mpu_id[] = {
> > {"mpu6000", INV_MPU6000},
> > {"mpu6500", INV_MPU6500},
> > + {"mpu6515", INV_MPU6515},
> > {"mpu9250", INV_MPU9250},
> > {"mpu9255", INV_MPU9255},
> > {"icm20608", INV_ICM20608},
> > @@ -83,6 +87,39 @@ static const struct spi_device_id inv_mpu_id[] = {
> >
> > MODULE_DEVICE_TABLE(spi, inv_mpu_id);
> >
> > +static const struct of_device_id inv_of_match[] = {
> > + {
> > + .compatible = "invensense,mpu6000",
> > + .data = (void *)INV_MPU6000
> > + },
> > + {
> > + .compatible = "invensense,mpu6500",
> > + .data = (void *)INV_MPU6500
> > + },
> > + {
> > + .compatible = "invensense,mpu6515",
> > + .data = (void *)INV_MPU6515
> > + },
> > + {
> > + .compatible = "invensense,mpu9250",
> > + .data = (void *)INV_MPU9250
> > + },
> > + {
> > + .compatible = "invensense,mpu9255",
> > + .data = (void *)INV_MPU9255
> > + },
> > + {
> > + .compatible = "invensense,icm20608",
> > + .data = (void *)INV_ICM20608
> > + },
> > + {
> > + .compatible = "invensense,icm20602",
> > + .data = (void *)INV_ICM20602
> > + },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, inv_of_match);
> > +
> > static const struct acpi_device_id inv_acpi_match[] = {
> > {"INVN6000", INV_MPU6000},
> > { },
> > @@ -93,6 +130,7 @@ static struct spi_driver inv_mpu_driver = {
> > .probe = inv_mpu_probe,
> > .id_table = inv_mpu_id,
> > .driver = {
> > + .of_match_table = inv_of_match,
> > .acpi_match_table = ACPI_PTR(inv_acpi_match),
> > .name = "inv-mpu6000-spi",
> > .pm = &inv_mpu_pmops,
>
>
next prev parent reply other threads:[~2021-11-08 17:52 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-06 10:30 [PATCH v2 0/6] Add support of similar chips Jean-Baptiste Maneyrol
2020-02-06 10:31 ` [PATCH v2 1/6] iio: imu: inv_mpu6050: cleanup of/acpi support Jean-Baptiste Maneyrol
2020-02-06 11:51 ` Jonathan Cameron
2021-11-08 17:52 ` Jonathan Cameron [this message]
2020-02-06 10:31 ` [PATCH v2 2/6] iio: imu: inv_mpu6050: add support of ICM20609 & ICM20689 Jean-Baptiste Maneyrol
2020-02-06 11:53 ` Jonathan Cameron
2020-02-06 10:31 ` [PATCH v2 3/6] iio: imu: inv_mpu6050: add support of IAM20680 Jean-Baptiste Maneyrol
2020-02-06 12:09 ` Jonathan Cameron
2020-02-06 10:31 ` [PATCH v2 4/6] iio: imu: inv_mpu6050: add support of ICM20690 Jean-Baptiste Maneyrol
2020-02-06 12:15 ` Jonathan Cameron
2020-02-06 10:31 ` [PATCH v2 5/6] iio: imu: inv_mpu6050: update LPF bandwidth settings Jean-Baptiste Maneyrol
2020-02-06 12:16 ` Jonathan Cameron
2020-02-06 10:31 ` [PATCH v2 6/6] dt-bindings: add description for new supported chips Jean-Baptiste Maneyrol
2020-02-06 12:16 ` Jonathan Cameron
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20211108175243.00005655@Huawei.com \
--to=jonathan.cameron@huawei.com \
--cc=devicetree@vger.kernel.org \
--cc=jic23@kernel.org \
--cc=jmaneyrol@invensense.com \
--cc=linux-iio@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).