| Message ID | 20240215-mbly-i2c-v1-1-19a336e91dca@bootlin.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Add Mobileye EyeQ5 support to the Nomadik I2C controller & use hrtimers for timeouts | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On Thu, Feb 15, 2024 at 05:52:08PM +0100, Théo Lebrun wrote: > Expose I2C device timeout configuration from devicetree. Use µs as time > unit and express it in the name. > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > --- > Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml b/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml > index 16024415a4a7..e6b95e3765ac 100644 > --- a/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml > +++ b/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml > @@ -70,6 +70,10 @@ properties: > minimum: 1 > maximum: 400000 > > + timeout-usecs: Use standard unit suffixes. We already have at least 2 device specific timeout properties. This one should be common. That means you need to add it to i2c-controller.yaml in dtschema. GH PR or patch to devicetree-spec list is fine. Rob
Hello, On Fri Feb 16, 2024 at 3:27 AM CET, Rob Herring wrote: > On Thu, Feb 15, 2024 at 05:52:08PM +0100, Théo Lebrun wrote: > > Expose I2C device timeout configuration from devicetree. Use µs as time > > unit and express it in the name. > > > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > > --- > > Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml b/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml > > index 16024415a4a7..e6b95e3765ac 100644 > > --- a/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml > > +++ b/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml > > @@ -70,6 +70,10 @@ properties: > > minimum: 1 > > maximum: 400000 > > > > + timeout-usecs: > > Use standard unit suffixes. > > We already have at least 2 device specific timeout properties. This one > should be common. That means you need to add it to i2c-controller.yaml > in dtschema. GH PR or patch to devicetree-spec list is fine. i2c-mpc (fsl,timeout) and i2c-gpio (i2c-gpio,timeout-ms). I agree this prop has no reason to be compatible-specific. Feedback from dt-bindings and I2C host maintainers would be useful: what should the property be named? Having the unit makes it self-descriptive, which sounds like a good idea to me. timeout-usecs, timeout-us, another option? Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On 16/02/2024 10:16, Théo Lebrun wrote: > Hello, > > On Fri Feb 16, 2024 at 3:27 AM CET, Rob Herring wrote: >> On Thu, Feb 15, 2024 at 05:52:08PM +0100, Théo Lebrun wrote: >>> Expose I2C device timeout configuration from devicetree. Use µs as time >>> unit and express it in the name. >>> >>> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> >>> --- >>> Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml | 5 +++++ >>> 1 file changed, 5 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml b/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml >>> index 16024415a4a7..e6b95e3765ac 100644 >>> --- a/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml >>> +++ b/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml >>> @@ -70,6 +70,10 @@ properties: >>> minimum: 1 >>> maximum: 400000 >>> >>> + timeout-usecs: >> >> Use standard unit suffixes. >> >> We already have at least 2 device specific timeout properties. This one >> should be common. That means you need to add it to i2c-controller.yaml >> in dtschema. GH PR or patch to devicetree-spec list is fine. > > i2c-mpc (fsl,timeout) and i2c-gpio (i2c-gpio,timeout-ms). I agree this > prop has no reason to be compatible-specific. > > Feedback from dt-bindings and I2C host maintainers would be useful: what > should the property be named? Having the unit makes it self-descriptive, > which sounds like a good idea to me. timeout-usecs, timeout-us, another > option? It must have an unit. That's not negotiable for new properties. Best regards, Krzysztof
Hi Théo, thanks for your patch! On Fri, Feb 16, 2024 at 10:16 AM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > i2c-mpc (fsl,timeout) and i2c-gpio (i2c-gpio,timeout-ms). I agree this > prop has no reason to be compatible-specific. > > Feedback from dt-bindings and I2C host maintainers would be useful: what > should the property be named? Having the unit makes it self-descriptive, > which sounds like a good idea to me. timeout-usecs, timeout-us, another > option? Use i2c-transfer-timeout-ms in my opinion, so it us crystal clear what that property is for. As Rob mentioned this isn't in the kernel schemas but in dtschema, so you need to patch this: https://github.com/robherring/dt-schema Yours, Linus Walleij
Hello, On Mon Feb 19, 2024 at 3:06 PM CET, Linus Walleij wrote: > Hi Théo, > > thanks for your patch! > > On Fri, Feb 16, 2024 at 10:16 AM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > > > i2c-mpc (fsl,timeout) and i2c-gpio (i2c-gpio,timeout-ms). I agree this > > prop has no reason to be compatible-specific. > > > > Feedback from dt-bindings and I2C host maintainers would be useful: what > > should the property be named? Having the unit makes it self-descriptive, > > which sounds like a good idea to me. timeout-usecs, timeout-us, another > > option? > > Use i2c-transfer-timeout-ms in my opinion, so it us crystal clear > what that property is for. Using µs (microseconds) would be OK? I'm not sure yet about the exact timeout desired but a one millisecond granularity might not be enough for the Mobileye usecase. Expect incoming patches to use the I2C controller in Fast Mode Plus (1Mbps) and High Speed Mode (3.4Mbps). Gotta go fast! > As Rob mentioned this isn't in the kernel schemas but in dtschema, so > you need to patch this: > https://github.com/robherring/dt-schema Indeed. The other question if we do microseconds is the suffix: -us, -usecs, -microseconds, etc? I picked -usecs for my v1, but a grep tells me I am the only user of this suffix. -us is much more common. BTW i2c-controller.yaml already has a µs timeout: i2c-scl-clk-low-timeout-us Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ------------------------------------------------------------------------
Hello, On Mon Feb 19, 2024 at 3:29 PM CET, Théo Lebrun wrote: > On Mon Feb 19, 2024 at 3:06 PM CET, Linus Walleij wrote: > > On Fri, Feb 16, 2024 at 10:16 AM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > > > i2c-mpc (fsl,timeout) and i2c-gpio (i2c-gpio,timeout-ms). I agree this > > > prop has no reason to be compatible-specific. > > > > > > Feedback from dt-bindings and I2C host maintainers would be useful: what > > > should the property be named? Having the unit makes it self-descriptive, > > > which sounds like a good idea to me. timeout-usecs, timeout-us, another > > > option? > > > > Use i2c-transfer-timeout-ms in my opinion, so it us crystal clear > > what that property is for. > > Using µs (microseconds) would be OK? I'm not sure yet about the exact > timeout desired but a one millisecond granularity might not be enough > for the Mobileye usecase. > > Expect incoming patches to use the I2C controller in Fast Mode Plus > (1Mbps) and High Speed Mode (3.4Mbps). Gotta go fast! > > > As Rob mentioned this isn't in the kernel schemas but in dtschema, so > > you need to patch this: > > https://github.com/robherring/dt-schema > > Indeed. The other question if we do microseconds is the > suffix: -us, -usecs, -microseconds, etc? I picked -usecs for my v1, but > a grep tells me I am the only user of this suffix. -us is much more > common. > > BTW i2c-controller.yaml already has a µs timeout: > i2c-scl-clk-low-timeout-us Note: I've sent a draft patch to dt-schema. See: https://github.com/devicetree-org/dt-schema/pull/129 Feedback from I2C maintainers would confirm or infirm that this goes in the right direction. Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hi, > > > > i2c-mpc (fsl,timeout) and i2c-gpio (i2c-gpio,timeout-ms). I agree this > > > > prop has no reason to be compatible-specific. Anyone up to convert these drivers to the new binding and mark the old ones as deprecated? > > > As Rob mentioned this isn't in the kernel schemas but in dtschema, so > > > you need to patch this: > > > https://github.com/robherring/dt-schema @Rob: My memory fails a little bit about these two schemas: we have the github one for generic bindings, not strictly related to Linux, right? But why do we have then i2c.txt in ther kernel tree? Why don't we sync regularly with the generic schema? > Note: I've sent a draft patch to dt-schema. See: > https://github.com/devicetree-org/dt-schema/pull/129 I used to argue that you can set this timeout to any value in userspace. I have been convinced that it might make sense to set it early so it is in use already when booting. So, for this pull request: Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com> All the best, Wolfram
On Wed, Feb 21, 2024 at 06:43:55PM +0100, Wolfram Sang wrote: > Hi, > > > > > > i2c-mpc (fsl,timeout) and i2c-gpio (i2c-gpio,timeout-ms). I agree this > > > > > prop has no reason to be compatible-specific. > > Anyone up to convert these drivers to the new binding and mark the old > ones as deprecated? > > > > > As Rob mentioned this isn't in the kernel schemas but in dtschema, so > > > > you need to patch this: > > > > https://github.com/robherring/dt-schema > > @Rob: My memory fails a little bit about these two schemas: we have the > github one for generic bindings, not strictly related to Linux, right? Well, NONE of the bindings are strictly related to linux unless they say 'linux,' prefix. > But why do we have then i2c.txt in ther kernel tree? Why don't we sync > regularly with the generic schema? We need to remove i2c.txt. Often that hasn't happened because we need to relicense the text from GPL only to dual licensed. From a quick look, i2c/i2c-controller.yaml appears to have everything in i2c.txt, so I think we can go ahead and remove it. There's only a few references to i2c.txt to update with that. I'll send a patch, but please double check whether you think i2c-controller.yaml is missing anything. Rob
> > @Rob: My memory fails a little bit about these two schemas: we have the > > github one for generic bindings, not strictly related to Linux, right? > > Well, NONE of the bindings are strictly related to linux unless they say > 'linux,' prefix. Ok, right, of course. What I meant was probably: why do we have controller bindings in the kernel and schema bindings in a github tree? For me, this is a tad more difficult to maintain. Like i2c-controller.yaml file has the "no-detect" binding which IMO is wrong in many ways. I rejected the supporting code for Linux. > We need to remove i2c.txt. Often that hasn't happened because we need to > relicense the text from GPL only to dual licensed. From a quick look, > i2c/i2c-controller.yaml appears to have everything in i2c.txt, so I > think we can go ahead and remove it. There's only a few references to > i2c.txt to update with that. I'll send a patch, but please double check > whether you think i2c-controller.yaml is missing anything. Will do, thanks!
On Thu, Feb 22, 2024 at 12:28 PM Wolfram Sang <wsa@kernel.org> wrote: > > > > > @Rob: My memory fails a little bit about these two schemas: we have the > > > github one for generic bindings, not strictly related to Linux, right? > > > > Well, NONE of the bindings are strictly related to linux unless they say > > 'linux,' prefix. > > Ok, right, of course. What I meant was probably: why do we have > controller bindings in the kernel and schema bindings in a github tree? Generally the split is common, stable bindings go in dtschema. This is anything we'd consider should be in the DT spec. Though I have 0 plans to add anything to the spec because I'd like to generate the spec from schema. (Not really working on doing that either though). What's stable? Well, no solid definition there other than not new. So new things generally go into the kernel tree first. For device specific bindings, they will never go into dtschema and will live where the dts files are. > For me, this is a tad more difficult to maintain. Like > i2c-controller.yaml file has the "no-detect" binding which IMO is wrong > in many ways. I rejected the supporting code for Linux. It was probably added to i2c.txt and I probably said, looks fine, but add it to dtschema. Then the Linux support got rejected. We can simply remove it if it is not being used. This is why I'm generally against moving all the DT stuff out of the kernel. The reviewing would dry up. I'll try to make sure you see any future i2c changes. I take either patches on devicetree-spec list or GH PR. Shockingly, I mainly get GH PR. Rob
Hi Rob,
> I'll try to make sure you see any future i2c changes.
Thanks, if you mention @wsakernel I should get notified.
I looked into watching specific directories in Github and I found the
CODEOWNERS file. But code owners need to have write permissions and I
am not sure this is worth the hazzle.
Now on to checking that remaining binding...
Wolfram
diff --git a/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml b/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml index 16024415a4a7..e6b95e3765ac 100644 --- a/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml +++ b/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml @@ -70,6 +70,10 @@ properties: minimum: 1 maximum: 400000 + timeout-usecs: + $ref: /schemas/types.yaml#/definitions/uint32 + description: Timeout on I2C xfers in µs. + required: - compatible - reg @@ -98,6 +102,7 @@ examples: clock-names = "i2cclk", "apb_pclk"; power-domains = <&pm_domains DOMAIN_VAPE>; resets = <&prcc_reset DB8500_PRCC_3 DB8500_PRCC_3_RESET_I2C0>; + timeout-usecs = <200000>; }; i2c@101f8000 {
Expose I2C device timeout configuration from devicetree. Use µs as time unit and express it in the name. Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> --- Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml | 5 +++++ 1 file changed, 5 insertions(+)