All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/6] iio: accel: bmc150: Add support for BMA253/BMA254
@ 2021-06-10  9:52 Stephan Gerhold
  2021-06-10  9:52 ` [PATCH 1/6] iio: accel: bmc150: Drop misleading/duplicate chip identifiers Stephan Gerhold
                   ` (7 more replies)
  0 siblings, 8 replies; 13+ messages in thread
From: Stephan Gerhold @ 2021-06-10  9:52 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Lars-Peter Clausen, Rob Herring, Linus Walleij, Peter Meerwald,
	linux-iio, devicetree, Bastien Nocera, Laurentiu Palcu,
	Hans de Goede, Andy Shevchenko, Stephan Gerhold

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

 .../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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/6] iio: accel: bmc150: Drop misleading/duplicate chip identifiers
  2021-06-10  9:52 [PATCH 0/6] iio: accel: bmc150: Add support for BMA253/BMA254 Stephan Gerhold
@ 2021-06-10  9:52 ` Stephan Gerhold
  2021-06-10 10:24   ` Andy Shevchenko
  2021-06-10  9:52 ` [PATCH 2/6] dt-bindings: iio: accel: bma255: Document bosch,bma253 Stephan Gerhold
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 13+ messages in thread
From: Stephan Gerhold @ 2021-06-10  9:52 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Lars-Peter Clausen, Rob Herring, Linus Walleij, Peter Meerwald,
	linux-iio, devicetree, Bastien Nocera, Laurentiu Palcu,
	Hans de Goede, Andy Shevchenko, Stephan Gerhold

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.

Cc: Bastien Nocera <hadess@hadess.net>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
 drivers/iio/accel/bmc150-accel-core.c | 34 ++++++---------------------
 drivers/iio/accel/bmc150-accel-i2c.c  | 30 +++++++++++------------
 drivers/iio/accel/bmc150-accel-spi.c  | 30 +++++++++++------------
 drivers/iio/accel/bmc150-accel.h      | 10 --------
 4 files changed, 37 insertions(+), 67 deletions(-)

diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
index a3d08d713362..d4349f749485 100644
--- a/drivers/iio/accel/bmc150-accel-core.c
+++ b/drivers/iio/accel/bmc150-accel-core.c
@@ -1097,28 +1097,8 @@ static const struct iio_chan_spec bma280_accel_channels[] =
 	BMC150_ACCEL_CHANNELS(14);
 
 static const struct bmc150_accel_chip_info bmc150_accel_chip_info_tbl[] = {
-	[bmc150] = {
-		.name = "BMC150A",
-		.chip_id = 0xFA,
-		.channels = bmc150_accel_channels,
-		.num_channels = ARRAY_SIZE(bmc150_accel_channels),
-		.scale_table = { {9610, BMC150_ACCEL_DEF_RANGE_2G},
-				 {19122, BMC150_ACCEL_DEF_RANGE_4G},
-				 {38344, BMC150_ACCEL_DEF_RANGE_8G},
-				 {76590, BMC150_ACCEL_DEF_RANGE_16G} },
-	},
-	[bmi055] = {
-		.name = "BMI055A",
-		.chip_id = 0xFA,
-		.channels = bmc150_accel_channels,
-		.num_channels = ARRAY_SIZE(bmc150_accel_channels),
-		.scale_table = { {9610, BMC150_ACCEL_DEF_RANGE_2G},
-				 {19122, BMC150_ACCEL_DEF_RANGE_4G},
-				 {38344, BMC150_ACCEL_DEF_RANGE_8G},
-				 {76590, BMC150_ACCEL_DEF_RANGE_16G} },
-	},
-	[bma255] = {
-		.name = "BMA0255",
+	{
+		.name = "BMC150/BMI055/BMA255",
 		.chip_id = 0xFA,
 		.channels = bmc150_accel_channels,
 		.num_channels = ARRAY_SIZE(bmc150_accel_channels),
@@ -1127,7 +1107,7 @@ static const struct bmc150_accel_chip_info bmc150_accel_chip_info_tbl[] = {
 				 {38344, BMC150_ACCEL_DEF_RANGE_8G},
 				 {76590, BMC150_ACCEL_DEF_RANGE_16G} },
 	},
-	[bma250e] = {
+	{
 		.name = "BMA250E",
 		.chip_id = 0xF9,
 		.channels = bma250e_accel_channels,
@@ -1137,7 +1117,7 @@ static const struct bmc150_accel_chip_info bmc150_accel_chip_info_tbl[] = {
 				 {153277, BMC150_ACCEL_DEF_RANGE_8G},
 				 {306457, BMC150_ACCEL_DEF_RANGE_16G} },
 	},
-	[bma222] = {
+	{
 		.name = "BMA222",
 		.chip_id = 0x03,
 		.channels = bma222e_accel_channels,
@@ -1151,7 +1131,7 @@ static const struct bmc150_accel_chip_info bmc150_accel_chip_info_tbl[] = {
 				 {625000, BMC150_ACCEL_DEF_RANGE_8G},
 				 {1250000, BMC150_ACCEL_DEF_RANGE_16G} },
 	},
-	[bma222e] = {
+	{
 		.name = "BMA222E",
 		.chip_id = 0xF8,
 		.channels = bma222e_accel_channels,
@@ -1161,8 +1141,8 @@ static const struct bmc150_accel_chip_info bmc150_accel_chip_info_tbl[] = {
 				 {612915, BMC150_ACCEL_DEF_RANGE_8G},
 				 {1225831, BMC150_ACCEL_DEF_RANGE_16G} },
 	},
-	[bma280] = {
-		.name = "BMA0280",
+	{
+		.name = "BMA280",
 		.chip_id = 0xFB,
 		.channels = bma280_accel_channels,
 		.num_channels = ARRAY_SIZE(bma280_accel_channels),
diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c
index d34dddb850d9..b8bda0dfb495 100644
--- a/drivers/iio/accel/bmc150-accel-i2c.c
+++ b/drivers/iio/accel/bmc150-accel-i2c.c
@@ -221,14 +221,14 @@ static int bmc150_accel_remove(struct i2c_client *client)
 }
 
 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"},
 	{ },
@@ -236,13 +236,13 @@ static const struct acpi_device_id bmc150_accel_acpi_match[] = {
 MODULE_DEVICE_TABLE(acpi, bmc150_accel_acpi_match);
 
 static const struct i2c_device_id bmc150_accel_id[] = {
-	{"bmc150_accel",	bmc150},
-	{"bmi055_accel",	bmi055},
-	{"bma255",		bma255},
-	{"bma250e",		bma250e},
-	{"bma222",		bma222},
-	{"bma222e",		bma222e},
-	{"bma280",		bma280},
+	{"bmc150_accel"},
+	{"bmi055_accel"},
+	{"bma255"},
+	{"bma250e"},
+	{"bma222"},
+	{"bma222e"},
+	{"bma280"},
 	{}
 };
 
diff --git a/drivers/iio/accel/bmc150-accel-spi.c b/drivers/iio/accel/bmc150-accel-spi.c
index 74a8aee4f612..01b42fa6a015 100644
--- a/drivers/iio/accel/bmc150-accel-spi.c
+++ b/drivers/iio/accel/bmc150-accel-spi.c
@@ -34,26 +34,26 @@ static int bmc150_accel_remove(struct spi_device *spi)
 }
 
 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"},
 	{ },
 };
 MODULE_DEVICE_TABLE(acpi, bmc150_accel_acpi_match);
 
 static const struct spi_device_id bmc150_accel_id[] = {
-	{"bmc150_accel",	bmc150},
-	{"bmi055_accel",	bmi055},
-	{"bma255",		bma255},
-	{"bma250e",		bma250e},
-	{"bma222",		bma222},
-	{"bma222e",		bma222e},
-	{"bma280",		bma280},
+	{"bmc150_accel"},
+	{"bmi055_accel"},
+	{"bma255"},
+	{"bma250e"},
+	{"bma222"},
+	{"bma222e"},
+	{"bma280"},
 	{}
 };
 MODULE_DEVICE_TABLE(spi, bmc150_accel_id);
diff --git a/drivers/iio/accel/bmc150-accel.h b/drivers/iio/accel/bmc150-accel.h
index d67d6ed6ae77..47121f070fe9 100644
--- a/drivers/iio/accel/bmc150-accel.h
+++ b/drivers/iio/accel/bmc150-accel.h
@@ -68,16 +68,6 @@ struct bmc150_accel_data {
 	struct iio_mount_matrix orientation;
 };
 
-enum {
-	bmc150,
-	bmi055,
-	bma255,
-	bma250e,
-	bma222,
-	bma222e,
-	bma280,
-};
-
 int bmc150_accel_core_probe(struct device *dev, struct regmap *regmap, int irq,
 			    const char *name, bool block_supported);
 int bmc150_accel_core_remove(struct device *dev);
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/6] dt-bindings: iio: accel: bma255: Document bosch,bma253
  2021-06-10  9:52 [PATCH 0/6] iio: accel: bmc150: Add support for BMA253/BMA254 Stephan Gerhold
  2021-06-10  9:52 ` [PATCH 1/6] iio: accel: bmc150: Drop misleading/duplicate chip identifiers Stephan Gerhold
