Re: [PATCH 01/10] dt-bindings: iio: accel: bosch,BMA220 improvements

[David Lechner <dlechner@baylibre.com>]

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
>