diff mbox series

[01/13] dt-bindings: i2c: nomadik: add timeout-usecs property bindings

Message ID 20240215-mbly-i2c-v1-1-19a336e91dca@bootlin.com
State New
Headers
Series Add Mobileye EyeQ5 support to the Nomadik I2C controller & use hrtimers for timeouts |

Commit Message

Théo Lebrun Feb. 15, 2024, 4:52 p.m. UTC 
  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(+)

Comments

Rob Herring Feb. 16, 2024, 2:27 a.m. UTC | #1 
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
Théo Lebrun Feb. 16, 2024, 9:16 a.m. UTC | #2 
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
Krzysztof Kozlowski Feb. 16, 2024, 9:17 a.m. UTC | #3 
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
Linus Walleij Feb. 19, 2024, 2:06 p.m. UTC | #4 
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
Théo Lebrun Feb. 19, 2024, 2:29 p.m. UTC | #5 
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

------------------------------------------------------------------------
Théo Lebrun Feb. 21, 2024, 4:58 p.m. UTC | #6 
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
Wolfram Sang Feb. 21, 2024, 5:43 p.m. UTC | #7 
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
Rob Herring Feb. 22, 2024, 5:14 p.m. UTC | #8 
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
Wolfram Sang Feb. 22, 2024, 7:28 p.m. UTC | #9 
> > @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!
Rob Herring Feb. 23, 2024, 4:34 p.m. UTC | #10 
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
Wolfram Sang Feb. 26, 2024, 8:41 a.m. UTC | #11 
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 mbox series

Patch

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 {