1. Patchew
  2. linux
  3. [PATCH v2 00/14] iio: accel: bma220 improvements
  4. View patch

[PATCH v2 07/14] iio: accel: bma220: reset registers during init stage

Petre Rodan posted 14 patches 4 months, 4 weeks ago
There is a newer version of this series
[PATCH v2 07/14] iio: accel: bma220: reset registers during init stage
Posted by Petre Rodan 4 months, 4 weeks ago 
Bring all configuration registers to default values during device probe().

Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
---
 drivers/iio/accel/bma220_core.c | 71 ++++++++++++++++++++++++++++-------------
 1 file changed, 49 insertions(+), 22 deletions(-)

diff --git a/drivers/iio/accel/bma220_core.c b/drivers/iio/accel/bma220_core.c
index b6f1374a9cca52966c1055113710061a7284cf5a..322df516c90a7c645eeca579cae9803eb31caad1 100644
--- a/drivers/iio/accel/bma220_core.c
+++ b/drivers/iio/accel/bma220_core.c
@@ -29,12 +29,15 @@
 #define BMA220_REG_ACCEL_Z			0x04
 #define BMA220_REG_RANGE			0x11
 #define BMA220_REG_SUSPEND			0x18
+#define BMA220_REG_SOFTRESET			0x19
 
 #define BMA220_CHIP_ID				0xDD
 #define BMA220_READ_MASK			BIT(7)
 #define BMA220_RANGE_MASK			GENMASK(1, 0)
 #define BMA220_SUSPEND_SLEEP			0xFF
 #define BMA220_SUSPEND_WAKE			0x00
+#define BMA220_RESET_MODE			0xFF
+#define BMA220_NONRESET_MODE			0x00
 
 #define BMA220_DEVICE_NAME			"bma220"
 
@@ -203,31 +206,28 @@ static const struct iio_info bma220_info = {
 	.read_avail		= bma220_read_avail,
 };
 
-static int bma220_init(struct spi_device *spi)
+static int bma220_reset(struct spi_device *spi, bool up)
 {
-	int ret;
-	static const char * const regulator_names[] = { "vddd", "vddio", "vdda" };
+	int i, ret;
 
-	ret = devm_regulator_bulk_get_enable(&spi->dev,
-					     ARRAY_SIZE(regulator_names),
-					     regulator_names);
-	if (ret)
-		return dev_err_probe(&spi->dev, ret, "Failed to get regulators\n");
+	/**
+	 * The chip can be reset by a simple register read.
+	 * We need up to 2 register reads of the softreset register
+	 * to make sure that the device is in the desired state.
+	 */
+	for (i = 0; i < 2; i++) {
+		ret = bma220_read_reg(spi, BMA220_REG_SOFTRESET);
+		if (ret < 0)
+			return ret;
 
-	ret = bma220_read_reg(spi, BMA220_REG_ID);
-	if (ret != BMA220_CHIP_ID)
-		return -ENODEV;
+		if (up && (ret == BMA220_RESET_MODE))
+			return 0;
 
-	/* Make sure the chip is powered on */
-	ret = bma220_read_reg(spi, BMA220_REG_SUSPEND);
-	if (ret == BMA220_SUSPEND_WAKE)
-		ret = bma220_read_reg(spi, BMA220_REG_SUSPEND);
-	if (ret < 0)
-		return ret;
-	if (ret == BMA220_SUSPEND_WAKE)
-		return -EBUSY;
+		if (!up && (ret == BMA220_NONRESET_MODE))
+			return 0;
+	}
 
-	return 0;
+	return -EBUSY;
 }
 
 static int bma220_power(struct spi_device *spi, bool up)
@@ -244,16 +244,43 @@ static int bma220_power(struct spi_device *spi, bool up)
 		if (ret < 0)
 			return ret;
 
-		if (up && ret == BMA220_SUSPEND_SLEEP)
+		if (up && (ret == BMA220_SUSPEND_SLEEP))
 			return 0;
 
-		if (!up && ret == BMA220_SUSPEND_WAKE)
+		if (!up && (ret == BMA220_SUSPEND_WAKE))
 			return 0;
 	}
 
 	return -EBUSY;
 }
 
