| Message ID | 20220510141753.3878390-4-Qing-wu.Li@leica-geosystems.com.cn (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: accel: bmi088: support BMI085 BMI090L | expand |
On Tue, May 10, 2022 at 5:18 PM LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn> wrote: > > It is possible to have multiple sensors connected on the same platform, > To support multiple sensors, the commit makes it possible to obtain the > device name by reading the chip ID instead of the device-tree name. > To be compatible with previous versions, renam bmi088a to bmi088-accel. // my spellcheck in GMail found this :p typo: renam -> rename I also have a comment about a duplication that is highlighted by this change. You can disregard my comment about the duplication and leave this change as-is. > > Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn> > --- > drivers/iio/accel/bmi088-accel-core.c | 6 +++--- > drivers/iio/accel/bmi088-accel-spi.c | 4 +--- > drivers/iio/accel/bmi088-accel.h | 2 +- > 3 files changed, 5 insertions(+), 7 deletions(-) > > diff --git a/drivers/iio/accel/bmi088-accel-core.c b/drivers/iio/accel/bmi088-accel-core.c > index 8fee1d02e773..de2385e4dad5 100644 > --- a/drivers/iio/accel/bmi088-accel-core.c > +++ b/drivers/iio/accel/bmi088-accel-core.c > @@ -459,7 +459,7 @@ static const struct iio_chan_spec bmi088_accel_channels[] = { > > static const struct bmi088_accel_chip_info bmi088_accel_chip_info_tbl[] = { > [0] = { > - .name = "bmi088a", > + .name = "bmi088-accel", > .chip_id = 0x1E, > .channels = bmi088_accel_channels, > .num_channels = ARRAY_SIZE(bmi088_accel_channels), > @@ -524,7 +524,7 @@ static int bmi088_accel_chip_init(struct bmi088_accel_data *data) > } > > int bmi088_accel_core_probe(struct device *dev, struct regmap *regmap, > - int irq, const char *name, bool block_supported) > + int irq, bool block_supported) > { > struct bmi088_accel_data *data; > struct iio_dev *indio_dev; > @@ -545,7 +545,7 @@ int bmi088_accel_core_probe(struct device *dev, struct regmap *regmap, > > indio_dev->channels = data->chip_info->channels; > indio_dev->num_channels = data->chip_info->num_channels; > - indio_dev->name = name ? name : data->chip_info->name; > + indio_dev->name = data->chip_info->name; (with this change) i can better see, a bit of duplication between the spi_device table and the chip_info table this was not introduced by this change, but it was made a bit more obvious by this change; one way to address this, is to remove the `const char *name;` and continue using the `name` provided as a parameter from bmi088_accel_core_probe(); (apologies if I seem to have changed my mind (from the previous changeset), but I did not see it too well before) and we can convert enum { ID_BMI088, ID_BMI085, ... }; static const struct bmi088_accel_chip_info bmi088_accel_chip_info_tbl[] = { [ID_BMI088] = { .chip_id = 0x1E, .channels = bmi088_accel_channels, .num_channels = ARRAY_SIZE(bmi088_accel_channels), }, [ID_BMI085] = { ........ > indio_dev->available_scan_masks = bmi088_accel_scan_masks; > indio_dev->modes = INDIO_DIRECT_MODE; > indio_dev->info = &bmi088_accel_info; > diff --git a/drivers/iio/accel/bmi088-accel-spi.c b/drivers/iio/accel/bmi088-accel-spi.c > index dd1e3f6cf211..0fed0081e1fd 100644 > --- a/drivers/iio/accel/bmi088-accel-spi.c > +++ b/drivers/iio/accel/bmi088-accel-spi.c > @@ -42,7 +42,6 @@ static struct regmap_bus bmi088_regmap_bus = { > static int bmi088_accel_probe(struct spi_device *spi) > { > struct regmap *regmap; > - const struct spi_device_id *id = spi_get_device_id(spi); > > regmap = devm_regmap_init(&spi->dev, &bmi088_regmap_bus, > spi, &bmi088_regmap_conf); > @@ -52,8 +51,7 @@ static int bmi088_accel_probe(struct spi_device *spi) > return PTR_ERR(regmap); > } > > - return bmi088_accel_core_probe(&spi->dev, regmap, spi->irq, id->name, > - true); > + return bmi088_accel_core_probe(&spi->dev, regmap, spi->irq, true); > } > > static int bmi088_accel_remove(struct spi_device *spi) > diff --git a/drivers/iio/accel/bmi088-accel.h b/drivers/iio/accel/bmi088-accel.h > index 5c25f16b672c..c32afe9606a8 100644 > --- a/drivers/iio/accel/bmi088-accel.h > +++ b/drivers/iio/accel/bmi088-accel.h > @@ -12,7 +12,7 @@ extern const struct regmap_config bmi088_regmap_conf; > extern const struct dev_pm_ops bmi088_accel_pm_ops; > > int bmi088_accel_core_probe(struct device *dev, struct regmap *regmap, int irq, > - const char *name, bool block_supported); > + bool block_supported); > int bmi088_accel_core_remove(struct device *dev); > > #endif /* BMI088_ACCEL_H */ > -- > 2.25.1 >
> -----Original Message----- > From: Alexandru Ardelean <ardeleanalex@gmail.com> > Sent: Thursday, May 12, 2022 3:32 PM > To: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn> > Cc: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen > <lars@metafoo.de>; mchehab+huawei@kernel.org; linux-iio > <linux-iio@vger.kernel.org>; LKML <linux-kernel@vger.kernel.org>; Rob Herring > <robh+dt@kernel.org>; Mike Looijmans <mike.looijmans@topic.nl>; devicetree > <devicetree@vger.kernel.org> > Subject: Re: [PATCH V2 3/6] iio: accel: bmi088: modified the device name > > This email is not from Hexagon’s Office 365 instance. Please be careful while > clicking links, opening attachments, or replying to this email. > > > On Tue, May 10, 2022 at 5:18 PM LI Qingwu > <Qing-wu.Li@leica-geosystems.com.cn> wrote: > > > > It is possible to have multiple sensors connected on the same > > platform, To support multiple sensors, the commit makes it possible to > > obtain the device name by reading the chip ID instead of the device-tree > name. > > To be compatible with previous versions, renam bmi088a to bmi088-accel. > > // my spellcheck in GMail found this :p > > typo: renam -> rename > > I also have a comment about a duplication that is highlighted by this change. > > You can disregard my comment about the duplication and leave this change > as-is. > > > > > Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn> > > --- > > drivers/iio/accel/bmi088-accel-core.c | 6 +++--- > > drivers/iio/accel/bmi088-accel-spi.c | 4 +--- > > drivers/iio/accel/bmi088-accel.h | 2 +- > > 3 files changed, 5 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/iio/accel/bmi088-accel-core.c > > b/drivers/iio/accel/bmi088-accel-core.c > > index 8fee1d02e773..de2385e4dad5 100644 > > --- a/drivers/iio/accel/bmi088-accel-core.c > > +++ b/drivers/iio/accel/bmi088-accel-core.c > > @@ -459,7 +459,7 @@ static const struct iio_chan_spec > > bmi088_accel_channels[] = { > > > > static const struct bmi088_accel_chip_info bmi088_accel_chip_info_tbl[] = { > > [0] = { > > - .name = "bmi088a", > > + .name = "bmi088-accel", > > .chip_id = 0x1E, > > .channels = bmi088_accel_channels, > > .num_channels = ARRAY_SIZE(bmi088_accel_channels), > > @@ -524,7 +524,7 @@ static int bmi088_accel_chip_init(struct > > bmi088_accel_data *data) } > > > > int bmi088_accel_core_probe(struct device *dev, struct regmap *regmap, > > - int irq, const char *name, bool block_supported) > > + int irq, bool block_supported) > > { > > struct bmi088_accel_data *data; > > struct iio_dev *indio_dev; > > @@ -545,7 +545,7 @@ int bmi088_accel_core_probe(struct device *dev, > > struct regmap *regmap, > > > > indio_dev->channels = data->chip_info->channels; > > indio_dev->num_channels = data->chip_info->num_channels; > > - indio_dev->name = name ? name : data->chip_info->name; > > + indio_dev->name = data->chip_info->name; > > (with this change) i can better see, a bit of duplication between the spi_device > table and the chip_info table > > this was not introduced by this change, but it was made a bit more obvious by > this change; > > one way to address this, is to remove the `const char *name;` and continue > using the `name` provided as a parameter from bmi088_accel_core_probe(); > (apologies if I seem to have changed my mind (from the previous changeset), but > I did not see it too well before) > > and we can convert > > enum { > ID_BMI088, > ID_BMI085, > ... > }; > > static const struct bmi088_accel_chip_info bmi088_accel_chip_info_tbl[] = { > [ID_BMI088] = { > .chip_id = 0x1E, > .channels = bmi088_accel_channels, > .num_channels = ARRAY_SIZE(bmi088_accel_channels), > }, > [ID_BMI085] = { > ........ > Thanks Ardelean, There is a case where some different sensors are connected to one system, For user space is nice if can detect which sensor is present, if using the name in spi_device table, the name may be inconsistent with the connected sensor. What's your opinion? > > indio_dev->available_scan_masks = bmi088_accel_scan_masks; > > indio_dev->modes = INDIO_DIRECT_MODE; > > indio_dev->info = &bmi088_accel_info; diff --git > > a/drivers/iio/accel/bmi088-accel-spi.c > > b/drivers/iio/accel/bmi088-accel-spi.c > > index dd1e3f6cf211..0fed0081e1fd 100644 > > --- a/drivers/iio/accel/bmi088-accel-spi.c > > +++ b/drivers/iio/accel/bmi088-accel-spi.c > > @@ -42,7 +42,6 @@ static struct regmap_bus bmi088_regmap_bus = { > > static int bmi088_accel_probe(struct spi_device *spi) { > > struct regmap *regmap; > > - const struct spi_device_id *id = spi_get_device_id(spi); > > > > regmap = devm_regmap_init(&spi->dev, &bmi088_regmap_bus, > > spi, &bmi088_regmap_conf); @@ -52,8 +51,7 > @@ > > static int bmi088_accel_probe(struct spi_device *spi) > > return PTR_ERR(regmap); > > } > > > > - return bmi088_accel_core_probe(&spi->dev, regmap, spi->irq, > id->name, > > - true); > > + return bmi088_accel_core_probe(&spi->dev, regmap, spi->irq, > > + true); > > } > > > > static int bmi088_accel_remove(struct spi_device *spi) diff --git > > a/drivers/iio/accel/bmi088-accel.h b/drivers/iio/accel/bmi088-accel.h > > index 5c25f16b672c..c32afe9606a8 100644 > > --- a/drivers/iio/accel/bmi088-accel.h > > +++ b/drivers/iio/accel/bmi088-accel.h > > @@ -12,7 +12,7 @@ extern const struct regmap_config > > bmi088_regmap_conf; extern const struct dev_pm_ops > > bmi088_accel_pm_ops; > > > > int bmi088_accel_core_probe(struct device *dev, struct regmap *regmap, > int irq, > > - const char *name, bool block_supported); > > + bool block_supported); > > int bmi088_accel_core_remove(struct device *dev); > > > > #endif /* BMI088_ACCEL_H */ > > -- > > 2.25.1 > >
On Thu, May 12, 2022 at 7:18 PM LI Qingwu <qing-wu.li@leica-geosystems.com.cn> wrote: > > > > -----Original Message----- > > From: Alexandru Ardelean <ardeleanalex@gmail.com> > > Sent: Thursday, May 12, 2022 3:32 PM > > To: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn> > > Cc: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen > > <lars@metafoo.de>; mchehab+huawei@kernel.org; linux-iio > > <linux-iio@vger.kernel.org>; LKML <linux-kernel@vger.kernel.org>; Rob Herring > > <robh+dt@kernel.org>; Mike Looijmans <mike.looijmans@topic.nl>; devicetree > > <devicetree@vger.kernel.org> > > Subject: Re: [PATCH V2 3/6] iio: accel: bmi088: modified the device name > > > > This email is not from Hexagon’s Office 365 instance. Please be careful while > > clicking links, opening attachments, or replying to this email. > > > > > > On Tue, May 10, 2022 at 5:18 PM LI Qingwu > > <Qing-wu.Li@leica-geosystems.com.cn> wrote: > > > > > > It is possible to have multiple sensors connected on the same > > > platform, To support multiple sensors, the commit makes it possible to > > > obtain the device name by reading the chip ID instead of the device-tree > > name. > > > To be compatible with previous versions, renam bmi088a to bmi088-accel. > > > > // my spellcheck in GMail found this :p > > > > typo: renam -> rename > > > > I also have a comment about a duplication that is highlighted by this change. > > > > You can disregard my comment about the duplication and leave this change > > as-is. > > > > > > > > Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn> > > > --- > > > drivers/iio/accel/bmi088-accel-core.c | 6 +++--- > > > drivers/iio/accel/bmi088-accel-spi.c | 4 +--- > > > drivers/iio/accel/bmi088-accel.h | 2 +- > > > 3 files changed, 5 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/iio/accel/bmi088-accel-core.c > > > b/drivers/iio/accel/bmi088-accel-core.c > > > index 8fee1d02e773..de2385e4dad5 100644 > > > --- a/drivers/iio/accel/bmi088-accel-core.c > > > +++ b/drivers/iio/accel/bmi088-accel-core.c > > > @@ -459,7 +459,7 @@ static const struct iio_chan_spec > > > bmi088_accel_channels[] = { > > > > > > static const struct bmi088_accel_chip_info bmi088_accel_chip_info_tbl[] = { > > > [0] = { > > > - .name = "bmi088a", > > > + .name = "bmi088-accel", > > > .chip_id = 0x1E, > > > .channels = bmi088_accel_channels, > > > .num_channels = ARRAY_SIZE(bmi088_accel_channels), > > > @@ -524,7 +524,7 @@ static int bmi088_accel_chip_init(struct > > > bmi088_accel_data *data) } > > > > > > int bmi088_accel_core_probe(struct device *dev, struct regmap *regmap, > > > - int irq, const char *name, bool block_supported) > > > + int irq, bool block_supported) > > > { > > > struct bmi088_accel_data *data; > > > struct iio_dev *indio_dev; > > > @@ -545,7 +545,7 @@ int bmi088_accel_core_probe(struct device *dev, > > > struct regmap *regmap, > > > > > > indio_dev->channels = data->chip_info->channels; > > > indio_dev->num_channels = data->chip_info->num_channels; > > > - indio_dev->name = name ? name : data->chip_info->name; > > > + indio_dev->name = data->chip_info->name; > > > > (with this change) i can better see, a bit of duplication between the spi_device > > table and the chip_info table > > > > this was not introduced by this change, but it was made a bit more obvious by > > this change; > > > > one way to address this, is to remove the `const char *name;` and continue > > using the `name` provided as a parameter from bmi088_accel_core_probe(); > > (apologies if I seem to have changed my mind (from the previous changeset), but > > I did not see it too well before) > > > > and we can convert > > > > enum { > > ID_BMI088, > > ID_BMI085, > > ... > > }; > > > > static const struct bmi088_accel_chip_info bmi088_accel_chip_info_tbl[] = { > > [ID_BMI088] = { > > .chip_id = 0x1E, > > .channels = bmi088_accel_channels, > > .num_channels = ARRAY_SIZE(bmi088_accel_channels), > > }, > > [ID_BMI085] = { > > ........ > > > Thanks Ardelean, > > There is a case where some different sensors are connected to one system, > For user space is nice if can detect which sensor is present, if using the name in spi_device table, > the name may be inconsistent with the connected sensor. What's your opinion? I'd let Jonathan have the last word on this. Typically, what I've seen (in practice), that when you write a device-tree for a board and you add (for example) BMI088 for board RevA, but on RevB it is decided to go with BMI085, then some people want to enforce this from the device-tree. So, if in the DT someone specifies bmi088-accel (as compatible string), then it could fail on probe if it detects bmi085-accel. Right now, this isn't happening with the driver (as it is now). On the other hand, some other people want this flexibility where if you specify bmi088-accel, bmi085-accel or bmi090-accel, then they like the interchange-ability at the device-tree level. I don't know which is better. I'm not sure this was enforced (or maybe I missed it). But a more common practice (that I remember in some IIO drivers) is that the chip_info table (or a chip_id value) can be added to spi_device_table. When the DT tries to probe the driver and reads a different ID (from SPI) it would fail the probe. These are just some thoughts. Not a final conclusion. > > > > > indio_dev->available_scan_masks = bmi088_accel_scan_masks; > > > indio_dev->modes = INDIO_DIRECT_MODE; > > > indio_dev->info = &bmi088_accel_info; diff --git > > > a/drivers/iio/accel/bmi088-accel-spi.c > > > b/drivers/iio/accel/bmi088-accel-spi.c > > > index dd1e3f6cf211..0fed0081e1fd 100644 > > > --- a/drivers/iio/accel/bmi088-accel-spi.c > > > +++ b/drivers/iio/accel/bmi088-accel-spi.c > > > @@ -42,7 +42,6 @@ static struct regmap_bus bmi088_regmap_bus = { > > > static int bmi088_accel_probe(struct spi_device *spi) { > > > struct regmap *regmap; > > > - const struct spi_device_id *id = spi_get_device_id(spi); > > > > > > regmap = devm_regmap_init(&spi->dev, &bmi088_regmap_bus, > > > spi, &bmi088_regmap_conf); @@ -52,8 +51,7 > > @@ > > > static int bmi088_accel_probe(struct spi_device *spi) > > > return PTR_ERR(regmap); > > > } > > > > > > - return bmi088_accel_core_probe(&spi->dev, regmap, spi->irq, > > id->name, > > > - true); > > > + return bmi088_accel_core_probe(&spi->dev, regmap, spi->irq, > > > + true); > > > } > > > > > > static int bmi088_accel_remove(struct spi_device *spi) diff --git > > > a/drivers/iio/accel/bmi088-accel.h b/drivers/iio/accel/bmi088-accel.h > > > index 5c25f16b672c..c32afe9606a8 100644 > > > --- a/drivers/iio/accel/bmi088-accel.h > > > +++ b/drivers/iio/accel/bmi088-accel.h > > > @@ -12,7 +12,7 @@ extern const struct regmap_config > > > bmi088_regmap_conf; extern const struct dev_pm_ops > > > bmi088_accel_pm_ops; > > > > > > int bmi088_accel_core_probe(struct device *dev, struct regmap *regmap, > > int irq, > > > - const char *name, bool block_supported); > > > + bool block_supported); > > > int bmi088_accel_core_remove(struct device *dev); > > > > > > #endif /* BMI088_ACCEL_H */ > > > -- > > > 2.25.1 > > >
On Fri, 13 May 2022 09:20:48 +0300 Alexandru Ardelean <ardeleanalex@gmail.com> wrote: > On Thu, May 12, 2022 at 7:18 PM LI Qingwu > <qing-wu.li@leica-geosystems.com.cn> wrote: > > > > > > > -----Original Message----- > > > From: Alexandru Ardelean <ardeleanalex@gmail.com> > > > Sent: Thursday, May 12, 2022 3:32 PM > > > To: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn> > > > Cc: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen > > > <lars@metafoo.de>; mchehab+huawei@kernel.org; linux-iio > > > <linux-iio@vger.kernel.org>; LKML <linux-kernel@vger.kernel.org>; Rob Herring > > > <robh+dt@kernel.org>; Mike Looijmans <mike.looijmans@topic.nl>; devicetree > > > <devicetree@vger.kernel.org> > > > Subject: Re: [PATCH V2 3/6] iio: accel: bmi088: modified the device name > > > > > > This email is not from Hexagon’s Office 365 instance. Please be careful while > > > clicking links, opening attachments, or replying to this email. > > > > > > > > > On Tue, May 10, 2022 at 5:18 PM LI Qingwu > > > <Qing-wu.Li@leica-geosystems.com.cn> wrote: > > > > > > > > It is possible to have multiple sensors connected on the same > > > > platform, To support multiple sensors, the commit makes it possible to > > > > obtain the device name by reading the chip ID instead of the device-tree > > > name. > > > > To be compatible with previous versions, renam bmi088a to bmi088-accel. > > > > > > // my spellcheck in GMail found this :p > > > > > > typo: renam -> rename > > > > > > I also have a comment about a duplication that is highlighted by this change. > > > > > > You can disregard my comment about the duplication and leave this change > > > as-is. > > > > > > > > > > > Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn> > > > > --- > > > > drivers/iio/accel/bmi088-accel-core.c | 6 +++--- > > > > drivers/iio/accel/bmi088-accel-spi.c | 4 +--- > > > > drivers/iio/accel/bmi088-accel.h | 2 +- > > > > 3 files changed, 5 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/drivers/iio/accel/bmi088-accel-core.c > > > > b/drivers/iio/accel/bmi088-accel-core.c > > > > index 8fee1d02e773..de2385e4dad5 100644 > > > > --- a/drivers/iio/accel/bmi088-accel-core.c > > > > +++ b/drivers/iio/accel/bmi088-accel-core.c > > > > @@ -459,7 +459,7 @@ static const struct iio_chan_spec > > > > bmi088_accel_channels[] = { > > > > > > > > static const struct bmi088_accel_chip_info bmi088_accel_chip_info_tbl[] = { > > > > [0] = { > > > > - .name = "bmi088a", > > > > + .name = "bmi088-accel", > > > > .chip_id = 0x1E, > > > > .channels = bmi088_accel_channels, > > > > .num_channels = ARRAY_SIZE(bmi088_accel_channels), > > > > @@ -524,7 +524,7 @@ static int bmi088_accel_chip_init(struct > > > > bmi088_accel_data *data) } > > > > > > > > int bmi088_accel_core_probe(struct device *dev, struct regmap *regmap, > > > > - int irq, const char *name, bool block_supported) > > > > + int irq, bool block_supported) > > > > { > > > > struct bmi088_accel_data *data; > > > > struct iio_dev *indio_dev; > > > > @@ -545,7 +545,7 @@ int bmi088_accel_core_probe(struct device *dev, > > > > struct regmap *regmap, > > > > > > > > indio_dev->channels = data->chip_info->channels; > > > > indio_dev->num_channels = data->chip_info->num_channels; > > > > - indio_dev->name = name ? name : data->chip_info->name; > > > > + indio_dev->name = data->chip_info->name; > > > > > > (with this change) i can better see, a bit of duplication between the spi_device > > > table and the chip_info table > > > > > > this was not introduced by this change, but it was made a bit more obvious by > > > this change; > > > > > > one way to address this, is to remove the `const char *name;` and continue > > > using the `name` provided as a parameter from bmi088_accel_core_probe(); > > > (apologies if I seem to have changed my mind (from the previous changeset), but > > > I did not see it too well before) > > > > > > and we can convert > > > > > > enum { > > > ID_BMI088, > > > ID_BMI085, > > > ... > > > }; > > > > > > static const struct bmi088_accel_chip_info bmi088_accel_chip_info_tbl[] = { > > > [ID_BMI088] = { > > > .chip_id = 0x1E, > > > .channels = bmi088_accel_channels, > > > .num_channels = ARRAY_SIZE(bmi088_accel_channels), > > > }, > > > [ID_BMI085] = { > > > ........ > > > > > Thanks Ardelean, > > > > There is a case where some different sensors are connected to one system, > > For user space is nice if can detect which sensor is present, if using the name in spi_device table, > > the name may be inconsistent with the connected sensor. What's your opinion? > > I'd let Jonathan have the last word on this. > > Typically, what I've seen (in practice), that when you write a > device-tree for a board and you add (for example) BMI088 for board > RevA, but on RevB it is decided to go with BMI085, then some people > want to enforce this from the device-tree. In my view the device tree should be using fallbacks for this, so actually always list the real device first then have other compatible entries. Thus if it turns out there is something unique that means we can't just the whoami value, we have a more precise description (this has happened with IIRC the mpu6050 supported parts). > > So, if in the DT someone specifies bmi088-accel (as compatible > string), then it could fail on probe if it detects bmi085-accel. > Right now, this isn't happening with the driver (as it is now). > > On the other hand, some other people want this flexibility where if > you specify bmi088-accel, bmi085-accel or bmi090-accel, then they like > the interchange-ability at the device-tree level. > > I don't know which is better. I'm not sure this was enforced (or maybe > I missed it). > > But a more common practice (that I remember in some IIO drivers) is > that the chip_info table (or a chip_id value) can be added to > spi_device_table. > When the DT tries to probe the driver and reads a different ID (from > SPI) it would fail the probe. So we've had a few threads about this recently and some advice from the device tree maintainers. Where there is a Whoami or similar register, the expectation is that a driver should at most 'warn' about an incorrect device tree binding rather than failing the probe. what it does then is dependent on whether the driver has info on the chip id value found. If it matches a different one then we can hopefully assume that is the correct part (this isn't always true as we have had device with the same ID that weren't compatible). If it is unknown then the assumption should be that the devicetree is correct and the part is compatible so we should carry on anyway with whatever was specified. Idea being that if a new truly compatible part is released then compatible = "bosch,newpart", "bosch,oldpart" should work even though we don't know the chip id for the new part. > > These are just some thoughts. Not a final conclusion. > > > > > > > > > indio_dev->available_scan_masks = bmi088_accel_scan_masks; > > > > indio_dev->modes = INDIO_DIRECT_MODE; > > > > indio_dev->info = &bmi088_accel_info; diff --git > > > > a/drivers/iio/accel/bmi088-accel-spi.c > > > > b/drivers/iio/accel/bmi088-accel-spi.c > > > > index dd1e3f6cf211..0fed0081e1fd 100644 > > > > --- a/drivers/iio/accel/bmi088-accel-spi.c > > > > +++ b/drivers/iio/accel/bmi088-accel-spi.c > > > > @@ -42,7 +42,6 @@ static struct regmap_bus bmi088_regmap_bus = { > > > > static int bmi088_accel_probe(struct spi_device *spi) { > > > > struct regmap *regmap; > > > > - const struct spi_device_id *id = spi_get_device_id(spi); > > > > > > > > regmap = devm_regmap_init(&spi->dev, &bmi088_regmap_bus, > > > > spi, &bmi088_regmap_conf); @@ -52,8 +51,7 > > > @@ > > > > static int bmi088_accel_probe(struct spi_device *spi) > > > > return PTR_ERR(regmap); > > > > } > > > > > > > > - return bmi088_accel_core_probe(&spi->dev, regmap, spi->irq, > > > id->name, > > > > - true); > > > > + return bmi088_accel_core_probe(&spi->dev, regmap, spi->irq, > > > > + true); > > > > } > > > > > > > > static int bmi088_accel_remove(struct spi_device *spi) diff --git > > > > a/drivers/iio/accel/bmi088-accel.h b/drivers/iio/accel/bmi088-accel.h > > > > index 5c25f16b672c..c32afe9606a8 100644 > > > > --- a/drivers/iio/accel/bmi088-accel.h > > > > +++ b/drivers/iio/accel/bmi088-accel.h > > > > @@ -12,7 +12,7 @@ extern const struct regmap_config > > > > bmi088_regmap_conf; extern const struct dev_pm_ops > > > > bmi088_accel_pm_ops; > > > > > > > > int bmi088_accel_core_probe(struct device *dev, struct regmap *regmap, > > > int irq, > > > > - const char *name, bool block_supported); > > > > + bool block_supported); > > > > int bmi088_accel_core_remove(struct device *dev); > > > > > > > > #endif /* BMI088_ACCEL_H */ > > > > -- > > > > 2.25.1 > > > >
On Tue, 10 May 2022 14:17:50 +0000 LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn> wrote: > It is possible to have multiple sensors connected on the same platform, > To support multiple sensors, the commit makes it possible to obtain the > device name by reading the chip ID instead of the device-tree name. > To be compatible with previous versions, renam bmi088a to bmi088-accel. rename This is technically a userspace ABI change. However, the value changed was a fallback anyway, so number of cases where you would actually get it are very few. Hence I'm not that worried about the change. I would add that it is an ABI change and ideally figure out under what circumstances it matters (I 'think' it might be when using fallback compatibles - but you should check). > > Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn> > --- > drivers/iio/accel/bmi088-accel-core.c | 6 +++--- > drivers/iio/accel/bmi088-accel-spi.c | 4 +--- > drivers/iio/accel/bmi088-accel.h | 2 +- > 3 files changed, 5 insertions(+), 7 deletions(-) > > diff --git a/drivers/iio/accel/bmi088-accel-core.c b/drivers/iio/accel/bmi088-accel-core.c > index 8fee1d02e773..de2385e4dad5 100644 > --- a/drivers/iio/accel/bmi088-accel-core.c > +++ b/drivers/iio/accel/bmi088-accel-core.c > @@ -459,7 +459,7 @@ static const struct iio_chan_spec bmi088_accel_channels[] = { > > static const struct bmi088_accel_chip_info bmi088_accel_chip_info_tbl[] = { > [0] = { > - .name = "bmi088a", > + .name = "bmi088-accel", > .chip_id = 0x1E, > .channels = bmi088_accel_channels, > .num_channels = ARRAY_SIZE(bmi088_accel_channels), > @@ -524,7 +524,7 @@ static int bmi088_accel_chip_init(struct bmi088_accel_data *data) > } > > int bmi088_accel_core_probe(struct device *dev, struct regmap *regmap, > - int irq, const char *name, bool block_supported) > + int irq, bool block_supported) > { > struct bmi088_accel_data *data; > struct iio_dev *indio_dev; > @@ -545,7 +545,7 @@ int bmi088_accel_core_probe(struct device *dev, struct regmap *regmap, > > indio_dev->channels = data->chip_info->channels; > indio_dev->num_channels = data->chip_info->num_channels; > - indio_dev->name = name ? name : data->chip_info->name; > + indio_dev->name = data->chip_info->name; > indio_dev->available_scan_masks = bmi088_accel_scan_masks; > indio_dev->modes = INDIO_DIRECT_MODE; > indio_dev->info = &bmi088_accel_info; > diff --git a/drivers/iio/accel/bmi088-accel-spi.c b/drivers/iio/accel/bmi088-accel-spi.c > index dd1e3f6cf211..0fed0081e1fd 100644 > --- a/drivers/iio/accel/bmi088-accel-spi.c > +++ b/drivers/iio/accel/bmi088-accel-spi.c > @@ -42,7 +42,6 @@ static struct regmap_bus bmi088_regmap_bus = { > static int bmi088_accel_probe(struct spi_device *spi) > { > struct regmap *regmap; > - const struct spi_device_id *id = spi_get_device_id(spi); > > regmap = devm_regmap_init(&spi->dev, &bmi088_regmap_bus, > spi, &bmi088_regmap_conf); > @@ -52,8 +51,7 @@ static int bmi088_accel_probe(struct spi_device *spi) > return PTR_ERR(regmap); > } > > - return bmi088_accel_core_probe(&spi->dev, regmap, spi->irq, id->name, > - true); > + return bmi088_accel_core_probe(&spi->dev, regmap, spi->irq, true); > } > > static int bmi088_accel_remove(struct spi_device *spi) > diff --git a/drivers/iio/accel/bmi088-accel.h b/drivers/iio/accel/bmi088-accel.h > index 5c25f16b672c..c32afe9606a8 100644 > --- a/drivers/iio/accel/bmi088-accel.h > +++ b/drivers/iio/accel/bmi088-accel.h > @@ -12,7 +12,7 @@ extern const struct regmap_config bmi088_regmap_conf; > extern const struct dev_pm_ops bmi088_accel_pm_ops; > > int bmi088_accel_core_probe(struct device *dev, struct regmap *regmap, int irq, > - const char *name, bool block_supported); > + bool block_supported); > int bmi088_accel_core_remove(struct device *dev); > > #endif /* BMI088_ACCEL_H */
diff --git a/drivers/iio/accel/bmi088-accel-core.c b/drivers/iio/accel/bmi088-accel-core.c index 8fee1d02e773..de2385e4dad5 100644 --- a/drivers/iio/accel/bmi088-accel-core.c +++ b/drivers/iio/accel/bmi088-accel-core.c @@ -459,7 +459,7 @@ static const struct iio_chan_spec bmi088_accel_channels[] = { static const struct bmi088_accel_chip_info bmi088_accel_chip_info_tbl[] = { [0] = { - .name = "bmi088a", + .name = "bmi088-accel", .chip_id = 0x1E, .channels = bmi088_accel_channels, .num_channels = ARRAY_SIZE(bmi088_accel_channels), @@ -524,7 +524,7 @@ static int bmi088_accel_chip_init(struct bmi088_accel_data *data) } int bmi088_accel_core_probe(struct device *dev, struct regmap *regmap, - int irq, const char *name, bool block_supported) + int irq, bool block_supported) { struct bmi088_accel_data *data; struct iio_dev *indio_dev; @@ -545,7 +545,7 @@ int bmi088_accel_core_probe(struct device *dev, struct regmap *regmap, indio_dev->channels = data->chip_info->channels; indio_dev->num_channels = data->chip_info->num_channels; - indio_dev->name = name ? name : data->chip_info->name; + indio_dev->name = data->chip_info->name; indio_dev->available_scan_masks = bmi088_accel_scan_masks; indio_dev->modes = INDIO_DIRECT_MODE; indio_dev->info = &bmi088_accel_info; diff --git a/drivers/iio/accel/bmi088-accel-spi.c b/drivers/iio/accel/bmi088-accel-spi.c index dd1e3f6cf211..0fed0081e1fd 100644 --- a/drivers/iio/accel/bmi088-accel-spi.c +++ b/drivers/iio/accel/bmi088-accel-spi.c @@ -42,7 +42,6 @@ static struct regmap_bus bmi088_regmap_bus = { static int bmi088_accel_probe(struct spi_device *spi) { struct regmap *regmap; - const struct spi_device_id *id = spi_get_device_id(spi); regmap = devm_regmap_init(&spi->dev, &bmi088_regmap_bus, spi, &bmi088_regmap_conf); @@ -52,8 +51,7 @@ static int bmi088_accel_probe(struct spi_device *spi) return PTR_ERR(regmap); } - return bmi088_accel_core_probe(&spi->dev, regmap, spi->irq, id->name, - true); + return bmi088_accel_core_probe(&spi->dev, regmap, spi->irq, true); } static int bmi088_accel_remove(struct spi_device *spi) diff --git a/drivers/iio/accel/bmi088-accel.h b/drivers/iio/accel/bmi088-accel.h index 5c25f16b672c..c32afe9606a8 100644 --- a/drivers/iio/accel/bmi088-accel.h +++ b/drivers/iio/accel/bmi088-accel.h @@ -12,7 +12,7 @@ extern const struct regmap_config bmi088_regmap_conf; extern const struct dev_pm_ops bmi088_accel_pm_ops; int bmi088_accel_core_probe(struct device *dev, struct regmap *regmap, int irq, - const char *name, bool block_supported); + bool block_supported); int bmi088_accel_core_remove(struct device *dev); #endif /* BMI088_ACCEL_H */
It is possible to have multiple sensors connected on the same platform, To support multiple sensors, the commit makes it possible to obtain the device name by reading the chip ID instead of the device-tree name. To be compatible with previous versions, renam bmi088a to bmi088-accel. Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn> --- drivers/iio/accel/bmi088-accel-core.c | 6 +++--- drivers/iio/accel/bmi088-accel-spi.c | 4 +--- drivers/iio/accel/bmi088-accel.h | 2 +- 3 files changed, 5 insertions(+), 7 deletions(-)