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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aLugeZiJjJhTpwUO@sunspire>
Date: Sat, 6 Sep 2025 05:46:17 +0300
From: Petre Rodan <petre.rodan@...dimension.ro>
To: David Lechner <dlechner@...libre.com>
Cc: linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
	Jonathan Cameron <jic23@...nel.org>, Nuno S?? <nuno.sa@...log.com>,
	Andy Shevchenko <andy@...nel.org>, Rob Herring <robh@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>
Subject: Re: [PATCH 01/10] dt-bindings: iio: accel: bosch,BMA220 improvements

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