On Thu, May 12, 2022 at 7:18 PM LI Qingwu
<qing-wu.li@xxxxxxxxxxxxxxxxxxxxxxx> wrote:
>
>
> > -----Original Message-----
> > From: Alexandru Ardelean <ardeleanalex@xxxxxxxxx>
> > Sent: Thursday, May 12, 2022 3:32 PM
> > To: LI Qingwu <Qing-wu.Li@xxxxxxxxxxxxxxxxxxxxxxx>
> > Cc: Jonathan Cameron <jic23@xxxxxxxxxx>; Lars-Peter Clausen
> > <lars@xxxxxxxxxx>; mchehab+huawei@xxxxxxxxxx; linux-iio
> > <linux-iio@xxxxxxxxxxxxxxx>; LKML <linux-kernel@xxxxxxxxxxxxxxx>; Rob Herring
> > <robh+dt@xxxxxxxxxx>; Mike Looijmans <mike.looijmans@xxxxxxxx>; devicetree
> > <devicetree@xxxxxxxxxxxxxxx>
> > 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@xxxxxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxx>
> > > ---
> > > 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
> > >
