On Tue, 27 Jul 2021 20:32:12 +0200
Stephan Gerhold <stephan@xxxxxxxxxxx> wrote:
> Hi Jonathan!
>
> On Sat, Jul 24, 2021 at 05:12:46PM +0100, Jonathan Cameron wrote:
> > On Mon, 19 Jul 2021 13:21:56 +0200
> > Stephan Gerhold <stephan@xxxxxxxxxxx> wrote:
> >
> > > BMC156 is another accelerometer that works just fine with the bmc150-accel
> > > driver. It's very similar to BMC150 (also a accelerometer+magnetometer
> > > combo) but with only one accelerometer interrupt pin. It would make sense
> > > if only INT1 was exposed but someone at Bosch was crazy and decided to only
> > > have an INT2 pin.
> > >
> > > Try to deal with this by making use of the INT2 support introduced
> > > in the previous commit and force using INT2 for BMC156. To detect
> > > that we need to bring up a simplified version of the previous type IDs.
> > >
> > > Note that unlike the type IDs removed in commit c06a6aba6835
> > > ("iio: accel: bmc150: Drop misleading/duplicate chip identifiers")
> > > here I only add one for the special case of BMC156. Everything else
> > > still happens by reading the CHIP_ID register since the chip type
> > > information often is not accurate in ACPI tables.
> > >
> > > Tested-by: Nikita Travkin <nikita@xxxxxxx> # BMC156
> > > Signed-off-by: Stephan Gerhold <stephan@xxxxxxxxxxx>
> > A few really minor things inline.
> >
> > Thanks,
> >
> > Jonathan
> >
> > > ---
> > > drivers/iio/accel/Kconfig | 5 +++--
> > > drivers/iio/accel/bmc150-accel-core.c | 8 +++++---
> > > drivers/iio/accel/bmc150-accel-i2c.c | 10 ++++++++--
> > > drivers/iio/accel/bmc150-accel-spi.c | 10 +++++++++-
> > > drivers/iio/accel/bmc150-accel.h | 9 ++++++++-
> > > 5 files changed, 33 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
> > > index 0e56ace61103..2f0c0d512ae7 100644
> > > --- a/drivers/iio/accel/Kconfig
> > > +++ b/drivers/iio/accel/Kconfig
> > > @@ -143,10 +143,11 @@ config BMC150_ACCEL
> > > select BMC150_ACCEL_SPI if SPI
> > > help
> > > Say yes here to build support for the following Bosch accelerometers:
> > > - BMA222, BMA222E, BMA250E, BMA253, BMA254, BMA255, BMA280, BMC150, BMI055.
> > > + BMA222, BMA222E, BMA250E, BMA253, BMA254, BMA255, BMA280, BMC150, BMC156
> > > + BMI055.
> > >
> > > Note that some of these are combo modules:
> > > - - BMC150: accelerometer and magnetometer
> > > + - BMC150/BMC156: accelerometer and magnetometer
> > > - BMI055: accelerometer and gyroscope
> > >
> > > This driver is only implementing accelerometer part, which has
> > > diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
> > > index 8d3dd3c2bcc2..a5d321e878ef 100644
> > > --- a/drivers/iio/accel/bmc150-accel-core.c
> > > +++ b/drivers/iio/accel/bmc150-accel-core.c
> > > @@ -553,7 +553,7 @@ static void bmc150_accel_interrupts_setup(struct iio_dev *indio_dev,
> > > * Without interrupt-names, we assume the irq belongs to INT1.
> > > */
> > > irq_info = bmc150_accel_interrupts_int1;
> > > - if (irq == of_irq_get_byname(dev->of_node, "INT2"))
> > > + if (data->type == BOSCH_BMC156 || irq == of_irq_get_byname(dev->of_node, "INT2"))
> >
> > It is still preferred to keep line lengths under 80 chars unless it hurts
> > readability to do so. So please wrap this one.
> >
>
> OK, will fix this in v2. :)
>
> > > irq_info = bmc150_accel_interrupts_int2;
> > >
> > > for (i = 0; i < BMC150_ACCEL_INTERRUPTS; i++)
> > > @@ -1174,7 +1174,7 @@ static const struct bmc150_accel_chip_info bmc150_accel_chip_info_tbl[] = {
> > > {306458, BMC150_ACCEL_DEF_RANGE_16G} },
> > > },
> > > {
> > > - .name = "BMA253/BMA254/BMA255/BMC150/BMI055",
> > > + .name = "BMA253/BMA254/BMA255/BMC150/BMC156/BMI055",
> > > .chip_id = 0xFA,
> > > .channels = bmc150_accel_channels,
> > > .num_channels = ARRAY_SIZE(bmc150_accel_channels),
> > > @@ -1661,7 +1661,8 @@ static int bmc150_accel_chip_init(struct bmc150_accel_data *data)
> > > }
> > >
> > > int bmc150_accel_core_probe(struct device *dev, struct regmap *regmap, int irq,
> > > - const char *name, bool block_supported)
> > > + enum bmc150_type type, const char *name,
> > > + bool block_supported)
> > > {
> > > const struct attribute **fifo_attrs;
> > > struct bmc150_accel_data *data;
> > > @@ -1676,6 +1677,7 @@ int bmc150_accel_core_probe(struct device *dev, struct regmap *regmap, int irq,
> > > dev_set_drvdata(dev, indio_dev);
> > >
> > > data->regmap = regmap;
> > > + data->type = type;
> > >
> > > if (!bmc150_apply_acpi_orientation(dev, &data->orientation)) {
> > > ret = iio_read_mount_matrix(dev, &data->orientation);
> > > diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c
> > > index 999495f0669d..88bd8a25f142 100644
> > > --- a/drivers/iio/accel/bmc150-accel-i2c.c
> > > +++ b/drivers/iio/accel/bmc150-accel-i2c.c
> > > @@ -176,6 +176,7 @@ static int bmc150_accel_probe(struct i2c_client *client,
> > > {
> > > struct regmap *regmap;
> > > const char *name = NULL;
> > > + enum bmc150_type type = BOSCH_UNKNOWN;
> > > bool block_supported =
> > > i2c_check_functionality(client->adapter, I2C_FUNC_I2C) ||
> > > i2c_check_functionality(client->adapter,
> > > @@ -188,10 +189,13 @@ static int bmc150_accel_probe(struct i2c_client *client,
> > > return PTR_ERR(regmap);
> > > }
> > >
> > > - if (id)
> > > + if (id) {
> > > name = id->name;
> > > + type = id->driver_data;
> > > + }
> > >
> > > - ret = bmc150_accel_core_probe(&client->dev, regmap, client->irq, name, block_supported);
> > > + ret = bmc150_accel_core_probe(&client->dev, regmap, client->irq,
> > > + type, name, block_supported);
> > > if (ret)
> > > return ret;
> > >
> > > @@ -236,6 +240,7 @@ static const struct i2c_device_id bmc150_accel_id[] = {
> > > {"bma255"},
> > > {"bma280"},
> > > {"bmc150_accel"},
> > > + {"bmc156_accel", BOSCH_BMC156},
> > > {"bmi055_accel"},
> > > {}
> > > };
> > > @@ -251,6 +256,7 @@ static const struct of_device_id bmc150_accel_of_match[] = {
> > > { .compatible = "bosch,bma255" },
> > > { .compatible = "bosch,bma280" },
> > > { .compatible = "bosch,bmc150_accel" },
> > > + { .compatible = "bosch,bmc156_accel" },
> > > { .compatible = "bosch,bmi055_accel" },
> > > { },
> > > };
> > > diff --git a/drivers/iio/accel/bmc150-accel-spi.c b/drivers/iio/accel/bmc150-accel-spi.c
> > > index 54b8c9c8068b..191e312dc91a 100644
> > > --- a/drivers/iio/accel/bmc150-accel-spi.c
> > > +++ b/drivers/iio/accel/bmc150-accel-spi.c
> > > @@ -16,6 +16,8 @@
> > > static int bmc150_accel_probe(struct spi_device *spi)
> > > {
> > > struct regmap *regmap;
> > > + const char *name = NULL;
> > > + enum bmc150_type type = BOSCH_UNKNOWN;
> > > const struct spi_device_id *id = spi_get_device_id(spi);
> > >
> > > regmap = devm_regmap_init_spi(spi, &bmc150_regmap_conf);
> > > @@ -24,7 +26,12 @@ static int bmc150_accel_probe(struct spi_device *spi)
> > > return PTR_ERR(regmap);
> > > }
> > >
> > > - return bmc150_accel_core_probe(&spi->dev, regmap, spi->irq, id->name,
> > > + if (id) {
> > > + name = id->name;
> > > + type = id->driver_data;
> > > + }
> > > +
> > > + return bmc150_accel_core_probe(&spi->dev, regmap, spi->irq, type, name,
> > > true);
> > > }
> > >
> > > @@ -54,6 +61,7 @@ static const struct spi_device_id bmc150_accel_id[] = {
> > > {"bma255"},
> > > {"bma280"},
> > > {"bmc150_accel"},
> > > + {"bmc156_accel", BOSCH_BMC156},
> > > {"bmi055_accel"},
> > > {}
> > > };
> > > diff --git a/drivers/iio/accel/bmc150-accel.h b/drivers/iio/accel/bmc150-accel.h
> > > index 47121f070fe9..a3f4905e48a3 100644
> > > --- a/drivers/iio/accel/bmc150-accel.h
> > > +++ b/drivers/iio/accel/bmc150-accel.h
> > > @@ -13,6 +13,11 @@ struct i2c_client;
> > > struct bmc150_accel_chip_info;
> > > struct bmc150_accel_interrupt_info;
> > >
> > > +enum bmc150_type {
> > > + BOSCH_UNKNOWN,
> > > + BOSCH_BMC156,
> > Whilst we only need to distinguish this one at the moment, the unknown naming
> > implies we don't know the type when often we actually do.
> >
>
> Hm, actually this is exactly what I want to imply! We do have seemingly
> obvious names listed in the ID tables, but unfortunately I don't think
> we can assume them to be accurate.
>
> The original reason why we no longer rely on the type implied by the ID
> is that there are some ACPI devices that specify an ID like "BMA250E"
> when they actually have a BMA222E, see commit 0ad4bf37017 [1]
> ("iio:accel:bmc150-accel: Use the chip ID to detect sensor variant").
>
> And this was the motivation for my commit c06a6aba6835 [2]
> ("iio: accel: bmc150: Drop misleading/duplicate chip identifiers").
> I removed them because they were not used. Also, we cannot make use of
> them in the general case because they are not reliable thanks to those
> ACPI devices.
Gah!
>
> We could perhaps add it only for the of_device_ids. However, even there
> it's easy to specify some similar compatible only because Bosch has so
> many compatible parts. For example, the device where these BMC156
> changes were tested on was using "bosch,bmc150_accel" so far simply
> because this was working fine (without interrupts) and we weren't
> actually aware that it has a BMC156 instead of BMC150.
>
> Im my opinion, adding type information to all of them would imply that
> it can be used reliably, which is not the case unfortunately. Perhaps
> I should instead add a comment to this enum to make this more clear?
>
> What do you think?
A comment seems a good solution given the situation.
Thanks,
Jonathan
>
> Thanks!
> Stephan
>
> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0ad4bf37017621e25fe157fa095fd8849779a873
> [2]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c06a6aba6835946bcccb9909c98ec110949ea630