+static int bma220_init(struct spi_device *spi)
+{
+	int ret;
+	static const char * const regulator_names[] = { "vddd", "vddio", "vdda" };
+
+	ret = devm_regulator_bulk_get_enable(&spi->dev,
+					     ARRAY_SIZE(regulator_names),
+					     regulator_names);
+	if (ret)
+		return dev_err_probe(&spi->dev, ret, "Failed to get regulators\n");
+
+	ret = bma220_read_reg(spi, BMA220_REG_ID);
+	if (ret != BMA220_CHIP_ID)
+		return -ENODEV;
+
+	/* Make sure the chip is powered on and config registers are reset */
+	ret = bma220_power(spi, true);
+	if (ret)
+		return dev_err_probe(&spi->dev, ret, "Failed to power-on chip\n");
+
+	ret = bma220_reset(spi, true);
+	if (ret)
+		return dev_err_probe(&spi->dev, ret, "Failed to soft reset chip\n");
+
+	return 0;
+}
+
 static void bma220_deinit(void *spi)
 {
 	bma220_power(spi, false);

-- 
2.49.1
Re: [PATCH v2 07/14] iio: accel: bma220: reset registers during init stage
Posted by David Lechner 4 months, 4 weeks ago 
On 9/10/25 2:57 AM, Petre Rodan wrote:
> Bring all configuration registers to default values during device probe().
> 
> Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> ---

...

>  static int bma220_power(struct spi_device *spi, bool up)
> @@ -244,16 +244,43 @@ static int bma220_power(struct spi_device *spi, bool up)
>  		if (ret < 0)
>  			return ret;
>  
> -		if (up && ret == BMA220_SUSPEND_SLEEP)
> +		if (up && (ret == BMA220_SUSPEND_SLEEP))

Over 80% of existing kernel code doesn't have the unnecessary ()
on similar expressions so I think we should leave it that way.

(just seems like adding noise to me)

>  			return 0;
>  
> -		if (!up && ret == BMA220_SUSPEND_WAKE)
> +		if (!up && (ret == BMA220_SUSPEND_WAKE))
>  			return 0;
>  	}
>  
>  	return -EBUSY;
>  }
>
Re: [PATCH v2 07/14] iio: accel: bma220: reset registers during init stage
Posted by Krzysztof Kozlowski 4 months, 4 weeks ago 
On 10/09/2025 09:57, Petre Rodan wrote:
> Bring all configuration registers to default values during device probe().
> 
> Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> ---
>  drivers/iio/accel/bma220_core.c | 71 ++++++++++++++++++++++++++++-------------
>  1 file changed, 49 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/iio/accel/bma220_core.c b/drivers/iio/accel/bma220_core.c
> index b6f1374a9cca52966c1055113710061a7284cf5a..322df516c90a7c645eeca579cae9803eb31caad1 100644
> --- a/drivers/iio/accel/bma220_core.c
> +++ b/drivers/iio/accel/bma220_core.c
> @@ -29,12 +29,15 @@
>  #define BMA220_REG_ACCEL_Z			0x04
>  #define BMA220_REG_RANGE			0x11
>  #define BMA220_REG_SUSPEND			0x18
> +#define BMA220_REG_SOFTRESET			0x19
>  
>  #define BMA220_CHIP_ID				0xDD
>  #define BMA220_READ_MASK			BIT(7)
>  #define BMA220_RANGE_MASK			GENMASK(1, 0)
>  #define BMA220_SUSPEND_SLEEP			0xFF
>  #define BMA220_SUSPEND_WAKE			0x00
> +#define BMA220_RESET_MODE			0xFF
> +#define BMA220_NONRESET_MODE			0x00
>  
>  #define BMA220_DEVICE_NAME			"bma220"
>  
> @@ -203,31 +206,28 @@ static const struct iio_info bma220_info = {
>  	.read_avail		= bma220_read_avail,
>  };
>  
> -static int bma220_init(struct spi_device *spi)
> +static int bma220_reset(struct spi_device *spi, bool up)
>  {
> -	int ret;
> -	static const char * const regulator_names[] = { "vddd", "vddio", "vdda" };
> +	int i, ret;
>  
> -	ret = devm_regulator_bulk_get_enable(&spi->dev,


You just added this code in patch 6. Don't add code which immediately
you remove. I understand you re-add this later, so basically it is a
move, but such patch diff is still confusing.


Best regards,
Krzysztof
Re: [PATCH v2 07/14] iio: accel: bma220: reset registers during init stage
Posted by Petre Rodan 4 months, 4 weeks ago 
Hi Krzysztof,

On Thu, Sep 11, 2025 at 09:35:52AM +0200, Krzysztof Kozlowski wrote:
> On 10/09/2025 09:57, Petre Rodan wrote:
> > Bring all configuration registers to default values during device probe().
> > 
> > Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> > ---
> >  drivers/iio/accel/bma220_core.c | 71 ++++++++++++++++++++++++++++-------------
> >  1 file changed, 49 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/iio/accel/bma220_core.c b/drivers/iio/accel/bma220_core.c
> > index b6f1374a9cca52966c1055113710061a7284cf5a..322df516c90a7c645eeca579cae9803eb31caad1 100644
> > --- a/drivers/iio/accel/bma220_core.c
> > -static int bma220_init(struct spi_device *spi)
> > +static int bma220_reset(struct spi_device *spi, bool up)
> >  {
> > -	int ret;
> > -	static const char * const regulator_names[] = { "vddd", "vddio", "vdda" };
> > +	int i, ret;
> >  
> > -	ret = devm_regulator_bulk_get_enable(&spi->dev,
> 
> 
> You just added this code in patch 6. Don't add code which immediately
> you remove. I understand you re-add this later, so basically it is a
> move, but such patch diff is still confusing.

sorry, but this is an artefact of 'git diff' I don't think I have no control of.

the bma220_reset() function was added to bma220_core.c with this patch and the
diff process merged lines from this new function with lines from bma220_init()
causing the apparent removal of the lines added in the previous patch.
if you look a few lines below your cut, the bma220_init() function contains the
code:

+static int bma220_init(struct spi_device *spi)
+{
+	int ret;
+	static const char * const regulator_names[] = { "vddd", "vddio", "vdda" };
+
+	ret = devm_regulator_bulk_get_enable(&spi->dev,
+					     ARRAY_SIZE(regulator_names),
+					     regulator_names);
+	if (ret)
+		return dev_err_probe(&spi->dev, ret, "Failed to get regulators\n");
[..]

Just for my curiosity, do reviewers apply the patches one by one to (a branch of)
the tree itself or do they provide feedback directly based on the diffs?

best regards,
peter
Re: [PATCH v2 07/14] iio: accel: bma220: reset registers during init stage
Posted by David Lechner 4 months, 4 weeks ago 
On 9/11/25 7:36 AM, Petre Rodan wrote:
> 

...

> Just for my curiosity, do reviewers apply the patches one by one to (a branch of)
> the tree itself or do they provide feedback directly based on the diffs?
> 

I think most reviewers are like me and only are looking at the diff in the
email. It would take too much time to apply every single patch I look at. So
I only do that in very rare cases when I have a special interest in a patch.
Re: [PATCH v2 07/14] iio: accel: bma220: reset registers during init stage
Posted by Jonathan Cameron 4 months, 3 weeks ago 
On Thu, 11 Sep 2025 08:44:16 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 9/11/25 7:36 AM, Petre Rodan wrote:
> >   
> 
> ...
> 
> > Just for my curiosity, do reviewers apply the patches one by one to (a branch of)
> > the tree itself or do they provide feedback directly based on the diffs?
> >   
> 
> I think most reviewers are like me and only are looking at the diff in the
> email. It would take too much time to apply every single patch I look at. So
> I only do that in very rare cases when I have a special interest in a patch.

Likewise.

Jonathan
Re: [PATCH v2 07/14] iio: accel: bma220: reset registers during init stage
Posted by Krzysztof Kozlowski 4 months, 4 weeks ago 
On 11/09/2025 14:36, Petre Rodan wrote:
> 
> Hi Krzysztof,
> 
> On Thu, Sep 11, 2025 at 09:35:52AM +0200, Krzysztof Kozlowski wrote:
>> On 10/09/2025 09:57, Petre Rodan wrote:
>>> Bring all configuration registers to default values during device probe().
>>>
>>> Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
>>> ---
>>>  drivers/iio/accel/bma220_core.c | 71 ++++++++++++++++++++++++++++-------------
>>>  1 file changed, 49 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/drivers/iio/accel/bma220_core.c b/drivers/iio/accel/bma220_core.c
>>> index b6f1374a9cca52966c1055113710061a7284cf5a..322df516c90a7c645eeca579cae9803eb31caad1 100644
>>> --- a/drivers/iio/accel/bma220_core.c
>>> -static int bma220_init(struct spi_device *spi)
>>> +static int bma220_reset(struct spi_device *spi, bool up)
>>>  {
>>> -	int ret;
>>> -	static const char * const regulator_names[] = { "vddd", "vddio", "vdda" };
>>> +	int i, ret;
>>>  
>>> -	ret = devm_regulator_bulk_get_enable(&spi->dev,
>>
>>
>> You just added this code in patch 6. Don't add code which immediately
>> you remove. I understand you re-add this later, so basically it is a
>> move, but such patch diff is still confusing.
> 
> sorry, but this is an artefact of 'git diff' I don't think I have no control of.


Don't think so. Before bma220_init() was above bma220_power(). After
your patch bma220_init() is BELOW bma220_power(), so that's a move.



> 
> the bma220_reset() function was added to bma220_core.c with this patch and the
> diff process merged lines from this new function with lines from bma220_init()
> causing the apparent removal of the lines added in the previous patch.
> if you look a few lines below your cut, the bma220_init() function contains the
> code:
> 
> +static int bma220_init(struct spi_device *spi)
> +{
> +	int ret;
> +	static const char * const regulator_names[] = { "vddd", "vddio", "vdda" };
> +
> +	ret = devm_regulator_bulk_get_enable(&spi->dev,
> +					     ARRAY_SIZE(regulator_names),
> +					     regulator_names);
> +	if (ret)
> +		return dev_err_probe(&spi->dev, ret, "Failed to get regulators\n");
> [..]
> 
> Just for my curiosity, do reviewers apply the patches one by one to (a branch of)
> the tree itself or do they provide feedback directly based on the diffs?


Each commit must stand on its own, is reviewed independently and entire
patchset must be 100% bisectable.


Best regards,
Krzysztof
Re: [PATCH v2 07/14] iio: accel: bma220: reset registers during init stage
Posted by Petre Rodan 4 months, 4 weeks ago 
Hello.

On Thu, Sep 11, 2025 at 03:07:05PM +0200, Krzysztof Kozlowski wrote:
> On 11/09/2025 14:36, Petre Rodan wrote:
> > 
> > Hi Krzysztof,
> > 
> > On Thu, Sep 11, 2025 at 09:35:52AM +0200, Krzysztof Kozlowski wrote:
> >> On 10/09/2025 09:57, Petre Rodan wrote:
> >>> Bring all configuration registers to default values during device probe().
> >>>
> >>> Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> >>> ---
> >>>  drivers/iio/accel/bma220_core.c | 71 ++++++++++++++++++++++++++++-------------
> >>>  1 file changed, 49 insertions(+), 22 deletions(-)
> >>>
> >>> diff --git a/drivers/iio/accel/bma220_core.c b/drivers/iio/accel/bma220_core.c
> >>> index b6f1374a9cca52966c1055113710061a7284cf5a..322df516c90a7c645eeca579cae9803eb31caad1 100644
> >>> --- a/drivers/iio/accel/bma220_core.c
> >>> -static int bma220_init(struct spi_device *spi)
> >>> +static int bma220_reset(struct spi_device *spi, bool up)
> >>>  {
> >>> -	int ret;
> >>> -	static const char * const regulator_names[] = { "vddd", "vddio", "vdda" };
> >>> +	int i, ret;
> >>>  
> >>> -	ret = devm_regulator_bulk_get_enable(&spi->dev,
> >>
> >>
> >> You just added this code in patch 6. Don't add code which immediately
> >> you remove. I understand you re-add this later, so basically it is a
> >> move, but such patch diff is still confusing.
> > 
> > sorry, but this is an artefact of 'git diff' I don't think I have no control of.
> 
> 
> Don't think so. Before bma220_init() was above bma220_power(). After
> your patch bma220_init() is BELOW bma220_power(), so that's a move.

you are correct, these two functions did change places due to the fact that
_init() started using _power(). I preffered to do the move instead
of adding a forward declaration and leaving _power() between _init() and _deinit().
the code was optimized for how it will look at the end of all this patching.

I thought you ment the code that was added the previous patch was not removed per
se from _init(), which was not the case.

best regards,
peter
Re: [PATCH v2 07/14] iio: accel: bma220: reset registers during init stage
Posted by Andy Shevchenko 4 months, 4 weeks ago 
On Thu, Sep 11, 2025 at 4:52 PM Petre Rodan <petre.rodan@subdimension.ro> wrote:
> On Thu, Sep 11, 2025 at 03:07:05PM +0200, Krzysztof Kozlowski wrote:
> > On 11/09/2025 14:36, Petre Rodan wrote:
> > > On Thu, Sep 11, 2025 at 09:35:52AM +0200, Krzysztof Kozlowski wrote:
> > >> On 10/09/2025 09:57, Petre Rodan wrote:

...

> > >> You just added this code in patch 6. Don't add code which immediately
> > >> you remove. I understand you re-add this later, so basically it is a
> > >> move, but such patch diff is still confusing.
> > >
> > > sorry, but this is an artefact of 'git diff' I don't think I have no control of.
> >
> >
> > Don't think so. Before bma220_init() was above bma220_power(). After
> > your patch bma220_init() is BELOW bma220_power(), so that's a move.
>
> you are correct, these two functions did change places due to the fact that
> _init() started using _power(). I preffered to do the move instead
> of adding a forward declaration and leaving _power() between _init() and _deinit().
> the code was optimized for how it will look at the end of all this patching.

The idea is to balance between two, but for certain I agree with
Krzysztof, we need to avoid "ping-pong"ing the code in the same
series. If you need to move, create a no change patch that _just
moves_ one function up in the code.

> I thought you ment the code that was added the previous patch was not removed per
> se from _init(), which was not the case.



-- 
With Best Regards,
Andy Shevchenko
Re: [PATCH v2 07/14] iio: accel: bma220: reset registers during init stage
Posted by Jonathan Cameron 4 months, 4 weeks ago 
On Wed, 10 Sep 2025 10:57:12 +0300
Petre Rodan <petre.rodan@subdimension.ro> wrote:

> Bring all configuration registers to default values during device probe().
> 
> Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>

Hi Petre

One comment on code you happened to be moving.  The patch itself looks good
to me so this is a 'whilst we are here' type comment, though should be addressed
in a separate patch.

thanks,

Jonathan

> +static int bma220_init(struct spi_device *spi)
> +{
> +	int ret;
> +	static const char * const regulator_names[] = { "vddd", "vddio", "vdda" };
> +
> +	ret = devm_regulator_bulk_get_enable(&spi->dev,
> +					     ARRAY_SIZE(regulator_names),
> +					     regulator_names);
> +	if (ret)
> +		return dev_err_probe(&spi->dev, ret, "Failed to get regulators\n");
> +
> +	ret = bma220_read_reg(spi, BMA220_REG_ID);
> +	if (ret != BMA220_CHIP_ID)
> +		return -ENODEV;

Not relevant to this patch as such, but we should relax this constraint so that
fallback compatibles work in device tree.  Convention is to just print a
dev_info() message if we get a wrong ID and carry on.

> +
> +	/* Make sure the chip is powered on and config registers are reset */
> +	ret = bma220_power(spi, true);
> +	if (ret)
> +		return dev_err_probe(&spi->dev, ret, "Failed to power-on chip\n");
> +
> +	ret = bma220_reset(spi, true);
> +	if (ret)
> +		return dev_err_probe(&spi->dev, ret, "Failed to soft reset chip\n");
> +
> +	return 0;
> +}

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.