| Message ID | 20210610122126.50504-1-stephan@gerhold.net |
|---|---|
| Headers | show |
| Series | iio: accel: bmc150: Add support for BMA253/BMA254 | expand |
On Thu, Jun 10, 2021 at 3:24 PM Stephan Gerhold <stephan@gerhold.net> wrote: > > The chips supported by the bmc150-accel driver are clearly documented > in Kconfig, in the bmc150_accel_chip_info_tbl as well as in all the > device ID tables in the I2C/SPI drivers. It's easy to forget to update > the lists in the file header. Drop those entirely to reduce the amount > of changes required to add new chip variants. > > Signed-off-by: Stephan Gerhold <stephan@gerhold.net> > --- > New patch in v2. Originally I tried to reorder those too but then it > caused conflicts in all my following patches so I'm not convinced > it's worth to try and keep those up to date. It's not user-visible, so I'm fine, but users should have a possibility to know about supported chips in the Kconfig option. Assuming above is done deal, Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > --- > drivers/iio/accel/bmc150-accel-core.c | 10 +--------- > drivers/iio/accel/bmc150-accel-i2c.c | 10 +--------- > 2 files changed, 2 insertions(+), 18 deletions(-) > > diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c > index a0df704730ee..6fb025b4228f 100644 > --- a/drivers/iio/accel/bmc150-accel-core.c > +++ b/drivers/iio/accel/bmc150-accel-core.c > @@ -1,14 +1,6 @@ > // SPDX-License-Identifier: GPL-2.0-only > /* > - * 3-axis accelerometer driver supporting following Bosch-Sensortec chips: > - * - BMC150 > - * - BMI055 > - * - BMA255 > - * - BMA250E > - * - BMA222 > - * - BMA222E > - * - BMA280 > - * > + * 3-axis accelerometer driver supporting many Bosch-Sensortec chips > * Copyright (c) 2014, Intel Corporation. > */ > > diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c > index b8bda0dfb495..a0e2782580b7 100644 > --- a/drivers/iio/accel/bmc150-accel-i2c.c > +++ b/drivers/iio/accel/bmc150-accel-i2c.c > @@ -1,14 +1,6 @@ > // SPDX-License-Identifier: GPL-2.0-only > /* > - * 3-axis accelerometer driver supporting following I2C Bosch-Sensortec chips: > - * - BMC150 > - * - BMI055 > - * - BMA255 > - * - BMA250E > - * - BMA222 > - * - BMA222E > - * - BMA280 > - * > + * 3-axis accelerometer driver supporting many I2C Bosch-Sensortec chips > * Copyright (c) 2014, Intel Corporation. > */ > > -- > 2.32.0 >
On Thu, Jun 10, 2021 at 03:45:17PM +0300, Andy Shevchenko wrote: > On Thu, Jun 10, 2021 at 3:24 PM Stephan Gerhold <stephan@gerhold.net> wrote: > > > > The chips supported by the bmc150-accel driver are clearly documented > > in Kconfig, in the bmc150_accel_chip_info_tbl as well as in all the > > device ID tables in the I2C/SPI drivers. It's easy to forget to update > > the lists in the file header. Drop those entirely to reduce the amount > > of changes required to add new chip variants. > > > > Signed-off-by: Stephan Gerhold <stephan@gerhold.net> > > --- > > New patch in v2. Originally I tried to reorder those too but then it > > caused conflicts in all my following patches so I'm not convinced > > it's worth to try and keep those up to date. > > It's not user-visible, so I'm fine, but users should have a > possibility to know about supported chips in the Kconfig option. > > Assuming above is done deal, > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Yep, it's documented in Kconfig already. Actually I even added one missing chip to Kconfig in patch 4/9 (BMA222) :) Thanks! Stephan
On Thu, Jun 10, 2021 at 3:24 PM Stephan Gerhold <stephan@gerhold.net> wrote: > > Right now all the device IDs are listed in seemingly random order, > make this consistent by ordering those alphabetically. Also, order > bmc150_accel_chip_info_tbl by chip ID for the same reason. Thanks! My comments below, after addressing them, Reviewed-by: Andy Shevchenko <andy.shevhcenko@gmail.com> ... > select BMC150_ACCEL_SPI if SPI > help > Say yes here to build support for the following Bosch accelerometers: > - BMC150, BMI055, BMA250E, BMA222E, BMA255, BMA280. > + BMA222, BMA222E, BMA250E, BMA255, BMA280, BMC150, BMI055. Thanks! > - This is a combo module with both accelerometer and magnetometer. > + BMC150 is a combo module with both accelerometer and magnetometer. BMC150 is only one from the list. Previous message applies to all listed components, so is this not true anymore for the rest? Or all the rest is not a combo? Please, clarify that in the commit message, or if this is a wrong change, drop it. > This driver is only implementing accelerometer part, which has > its own address and register map.
On Thu, Jun 10, 2021 at 3:24 PM Stephan Gerhold <stephan@gerhold.net> wrote: > BMA253 is mostly like BMA255 and has exactly the same register layout > as used by the bmc150-accel driver as far I can tell. Making it work > is as simple as adding new device IDs for it since it has the same > chip_id = 0xFA (250) as BMA255 and others. ... > - .name = "BMC150/BMI055/BMA255", Somehow this is unsorted. > + .name = "BMC150/BMI055/BMA253/BMA255", So does this. -- With Best Regards, Andy Shevchenko
On Thu, Jun 10, 2021 at 3:24 PM Stephan Gerhold <stephan@gerhold.net> wrote: > > Commit c1d1c4a62db5 ("iio: accel: bma180: BMA254 support") added > BMA254 support to the bma180 driver and changed some naming to BMA25x > to make it easier to add support for BMA253 and BMA255. > > Unfortunately, there is quite some overlap between the bma180 driver > and the bmc150-accel driver. Back when the commit was made, the > bmc150-accel driver actually already had support for BMA255, and > adding support for BMA254 would have been as simple as adding a new > compatible to bmc150-accel. > > The bmc150-accel driver is a bit better for BMA254 since it also > supports the motion trigger/interrupt functionality. Fortunately, > moving BMA254 support over to bmc150-accel is fairly simple because > the drivers have compatible device tree bindings. > > Revert most of the changes for BMA254 support in bma180 and move > BMA254 over to bmc150-accel. This has the following advantages: > > - Support for motion trigger/interrupt > - Fix incorrect scale values (BMA254 currently uses the same as > BMA250 but actually they're different because of 10 vs 12 bits > data size) > - Less code than before :) > > BMA250 could be potentially also moved but it's more complicated > because its chip_id conflicts with the one for BMA222 in bmc150-accel. > Perhaps there are also other register differences, I did not investigate > further yet (and I have no way to test it). ... > Say Y here if you want to build a driver for the Bosch BMA023, BMA150 > - BMA180, SMB380, or BMA25x triaxial acceleration sensor. > + BMA180, SMB380, or BMA250 triaxial acceleration sensor. Unsorted before and after. ... > - .name = "BMC150/BMI055/BMA253/BMA255", > + .name = "BMC150/BMI055/BMA253/BMA254/BMA255", Ditto.
On Thu, Jun 10, 2021 at 03:50:25PM +0300, Andy Shevchenko wrote: > On Thu, Jun 10, 2021 at 3:24 PM Stephan Gerhold <stephan@gerhold.net> wrote: > > > > Right now all the device IDs are listed in seemingly random order, > > make this consistent by ordering those alphabetically. Also, order > > bmc150_accel_chip_info_tbl by chip ID for the same reason. > > Thanks! > My comments below, after addressing them, > Reviewed-by: Andy Shevchenko <andy.shevhcenko@gmail.com> > > ... > > > select BMC150_ACCEL_SPI if SPI > > help > > Say yes here to build support for the following Bosch accelerometers: > > - BMC150, BMI055, BMA250E, BMA222E, BMA255, BMA280. > > + BMA222, BMA222E, BMA250E, BMA255, BMA280, BMC150, BMI055. > > Thanks! > > > - This is a combo module with both accelerometer and magnetometer. > > > + BMC150 is a combo module with both accelerometer and magnetometer. > > BMC150 is only one from the list. Previous message applies to all > listed components, so is this not true anymore for the rest? > Or all the rest is not a combo? Please, clarify that in the commit > message, or if this is a wrong change, drop it. > I stumbled on that sentence when making the changes and it definitely does not apply to the BMA* variants. Those are accelerometer only. As far I can tell the prefix in the chip name says which kind of sensors are included, i.e. - BMC150: accelerometer + magnetometer - BMA*: only accelerometer I'm not familiar with BMI055 but funnily the datasheet suggests it's - BMI055: accelerometer + gyroscope So for BMI055 the previous message is wrong too. I guess I need to do yet another commit in v3 to make the Kconfig option more clear for all the sensor variants. :) Thanks! Stephan
On Thu, Jun 10, 2021 at 03:51:46PM +0300, Andy Shevchenko wrote: > On Thu, Jun 10, 2021 at 3:24 PM Stephan Gerhold <stephan@gerhold.net> wrote: > > BMA253 is mostly like BMA255 and has exactly the same register layout > > as used by the bmc150-accel driver as far I can tell. Making it work > > is as simple as adding new device IDs for it since it has the same > > chip_id = 0xFA (250) as BMA255 and others. > > ... > > > - .name = "BMC150/BMI055/BMA255", > > Somehow this is unsorted. > > > + .name = "BMC150/BMI055/BMA253/BMA255", > > So does this. > Yeah I sorted multi-line lists and Kconfig but not those "one-line" lists... :-) Time for v3... Thanks for your review! Stephan
The Bosch BMA253 accelerometer is very similar to both BMA254 and BMA255. The current situation is very confusing: BMA254 is supported by the bma180 driver, but BMA255 is supported by the bmc150-accel driver. It turns out the bma180 and bmc150-accel drivers have quite some overlap, and BMA253/BMA254 would be a bit better supported in bmc150-accel (which has support for the motion trigger/interrupt). This series adds BMA253 support to bmc150-accel and also moves BMA254 over to bmc150, removing some unnecessary code from the bma180 driver. In v2 I also sneaked in a small fix for the scale table of BMA222 to simplify backporting for the stable people. --- Changes in v2: - Add new "iio: accel: bmc150: Fix bma222 scale unit" patch at the beginning so the stable people can backport it without conflicts - Add Reviewed-by: from Hans and Andy for all previous patches - Add patch 3 and 4 to have all the chip lists in a consistent order - Fix last patch to also drop BMA254 from the file header in bma180.c v1: https://lore.kernel.org/linux-iio/20210610095300.3613-1-stephan@gerhold.net/ Stephan Gerhold (9): iio: accel: bmc150: Fix bma222 scale unit iio: accel: bmc150: Drop misleading/duplicate chip identifiers iio: accel: bmc150: Drop duplicated documentation of supported chips iio: accel: bmc150: Sort all chip names alphabetically / by chip ID dt-bindings: iio: accel: bma255: Document bosch,bma253 iio: accel: bmc150: Add device IDs for BMA253 dt-bindings: iio: bma255: Allow multiple interrupts dt-bindings: iio: accel: bma180/bma255: Move bma254 to bma255 schema iio: accel: bma180/bmc150: Move BMA254 to bmc150-accel driver .../bindings/iio/accel/bosch,bma180.yaml | 3 +- .../bindings/iio/accel/bosch,bma255.yaml | 9 +- drivers/iio/accel/Kconfig | 8 +- drivers/iio/accel/bma180.c | 92 +++---------------- drivers/iio/accel/bmc150-accel-core.c | 87 ++++++------------ drivers/iio/accel/bmc150-accel-i2c.c | 52 +++++------ drivers/iio/accel/bmc150-accel-spi.c | 31 ++++--- drivers/iio/accel/bmc150-accel.h | 10 -- 8 files changed, 95 insertions(+), 197 deletions(-)