[PATCH 0/2] iio: accel: bma180: Fix BMA25x bandwidth selection

[PATCH 0/2] iio: accel: bma180: Fix BMA25x bandwidth selection

[Stephan Gerhold]

This patch series fixes the bandwidth selection for BMA25x
in the bma180 driver. See the patches for details.

(Normally I would omit the cover letter here but Jonathan
 mentioned this makes things easier... :D)

Stephan Gerhold (2):
  iio: accel: bma180: Fix BMA25x bandwidth register values
  iio: accel: bma180: Add missing 500 Hz / 1000 Hz bandwidth

 drivers/iio/accel/bma180.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

-- 
2.31.1

[PATCH 1/2] iio: accel: bma180: Fix BMA25x bandwidth register values

[Stephan Gerhold]

According to the BMA253 datasheet [1] and BMA250 datasheet [2] the
bandwidth value for BMA25x should be set as 01xxx:

  "Settings 00xxx result in a bandwidth of 7.81 Hz; [...]
   It is recommended [...] to use the range from ´01000b´ to ´01111b´
   only in order to be compatible with future products."

However, at the moment the drivers sets bandwidth values from 0 to 6,
which is not recommended and always results into 7.81 Hz bandwidth
according to the datasheet.

Fix this by introducing a bw_offset = 8 = 01000b for BMA25x,
so the additional bit is always set for BMA25x.

[1]: https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bma253-ds000.pdf
[2]: https://datasheet.octopart.com/BMA250-Bosch-datasheet-15540103.pdf