@ 2021-06-10  9:52 ` Stephan Gerhold
  2021-06-10  9:52 ` [PATCH 3/6] iio: accel: bmc150: Add device IDs for BMA253 Stephan Gerhold
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: Stephan Gerhold @ 2021-06-10  9:52 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Lars-Peter Clausen, Rob Herring, Linus Walleij, Peter Meerwald,
	linux-iio, devicetree, Bastien Nocera, Laurentiu Palcu,
	Hans de Goede, Andy Shevchenko, Stephan Gerhold

BMA253 is mostly like BMA255 that is already supported by the
bmc150-accel driver. Document an extra bosch,bma253 compatible for it.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
 Documentation/devicetree/bindings/iio/accel/bosch,bma255.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/iio/accel/bosch,bma255.yaml b/Documentation/devicetree/bindings/iio/accel/bosch,bma255.yaml
index c2efbb813ca2..8afb0fe8ef5c 100644
--- a/Documentation/devicetree/bindings/iio/accel/bosch,bma255.yaml
+++ b/Documentation/devicetree/bindings/iio/accel/bosch,bma255.yaml
@@ -18,6 +18,7 @@ properties:
     enum:
       - bosch,bmc150_accel
       - bosch,bmi055_accel
+      - bosch,bma253
       - bosch,bma255
       - bosch,bma250e
       - bosch,bma222
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 3/6] iio: accel: bmc150: Add device IDs for BMA253
  2021-06-10  9:52 [PATCH 0/6] iio: accel: bmc150: Add support for BMA253/BMA254 Stephan Gerhold
  2021-06-10  9:52 ` [PATCH 1/6] iio: accel: bmc150: Drop misleading/duplicate chip identifiers Stephan Gerhold
  2021-06-10  9:52 ` [PATCH 2/6] dt-bindings: iio: accel: bma255: Document bosch,bma253 Stephan Gerhold
