- fix title typo
 - add optional watchdog setting that recovers the sensor from a stuck-low
 SDA condition
 - set correct SPI phase and polarity
 - interrupt on rising edge. the level-based interrupt that is being
 replaced was not actually implemented in the driver.

This set of changes should not negatively affect existing users.

Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
---
 .../bindings/iio/accel/bosch,bma220.yaml      | 20 +++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/accel/bosch,bma220.yaml b/Documentation/devicetree/bindings/iio/accel/bosch,bma220.yaml
index ec643de031a3..f71b2320b010 100644
--- a/Documentation/devicetree/bindings/iio/accel/bosch,bma220.yaml
+++ b/Documentation/devicetree/bindings/iio/accel/bosch,bma220.yaml
@@ -4,7 +4,7 @@
 $id: http://devicetree.org/schemas/iio/accel/bosch,bma220.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#

-title: Bosch BMA220 Trixial Acceleration Sensor
+title: Bosch BMA220 Triaxial Acceleration Sensor

 maintainers:
   - Jonathan Cameron <Jonathan.Cameron@huawei.com>
@@ -20,6 +20,20 @@ properties:
   interrupts:
     maxItems: 1

+  bosch,watchdog:
+    description:
+      In order to prevent the built-in I2C slave to lock-up the I2C bus, a
+      watchdog timer is introduced. The WDT observes internal I2C signals and
+      resets the I2C interface if the bus is locked-up by the BMA220.
+      0 - off
+      1 - 1ms
+      2 - 10ms
+    enum: [0, 1, 2]
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  spi-cpha: true
+  spi-cpol: true
+
   vdda-supply: true
   vddd-supply: true
   vddio-supply: true
@@ -44,8 +58,10 @@ examples:
             compatible = "bosch,bma220";
             reg = <0>;
             spi-max-frequency = <2500000>;
+            spi-cpol;
+            spi-cpha;
             interrupt-parent = <&gpio0>;
-            interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
+            interrupts = <0 IRQ_TYPE_EDGE_RISING>;
         };
     };
 ...
--
2.49.1

On 9/1/25 2:47 PM, Petre Rodan wrote:
>  - fix title typo
>  - add optional watchdog setting that recovers the sensor from a stuck-low
>  SDA condition
>  - set correct SPI phase and polarity
>  - interrupt on rising edge. the level-based interrupt that is being
>  replaced was not actually implemented in the driver.
> 
> This set of changes should not negatively affect existing users.
> 
> Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> ---
>  .../bindings/iio/accel/bosch,bma220.yaml      | 20 +++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/accel/bosch,bma220.yaml b/Documentation/devicetree/bindings/iio/accel/bosch,bma220.yaml
> index ec643de031a3..f71b2320b010 100644
> --- a/Documentation/devicetree/bindings/iio/accel/bosch,bma220.yaml
> +++ b/Documentation/devicetree/bindings/iio/accel/bosch,bma220.yaml
> @@ -4,7 +4,7 @@
>  $id: http://devicetree.org/schemas/iio/accel/bosch,bma220.yaml#
>  $schema: http://devicetree.org/meta-schemas/core.yaml#
> 
> -title: Bosch BMA220 Trixial Acceleration Sensor
> +title: Bosch BMA220 Triaxial Acceleration Sensor
> 
>  maintainers:
>    - Jonathan Cameron <Jonathan.Cameron@huawei.com>
> @@ -20,6 +20,20 @@ properties:
>    interrupts:
>      maxItems: 1
> 
> +  bosch,watchdog:
> +    description:
> +      In order to prevent the built-in I2C slave to lock-up the I2C bus, a
> +      watchdog timer is introduced. The WDT observes internal I2C signals and
> +      resets the I2C interface if the bus is locked-up by the BMA220.
> +      0 - off
> +      1 - 1ms
> +      2 - 10ms
> +    enum: [0, 1, 2]
> +    $ref: /schemas/types.yaml#/definitions/uint32

Why should this depend on how the chip is wired up? Normally, we don't have this
sort of control in devicetree. E.g. if it is useful, why shouldn't drivers just
always enable it?

If we can make the case that it belongs in the devicetree, it should use
standard units, e.g. property should be watchdog-timeout-ms with enum: [1, 10].
Maybe 0 for disabled is OK too - in that case should have default: 0.

> +
> +  spi-cpha: true
> +  spi-cpol: true
> +
>    vdda-supply: true
>    vddd-supply: true
>    vddio-supply: true
> @@ -44,8 +58,10 @@ examples:
>              compatible = "bosch,bma220";
>              reg = <0>;
>              spi-max-frequency = <2500000>;
> +            spi-cpol;
> +            spi-cpha;
>              interrupt-parent = <&gpio0>;
> -            interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
> +            interrupts = <0 IRQ_TYPE_EDGE_RISING>;
>          };
>      };
>  ...
> --
> 2.49.1
>