Cc: Peter Meerwald <pmeerw@pmeerw.net>
Cc: Linus Walleij <linus.walleij@linaro.org>
Fixes: 2017cff24cc0 ("iio:bma180: Add BMA250 chip support")
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
After spending some more time with the driver I noticed that BMA253/BMA254
would actually be better supported by the bmc150-accel driver (which
doesn't have this problem and also supports the motion trigger/interrupt).

I'm preparing a separate patch set to move BMA254 to bmc150-accel at least,
but I'm not sure about BMA250 so this patch is relevant anyway. And I had
it almost done already, so I thought it's better to send it. :)
---
 drivers/iio/accel/bma180.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/accel/bma180.c b/drivers/iio/accel/bma180.c
index 97e991581960..81d1e070a2ee 100644
--- a/drivers/iio/accel/bma180.c
+++ b/drivers/iio/accel/bma180.c
@@ -55,7 +55,7 @@ struct bma180_part_info {
 
 	u8 int_reset_reg, int_reset_mask;
 	u8 sleep_reg, sleep_mask;
-	u8 bw_reg, bw_mask;
+	u8 bw_reg, bw_mask, bw_offset;
 	u8 scale_reg, scale_mask;
 	u8 power_reg, power_mask, lowpower_val;
 	u8 int_enable_reg, int_enable_mask;
@@ -127,6 +127,7 @@ struct bma180_part_info {
 
 #define BMA250_RANGE_MASK	GENMASK(3, 0) /* Range of accel values */
 #define BMA250_BW_MASK		GENMASK(4, 0) /* Accel bandwidth */
+#define BMA250_BW_OFFSET	8
 #define BMA250_SUSPEND_MASK	BIT(7) /* chip will sleep */
 #define BMA250_LOWPOWER_MASK	BIT(6)
 #define BMA250_DATA_INTEN_MASK	BIT(4)
@@ -143,6 +144,7 @@ struct bma180_part_info {
 
 #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)
@@ -287,7 +289,8 @@ static int bma180_set_bw(struct bma180_data *data, int val)
 	for (i = 0; i < data->part_info->num_bw; ++i) {
 		if (data->part_info->bw_table[i] == val) {
 			ret = bma180_set_bits(data, data->part_info->bw_reg,
-				data->part_info->bw_mask, i);
+				data->part_info->bw_mask,
+				i + data->part_info->bw_offset);
 			if (ret) {
 				dev_err(&data->client->dev,
 					"failed to set bandwidth\n");
@@ -880,6 +883,7 @@ static const struct bma180_part_info bma180_part_info[] = {
 		.sleep_mask = BMA250_SUSPEND_MASK,
 		.bw_reg = BMA250_BW_REG,
 		.bw_mask = BMA250_BW_MASK,
+		.bw_offset = BMA250_BW_OFFSET,
 		.scale_reg = BMA250_RANGE_REG,
 		.scale_mask = BMA250_RANGE_MASK,
 		.power_reg = BMA250_POWER_REG,
@@ -909,6 +913,7 @@ static const struct bma180_part_info bma180_part_info[] = {
 		.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,
-- 
2.31.1

Re: [PATCH 1/2] iio: accel: bma180: Fix BMA25x bandwidth register values

[Linus Walleij]

On Wed, May 26, 2021 at 11:44 AM Stephan Gerhold <stephan@gerhold.net> wrote:

> According to the BMA253 datasheet [1] and BMA250 datasheet [2] the
> bandwidth value for BMA25x should be set as 01xxx:
>
>   "Settings 00xxx result in a bandwidth of 7.81 Hz; [...]
>    It is recommended [...] to use the range from ´01000b´ to ´01111b´
>    only in order to be compatible with future products."
>
> However, at the moment the drivers sets bandwidth values from 0 to 6,
> which is not recommended and always results into 7.81 Hz bandwidth
> according to the datasheet.
>
> Fix this by introducing a bw_offset = 8 = 01000b for BMA25x,
> so the additional bit is always set for BMA25x.
>
> [1]: https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bma253-ds000.pdf
> [2]: https://datasheet.octopart.com/BMA250-Bosch-datasheet-15540103.pdf
>
> Cc: Peter Meerwald <pmeerw@pmeerw.net>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Fixes: 2017cff24cc0 ("iio:bma180: Add BMA250 chip support")
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>

Good catch!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

> After spending some more time with the driver I noticed that BMA253/BMA254
> would actually be better supported by the bmc150-accel driver (which
> doesn't have this problem and also supports the motion trigger/interrupt).
>
> I'm preparing a separate patch set to move BMA254 to bmc150-accel at least,
> but I'm not sure about BMA250 so this patch is relevant anyway. And I had
> it almost done already, so I thought it's better to send it. :)

I looked at the preliminary patches and this looks really good
too.

Yours,
Linus Walleij

Re: [PATCH 1/2] iio: accel: bma180: Fix BMA25x bandwidth register values

[Jonathan Cameron]

On Wed, 26 May 2021 14:12:02 +0200
Linus Walleij <linus.walleij@linaro.org> wrote:

> On Wed, May 26, 2021 at 11:44 AM Stephan Gerhold <stephan@gerhold.net> wrote:
> 
> > According to the BMA253 datasheet [1] and BMA250 datasheet [2] the
> > bandwidth value for BMA25x should be set as 01xxx:
> >
> >   "Settings 00xxx result in a bandwidth of 7.81 Hz; [...]
> >    It is recommended [...] to use the range from ´01000b´ to ´01111b´
> >    only in order to be compatible with future products."
> >
> > However, at the moment the drivers sets bandwidth values from 0 to 6,
> > which is not recommended and always results into 7.81 Hz bandwidth
> > according to the datasheet.
> >
> > Fix this by introducing a bw_offset = 8 = 01000b for BMA25x,
> > so the additional bit is always set for BMA25x.
> >
> > [1]: https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bma253-ds000.pdf
> > [2]: https://datasheet.octopart.com/BMA250-Bosch-datasheet-15540103.pdf
> >
> > Cc: Peter Meerwald <pmeerw@pmeerw.net>
> > Cc: Linus Walleij <linus.walleij@linaro.org>
> > Fixes: 2017cff24cc0 ("iio:bma180: Add BMA250 chip support")
> > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>  
> 
> Good catch!
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Applied to the fixes-togreg branch of iio.git and marked for stable.

I think I can probably get away with picking up patch 2 into the togreg branch
without anything blowing up during the merge.

Thanks,

Jonathan

> 
> > After spending some more time with the driver I noticed that BMA253/BMA254
> > would actually be better supported by the bmc150-accel driver (which
> > doesn't have this problem and also supports the motion trigger/interrupt).
> >
> > I'm preparing a separate patch set to move BMA254 to bmc150-accel at least,
> > but I'm not sure about BMA250 so this patch is relevant anyway. And I had
> > it almost done already, so I thought it's better to send it. :)  
> 
> I looked at the preliminary patches and this looks really good
> too.
> 
> Yours,
> Linus Walleij

Re: [PATCH 1/2] iio: accel: bma180: Fix BMA25x bandwidth register values

[Jonathan Cameron]

On Thu, 3 Jun 2021 18:12:08 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Wed, 26 May 2021 14:12:02 +0200
> Linus Walleij <linus.walleij@linaro.org> wrote:
> 
> > On Wed, May 26, 2021 at 11:44 AM Stephan Gerhold <stephan@gerhold.net> wrote:
> >   
> > > According to the BMA253 datasheet [1] and BMA250 datasheet [2] the
> > > bandwidth value for BMA25x should be set as 01xxx:
> > >
> > >   "Settings 00xxx result in a bandwidth of 7.81 Hz; [...]
> > >    It is recommended [...] to use the range from ´01000b´ to ´01111b´
> > >    only in order to be compatible with future products."
> > >
> > > However, at the moment the drivers sets bandwidth values from 0 to 6,
> > > which is not recommended and always results into 7.81 Hz bandwidth
> > > according to the datasheet.
> > >
> > > Fix this by introducing a bw_offset = 8 = 01000b for BMA25x,
> > > so the additional bit is always set for BMA25x.
> > >
> > > [1]: https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bma253-ds000.pdf
> > > [2]: https://datasheet.octopart.com/BMA250-Bosch-datasheet-15540103.pdf
> > >
> > > Cc: Peter Meerwald <pmeerw@pmeerw.net>
> > > Cc: Linus Walleij <linus.walleij@linaro.org>
> > > Fixes: 2017cff24cc0 ("iio:bma180: Add BMA250 chip support")
> > > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>    
> > 
> > Good catch!
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>  
> 
> Applied to the fixes-togreg branch of iio.git and marked for stable.
> 
> I think I can probably get away with picking up patch 2 into the togreg branch
> without anything blowing up during the merge.

Change of plan here given dependency on this of another set and how late in
the cycle we are. I've dragged this across from the fixes-togreg branch to the
togreg branch lining it up for the next merge window.


> 
> Thanks,
> 
> Jonathan
> 
> >   
> > > After spending some more time with the driver I noticed that BMA253/BMA254
> > > would actually be better supported by the bmc150-accel driver (which
> > > doesn't have this problem and also supports the motion trigger/interrupt).
> > >
> > > I'm preparing a separate patch set to move BMA254 to bmc150-accel at least,
> > > but I'm not sure about BMA250 so this patch is relevant anyway. And I had
> > > it almost done already, so I thought it's better to send it. :)    
> > 
> > I looked at the preliminary patches and this looks really good
> > too.
> > 
> > Yours,
> > Linus Walleij  
> 

[PATCH 2/2] iio: accel: bma180: Add missing 500 Hz / 1000 Hz bandwidth

[Stephan Gerhold]

According to the BMA253 datasheet [1] and BMA250 datasheet [2] BMA25x
also supports a bandwidth of 500 Hz and 1000 Hz but this was not listed
in the driver for some reason.

Add it to the bw_table to make the driver match the datasheet.

[1]: https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bma253-ds000.pdf
[2]: https://datasheet.octopart.com/BMA250-Bosch-datasheet-15540103.pdf

Cc: Peter Meerwald <pmeerw@pmeerw.net>
Cc: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
 drivers/iio/accel/bma180.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/accel/bma180.c b/drivers/iio/accel/bma180.c
index 81d1e070a2ee..e7c6b3096cb7 100644
--- a/drivers/iio/accel/bma180.c
+++ b/drivers/iio/accel/bma180.c
@@ -184,7 +184,7 @@ 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 }; /* Hz */
+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,
 	0, 0, 306458 };
 
-- 
2.31.1

Re: [PATCH 2/2] iio: accel: bma180: Add missing 500 Hz / 1000 Hz bandwidth

[Linus Walleij]

On Wed, May 26, 2021 at 11:44 AM Stephan Gerhold <stephan@gerhold.net> wrote:

> According to the BMA253 datasheet [1] and BMA250 datasheet [2] BMA25x
> also supports a bandwidth of 500 Hz and 1000 Hz but this was not listed
> in the driver for some reason.
>
> Add it to the bw_table to make the driver match the datasheet.
>
> [1]: https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bma253-ds000.pdf
> [2]: https://datasheet.octopart.com/BMA250-Bosch-datasheet-15540103.pdf
>
> Cc: Peter Meerwald <pmeerw@pmeerw.net>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 2/2] iio: accel: bma180: Add missing 500 Hz / 1000 Hz bandwidth

[Jonathan Cameron]

On Wed, 26 May 2021 14:12:44 +0200
Linus Walleij <linus.walleij@linaro.org> wrote:

> On Wed, May 26, 2021 at 11:44 AM Stephan Gerhold <stephan@gerhold.net> wrote:
> 
> > According to the BMA253 datasheet [1] and BMA250 datasheet [2] BMA25x
> > also supports a bandwidth of 500 Hz and 1000 Hz but this was not listed
> > in the driver for some reason.
> >
> > Add it to the bw_table to make the driver match the datasheet.
> >
> > [1]: https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bma253-ds000.pdf
> > [2]: https://datasheet.octopart.com/BMA250-Bosch-datasheet-15540103.pdf
> >
> > Cc: Peter Meerwald <pmeerw@pmeerw.net>
> > Cc: Linus Walleij <linus.walleij@linaro.org>
> > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>  
> 
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Applied to the togreg branch of iio.git and pushed out as testing for 0-day
and any friends to play with.

Thanks,

Jonathan

> 
> Yours,
> Linus Walleij