@ 2021-06-10  9:52 ` Stephan Gerhold
  2021-06-10  9:52 ` [PATCH 4/6] dt-bindings: iio: bma255: Allow multiple interrupts Stephan Gerhold
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: Stephan Gerhold @ 2021-06-10  9:52 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Lars-Peter Clausen, Rob Herring, Linus Walleij, Peter Meerwald,
	linux-iio, devicetree, Bastien Nocera, Laurentiu Palcu,
	Hans de Goede, Andy Shevchenko, Stephan Gerhold

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.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
 drivers/iio/accel/Kconfig             | 2 +-
 drivers/iio/accel/bmc150-accel-core.c | 4 ++--
 drivers/iio/accel/bmc150-accel-i2c.c  | 2 ++
 drivers/iio/accel/bmc150-accel-spi.c  | 1 +
 4 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
index 17f6bdcf1db3..f68c14f02d6b 100644
--- a/drivers/iio/accel/Kconfig
+++ b/drivers/iio/accel/Kconfig
@@ -143,7 +143,7 @@ config BMC150_ACCEL
 	select BMC150_ACCEL_SPI if SPI
 	help
 	  Say yes here to build support for the following Bosch accelerometers:
-	  BMC150, BMI055, BMA250E, BMA222E, BMA255, BMA280.
+	  BMC150, BMI055, BMA250E, BMA222E, BMA253, BMA255, BMA280.
 
 	  This is a combo module with both accelerometer and magnetometer.
 	  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 d4349f749485..21abc8d166e6 100644
--- a/drivers/iio/accel/bmc150-accel-core.c
+++ b/drivers/iio/accel/bmc150-accel-core.c
@@ -3,7 +3,7 @@
  * 3-axis accelerometer driver supporting following Bosch-Sensortec chips:
  *  - BMC150
  *  - BMI055
- *  - BMA255
+ *  - BMA253/BMA255
  *  - BMA250E
  *  - BMA222
  *  - BMA222E