Good morning.

Thank you for your feedback.

On Fri, Sep 05, 2025 at 03:15:55PM -0500, David Lechner wrote:
> On 9/1/25 2:47 PM, Petre Rodan wrote:
> > diff --git a/Documentation/devicetree/bindings/iio/accel/bosch,bma220.yaml b/Documentation/devicetree/bindings/iio/accel/bosch,bma220.yaml

[..]

> > +  bosch,watchdog:
> > +    description:
> > +      In order to prevent the built-in I2C slave to lock-up the I2C bus, a
> > +      watchdog timer is introduced. The WDT observes internal I2C signals and
> > +      resets the I2C interface if the bus is locked-up by the BMA220.
> > +      0 - off
> > +      1 - 1ms
> > +      2 - 10ms
> > +    enum: [0, 1, 2]
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> 
> Why should this depend on how the chip is wired up? Normally, we don't have this
> sort of control in devicetree.

I was also unsure on how it would be best to implement the feature, bellow is my thought process.

The feature itself is definitely required for the i2c implementation of this chip. I have witnessed it pull sda low for no good reason twice over a 100h period and this would render not only the chip but the entire bus unusable until a power cycle.

I think from a driver perspective ideally WDT should be set very early - within bma220_common_probe() would be ideal.

> E.g. if it is useful, why shouldn't drivers just always enable it?

The registers holding the watchdog are all zeroed out after power on which mean it's off. I think the driver should also default on this setting. In my first implementation I had it hard-wired to 1ms, but I felt this would impose my point of view on users and it would be nicer to give them control over it.

If you guys think that the devicetree is not the place where the WDT should be set that is fine by me, would you recommend something like module_param() instead?

> If we can make the case that it belongs in the devicetree, it should use
> standard units, e.g. property should be watchdog-timeout-ms with enum: [1, 10].
> Maybe 0 for disabled is OK too - in that case should have default: 0.

Oh yes I can see it in bq256xx.yaml, to me this sounds absolutely perfect.


On a different note, from a reviewer's perspective would you prefer the next revision of this patch series to cover less ground? I was thinking about leaving everything event related for later since I might go past 15 separate patches if I split every modification into it's own separate entry.

thank you again,
peter

-- 
petre rodan

On 9/5/25 9:46 PM, Petre Rodan wrote:
> 
> Good morning.
> 
> Thank you for your feedback.
> 
> On Fri, Sep 05, 2025 at 03:15:55PM -0500, David Lechner wrote:
>> On 9/1/25 2:47 PM, Petre Rodan wrote:
>>> diff --git a/Documentation/devicetree/bindings/iio/accel/bosch,bma220.yaml b/Documentation/devicetree/bindings/iio/accel/bosch,bma220.yaml
> 
> [..]
> 
>>> +  bosch,watchdog:
>>> +    description:
>>> +      In order to prevent the built-in I2C slave to lock-up the I2C bus, a
>>> +      watchdog timer is introduced. The WDT observes internal I2C signals and
>>> +      resets the I2C interface if the bus is locked-up by the BMA220.
>>> +      0 - off
>>> +      1 - 1ms
>>> +      2 - 10ms
>>> +    enum: [0, 1, 2]
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>
>> Why should this depend on how the chip is wired up? Normally, we don't have this
>> sort of control in devicetree.
> 
> I was also unsure on how it would be best to implement the feature, bellow is my thought process.
> 
> The feature itself is definitely required for the i2c implementation of this chip. I have witnessed it pull sda low for no good reason twice over a 100h period and this would render not only the chip but the entire bus unusable until a power cycle.
> 
> I think from a driver perspective ideally WDT should be set very early - within bma220_common_probe() would be ideal.
> 
>> E.g. if it is useful, why shouldn't drivers just always enable it?
> 
> The registers holding the watchdog are all zeroed out after power on which mean it's off. I think the driver should also default on this setting. In my first implementation I had it hard-wired to 1ms, but I felt this would impose my point of view on users and it would be nicer to give them control over it.
> 
> If you guys think that the devicetree is not the place where the WDT should be set that is fine by me, would you recommend something like module_param() instead?

I wish I had a good answer, but I don't have the right kind of
experience with this sort of thing to know what works best.

We could start with just always enabling it and if we find it
actually does cause some problem for someone, then we would have
more information about that use case and could make a more
informed decision on how to handle it at that point in time.

