Message ID | 20210610095300.3613-1-stephan@gerhold.net |
---|---|
Headers | show |
Series | iio: accel: bmc150: Add support for BMA253/BMA254 | expand |
Hi, On 6/10/21 11:52 AM, Stephan Gerhold wrote: > 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 > (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. > > I asked Linus Walleij to test these patches on BMA254 a while ago > and he suggested that I already add his Reviewed-by. > > Stephan Gerhold (6): > iio: accel: bmc150: Drop misleading/duplicate chip identifiers > 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 Thanks, the entire series looks good to me: Reviewed-by: Hans de Goede <hdegoede@redhat.com> for the series. Regards, Hans > > .../bindings/iio/accel/bosch,bma180.yaml | 3 +- > .../bindings/iio/accel/bosch,bma255.yaml | 9 +- > drivers/iio/accel/Kconfig | 6 +- > drivers/iio/accel/bma180.c | 91 +++---------------- > drivers/iio/accel/bmc150-accel-core.c | 36 ++------ > drivers/iio/accel/bmc150-accel-i2c.c | 34 ++++--- > drivers/iio/accel/bmc150-accel-spi.c | 31 ++++--- > drivers/iio/accel/bmc150-accel.h | 10 -- > 8 files changed, 67 insertions(+), 153 deletions(-) >
On Thu, Jun 10, 2021 at 12:56 PM Stephan Gerhold <stephan@gerhold.net> wrote: > > Commit 0ad4bf370176 ("iio:accel:bmc150-accel: Use the chip ID to detect > sensor variant") stopped using the I2C/ACPI match data to look up the > bmc150_accel_chip_info. However, the bmc150_accel_chip_info_tbl remained > as-is, with multiple entries with the same chip_id (e.g. 0xFA for > BMC150/BMI055/BMA255). This is redundant now because actually the driver > will always select the first entry with a matching chip_id. > > So even if a device probes e.g. with BMA0255 it will end up using the > chip_info for BMC150. And in general that's fine for now, the entries > for BMC150/BMI055/BMA255 were exactly the same anyway (except for the > name, which is replaced with the more accurate one later). > > But in this case it's misleading because it suggests that one should > add even more entries with the same chip_id when adding support for > new variants. Let's make that more clear by removing the enum with > the chip identifiers entirely and instead have only one entry per > chip_id. > > Note that we may need to bring back some mechanism to differentiate > between different chips with the same chip_id in the future. > For example, BMA250 (currently supported by the bma180 driver) has the > same chip_id = 0x03 as BMA222 even though they have different channel > sizes (8 bits vs 10 bits). But in any case, that mechanism would > need to look quite different from what we have right now. ... > static const struct bmc150_accel_chip_info bmc150_accel_chip_info_tbl[] = { Perhaps sort this by chip_id value? ... > static const struct acpi_device_id bmc150_accel_acpi_match[] = { > - {"BSBA0150", bmc150}, > - {"BMC150A", bmc150}, > - {"BMI055A", bmi055}, > - {"BMA0255", bma255}, > - {"BMA250E", bma250e}, > - {"BMA222", bma222}, > - {"BMA222E", bma222e}, > - {"BMA0280", bma280}, > + {"BSBA0150"}, > + {"BMC150A"}, > + {"BMI055A"}, > + {"BMA0255"}, > + {"BMA250E"}, > + {"BMA222"}, > + {"BMA222E"}, > + {"BMA0280"}, > {"BOSC0200"}, > {"DUAL250E"}, I have noticed during review patch 3 that the arrays are unsorted, can we at the same time sort them by ID, please? ... > static const struct i2c_device_id bmc150_accel_id[] = { Ditto. > {} > }; ... > static const struct acpi_device_id bmc150_accel_acpi_match[] = { Ditto. > { }, > }; ... > static const struct spi_device_id bmc150_accel_id[] = { Ditto. > {} > };
On Thu, Jun 10, 2021 at 12:56 PM Stephan Gerhold <stephan@gerhold.net> wrote: > > 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 > (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. > > I asked Linus Walleij to test these patches on BMA254 a while ago > and he suggested that I already add his Reviewed-by. I add After addressing comments per patch 1, feel free to add my Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> to the series. And I guess you can drop Laurentiu Palcu email because most of those sensor drivers were done by an Intel team that is not at the company anymore for a few years. > Stephan Gerhold (6): > iio: accel: bmc150: Drop misleading/duplicate chip identifiers > 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 | 6 +- > drivers/iio/accel/bma180.c | 91 +++---------------- > drivers/iio/accel/bmc150-accel-core.c | 36 ++------ > drivers/iio/accel/bmc150-accel-i2c.c | 34 ++++--- > drivers/iio/accel/bmc150-accel-spi.c | 31 ++++--- > drivers/iio/accel/bmc150-accel.h | 10 -- > 8 files changed, 67 insertions(+), 153 deletions(-) > > -- > 2.31.1 >
On Thu, Jun 10, 2021 at 01:29:26PM +0300, Andy Shevchenko wrote: > On Thu, Jun 10, 2021 at 12:56 PM Stephan Gerhold <stephan@gerhold.net> wrote: > > > > 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 > > (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. > > > > I asked Linus Walleij to test these patches on BMA254 a while ago > > and he suggested that I already add his Reviewed-by. > > I add > > > After addressing comments per patch 1, feel free to add my > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > to the series. > Thanks for the review! I think the re-ordering should be a separate commit to make the diff not too confusing. Is it fine for you if I send that as a follow-up patch? I already have two more patches that would conflict with the reordering, so it would be easier to include that in the next series. But I can also re-send the entire series with the extra patch if you prefer that, just let me know. :) > And I guess you can drop Laurentiu Palcu email because most of those > sensor drivers were done by an Intel team that is not at the company > anymore for a few years. > Oh yeah, got a lot of mails that it couldn't be delivered to Laurentiu. Oh well. Thanks, Stephan
On Thu, Jun 10, 2021 at 1:48 PM Stephan Gerhold <stephan@gerhold.net> wrote: > On Thu, Jun 10, 2021 at 01:29:26PM +0300, Andy Shevchenko wrote: > > On Thu, Jun 10, 2021 at 12:56 PM Stephan Gerhold <stephan@gerhold.net> wrote: > > > > > > 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 > > > (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. > > > > > > I asked Linus Walleij to test these patches on BMA254 a while ago > > > and he suggested that I already add his Reviewed-by. > > > > I add > > > > > > After addressing comments per patch 1, feel free to add my > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > to the series. > > > > Thanks for the review! > > I think the re-ordering should be a separate commit to make the diff not > too confusing. Is it fine for you if I send that as a follow-up patch? > I already have two more patches that would conflict with the reordering, > so it would be easier to include that in the next series. > > But I can also re-send the entire series with the extra patch if you > prefer that, just let me know. :) I think that doing the reordering first (if there are no fixes so far) is a good idea.
On Thu, Jun 10, 2021 at 01:51:22PM +0300, Andy Shevchenko wrote: > On Thu, Jun 10, 2021 at 1:48 PM Stephan Gerhold <stephan@gerhold.net> wrote: > > On Thu, Jun 10, 2021 at 01:29:26PM +0300, Andy Shevchenko wrote: > > > On Thu, Jun 10, 2021 at 12:56 PM Stephan Gerhold <stephan@gerhold.net> wrote: > > > > > > > > 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 > > > > (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. > > > > > > > > I asked Linus Walleij to test these patches on BMA254 a while ago > > > > and he suggested that I already add his Reviewed-by. > > > > > > I add > > > > > > > > > After addressing comments per patch 1, feel free to add my > > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > > to the series. > > > > > > > Thanks for the review! > > > > I think the re-ordering should be a separate commit to make the diff not > > too confusing. Is it fine for you if I send that as a follow-up patch? > > I already have two more patches that would conflict with the reordering, > > so it would be easier to include that in the next series. > > > > But I can also re-send the entire series with the extra patch if you > > prefer that, just let me know. :) > > I think that doing the reordering first (if there are no fixes so far) > is a good idea. > OK, I will try to do that somehow. I will probably prepend one of my additional patches to this series since it has a Fixes: tag that would just cause the stable people headaches later when backporting. Thanks, Stephan