@@ -1098,7 +1098,7 @@ static const struct iio_chan_spec bma280_accel_channels[] =
 
 static const struct bmc150_accel_chip_info bmc150_accel_chip_info_tbl[] = {
 	{
-		.name = "BMC150/BMI055/BMA255",
+		.name = "BMC150/BMI055/BMA253/BMA255",
 		.chip_id = 0xFA,
 		.channels = bmc150_accel_channels,
 		.num_channels = ARRAY_SIZE(bmc150_accel_channels),
diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c
index b8bda0dfb495..a87306f03a4c 100644
--- a/drivers/iio/accel/bmc150-accel-i2c.c
+++ b/drivers/iio/accel/bmc150-accel-i2c.c
@@ -238,6 +238,7 @@ MODULE_DEVICE_TABLE(acpi, bmc150_accel_acpi_match);
 static const struct i2c_device_id bmc150_accel_id[] = {
 	{"bmc150_accel"},
 	{"bmi055_accel"},
+	{"bma253"},
 	{"bma255"},
 	{"bma250e"},
 	{"bma222"},
@@ -251,6 +252,7 @@ MODULE_DEVICE_TABLE(i2c, bmc150_accel_id);
 static const struct of_device_id bmc150_accel_of_match[] = {
 	{ .compatible = "bosch,bmc150_accel" },
 	{ .compatible = "bosch,bmi055_accel" },
+	{ .compatible = "bosch,bma253" },
 	{ .compatible = "bosch,bma255" },
 	{ .compatible = "bosch,bma250e" },
 	{ .compatible = "bosch,bma222" },
diff --git a/drivers/iio/accel/bmc150-accel-spi.c b/drivers/iio/accel/bmc150-accel-spi.c
index 01b42fa6a015..dec7dd252658 100644
--- a/drivers/iio/accel/bmc150-accel-spi.c
+++ b/drivers/iio/accel/bmc150-accel-spi.c
@@ -49,6 +49,7 @@ MODULE_DEVICE_TABLE(acpi, bmc150_accel_acpi_match);
 static const struct spi_device_id bmc150_accel_id[] = {
 	{"bmc150_accel"},
 	{"bmi055_accel"},
+	{"bma253"},
 	{"bma255"},
 	{"bma250e"},
 	{"bma222"},
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 4/6] dt-bindings: iio: bma255: Allow multiple interrupts
  2021-06-10  9:52 [PATCH 0/6] iio: accel: bmc150: Add support for BMA253/BMA254 Stephan Gerhold
                   ` (2 preceding siblings ...)
  2021-06-10  9:52 ` [PATCH 3/6] iio: accel: bmc150: Add device IDs for BMA253 Stephan Gerhold
@ 2021-06-10  9:52 ` Stephan Gerhold
  2021-06-10  9:52 ` [PATCH 5/6] dt-bindings: iio: accel: bma180/bma255: Move bma254 to bma255 schema Stephan Gerhold
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: Stephan Gerhold @ 2021-06-10  9:52 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Lars-Peter Clausen, Rob Herring, Linus Walleij, Peter Meerwald,
	linux-iio, devicetree, Bastien Nocera, Laurentiu Palcu,
	Hans de Goede, Andy Shevchenko, Stephan Gerhold

BMA253 has two interrupt pins (INT1 and INT2) that can be configured
independently. At the moment the bmc150-accel driver does not make use
of them but it might be able to in the future, so it's useful to already
specify all available interrupts in the device tree.

Set maxItems: 2 for interrupts to allow specifying a second one.
This is necessary as preparation to move the bosch,bma254 compatible
from bosch,bma180.yaml to bosch,bma255.yaml since bma180 allows two
interrupts, but BMA254 is better supported by the bmc150-accel driver.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
 .../devicetree/bindings/iio/accel/bosch,bma255.yaml        | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/iio/accel/bosch,bma255.yaml b/Documentation/devicetree/bindings/iio/accel/bosch,bma255.yaml
index 8afb0fe8ef5c..65b299a5619b 100644
--- a/Documentation/devicetree/bindings/iio/accel/bosch,bma255.yaml
+++ b/Documentation/devicetree/bindings/iio/accel/bosch,bma255.yaml
@@ -32,7 +32,12 @@ properties:
   vddio-supply: true
 
   interrupts:
-    maxItems: 1
+    minItems: 1
+    maxItems: 2
+    description: |
+      The first interrupt listed must be the one connected to the INT1 pin,
+      the second (optional) interrupt listed must be the one connected to the
+      INT2 pin (if available).
 
   mount-matrix:
     description: an optional 3x3 mounting rotation matrix.
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 5/6] dt-bindings: iio: accel: bma180/bma255: Move bma254 to bma255 schema
  2021-06-10  9:52 [PATCH 0/6] iio: accel: bmc150: Add support for BMA253/BMA254 Stephan Gerhold
                   ` (3 preceding siblings ...)
  2021-06-10  9:52 ` [PATCH 4/6] dt-bindings: iio: bma255: Allow multiple interrupts Stephan Gerhold
@ 2021-06-10  9:52 ` Stephan Gerhold
  2021-06-10  9:53 ` [PATCH 6/6] iio: accel: bma180/bmc150: Move BMA254 to bmc150-accel driver Stephan Gerhold
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: Stephan Gerhold @ 2021-06-10  9:52 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Lars-Peter Clausen, Rob Herring, Linus Walleij, Peter Meerwald,
	linux-iio, devicetree, Bastien Nocera, Laurentiu Palcu,
	Hans de Goede, Andy Shevchenko, Stephan Gerhold

BMA254 is very similar to BMA253/BMA255 which are both supported by
the bmc150-accel driver. In general, there is quite some overlap between
the bma180 and bmc150-accel driver, but the bmc150-accel driver has a few
more features (e.g. motion trigger/interrupt).

Let's move bma254 over to the bma255 schema (bmc150-accel driver).

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
 Documentation/devicetree/bindings/iio/accel/bosch,bma180.yaml | 3 +--
 Documentation/devicetree/bindings/iio/accel/bosch,bma255.yaml | 1 +
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/accel/bosch,bma180.yaml b/Documentation/devicetree/bindings/iio/accel/bosch,bma180.yaml
index 45b3abde298f..a7e84089cc3d 100644
--- a/Documentation/devicetree/bindings/iio/accel/bosch,bma180.yaml
+++ b/Documentation/devicetree/bindings/iio/accel/bosch,bma180.yaml
@@ -4,7 +4,7 @@
 $id: http://devicetree.org/schemas/iio/accel/bosch,bma180.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: Bosch BMA023 / BMA150/ BMA180 / BMA25x / SMB380 triaxial accelerometers
+title: Bosch BMA023 / BMA150/ BMA180 / BMA250 / SMB380 triaxial accelerometers
 
 maintainers:
   - Jonathan Cameron <jic23@kernel.org>
@@ -21,7 +21,6 @@ properties:
       - bosch,bma150
       - bosch,bma180
       - bosch,bma250
-      - bosch,bma254
       - bosch,smb380
 
   reg:
diff --git a/Documentation/devicetree/bindings/iio/accel/bosch,bma255.yaml b/Documentation/devicetree/bindings/iio/accel/bosch,bma255.yaml
index 65b299a5619b..e830d5295b92 100644
--- a/Documentation/devicetree/bindings/iio/accel/bosch,bma255.yaml
+++ b/Documentation/devicetree/bindings/iio/accel/bosch,bma255.yaml
@@ -19,6 +19,7 @@ properties:
       - bosch,bmc150_accel
       - bosch,bmi055_accel
       - bosch,bma253
+      - bosch,bma254
       - bosch,bma255
       - bosch,bma250e
       - bosch,bma222
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 6/6] iio: accel: bma180/bmc150: Move BMA254 to bmc150-accel driver
  2021-06-10  9:52 [PATCH 0/6] iio: accel: bmc150: Add support for BMA253/BMA254 Stephan Gerhold
                   ` (4 preceding siblings ...)
  2021-06-10  9:52 ` [PATCH 5/6] dt-bindings: iio: accel: bma180/bma255: Move bma254 to bma255 schema Stephan Gerhold
@ 2021-06-10  9:53 ` Stephan Gerhold
  2021-06-10 10:17 ` [PATCH 0/6] iio: accel: bmc150: Add support for BMA253/BMA254 Hans de Goede
  2021-06-10 10:29 ` Andy Shevchenko
  7 siblings, 0 replies; 13+ messages in thread
From: Stephan Gerhold @ 2021-06-10  9:53 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Lars-Peter Clausen, Rob Herring, Linus Walleij, Peter Meerwald,
	linux-iio, devicetree, Bastien Nocera, Laurentiu Palcu,
	Hans de Goede, Andy Shevchenko, Stephan Gerhold

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).

Cc: Peter Meerwald <pmeerw@pmeerw.net>
Cc: Laurentiu Palcu <laurentiu.palcu@intel.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
Note that the similarity between bma180 and bmc150-accel has already
caused a lot of confusion in the past. See e.g. [1] where Hans
accidentally added BMA250E support to bma180 even though that was
already supported in bmc150-accel.

Having BMA250 in bma180 and BMA250E in bmc150-accel was also discussed
before in [2] but for some reason back then the conclusion was that
they are different enough to justify that.

Also note that this patch only applies cleanly on top of the
"iio: accel: bma180: Fix BMA25x bandwidth register values" patch [3]
that I sent earlier, which is not present in togreg/testing
at the moment (only in fixes-togreg).

[1]: https://lore.kernel.org/linux-iio/20170606203538.15250-1-hdegoede@redhat.com/
[2]: https://lore.kernel.org/linux-iio/alpine.DEB.2.01.1408281111250.3259@pmeerw.net/
[3]: https://lore.kernel.org/linux-iio/20210526094408.34298-2-stephan@gerhold.net/
---
 drivers/iio/accel/Kconfig             |  6 +-
 drivers/iio/accel/bma180.c            | 91 ++++-----------------------
 drivers/iio/accel/bmc150-accel-core.c |  4 +-
 drivers/iio/accel/bmc150-accel-i2c.c  |  2 +
 4 files changed, 19 insertions(+), 84 deletions(-)

diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
index f68c14f02d6b..b00c228b9bdc 100644
--- a/drivers/iio/accel/Kconfig
+++ b/drivers/iio/accel/Kconfig
@@ -89,13 +89,13 @@ config ADXL372_I2C
 	  module will be called adxl372_i2c.
 
 config BMA180
-	tristate "Bosch BMA023/BMA1x0/BMA25x 3-Axis Accelerometer Driver"
+	tristate "Bosch BMA023/BMA1x0/BMA250 3-Axis Accelerometer Driver"
 	depends on I2C && INPUT_BMA150=n
 	select IIO_BUFFER
 	select IIO_TRIGGERED_BUFFER
 	help
 	  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.
 
 	  To compile this driver as a module, choose M here: the
 	  module will be called bma180.
@@ -143,7 +143,7 @@ config BMC150_ACCEL
 	select BMC150_ACCEL_SPI if SPI
 	help
 	  Say yes here to build support for the following Bosch accelerometers:
-	  BMC150, BMI055, BMA250E, BMA222E, BMA253, BMA255, BMA280.
+	  BMC150, BMI055, BMA250E, BMA222E, BMA253, BMA254, BMA255, BMA280.
 
 	  This is a combo module with both accelerometer and magnetometer.
 	  This driver is only implementing accelerometer part, which has
diff --git a/drivers/iio/accel/bma180.c b/drivers/iio/accel/bma180.c
index e7c6b3096cb7..ea1a604a53bf 100644
--- a/drivers/iio/accel/bma180.c
+++ b/drivers/iio/accel/bma180.c
@@ -38,7 +38,6 @@ enum chip_ids {
 	BMA150,
 	BMA180,
 	BMA250,
-	BMA254,
 };
 
 struct bma180_data;
@@ -59,7 +58,6 @@ struct bma180_part_info {
 	u8 scale_reg, scale_mask;
 	u8 power_reg, power_mask, lowpower_val;
 	u8 int_enable_reg, int_enable_mask;
-	u8 int_map_reg, int_enable_dataready_int1_mask;
 	u8 softreset_reg, softreset_val;
 
 	int (*chip_config)(struct bma180_data *data);
@@ -112,7 +110,6 @@ struct bma180_part_info {
 #define BMA023_ID_REG_VAL	0x02
 #define BMA180_ID_REG_VAL	0x03
 #define BMA250_ID_REG_VAL	0x03
-#define BMA254_ID_REG_VAL	0xfa /* 250 decimal */
 
 /* Chip power modes */
 #define BMA180_LOW_POWER	0x03
@@ -134,24 +131,6 @@ struct bma180_part_info {
 #define BMA250_INT1_DATA_MASK	BIT(0)
 #define BMA250_INT_RESET_MASK	BIT(7) /* Reset pending interrupts */
 
-#define BMA254_RANGE_REG	0x0f
-#define BMA254_BW_REG		0x10
-#define BMA254_POWER_REG	0x11
-#define BMA254_RESET_REG	0x14
-#define BMA254_INT_ENABLE_REG	0x17
-#define BMA254_INT_MAP_REG	0x1a
-#define BMA254_INT_RESET_REG	0x21
-
-#define BMA254_RANGE_MASK	GENMASK(3, 0) /* Range of accel values */
-#define BMA254_BW_MASK		GENMASK(4, 0) /* Accel bandwidth */
-#define BMA254_BW_OFFSET	8
-#define BMA254_SUSPEND_MASK	BIT(7) /* chip will sleep */
-#define BMA254_LOWPOWER_MASK	BIT(6)
-#define BMA254_DATA_INTEN_MASK	BIT(4)
-#define BMA254_INT2_DATA_MASK	BIT(7)
-#define BMA254_INT1_DATA_MASK	BIT(0)
-#define BMA254_INT_RESET_MASK	BIT(7) /* Reset pending interrupts */
-
 struct bma180_data {
 	struct regulator *vdd_supply;
 	struct regulator *vddio_supply;
@@ -184,8 +163,8 @@ static int bma023_scale_table[] = { 2452, 4903, 9709, };
 static int bma180_bw_table[] = { 10, 20, 40, 75, 150, 300 }; /* Hz */
 static int bma180_scale_table[] = { 1275, 1863, 2452, 3727, 4903, 9709, 19417 };
 
-static int bma25x_bw_table[] = { 8, 16, 31, 63, 125, 250, 500, 1000 }; /* Hz */
-static int bma25x_scale_table[] = { 0, 0, 0, 38344, 0, 76590, 0, 0, 153180, 0,
+static int bma250_bw_table[] = { 8, 16, 31, 63, 125, 250, 500, 1000 }; /* Hz */
+static int bma250_scale_table[] = { 0, 0, 0, 38344, 0, 76590, 0, 0, 153180, 0,
 	0, 0, 306458 };
 
 static int bma180_get_data_reg(struct bma180_data *data, enum bma180_chan chan)
@@ -432,7 +411,7 @@ static int bma180_chip_config(struct bma180_data *data)
 	return ret;
 }
 
-static int bma25x_chip_config(struct bma180_data *data)
+static int bma250_chip_config(struct bma180_data *data)
 {
 	int ret = bma180_chip_init(data);
 
@@ -451,8 +430,7 @@ static int bma25x_chip_config(struct bma180_data *data)
 	 * This enables dataready interrupt on the INT1 pin
 	 * FIXME: support using the INT2 pin
 	 */
-	ret = bma180_set_bits(data, data->part_info->int_map_reg,
-		data->part_info->int_enable_dataready_int1_mask, 1);
+	ret = bma180_set_bits(data, BMA250_INT_MAP_REG, BMA250_INT1_DATA_MASK, 1);
 	if (ret)
 		goto err;
 
@@ -489,7 +467,7 @@ static void bma180_chip_disable(struct bma180_data *data)
 	dev_err(&data->client->dev, "failed to disable the chip\n");
 }
 
-static void bma25x_chip_disable(struct bma180_data *data)
+static void bma250_chip_disable(struct bma180_data *data)
 {
 	if (bma180_set_new_data_intr_state(data, false))
 		goto err;
@@ -775,14 +753,6 @@ static const struct iio_chan_spec bma250_channels[] = {
 	IIO_CHAN_SOFT_TIMESTAMP(4),
 };
 
-static const struct iio_chan_spec bma254_channels[] = {
-	BMA180_ACC_CHANNEL(X, 12),
-	BMA180_ACC_CHANNEL(Y, 12),
-	BMA180_ACC_CHANNEL(Z, 12),
-	BMA180_TEMP_CHANNEL,
-	IIO_CHAN_SOFT_TIMESTAMP(4),
-};
-
 static const struct bma180_part_info bma180_part_info[] = {
 	[BMA023] = {
 		.chip_id = BMA023_ID_REG_VAL,
@@ -872,10 +842,10 @@ static const struct bma180_part_info bma180_part_info[] = {
 		.chip_id = BMA250_ID_REG_VAL,
 		.channels = bma250_channels,
 		.num_channels = ARRAY_SIZE(bma250_channels),
-		.scale_table = bma25x_scale_table,
-		.num_scales = ARRAY_SIZE(bma25x_scale_table),
-		.bw_table = bma25x_bw_table,
-		.num_bw = ARRAY_SIZE(bma25x_bw_table),
+		.scale_table = bma250_scale_table,
+		.num_scales = ARRAY_SIZE(bma250_scale_table),
+		.bw_table = bma250_bw_table,
+		.num_bw = ARRAY_SIZE(bma250_bw_table),
 		.temp_offset = 48, /* 0 LSB @ 24 degree C */
 		.int_reset_reg = BMA250_INT_RESET_REG,
 		.int_reset_mask = BMA250_INT_RESET_MASK,
@@ -891,42 +861,10 @@ static const struct bma180_part_info bma180_part_info[] = {
 		.lowpower_val = 1,
 		.int_enable_reg = BMA250_INT_ENABLE_REG,
 		.int_enable_mask = BMA250_DATA_INTEN_MASK,
-		.int_map_reg = BMA250_INT_MAP_REG,
-		.int_enable_dataready_int1_mask = BMA250_INT1_DATA_MASK,
 		.softreset_reg = BMA250_RESET_REG,
 		.softreset_val = BMA180_RESET_VAL,
-		.chip_config = bma25x_chip_config,
-		.chip_disable = bma25x_chip_disable,
-	},
-	[BMA254] = {
-		.chip_id = BMA254_ID_REG_VAL,
-		.channels = bma254_channels,
-		.num_channels = ARRAY_SIZE(bma254_channels),
-		.scale_table = bma25x_scale_table,
-		.num_scales = ARRAY_SIZE(bma25x_scale_table),
-		.bw_table = bma25x_bw_table,
-		.num_bw = ARRAY_SIZE(bma25x_bw_table),
-		.temp_offset = 46, /* 0 LSB @ 23 degree C */
-		.int_reset_reg = BMA254_INT_RESET_REG,
-		.int_reset_mask = BMA254_INT_RESET_MASK,
-		.sleep_reg = BMA254_POWER_REG,
-		.sleep_mask = BMA254_SUSPEND_MASK,
-		.bw_reg = BMA254_BW_REG,
-		.bw_mask = BMA254_BW_MASK,
-		.bw_offset = BMA254_BW_OFFSET,
-		.scale_reg = BMA254_RANGE_REG,
-		.scale_mask = BMA254_RANGE_MASK,
-		.power_reg = BMA254_POWER_REG,
-		.power_mask = BMA254_LOWPOWER_MASK,
-		.lowpower_val = 1,
-		.int_enable_reg = BMA254_INT_ENABLE_REG,
-		.int_enable_mask = BMA254_DATA_INTEN_MASK,
-		.int_map_reg = BMA254_INT_MAP_REG,
-		.int_enable_dataready_int1_mask = BMA254_INT1_DATA_MASK,
-		.softreset_reg = BMA254_RESET_REG,
-		.softreset_val = BMA180_RESET_VAL,
-		.chip_config = bma25x_chip_config,
-		.chip_disable = bma25x_chip_disable,
+		.chip_config = bma250_chip_config,
+		.chip_disable = bma250_chip_disable,
 	},
 };
 
@@ -1166,7 +1104,6 @@ static const struct i2c_device_id bma180_ids[] = {
 	{ "bma150", BMA150 },
 	{ "bma180", BMA180 },
 	{ "bma250", BMA250 },
-	{ "bma254", BMA254 },
 	{ "smb380", BMA150 },
 	{ }
 };
@@ -1190,10 +1127,6 @@ static const struct of_device_id bma180_of_match[] = {
 		.compatible = "bosch,bma250",
 		.data = (void *)BMA250
 	},
-	{
-		.compatible = "bosch,bma254",
-		.data = (void *)BMA254
-	},
 	{
 		.compatible = "bosch,smb380",
 		.data = (void *)BMA150
@@ -1217,5 +1150,5 @@ module_i2c_driver(bma180_driver);
 
 MODULE_AUTHOR("Kravchenko Oleksandr <x0199363@ti.com>");
 MODULE_AUTHOR("Texas Instruments, Inc.");
-MODULE_DESCRIPTION("Bosch BMA023/BMA1x0/BMA25x triaxial acceleration sensor");
+MODULE_DESCRIPTION("Bosch BMA023/BMA1x0/BMA250 triaxial acceleration sensor");
 MODULE_LICENSE("GPL");
diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
index 21abc8d166e6..979da58fdb60 100644
--- a/drivers/iio/accel/bmc150-accel-core.c
+++ b/drivers/iio/accel/bmc150-accel-core.c
@@ -3,7 +3,7 @@
  * 3-axis accelerometer driver supporting following Bosch-Sensortec chips:
  *  - BMC150
  *  - BMI055
- *  - BMA253/BMA255
+ *  - BMA253/BMA254/BMA255
  *  - BMA250E
  *  - BMA222
  *  - BMA222E
@@ -1098,7 +1098,7 @@ static const struct iio_chan_spec bma280_accel_channels[] =
 
 static const struct bmc150_accel_chip_info bmc150_accel_chip_info_tbl[] = {
 	{
-		.name = "BMC150/BMI055/BMA253/BMA255",
+		.name = "BMC150/BMI055/BMA253/BMA254/BMA255",
 		.chip_id = 0xFA,
 		.channels = bmc150_accel_channels,
 		.num_channels = ARRAY_SIZE(bmc150_accel_channels),
diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c
index a87306f03a4c..8f0dfe03c919 100644
--- a/drivers/iio/accel/bmc150-accel-i2c.c
+++ b/drivers/iio/accel/bmc150-accel-i2c.c
@@ -239,6 +239,7 @@ static const struct i2c_device_id bmc150_accel_id[] = {
 	{"bmc150_accel"},
 	{"bmi055_accel"},
 	{"bma253"},
+	{"bma254"},
 	{"bma255"},
 	{"bma250e"},
 	{"bma222"},
@@ -253,6 +254,7 @@ static const struct of_device_id bmc150_accel_of_match[] = {
 	{ .compatible = "bosch,bmc150_accel" },
 	{ .compatible = "bosch,bmi055_accel" },
 	{ .compatible = "bosch,bma253" },
+	{ .compatible = "bosch,bma254" },
 	{ .compatible = "bosch,bma255" },
 	{ .compatible = "bosch,bma250e" },
 	{ .compatible = "bosch,bma222" },
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/6] iio: accel: bmc150: Add support for BMA253/BMA254
  2021-06-10  9:52 [PATCH 0/6] iio: accel: bmc150: Add support for BMA253/BMA254 Stephan Gerhold
                   ` (5 preceding siblings ...)
  2021-06-10  9:53 ` [PATCH 6/6] iio: accel: bma180/bmc150: Move BMA254 to bmc150-accel driver Stephan Gerhold
@ 2021-06-10 10:17 ` Hans de Goede
  2021-06-10 10:29 ` Andy Shevchenko
  7 siblings, 0 replies; 13+ messages in thread
From: Hans de Goede @ 2021-06-10 10:17 UTC (permalink / raw)
  To: Stephan Gerhold, Jonathan Cameron
  Cc: Lars-Peter Clausen, Rob Herring, Linus Walleij, Peter Meerwald,
	linux-iio, devicetree, Bastien Nocera, Laurentiu Palcu,
	Andy Shevchenko

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(-)
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/6] iio: accel: bmc150: Drop misleading/duplicate chip identifiers
  2021-06-10  9:52 ` [PATCH 1/6] iio: accel: bmc150: Drop misleading/duplicate chip identifiers Stephan Gerhold
@ 2021-06-10 10:24   ` Andy Shevchenko
  0 siblings, 0 replies; 13+ messages in thread
From: Andy Shevchenko @ 2021-06-10 10:24 UTC (permalink / raw)
  To: Stephan Gerhold
  Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Linus Walleij,
	Peter Meerwald, linux-iio, devicetree, Bastien Nocera,
	Laurentiu Palcu, Hans de Goede

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.

>         {}
>  };

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/6] iio: accel: bmc150: Add support for BMA253/BMA254
  2021-06-10  9:52 [PATCH 0/6] iio: accel: bmc150: Add support for BMA253/BMA254 Stephan Gerhold
                   ` (6 preceding siblings ...)
  2021-06-10 10:17 ` [PATCH 0/6] iio: accel: bmc150: Add support for BMA253/BMA254 Hans de Goede
@ 2021-06-10 10:29 ` Andy Shevchenko
  2021-06-10 10:47   ` Stephan Gerhold
  7 siblings, 1 reply; 13+ messages in thread
From: Andy Shevchenko @ 2021-06-10 10:29 UTC (permalink / raw)
  To: Stephan Gerhold
  Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Linus Walleij,
	Peter Meerwald, linux-iio, devicetree, Bastien Nocera,
	Hans de Goede

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
>


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/6] iio: accel: bmc150: Add support for BMA253/BMA254
  2021-06-10 10:29 ` Andy Shevchenko
@ 2021-06-10 10:47   ` Stephan Gerhold
  2021-06-10 10:51     ` Andy Shevchenko
  0 siblings, 1 reply; 13+ messages in thread
From: Stephan Gerhold @ 2021-06-10 10:47 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Linus Walleij,
	Peter Meerwald, linux-iio, devicetree, Bastien Nocera,
	Hans de Goede

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/6] iio: accel: bmc150: Add support for BMA253/BMA254
  2021-06-10 10:47   ` Stephan Gerhold
@ 2021-06-10 10:51     ` Andy Shevchenko
  2021-06-10 11:25       ` Stephan Gerhold
  0 siblings, 1 reply; 13+ messages in thread
From: Andy Shevchenko @ 2021-06-10 10:51 UTC (permalink / raw)
  To: Stephan Gerhold
  Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Linus Walleij,
	Peter Meerwald, linux-iio, devicetree, Bastien Nocera,
	Hans de Goede

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.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/6] iio: accel: bmc150: Add support for BMA253/BMA254
  2021-06-10 10:51     ` Andy Shevchenko
@ 2021-06-10 11:25       ` Stephan Gerhold
  0 siblings, 0 replies; 13+ messages in thread
From: Stephan Gerhold @ 2021-06-10 11:25 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Linus Walleij,
	Peter Meerwald, linux-iio, devicetree, Bastien Nocera,
	Hans de Goede

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2021-06-10 11:25 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-10  9:52 [PATCH 0/6] iio: accel: bmc150: Add support for BMA253/BMA254 Stephan Gerhold
2021-06-10  9:52 ` [PATCH 1/6] iio: accel: bmc150: Drop misleading/duplicate chip identifiers Stephan Gerhold
2021-06-10 10:24   ` Andy Shevchenko
2021-06-10  9:52 ` [PATCH 2/6] dt-bindings: iio: accel: bma255: Document bosch,bma253 Stephan Gerhold
2021-06-10  9:52 ` [PATCH 3/6] iio: accel: bmc150: Add device IDs for BMA253 Stephan Gerhold
2021-06-10  9:52 ` [PATCH 4/6] dt-bindings: iio: bma255: Allow multiple interrupts Stephan Gerhold
2021-06-10  9:52 ` [PATCH 5/6] dt-bindings: iio: accel: bma180/bma255: Move bma254 to bma255 schema Stephan Gerhold
2021-06-10  9:53 ` [PATCH 6/6] iio: accel: bma180/bmc150: Move BMA254 to bmc150-accel driver Stephan Gerhold
2021-06-10 10:17 ` [PATCH 0/6] iio: accel: bmc150: Add support for BMA253/BMA254 Hans de Goede
2021-06-10 10:29 ` Andy Shevchenko
2021-06-10 10:47   ` Stephan Gerhold
2021-06-10 10:51     ` Andy Shevchenko
2021-06-10 11:25       ` Stephan Gerhold

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.