> 
>> If we can make the case that it belongs in the devicetree, it should use
>> standard units, e.g. property should be watchdog-timeout-ms with enum: [1, 10].
>> Maybe 0 for disabled is OK too - in that case should have default: 0.
> 
> Oh yes I can see it in bq256xx.yaml, to me this sounds absolutely perfect.
> 
> 
> On a different note, from a reviewer's perspective would you prefer the next revision of this patch series to cover less ground? I was thinking about leaving everything event related for later since I might go past 15 separate patches if I split every modification into it's own separate entry.

Yes, smaller series will get more thorough reviews so I'm always
in favor of splitting things up like that.

> 
> thank you again,
> peter
>

On 01/09/2025 21:47, Petre Rodan wrote:
>  - fix title typo
>  - add optional watchdog setting that recovers the sensor from a stuck-low
>  SDA condition
>  - set correct SPI phase and polarity
>  - interrupt on rising edge. the level-based interrupt that is being

Cleanup and new features must never be mixed together.

<form letter>
Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. It might happen, that command when run on an older
kernel, gives you outdated entries. Therefore please be sure you base
your patches on recent Linux kernel.

Tools like b4 or scripts/get_maintainer.pl provide you proper list of
people, so fix your workflow. Tools might also fail if you work on some
ancient tree (don't, instead use mainline) or work on fork of kernel
(don't, instead use mainline). Just use b4 and everything should be
fine, although remember about `b4 prep --auto-to-cc` if you added new
patches to the patchset.

You missed at least devicetree list (maybe more), so this won't be
tested by automated tooling. Performing review on untested code might be
a waste of time.

Please kindly resend and include all necessary To/Cc entries.
</form letter>


Best regards,
Krzysztof

hello,

On Tue, Sep 02, 2025 at 07:57:03AM +0200, Krzysztof Kozlowski wrote:
> <form letter>
> Please use scripts/get_maintainers.pl to get a list of necessary people
> and lists to CC. It might happen, that command when run on an older
> kernel, gives you outdated entries. Therefore please be sure you base
> your patches on recent Linux kernel.

I'm using the bleeding edge togreg branch of the iio tree, git pulled yesterday.
I indeed missed devicetree@ while manually copy-pasting from get_maintainer.pl on the bindings patch. I wish that script would provide a valid rfc822 email header instead of it's current verbose output.

> You missed at least devicetree list (maybe more), so this won't be
> tested by automated tooling. Performing review on untested code might be
> a waste of time.

the patch set had gone thru multiple static scans, unit tests, checkpatch.pl and the dt_binding_check on my end.
since devicetree@ is not emailed with driver code, I will still wait for any feedback to the driver part of my submission, I guess.

> >  - fix title typo
> >  - add optional watchdog setting that recovers the sensor from a stuck-low
> >  SDA condition
> >  - set correct SPI phase and polarity
> >  - interrupt on rising edge. the level-based interrupt that is being
> 
> Cleanup and new features must never be mixed together.

ok I will split up all the bindings changes in the next revision.

best regards.
peter

On 02/09/2025 18:02, Petre Rodan wrote:
> 
> hello,
> 
> On Tue, Sep 02, 2025 at 07:57:03AM +0200, Krzysztof Kozlowski wrote:
>> <form letter>
>> Please use scripts/get_maintainers.pl to get a list of necessary people
>> and lists to CC. It might happen, that command when run on an older
>> kernel, gives you outdated entries. Therefore please be sure you base
>> your patches on recent Linux kernel.
> 
> I'm using the bleeding edge togreg branch of the iio tree, git pulled yesterday.
> I indeed missed devicetree@ while manually copy-pasting from get_maintainer.pl on the bindings patch. I wish that script would provide a valid rfc822 email header instead of it's current verbose output.

Recommended is to use b4. Simple wrapper like git_send_email() also
would work:
https://lore.kernel.org/all/?q=git_send_email


Best regards,
Krzysztof

On 9/2/25 11:02 AM, Petre Rodan wrote:
> 
> hello,
> 
> On Tue, Sep 02, 2025 at 07:57:03AM +0200, Krzysztof Kozlowski wrote:
>> <form letter>
>> Please use scripts/get_maintainers.pl to get a list of necessary people
>> and lists to CC. It might happen, that command when run on an older
>> kernel, gives you outdated entries. Therefore please be sure you base
>> your patches on recent Linux kernel.
> 
> I'm using the bleeding edge togreg branch of the iio tree, git pulled yesterday.
> I indeed missed devicetree@ while manually copy-pasting from get_maintainer.pl on the bindings patch. I wish that script would provide a valid rfc822 email header instead of it's current verbose output.
> 
IIRC, the --no-rolestats option will fix that.

Suggest to use b4 or other tools to automate this.