[PATCH 00/13] Add Mobileye EyeQ5 support to the Nomadik I2C controller & use hrtimers for timeouts

[PATCH 00/13] Add Mobileye EyeQ5 support to the Nomadik I2C controller & use hrtimers for timeouts

[Théo Lebrun]

Hi,

This series adds two tangent features to the Nomadik I2C controller:

 - Add a new compatible to support Mobileye EyeQ5 which uses the same IP
   block as Nomadik.

   It has two quirks to be handled:
    - The memory bus only supports 32-bit accesses. A writeb() is used
      which we avoid.
    - We must write a value into a shared register region (OLB, "Other
      Logic Block") depending on the I2C bus speed.

 - Allow xfer timeouts below one jiffy by using a workqueue and hrtimers
   instead of a completion.

   The situation to be addressed is:
    - Many devices on the same I2C bus.
    - One xfer to each device is sent at regular interval.
    - One device gets stuck and does not answer.
    - With long timeouts, following devices won't get their message. A
      shorter timeout ensures we can still talk to the following
      devices.

   This clashes a bit with the current i2c_adapter timeout field that
   stores a jiffies amount. We cannot rely on it and therefore we take
   a value from devicetree as a µs value. If the timeout is less than a
   jiffy duration, we switch from standard jiffies timeout to
   hrtimers.

There is one patch targeting a hwmon dt-bindings file:
Documentation/devicetree/bindings/hwmon/lm75.yaml. The rest is touching
the I2C bus driver, its bindings and platform devicetrees.

About dependencies:
 - The series is based upon v6.8-rc4.
 - For testing on EyeQ5 hardware and devicetree patches, we need the
   base platform series from Grégory [0].
 - The last commit (adding DT phandles for resets), we need the syscon
   series [1] that provides the reset controller node.

I think there are discussions to be had about:

 - The handling of timeouts. Having a non-jiffy value is not driver
   specific. Should this change be done at the subsystem layer? The
   subsystem could even fetch the value from devicetree and auto-fill
   timeout, with a default given by the driver. Not many drivers seem
   to use the i2c_adapter timeout field from my quick grepping.

 - The DT prop for timeout. I've picked "timeout-usecs". Some drivers
   use vendor prefixes, but this is not vendor-specific and only a
   software implementation detail.

 - The shape of this series. Initially it was split in two. However I
   brought them together as they cannot be applied independently.
   Please tell me if a better approach is to be preferred.

Those are thoughts, I'm sure people will have feedback on this.

Have a nice day,
Théo Lebrun

[0]: https://lore.kernel.org/lkml/20240205153503.574468-1-gregory.clement@bootlin.com/
[1]: https://lore.kernel.org/lkml/20240212-mbly-clk-v6-0-c46fa1f93839@bootlin.com/

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
Théo Lebrun (13):
      dt-bindings: i2c: nomadik: add timeout-usecs property bindings
      dt-bindings: i2c: nomadik: add mobileye,eyeq5-i2c bindings and example
      dt-bindings: hwmon: lm75: add label property
      i2c: nomadik: rename private struct pointers from dev to priv
      i2c: nomadik: simplify IRQ masking logic
      i2c: nomadik: use bitops helpers
      i2c: nomadik: support short xfer timeouts using waitqueue & hrtimer
      i2c: nomadik: replace jiffies by ktime for FIFO flushing timeout
      i2c: nomadik: fetch timeout-usecs property from devicetree
      i2c: nomadik: support Mobileye EyeQ5 I2C controller
      MIPS: mobileye: eyeq5: add 5 I2C controller nodes
      MIPS: mobileye: eyeq5: add evaluation board I2C temp sensor
      MIPS: mobileye: eyeq5: add resets to I2C controllers

 Documentation/devicetree/bindings/hwmon/lm75.yaml  |   4 +
 .../devicetree/bindings/i2c/st,nomadik-i2c.yaml    |  49 +-
 arch/mips/boot/dts/mobileye/eyeq5-epm5.dts         |   8 +
 arch/mips/boot/dts/mobileye/eyeq5.dtsi             |  75 +++
 drivers/i2c/busses/i2c-nomadik.c                   | 710 ++++++++++++---------
 5 files changed, 534 insertions(+), 312 deletions(-)
---
base-commit: d55aa725e32849f709b61eab3b7a50b810a71a84
change-id: 20231023-mbly-i2c-7c2fbbb1299f

Best regards,
-- 
Théo Lebrun <theo.lebrun@bootlin.com>

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

[Théo Lebrun]

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:
+    $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 {

-- 
2.43.1

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

[Rob Herring]

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

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

[Théo Lebrun]

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

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

[Krzysztof Kozlowski]

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

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

[Linus Walleij]

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

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

[Théo Lebrun]

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

------------------------------------------------------------------------

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

[Théo Lebrun]

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

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1032 bytes --]

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Rob Herring]

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

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1043 bytes --]


> > @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!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Rob Herring]

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

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 361 bytes --]

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 02/13] dt-bindings: i2c: nomadik: add mobileye,eyeq5-i2c bindings and example

[Théo Lebrun]

Add EyeQ5 bindings to the existing Nomadik I2C dt-bindings. Add the two
EyeQ5-specific properties behind a conditional. Add an example for this
compatible.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 .../devicetree/bindings/i2c/st,nomadik-i2c.yaml    | 44 ++++++++++++++++++++--
 1 file changed, 40 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml b/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml
index e6b95e3765ac..eaade14b6d4c 100644
--- a/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml
+++ b/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml
@@ -14,9 +14,6 @@ description: The Nomadik I2C host controller began its life in the ST
 maintainers:
   - Linus Walleij <linus.walleij@linaro.org>
 
-allOf:
-  - $ref: /schemas/i2c/i2c-controller.yaml#
-
 # Need a custom select here or 'arm,primecell' will match on lots of nodes
 select:
   properties:
@@ -24,6 +21,7 @@ select:
       contains:
         enum:
           - st,nomadik-i2c
+          - mobileye,eyeq5-i2c
   required:
     - compatible
 
@@ -39,6 +37,10 @@ properties:
           - const: stericsson,db8500-i2c
           - const: st,nomadik-i2c
           - const: arm,primecell
+      # The variant found on Mobileye EyeQ5
+      - items:
+          - const: mobileye,eyeq5-i2c
+          - const: arm,primecell
 
   reg:
     maxItems: 1
@@ -55,7 +57,7 @@ properties:
       - items:
           - const: mclk
           - const: apb_pclk
-      # Clock name in DB8500
+      # Clock name in DB8500 or EyeQ5
       - items:
           - const: i2cclk
           - const: apb_pclk
@@ -83,6 +85,25 @@ required:
 
 unevaluatedProperties: false
 
+allOf:
+  - $ref: /schemas/i2c/i2c-controller.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: mobileye,eyeq5-i2c
+    then:
+      properties:
+        mobileye,olb:
+          $ref: /schemas/types.yaml#/definitions/phandle
+          description: A phandle to the OLB syscon.
+        mobileye,id:
+          $ref: /schemas/types.yaml#/definitions/uint32
+          description: Platform-wide controller ID (integer starting from zero).
+      required:
+        - mobileye,olb
+        - mobileye,id
+
 examples:
   - |
     #include <dt-bindings/interrupt-controller/irq.h>
@@ -116,5 +137,20 @@ examples:
       clocks = <&i2c0clk>, <&pclki2c0>;
       clock-names = "mclk", "apb_pclk";
     };
+  - |
+    #include <dt-bindings/interrupt-controller/mips-gic.h>
+    i2c@300000 {
+      compatible = "mobileye,eyeq5-i2c", "arm,primecell";
+      reg = <0x300000 0x1000>;
+      interrupt-parent = <&gic>;
+      interrupts = <GIC_SHARED 1 IRQ_TYPE_LEVEL_HIGH>;
+      clock-frequency = <400000>;
+      #address-cells = <1>;
+      #size-cells = <0>;
+      clocks = <&i2c_ser_clk>, <&i2c_clk>;
+      clock-names = "i2cclk", "apb_pclk";
+      mobileye,olb = <&olb>;
+      mobileye,id = <0>;
+    };
 
 ...

-- 
2.43.1

Re: [PATCH 02/13] dt-bindings: i2c: nomadik: add mobileye,eyeq5-i2c bindings and example

[Rob Herring]

On Thu, 15 Feb 2024 17:52:09 +0100, Théo Lebrun wrote:
> Add EyeQ5 bindings to the existing Nomadik I2C dt-bindings. Add the two
> EyeQ5-specific properties behind a conditional. Add an example for this
> compatible.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  .../devicetree/bindings/i2c/st,nomadik-i2c.yaml    | 44 ++++++++++++++++++++--
>  1 file changed, 40 insertions(+), 4 deletions(-)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.example.dtb: i2c@300000: 'mobileye,id', 'mobileye,olb' do not match any of the regexes: '^#.*', '^(at25|bm|devbus|dmacap|dsa|exynos|fsi[ab]|gpio-fan|gpio-key|gpio|gpmc|hdmi|i2c-gpio),.*', '^(keypad|m25p|max8952|max8997|max8998|mpmc),.*', '^(pinctrl-single|#pinctrl-single|PowerPC),.*', '^(pl022|pxa-mmc|rcar_sound|rotary-encoder|s5m8767|sdhci),.*', '^(simple-audio-card|st-plgpio|st-spics|ts),.*', '^100ask,.*', '^70mai,.*', '^8dev,.*', '^GEFanuc,.*', '^ORCL,.*', '^SUNW,.*', '^[a-zA-Z0-9#_][a-zA-Z0-9+\\-._@]{0,63}$', '^[a-zA-Z0-9+\\-._]*@[0-9a-zA-Z,]*$', '^abb,.*', '^abilis,.*', '^abracon,.*', '^abt,.*', '^acbel,.*', '^acer,.*', '^acme,.*', '^actions,.*', '^active-semi,.*', '^ad,.*', '^adafruit,.*', '^adapteva,.*', '^adaptrum,.*', '^adh,.*', '^adi,.*', '^adieng,.*', '^advantech,.*', '^aeroflexgaisler,.*', '^aesop,.*', '^airoha,.*', '^al,.*', '^alcatel,.*', '^aldec,.*', '^alfa-network,.*', '^allegro,.*', '^alliedvision,.*', '^allo,.*', '^allwinner,.*', '^alphascale,.*', '^alps,.*', '^alt,.*', '^altr,.*', '^amarula,.*', '^amazon,.*', '^amcc,.*', '^amd,.*', '^amediatech,.*', '^amlogic,.*', '^ampere,.*', '^ampire,.*', '^ams,.*', '^amstaos,.*', '^analogix,.*', '^anbernic,.*', '^andestech,.*', '^anvo,.*', '^aosong,.*', '^apm,.*', '^apple,.*', '^aptina,.*', '^arasan,.*', '^archermind,.*', '^arcom,.*', '^arctic,.*', '^arcx,.*', '^aries,.*', '^arm,.*', '^armadeus,.*', '^arrow,.*', '^artesyn,.*', '^asahi-kasei,.*', '^asc,.*', '^asix,.*', '^aspeed,.*', '^asrock,.*', '^asus,.*', '^atheros,.*', '^atlas,.*', '^atmel,.*', '^auo,.*', '^auvidea,.*', '^avago,.*', '^avia,.*', '^avic,.*', '^avnet,.*', '^awinic,.*', '^axentia,.*', '^axis,.*', '^azoteq,.*', '^azw,.*', '^baikal,.*', '^bananapi,.*', '^beacon,.*', '^beagle,.*', '^belling,.*', '^bhf,.*', '^bigtreetech,.*', '^bitmain,.*', '^blutek,.*', '^boe,.*', '^bosch,.*', '^boundary,.*', '^brcm,.*', '^broadmobi,.*', '^bsh,.*', '^bticino,.*', '^buffalo,.*', '^bur,.*', '^bytedance,.*', '^calamp,.*', '^calaosystems,.*', '^calxeda,.*', '^canaan,.*', '^caninos,.*', '^capella,.*', '^cascoda,.*', '^catalyst,.*', '^cavium,.*', '^cdns,.*', '^cdtech,.*', '^cellwise,.*', '^ceva,.*', '^chargebyte,.*', '^checkpoint,.*', '^chefree,.*', '^chipidea,.*', '^chipone,.*', '^chipspark,.*', '^chongzhou,.*', '^chrontel,.*', '^chrp,.*', '^chunghwa,.*', '^chuwi,.*', '^ciaa,.*', '^cirrus,.*', '^cisco,.*', '^clockwork,.*', '^cloos,.*', '^cloudengines,.*', '^cnm,.*', '^cnxt,.*', '^colorfly,.*', '^compulab,.*', '^congatec,.*', '^coolpi,.*', '^coreriver,.*', '^corpro,.*', '^cortina,.*', '^cosmic,.*', '^crane,.*', '^creative,.*', '^crystalfontz,.*', '^csky,.*', '^csq,.*', '^ctera,.*', '^ctu,.*', '^cubietech,.*', '^cui,.*', '^cypress,.*', '^cyx,.*', '^cznic,.*', '^dallas,.*', '^dataimage,.*', '^davicom,.*', '^dell,.*', '^delta,.*', '^densitron,.*', '^denx,.*', '^devantech,.*', '^dfi,.*', '^dh,.*', '^difrnce,.*', '^digi,.*', '^digilent,.*', '^dimonoff,.*', '^diodes,.*', '^dioo,.*', '^dlc,.*', '^dlg,.*', '^dlink,.*', '^dmo,.*', '^domintech,.*', '^dongwoon,.*', '^dptechnics,.*', '^dragino,.*', '^ds,.*', '^dserve,.*', '^dynaimage,.*', '^ea,.*', '^ebang,.*', '^ebbg,.*', '^ebs-systart,.*', '^ebv,.*', '^eckelmann,.*', '^edgeble,.*', '^edimax,.*', '^edt,.*', '^ees,.*', '^eeti,.*', '^einfochips,.*', '^eink,.*', '^elan,.*', '^element14,.*', '^elgin,.*', '^elida,.*', '^elimo,.*', '^elpida,.*', '^embedfire,.*', '^embest,.*', '^emlid,.*', '^emmicro,.*', '^empire-electronix,.*', '^emtrion,.*', '^enclustra,.*', '^endless,.*', '^ene,.*', '^energymicro,.*', '^engicam,.*', '^engleder,.*', '^epcos,.*', '^epfl,.*', '^epson,.*', '^esp,.*', '^est,.*', '^ettus,.*', '^eukrea,.*', '^everest,.*', '^everspin,.*', '^evervision,.*', '^exar,.*', '^excito,.*', '^exegin,.*', '^ezchip,.*', '^facebook,.*', '^fairphone,.*', '^faraday,.*', '^fascontek,.*', '^fastrax,.*', '^fcs,.*', '^feixin,.*', '^feiyang,.*', '^fii,.*', '^firefly,.*', '^focaltech,.*', '^forlinx,.*', '^freecom,.*', '^frida,.*', '^friendlyarm,.*', '^fsl,.*', '^fujitsu,.*', '^fxtec,.*', '^galaxycore,.*', '^gardena,.*', '^gateway,.*', '^gateworks,.*', '^gcw,.*', '^ge,.*', '^geekbuying,.*', '^gef,.*', '^gemei,.*', '^gemtek,.*', '^genesys,.*', '^geniatech,.*', '^giantec,.*', '^giantplus,.*', '^globalscale,.*', '^globaltop,.*', '^gmt,.*', '^goldelico,.*', '^goodix,.*', '^google,.*', '^goramo,.*', '^gplus,.*', '^grinn,.*', '^grmn,.*', '^gumstix,.*', '^gw,.*', '^hannstar,.*', '^haochuangyi,.*', '^haoyu,.*', '^hardkernel,.*', '^hechuang,.*', '^hideep,.*', '^himax,.*', '^hirschmann,.*', '^hisi,.*', '^hisilicon,.*', '^hit,.*', '^hitex,.*', '^holt,.*', '^holtek,.*', '^honestar,.*', '^honeywell,.*', '^hoperun,.*', '^hp,.*', '^hpe,.*', '^hsg,.*', '^htc,.*', '^huawei,.*', '^hugsun,.*', '^hwacom,.*', '^hxt,.*', '^hycon,.*', '^hydis,.*', '^hynitron,.*', '^hynix,.*', '^hyundai,.*', '^i2se,.*', '^ibm,.*', '^icplus,.*', '^idt,.*', '^ifi,.*', '^ilitek,.*', '^imagis,.*', '^img,.*', '^imi,.*', '^inanbo,.*', '^incircuit,.*', '^indiedroid,.*', '^inet-tek,.*', '^infineon,.*', '^inforce,.*', '^ingenic,.*', '^ingrasys,.*', '^injoinic,.*', '^innocomm,.*', '^innolux,.*', '^inside-secure,.*', '^insignal,.*', '^inspur,.*', '^intel,.*', '^intercontrol,.*', '^invensense,.*', '^inventec,.*', '^inversepath,.*', '^iom,.*', '^irondevice,.*', '^isee,.*', '^isil,.*', '^issi,.*', '^ite,.*', '^itead,.*', '^itian,.*', '^ivo,.*', '^iwave,.*', '^jadard,.*', '^jasonic,.*', '^jdi,.*', '^jedec,.*', '^jesurun,.*', '^jethome,.*', '^jianda,.*', '^joz,.*', '^kam,.*', '^karo,.*', '^keithkoep,.*', '^keymile,.*', '^khadas,.*', '^kiebackpeter,.*', '^kinetic,.*', '^kingdisplay,.*', '^kingnovel,.*', '^kionix,.*', '^kobo,.*', '^kobol,.*', '^koe,.*', '^kontron,.*', '^kosagi,.*', '^kvg,.*', '^kyo,.*', '^lacie,.*', '^laird,.*', '^lamobo,.*', '^lantiq,.*', '^lattice,.*', '^lctech,.*', '^leadtek,.*', '^leez,.*', '^lego,.*', '^lemaker,.*', '^lenovo,.*', '^lg,.*', '^lgphilips,.*', '^libretech,.*', '^licheepi,.*', '^linaro,.*', '^lineartechnology,.*', '^linksprite,.*', '^linksys,.*', '^linutronix,.*', '^linux,.*', '^linx,.*', '^liteon,.*', '^litex,.*', '^lltc,.*', '^logicpd,.*', '^logictechno,.*', '^longcheer,.*', '^lontium,.*', '^loongmasses,.*', '^loongson,.*', '^lsi,.*', '^lunzn,.*', '^lwn,.*', '^lxa,.*', '^m5stack,.*', '^macnica,.*', '^mantix,.*', '^mapleboard,.*', '^marantec,.*', '^marvell,.*', '^maxbotix,.*', '^maxim,.*', '^maxlinear,.*', '^mbvl,.*', '^mcube,.*', '^meas,.*', '^mecer,.*', '^mediatek,.*', '^megachips,.*', '^mele,.*', '^melexis,.*', '^melfas,.*', '^mellanox,.*', '^memsensing,.*', '^memsic,.*', '^menlo,.*', '^mentor,.*', '^meraki,.*', '^merrii,.*', '^methode,.*', '^micrel,.*', '^microchip,.*', '^microcrystal,.*', '^micron,.*', '^microsoft,.*', '^microsys,.*', '^mikroe,.*', '^mikrotik,.*', '^milkv,.*', '^miniand,.*', '^minix,.*', '^miramems,.*', '^mitsubishi,.*', '^mitsumi,.*', '^mixel,.*', '^miyoo,.*', '^mntre,.*', '^modtronix,.*', '^moortec,.*', '^mosaixtech,.*', '^motorcomm,.*', '^motorola,.*', '^moxa,.*', '^mpl,.*', '^mps,.*', '^mqmaker,.*', '^mrvl,.*', '^mscc,.*', '^msi,.*', '^mstar,.*', '^mti,.*', '^multi-inno,.*', '^mundoreader,.*', '^murata,.*', '^mxic,.*', '^mxicy,.*', '^myir,.*', '^national,.*', '^nec,.*', '^neonode,.*', '^netgear,.*', '^netlogic,.*', '^netron-dy,.*', '^netronix,.*', '^netxeon,.*', '^neweast,.*', '^newhaven,.*', '^newvision,.*', '^nexbox,.*', '^nextthing,.*', '^ni,.*', '^nintendo,.*', '^nlt,.*', '^nokia,.*', '^nordic,.*', '^novatek,.*', '^novtech,.*', '^nutsboard,.*', '^nuvoton,.*', '^nvd,.*', '^nvidia,.*', '^nxp,.*', '^oceanic,.*', '^ocs,.*', '^oct,.*', '^okaya,.*', '^oki,.*', '^olimex,.*', '^olpc,.*', '^oneplus,.*', '^onie,.*', '^onion,.*', '^onnn,.*', '^ontat,.*', '^opalkelly,.*', '^openailab,.*', '^opencores,.*', '^openembed,.*', '^openpandora,.*', '^openrisc,.*', '^option,.*', '^oranth,.*', '^orisetech,.*', '^ortustech,.*', '^osddisplays,.*', '^osmc,.*', '^ouya,.*', '^overkiz,.*', '^ovti,.*', '^oxsemi,.*', '^ozzmaker,.*', '^panasonic,.*', '^parade,.*', '^parallax,.*', '^pda,.*', '^pericom,.*', '^pervasive,.*', '^phicomm,.*', '^phytec,.*', '^picochip,.*', '^pine64,.*', '^pineriver,.*', '^pixcir,.*', '^plantower,.*', '^plathome,.*', '^plda,.*', '^plx,.*', '^ply,.*', '^pni,.*', '^pocketbook,.*', '^polaroid,.*', '^polyhex,.*', '^portwell,.*', '^poslab,.*', '^pov,.*', '^powertip,.*', '^powervr,.*', '^powkiddy,.*', '^primux,.*', '^probox2,.*', '^prt,.*', '^pulsedlight,.*', '^purism,.*', '^qca,.*', '^qcom,.*', '^qemu,.*', '^qi,.*', '^qiaodian,.*', '^qihua,.*', '^qishenglong,.*', '^qnap,.*', '^quanta,.*', '^radxa,.*', '^raidsonic,.*', '^ralink,.*', '^ramtron,.*', '^raspberrypi,.*', '^raydium,.*', '^rda,.*', '^realtek,.*', '^remarkable,.*', '^renesas,.*', '^rervision,.*', '^revotics,.*', '^rex,.*', '^richtek,.*', '^ricoh,.*', '^rikomagic,.*', '^riot,.*', '^riscv,.*', '^rockchip,.*', '^rocktech,.*', '^rohm,.*', '^ronbo,.*', '^roofull,.*', '^roseapplepi,.*', '^rve,.*', '^saef,.*', '^samsung,.*', '^samtec,.*', '^sancloud,.*', '^sandisk,.*', '^satoz,.*', '^sbs,.*', '^schindler,.*', '^seagate,.*', '^seeed,.*', '^seirobotics,.*', '^semtech,.*', '^senseair,.*', '^sensirion,.*', '^sensortek,.*', '^sercomm,.*', '^sff,.*', '^sgd,.*', '^sgmicro,.*', '^sgx,.*', '^sharp,.*', '^shift,.*', '^shimafuji,.*', '^shineworld,.*', '^shiratech,.*', '^si-en,.*', '^si-linux,.*', '^siemens,.*', '^sifive,.*', '^sigma,.*', '^sii,.*', '^sil,.*', '^silabs,.*', '^silan,.*', '^silead,.*', '^silergy,.*', '^silex-insight,.*', '^siliconfile,.*', '^siliconmitus,.*', '^silvaco,.*', '^simtek,.*', '^sinlinx,.*', '^sinovoip,.*', '^sinowealth,.*', '^sipeed,.*', '^sirf,.*', '^sis,.*', '^sitronix,.*', '^skov,.*', '^skyworks,.*', '^smartlabs,.*', '^smi,.*', '^smsc,.*', '^snps,.*', '^sochip,.*', '^socionext,.*', '^solidrun,.*', '^solomon,.*', '^sony,.*', '^sophgo,.*', '^sourceparts,.*', '^spansion,.*', '^sparkfun,.*', '^spinalhdl,.*', '^sprd,.*', '^square,.*', '^ssi,.*', '^sst,.*', '^sstar,.*', '^st,.*', '^st-ericsson,.*', '^starfive,.*', '^starry,.*', '^startek,.*', '^starterkit,.*', '^ste,.*', '^stericsson,.*', '^storlink,.*', '^storm,.*', '^storopack,.*', '^summit,.*', '^sunchip,.*', '^sundance,.*', '^sunplus,.*', '^supermicro,.*', '^swir,.*', '^syna,.*', '^synology,.*', '^synopsys,.*', '^tbs,.*', '^tbs-biometrics,.*', '^tcg,.*', '^tcl,.*', '^tcs,.*', '^tdo,.*', '^team-source-display,.*', '^technexion,.*', '^technologic,.*', '^techstar,.*', '^techwell,.*', '^teejet,.*', '^teltonika,.*', '^tempo,.*', '^terasic,.*', '^tesla,.*', '^tfc,.*', '^thead,.*', '^thine,.*', '^thingyjp,.*', '^thundercomm,.*', '^thwc,.*', '^ti,.*', '^tianma,.*', '^tlm,.*', '^tmt,.*', '^topeet,.*', '^topic,.*', '^toppoly,.*', '^topwise,.*', '^toradex,.*', '^toshiba,.*', '^toumaz,.*', '^tpk,.*', '^tplink,.*', '^tpo,.*', '^tq,.*', '^transpeed,.*', '^traverse,.*', '^tronfy,.*', '^tronsmart,.*', '^truly,.*', '^tsd,.*', '^turing,.*', '^tyan,.*', '^u-blox,.*', '^u-boot,.*', '^ubnt,.*', '^ucrobotics,.*', '^udoo,.*', '^ufispace,.*', '^ugoos,.*', '^uniwest,.*', '^upisemi,.*', '^urt,.*', '^usi,.*', '^usr,.*', '^utoo,.*', '^v3,.*', '^vaisala,.*', '^vamrs,.*', '^variscite,.*', '^vdl,.*', '^vertexcom,.*', '^via,.*', '^vialab,.*', '^vicor,.*', '^videostrong,.*', '^virtio,.*', '^virtual,.*', '^vishay,.*', '^visionox,.*', '^vitesse,.*', '^vivante,.*', '^vivax,.*', '^vocore,.*', '^voipac,.*', '^vot,.*', '^vxt,.*', '^wanchanglong,.*', '^wand,.*', '^waveshare,.*', '^wd,.*', '^we,.*', '^welltech,.*', '^wetek,.*', '^wexler,.*', '^whwave,.*', '^wi2wi,.*', '^widora,.*', '^wiligear,.*', '^willsemi,.*', '^winbond,.*', '^wingtech,.*', '^winlink,.*', '^winstar,.*', '^wirelesstag,.*', '^wits,.*', '^wlf,.*', '^wm,.*', '^wobo,.*', '^x-powers,.*', '^xen,.*', '^xes,.*', '^xiaomi,.*', '^xillybus,.*', '^xingbangda,.*', '^xinpeng,.*', '^xiphera,.*', '^xlnx,.*', '^xnano,.*', '^xunlong,.*', '^xylon,.*', '^yadro,.*', '^yamaha,.*', '^yes-optoelectronics,.*', '^yic,.*', '^yiming,.*', '^ylm,.*', '^yna,.*', '^yones-toptech,.*', '^ys,.*', '^ysoft,.*', '^zarlink,.*', '^zealz,.*', '^zeitec,.*', '^zidoo,.*', '^zii,.*', '^zinitix,.*', '^zkmagic,.*', '^zte,.*', '^zyxel,.*', 'pinctrl-[0-9]+'
	from schema $id: http://devicetree.org/schemas/vendor-prefixes.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240215-mbly-i2c-v1-2-19a336e91dca@bootlin.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH 02/13] dt-bindings: i2c: nomadik: add mobileye,eyeq5-i2c bindings and example

[Rob Herring]

On Thu, Feb 15, 2024 at 05:52:09PM +0100, Théo Lebrun wrote:
> Add EyeQ5 bindings to the existing Nomadik I2C dt-bindings. Add the two
> EyeQ5-specific properties behind a conditional. Add an example for this
> compatible.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  .../devicetree/bindings/i2c/st,nomadik-i2c.yaml    | 44 ++++++++++++++++++++--
>  1 file changed, 40 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml b/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml
> index e6b95e3765ac..eaade14b6d4c 100644
> --- a/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml
> +++ b/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml
> @@ -14,9 +14,6 @@ description: The Nomadik I2C host controller began its life in the ST
>  maintainers:
>    - Linus Walleij <linus.walleij@linaro.org>
>  
> -allOf:
> -  - $ref: /schemas/i2c/i2c-controller.yaml#
> -
>  # Need a custom select here or 'arm,primecell' will match on lots of nodes
>  select:
>    properties:
> @@ -24,6 +21,7 @@ select:
>        contains:
>          enum:
>            - st,nomadik-i2c
> +          - mobileye,eyeq5-i2c
>    required:
>      - compatible
>  
> @@ -39,6 +37,10 @@ properties:
>            - const: stericsson,db8500-i2c
>            - const: st,nomadik-i2c
>            - const: arm,primecell
> +      # The variant found on Mobileye EyeQ5
> +      - items:
> +          - const: mobileye,eyeq5-i2c
> +          - const: arm,primecell
>  
>    reg:
>      maxItems: 1
> @@ -55,7 +57,7 @@ properties:
>        - items:
>            - const: mclk
>            - const: apb_pclk
> -      # Clock name in DB8500
> +      # Clock name in DB8500 or EyeQ5
>        - items:
>            - const: i2cclk
>            - const: apb_pclk
> @@ -83,6 +85,25 @@ required:
>  
>  unevaluatedProperties: false
>  
> +allOf:
> +  - $ref: /schemas/i2c/i2c-controller.yaml#
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: mobileye,eyeq5-i2c
> +    then:
> +      properties:
> +        mobileye,olb:
> +          $ref: /schemas/types.yaml#/definitions/phandle
> +          description: A phandle to the OLB syscon.

Define properties at the top-level and then restrict them in if/then 
schemas.

> +        mobileye,id:
> +          $ref: /schemas/types.yaml#/definitions/uint32
> +          description: Platform-wide controller ID (integer starting from zero).

instance indexes are a NAK. You can use i2cN aliases if you must.

Why do you need it? To access OLB? If so, add cell args to the OLB 
phandle instead.

Rob

Re: [PATCH 02/13] dt-bindings: i2c: nomadik: add mobileye,eyeq5-i2c bindings and example

[Théo Lebrun]

Hello,

On Fri Feb 16, 2024 at 3:22 AM CET, Rob Herring wrote:
> On Thu, Feb 15, 2024 at 05:52:09PM +0100, Théo Lebrun wrote:
> > Add EyeQ5 bindings to the existing Nomadik I2C dt-bindings. Add the two
> > EyeQ5-specific properties behind a conditional. Add an example for this
> > compatible.

[...]

> > +allOf:
> > +  - $ref: /schemas/i2c/i2c-controller.yaml#
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: mobileye,eyeq5-i2c
> > +    then:
> > +      properties:
> > +        mobileye,olb:
> > +          $ref: /schemas/types.yaml#/definitions/phandle
> > +          description: A phandle to the OLB syscon.
>
> Define properties at the top-level and then restrict them in if/then 
> schemas.

Noted, thanks.

> > +        mobileye,id:
> > +          $ref: /schemas/types.yaml#/definitions/uint32
> > +          description: Platform-wide controller ID (integer starting from zero).
>
> instance indexes are a NAK. You can use i2cN aliases if you must.
>
> Why do you need it? To access OLB? If so, add cell args to the OLB 
> phandle instead.

Why we do what we do: I2C controller must write a 2 bit value depending
on the bus speed. All I2C controllers write into the same register.
Index is used to compute the shift.

mobileye,olb is a phandle to a syscon. I'll be using i2cN aliases I
guess.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 02/13] dt-bindings: i2c: nomadik: add mobileye,eyeq5-i2c bindings and example

[Krzysztof Kozlowski]

On 16/02/2024 11:18, Théo Lebrun wrote:
> 
>>> +        mobileye,id:
>>> +          $ref: /schemas/types.yaml#/definitions/uint32
>>> +          description: Platform-wide controller ID (integer starting from zero).
>>
>> instance indexes are a NAK. You can use i2cN aliases if you must.
>>
>> Why do you need it? To access OLB? If so, add cell args to the OLB 
>> phandle instead.
> 
> Why we do what we do: I2C controller must write a 2 bit value depending
> on the bus speed. All I2C controllers write into the same register.

Which register?  Your devices do not share IO address space.

> Index is used to compute the shift.
> 
> mobileye,olb is a phandle to a syscon. I'll be using i2cN aliases I
> guess.
> 
> Thanks,
> 
> --
> Théo Lebrun, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Best regards,
Krzysztof

Re: [PATCH 02/13] dt-bindings: i2c: nomadik: add mobileye,eyeq5-i2c bindings and example

[Théo Lebrun]

Hello,

On Fri Feb 16, 2024 at 11:33 AM CET, Krzysztof Kozlowski wrote:
> On 16/02/2024 11:18, Théo Lebrun wrote:
> > 
> >>> +        mobileye,id:
> >>> +          $ref: /schemas/types.yaml#/definitions/uint32
> >>> +          description: Platform-wide controller ID (integer starting from zero).
> >>
> >> instance indexes are a NAK. You can use i2cN aliases if you must.
> >>
> >> Why do you need it? To access OLB? If so, add cell args to the OLB 
> >> phandle instead.
> > 
> > Why we do what we do: I2C controller must write a 2 bit value depending
> > on the bus speed. All I2C controllers write into the same register.
>
> Which register?  Your devices do not share IO address space.

mobileye,olb is a prop with a phandle to a syscon. That syscon contains
the register we are interested in.

The Linux code side of things is in the following patch. We use
syscon_regmap_lookup_by_phandle().

   [PATCH 10/13] i2c: nomadik: support Mobileye EyeQ5 I2C controller
   https://lore.kernel.org/lkml/20240215-mbly-i2c-v1-10-19a336e91dca@bootlin.com/

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 02/13] dt-bindings: i2c: nomadik: add mobileye,eyeq5-i2c bindings and example

[Krzysztof Kozlowski]

On 16/02/2024 11:40, Théo Lebrun wrote:
> Hello,
> 
> On Fri Feb 16, 2024 at 11:33 AM CET, Krzysztof Kozlowski wrote:
>> On 16/02/2024 11:18, Théo Lebrun wrote:
>>>
>>>>> +        mobileye,id:
>>>>> +          $ref: /schemas/types.yaml#/definitions/uint32
>>>>> +          description: Platform-wide controller ID (integer starting from zero).
>>>>
>>>> instance indexes are a NAK. You can use i2cN aliases if you must.
>>>>
>>>> Why do you need it? To access OLB? If so, add cell args to the OLB 
>>>> phandle instead.
>>>
>>> Why we do what we do: I2C controller must write a 2 bit value depending
>>> on the bus speed. All I2C controllers write into the same register.
>>
>> Which register?  Your devices do not share IO address space.
> 
> mobileye,olb is a prop with a phandle to a syscon. That syscon contains
> the register we are interested in.

So exactly what Rob said... I don't understand why you have chosen to go
with alias.


Best regards,
Krzysztof

Re: [PATCH 02/13] dt-bindings: i2c: nomadik: add mobileye,eyeq5-i2c bindings and example

[Théo Lebrun]

Hello,

On Sat Feb 17, 2024 at 9:25 AM CET, Krzysztof Kozlowski wrote:
> On 16/02/2024 11:40, Théo Lebrun wrote:
> > On Fri Feb 16, 2024 at 11:33 AM CET, Krzysztof Kozlowski wrote:
> >> On 16/02/2024 11:18, Théo Lebrun wrote:
> >>>
> >>>>> +        mobileye,id:
> >>>>> +          $ref: /schemas/types.yaml#/definitions/uint32
> >>>>> +          description: Platform-wide controller ID (integer starting from zero).
> >>>>
> >>>> instance indexes are a NAK. You can use i2cN aliases if you must.
> >>>>
> >>>> Why do you need it? To access OLB? If so, add cell args to the OLB 
> >>>> phandle instead.
> >>>
> >>> Why we do what we do: I2C controller must write a 2 bit value depending
> >>> on the bus speed. All I2C controllers write into the same register.
> >>
> >> Which register?  Your devices do not share IO address space.
> > 
> > mobileye,olb is a prop with a phandle to a syscon. That syscon contains
> > the register we are interested in.
>
> So exactly what Rob said... I don't understand why you have chosen to go
> with alias.

I had misunderstood Rob's original message. Now that I've done some
tests to use cells I get what was meant. I'd have a follow-up question.
What should the cells contain? I see two options:

 - phandle + I2C controller global index (from 0 thru 4). Then Linux
   (or other) driver know how to map that index to register + mask
   combo. ie:

      i2c2: i2c@500000 {
         compatible = "mobileye,eyeq5-i2c", "arm,primecell";
         reg = <0 0x500000 0x0 0x1000>;
         /* ... */
         mobileye,olb = <&olb 2>;
      };

 - phandle + register offset + mask. ie:

      i2c2: i2c@500000 {
         compatible = "mobileye,eyeq5-i2c", "arm,primecell";
         reg = <0 0x500000 0x0 0x1000>;
         /* ... */
         mobileye,olb = <&olb 0xB8 0x300>; /* phandle + offset + mask */
      };

I would have guessed the second approach was frown upon as DT aren't
meant to contain iomem offsets. However I'm seeing quite a few drivers
using this approach, and no driver doing the first approach. Maybe my
instinct isn't leading me the right way.

See those bindings that use the second approach. They were found because
their drivers use the syscon_regmap_lookup_by_phandle_args() function
call. I've added the file creation date to highlight recent bindings
(that hopefully are closer to the right way).
 - phy/starfive,jh7110-pcie-phy.yaml    2023-06-29T15:51:12+08:00
 - usb/starfive,jh7110-usb.yaml         2023-05-18T19:27:48+08:00
 - net/starfive,jh7110-dwmac.yaml       2023-04-17T18:02:49+08:00
 - phy/qcom,sc8280xp-qmp-pcie-phy.yaml  2022-11-05T15:59:34+01:00
 - sound/snps,designware-i2s.yaml       2022-07-01T20:22:49+01:00
 - pinctrl/canaan,k210-fpioa.yaml       2020-12-13T22:50:44+09:00
 - media/ti,cal.yaml                    2019-11-12T15:53:47+01:00

I know looking at existing drivers/bindings isn't the right way, but I
have no other frame of reference. That's why I'm asking for guidance on
this one.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

------------------------------------------------------------------------

Re: [PATCH 02/13] dt-bindings: i2c: nomadik: add mobileye,eyeq5-i2c bindings and example

[Krzysztof Kozlowski]

On 19/02/2024 14:41, Théo Lebrun wrote:
> Hello,
> 
> On Sat Feb 17, 2024 at 9:25 AM CET, Krzysztof Kozlowski wrote:
>> On 16/02/2024 11:40, Théo Lebrun wrote:
>>> On Fri Feb 16, 2024 at 11:33 AM CET, Krzysztof Kozlowski wrote:
>>>> On 16/02/2024 11:18, Théo Lebrun wrote:
>>>>>
>>>>>>> +        mobileye,id:
>>>>>>> +          $ref: /schemas/types.yaml#/definitions/uint32
>>>>>>> +          description: Platform-wide controller ID (integer starting from zero).
>>>>>>
>>>>>> instance indexes are a NAK. You can use i2cN aliases if you must.
>>>>>>
>>>>>> Why do you need it? To access OLB? If so, add cell args to the OLB 
>>>>>> phandle instead.
>>>>>
>>>>> Why we do what we do: I2C controller must write a 2 bit value depending
>>>>> on the bus speed. All I2C controllers write into the same register.
>>>>
>>>> Which register?  Your devices do not share IO address space.
>>>
>>> mobileye,olb is a prop with a phandle to a syscon. That syscon contains
>>> the register we are interested in.
>>
>> So exactly what Rob said... I don't understand why you have chosen to go
>> with alias.
> 
> I had misunderstood Rob's original message. Now that I've done some
> tests to use cells I get what was meant. I'd have a follow-up question.
> What should the cells contain? I see two options:
> 
>  - phandle + I2C controller global index (from 0 thru 4). Then Linux
>    (or other) driver know how to map that index to register + mask
>    combo. ie:
> 
>       i2c2: i2c@500000 {
>          compatible = "mobileye,eyeq5-i2c", "arm,primecell";
>          reg = <0 0x500000 0x0 0x1000>;
>          /* ... */
>          mobileye,olb = <&olb 2>;
>       };
> 
>  - phandle + register offset + mask. ie:
> 
>       i2c2: i2c@500000 {
>          compatible = "mobileye,eyeq5-i2c", "arm,primecell";
>          reg = <0 0x500000 0x0 0x1000>;
>          /* ... */
>          mobileye,olb = <&olb 0xB8 0x300>; /* phandle + offset + mask */
>       };

Whichever works for your current and possibly future needs and hardware,
because property should have one meaning. It's anyway specific to the
property. Second option is quite popular. Please design it for entire
hardware, not for this one particular case.

Best regards,
Krzysztof

[PATCH 03/13] dt-bindings: hwmon: lm75: add label property

[Théo Lebrun]

Declare optional label devicetree property. Show usage in one example
with dummy name.

To: Jean Delvare <jdelvare@suse.com>
To: Guenter Roeck <linux@roeck-us.net>
Cc: <linux-hwmon@vger.kernel.org>
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 Documentation/devicetree/bindings/hwmon/lm75.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/hwmon/lm75.yaml b/Documentation/devicetree/bindings/hwmon/lm75.yaml
index ed269e428a3d..5ca2c83b413d 100644
--- a/Documentation/devicetree/bindings/hwmon/lm75.yaml
+++ b/Documentation/devicetree/bindings/hwmon/lm75.yaml
@@ -52,6 +52,9 @@ properties:
   interrupts:
     maxItems: 1
 
+  label:
+    description: user-facing name of the hardware monitor
+
 required:
   - compatible
   - reg
@@ -83,6 +86,7 @@ examples:
         compatible = "st,stlm75";
         reg = <0x48>;
         vs-supply = <&vs>;
+        label = "CPU temp";
       };
     };
   - |

-- 
2.43.1

Re: [PATCH 03/13] dt-bindings: hwmon: lm75: add label property

[Krzysztof Kozlowski]

On 15/02/2024 17:52, Théo Lebrun wrote:
> Declare optional label devicetree property. Show usage in one example
> with dummy name.
> 
> To: Jean Delvare <jdelvare@suse.com>
> To: Guenter Roeck <linux@roeck-us.net>
> Cc: <linux-hwmon@vger.kernel.org>
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH 03/13] dt-bindings: hwmon: lm75: add label property

[Krzysztof Kozlowski]

On 16/02/2024 08:41, Krzysztof Kozlowski wrote:
> On 15/02/2024 17:52, Théo Lebrun wrote:
>> Declare optional label devicetree property. Show usage in one example
>> with dummy name.
>>
>> To: Jean Delvare <jdelvare@suse.com>
>> To: Guenter Roeck <linux@roeck-us.net>
>> Cc: <linux-hwmon@vger.kernel.org>
>> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
>> ---
> 
> 
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> 

Although you might need to re-work this patch if this gets accepted:

https://lore.kernel.org/linux-devicetree/20240216-dt-bindings-hwmon-common-v1-0-3c2c24ff1260@linaro.org/T/#m341d8f47ca5badd2637cd662fb331f144e16a54c

Best regards,
Krzysztof

Re: [PATCH 03/13] dt-bindings: hwmon: lm75: add label property

[Théo Lebrun]

Hello,

On Fri Feb 16, 2024 at 9:00 AM CET, Krzysztof Kozlowski wrote:
> On 16/02/2024 08:41, Krzysztof Kozlowski wrote:
> > On 15/02/2024 17:52, Théo Lebrun wrote:
> >> Declare optional label devicetree property. Show usage in one example
> >> with dummy name.
> >>
> >> To: Jean Delvare <jdelvare@suse.com>
> >> To: Guenter Roeck <linux@roeck-us.net>
> >> Cc: <linux-hwmon@vger.kernel.org>
> >> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> >> ---
> > 
> > 
> > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > 
>
> Although you might need to re-work this patch if this gets accepted:
>
> https://lore.kernel.org/linux-devicetree/20240216-dt-bindings-hwmon-common-v1-0-3c2c24ff1260@linaro.org/T/#m341d8f47ca5badd2637cd662fb331f144e16a54c

Cool that this is made generic. Do not hesitate to Cc me in follow-ups.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 03/13] dt-bindings: hwmon: lm75: add label property

[Guenter Roeck]

On Thu, Feb 15, 2024 at 05:52:10PM +0100, Théo Lebrun wrote:
> Declare optional label devicetree property. Show usage in one example
> with dummy name.
> 
> To: Jean Delvare <jdelvare@suse.com>
> To: Guenter Roeck <linux@roeck-us.net>
> Cc: <linux-hwmon@vger.kernel.org>
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

What is the verdict here ? Should I apply it and accept another patch
to update it, or should it be reworked to be based on the new common
hwmon schema ?

Thanks,
Guenter

> ---
>  Documentation/devicetree/bindings/hwmon/lm75.yaml | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/lm75.yaml b/Documentation/devicetree/bindings/hwmon/lm75.yaml
> index ed269e428a3d..5ca2c83b413d 100644
> --- a/Documentation/devicetree/bindings/hwmon/lm75.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/lm75.yaml
> @@ -52,6 +52,9 @@ properties:
>    interrupts:
>      maxItems: 1
>  
> +  label:
> +    description: user-facing name of the hardware monitor
> +
>  required:
>    - compatible
>    - reg
> @@ -83,6 +86,7 @@ examples:
>          compatible = "st,stlm75";
>          reg = <0x48>;
>          vs-supply = <&vs>;
> +        label = "CPU temp";
>        };
>      };
>    - |

Re: [PATCH 03/13] dt-bindings: hwmon: lm75: add label property

[Krzysztof Kozlowski]

On 24/02/2024 18:30, Guenter Roeck wrote:
> On Thu, Feb 15, 2024 at 05:52:10PM +0100, Théo Lebrun wrote:
>> Declare optional label devicetree property. Show usage in one example
>> with dummy name.
>>
>> To: Jean Delvare <jdelvare@suse.com>
>> To: Guenter Roeck <linux@roeck-us.net>
>> Cc: <linux-hwmon@vger.kernel.org>
>> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> 
> What is the verdict here ? Should I apply it and accept another patch
> to update it, or should it be reworked to be based on the new common
> hwmon schema ?

This should be reworked based on the new common schema (just like my
follow up patches did for some of the bindings).

Best regards,
Krzysztof

[PATCH 04/13] i2c: nomadik: rename private struct pointers from dev to priv

[Théo Lebrun]

Disambiguate the usage of dev as a variable name; it is usually best to
keep it reserved for struct device pointers. Avoid having multiple
names for the same struct pointer (previously: dev, nmk, nmk_i2c).

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/i2c/busses/i2c-nomadik.c | 428 +++++++++++++++++++--------------------
 1 file changed, 214 insertions(+), 214 deletions(-)

diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index b10574d42b7a..cd511c884f99 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -206,12 +206,12 @@ static inline void i2c_clr_bit(void __iomem *reg, u32 mask)
 
 /**
  * flush_i2c_fifo() - This function flushes the I2C FIFO
- * @dev: private data of I2C Driver
+ * @priv: private data of I2C Driver
  *
  * This function flushes the I2C Tx and Rx FIFOs. It returns
  * 0 on successful flushing of FIFO
  */
-static int flush_i2c_fifo(struct nmk_i2c_dev *dev)
+static int flush_i2c_fifo(struct nmk_i2c_dev *priv)
 {
 #define LOOP_ATTEMPTS 10
 	int i;
@@ -224,19 +224,19 @@ static int flush_i2c_fifo(struct nmk_i2c_dev *dev)
 	 * bits, until then no one must access Tx, Rx FIFO and
 	 * should poll on these bits waiting for the completion.
 	 */
-	writel((I2C_CR_FTX | I2C_CR_FRX), dev->virtbase + I2C_CR);
+	writel((I2C_CR_FTX | I2C_CR_FRX), priv->virtbase + I2C_CR);
 
 	for (i = 0; i < LOOP_ATTEMPTS; i++) {
-		timeout = jiffies + dev->adap.timeout;
+		timeout = jiffies + priv->adap.timeout;
 
 		while (!time_after(jiffies, timeout)) {
-			if ((readl(dev->virtbase + I2C_CR) &
+			if ((readl(priv->virtbase + I2C_CR) &
 				(I2C_CR_FTX | I2C_CR_FRX)) == 0)
-					return 0;
+				return 0;
 		}
 	}
 
-	dev_err(&dev->adev->dev,
+	dev_err(&priv->adev->dev,
 		"flushing operation timed out giving up after %d attempts",
 		LOOP_ATTEMPTS);
 
@@ -245,45 +245,45 @@ static int flush_i2c_fifo(struct nmk_i2c_dev *dev)
 
 /**
  * disable_all_interrupts() - Disable all interrupts of this I2c Bus
- * @dev: private data of I2C Driver
+ * @priv: private data of I2C Driver
  */
-static void disable_all_interrupts(struct nmk_i2c_dev *dev)
+static void disable_all_interrupts(struct nmk_i2c_dev *priv)
 {
 	u32 mask = IRQ_MASK(0);
-	writel(mask, dev->virtbase + I2C_IMSCR);
+	writel(mask, priv->virtbase + I2C_IMSCR);
 }
 
 /**
  * clear_all_interrupts() - Clear all interrupts of I2C Controller
- * @dev: private data of I2C Driver
+ * @priv: private data of I2C Driver
  */
-static void clear_all_interrupts(struct nmk_i2c_dev *dev)
+static void clear_all_interrupts(struct nmk_i2c_dev *priv)
 {
 	u32 mask;
 	mask = IRQ_MASK(I2C_CLEAR_ALL_INTS);
-	writel(mask, dev->virtbase + I2C_ICR);
+	writel(mask, priv->virtbase + I2C_ICR);
 }
 
 /**
  * init_hw() - initialize the I2C hardware
- * @dev: private data of I2C Driver
+ * @priv: private data of I2C Driver
  */
-static int init_hw(struct nmk_i2c_dev *dev)
+static int init_hw(struct nmk_i2c_dev *priv)
 {
 	int stat;
 
-	stat = flush_i2c_fifo(dev);
+	stat = flush_i2c_fifo(priv);
 	if (stat)
 		goto exit;
 
 	/* disable the controller */
-	i2c_clr_bit(dev->virtbase + I2C_CR, I2C_CR_PE);
+	i2c_clr_bit(priv->virtbase + I2C_CR, I2C_CR_PE);
 
-	disable_all_interrupts(dev);
+	disable_all_interrupts(priv);
 
-	clear_all_interrupts(dev);
+	clear_all_interrupts(priv);
 
-	dev->cli.operation = I2C_NO_OPERATION;
+	priv->cli.operation = I2C_NO_OPERATION;
 
 exit:
 	return stat;
@@ -294,15 +294,15 @@ static int init_hw(struct nmk_i2c_dev *dev)
 
 /**
  * load_i2c_mcr_reg() - load the MCR register
- * @dev: private data of controller
+ * @priv: private data of controller
  * @flags: message flags
  */
-static u32 load_i2c_mcr_reg(struct nmk_i2c_dev *dev, u16 flags)
+static u32 load_i2c_mcr_reg(struct nmk_i2c_dev *priv, u16 flags)
 {
 	u32 mcr = 0;
 	unsigned short slave_adr_3msb_bits;
 
-	mcr |= GEN_MASK(dev->cli.slave_adr, I2C_MCR_A7, 1);
+	mcr |= GEN_MASK(priv->cli.slave_adr, I2C_MCR_A7, 1);
 
 	if (unlikely(flags & I2C_M_TEN)) {
 		/* 10-bit address transaction */
@@ -313,7 +313,7 @@ static u32 load_i2c_mcr_reg(struct nmk_i2c_dev *dev, u16 flags)
 		 * the extension (MSB bits) of the 7 bit address loaded
 		 * in A7
 		 */
-		slave_adr_3msb_bits = (dev->cli.slave_adr >> 7) & 0x7;
+		slave_adr_3msb_bits = (priv->cli.slave_adr >> 7) & 0x7;
 
 		mcr |= GEN_MASK(slave_adr_3msb_bits, I2C_MCR_EA10, 8);
 	} else {
@@ -325,40 +325,40 @@ static u32 load_i2c_mcr_reg(struct nmk_i2c_dev *dev, u16 flags)
 	mcr |= GEN_MASK(0, I2C_MCR_SB, 11);
 
 	/* check the operation, master read/write? */
-	if (dev->cli.operation == I2C_WRITE)
+	if (priv->cli.operation == I2C_WRITE)
 		mcr |= GEN_MASK(I2C_WRITE, I2C_MCR_OP, 0);
 	else
 		mcr |= GEN_MASK(I2C_READ, I2C_MCR_OP, 0);
 
 	/* stop or repeated start? */
-	if (dev->stop)
+	if (priv->stop)
 		mcr |= GEN_MASK(1, I2C_MCR_STOP, 14);
 	else
 		mcr &= ~(GEN_MASK(1, I2C_MCR_STOP, 14));
 
-	mcr |= GEN_MASK(dev->cli.count, I2C_MCR_LENGTH, 15);
+	mcr |= GEN_MASK(priv->cli.count, I2C_MCR_LENGTH, 15);
 
 	return mcr;
 }
 
 /**
  * setup_i2c_controller() - setup the controller
- * @dev: private data of controller
+ * @priv: private data of controller
  */
-static void setup_i2c_controller(struct nmk_i2c_dev *dev)
+static void setup_i2c_controller(struct nmk_i2c_dev *priv)
 {
 	u32 brcr1, brcr2;
 	u32 i2c_clk, div;
 	u32 ns;
 	u16 slsu;
 
-	writel(0x0, dev->virtbase + I2C_CR);
-	writel(0x0, dev->virtbase + I2C_HSMCR);
-	writel(0x0, dev->virtbase + I2C_TFTR);
-	writel(0x0, dev->virtbase + I2C_RFTR);
-	writel(0x0, dev->virtbase + I2C_DMAR);
+	writel(0x0, priv->virtbase + I2C_CR);
+	writel(0x0, priv->virtbase + I2C_HSMCR);
+	writel(0x0, priv->virtbase + I2C_TFTR);
+	writel(0x0, priv->virtbase + I2C_RFTR);
+	writel(0x0, priv->virtbase + I2C_DMAR);
 
-	i2c_clk = clk_get_rate(dev->clk);
+	i2c_clk = clk_get_rate(priv->clk);
 
 	/*
 	 * set the slsu:
@@ -373,7 +373,7 @@ static void setup_i2c_controller(struct nmk_i2c_dev *dev)
 	 * slsu = cycles / (1000000000 / f) + 1
 	 */
 	ns = DIV_ROUND_UP_ULL(1000000000ULL, i2c_clk);
-	switch (dev->sm) {
+	switch (priv->sm) {
 	case I2C_FREQ_MODE_FAST:
 	case I2C_FREQ_MODE_FAST_PLUS:
 		slsu = DIV_ROUND_UP(100, ns); /* Fast */
@@ -388,15 +388,15 @@ static void setup_i2c_controller(struct nmk_i2c_dev *dev)
 	}
 	slsu += 1;
 
-	dev_dbg(&dev->adev->dev, "calculated SLSU = %04x\n", slsu);
-	writel(slsu << 16, dev->virtbase + I2C_SCR);
+	dev_dbg(&priv->adev->dev, "calculated SLSU = %04x\n", slsu);
+	writel(slsu << 16, priv->virtbase + I2C_SCR);
 
 	/*
 	 * The spec says, in case of std. mode the divider is
 	 * 2 whereas it is 3 for fast and fastplus mode of
 	 * operation. TODO - high speed support.
 	 */
-	div = (dev->clk_freq > I2C_MAX_STANDARD_MODE_FREQ) ? 3 : 2;
+	div = (priv->clk_freq > I2C_MAX_STANDARD_MODE_FREQ) ? 3 : 2;
 
 	/*
 	 * generate the mask for baud rate counters. The controller
@@ -406,10 +406,10 @@ static void setup_i2c_controller(struct nmk_i2c_dev *dev)
 	 * so set brcr1 to 0.
 	 */
 	brcr1 = 0 << 16;
-	brcr2 = (i2c_clk/(dev->clk_freq * div)) & 0xffff;
+	brcr2 = (i2c_clk / (priv->clk_freq * div)) & 0xffff;
 
 	/* set the baud rate counter register */
-	writel((brcr1 | brcr2), dev->virtbase + I2C_BRCR);
+	writel((brcr1 | brcr2), priv->virtbase + I2C_BRCR);
 
 	/*
 	 * set the speed mode. Currently we support
@@ -417,125 +417,124 @@ static void setup_i2c_controller(struct nmk_i2c_dev *dev)
 	 * TODO - support for fast mode plus (up to 1Mb/s)
 	 * and high speed (up to 3.4 Mb/s)
 	 */
-	if (dev->sm > I2C_FREQ_MODE_FAST) {
-		dev_err(&dev->adev->dev,
+	if (priv->sm > I2C_FREQ_MODE_FAST) {
+		dev_err(&priv->adev->dev,
 			"do not support this mode defaulting to std. mode\n");
 		brcr2 = i2c_clk / (I2C_MAX_STANDARD_MODE_FREQ * 2) & 0xffff;
-		writel((brcr1 | brcr2), dev->virtbase + I2C_BRCR);
+		writel((brcr1 | brcr2), priv->virtbase + I2C_BRCR);
 		writel(I2C_FREQ_MODE_STANDARD << 4,
-				dev->virtbase + I2C_CR);
+				priv->virtbase + I2C_CR);
 	}
-	writel(dev->sm << 4, dev->virtbase + I2C_CR);
+	writel(priv->sm << 4, priv->virtbase + I2C_CR);
 
 	/* set the Tx and Rx FIFO threshold */
-	writel(dev->tft, dev->virtbase + I2C_TFTR);
-	writel(dev->rft, dev->virtbase + I2C_RFTR);
+	writel(priv->tft, priv->virtbase + I2C_TFTR);
+	writel(priv->rft, priv->virtbase + I2C_RFTR);
 }
 
 /**
  * read_i2c() - Read from I2C client device
- * @dev: private data of I2C Driver
+ * @priv: private data of I2C Driver
  * @flags: message flags
  *
  * This function reads from i2c client device when controller is in
  * master mode. There is a completion timeout. If there is no transfer
  * before timeout error is returned.
  */
-static int read_i2c(struct nmk_i2c_dev *dev, u16 flags)
+static int read_i2c(struct nmk_i2c_dev *priv, u16 flags)
 {
 	int status = 0;
 	u32 mcr, irq_mask;
 	unsigned long timeout;
 
-	mcr = load_i2c_mcr_reg(dev, flags);
-	writel(mcr, dev->virtbase + I2C_MCR);
+	mcr = load_i2c_mcr_reg(priv, flags);
+	writel(mcr, priv->virtbase + I2C_MCR);
 
 	/* load the current CR value */
-	writel(readl(dev->virtbase + I2C_CR) | DEFAULT_I2C_REG_CR,
-			dev->virtbase + I2C_CR);
+	writel(readl(priv->virtbase + I2C_CR) | DEFAULT_I2C_REG_CR,
+	       priv->virtbase + I2C_CR);
 
 	/* enable the controller */
-	i2c_set_bit(dev->virtbase + I2C_CR, I2C_CR_PE);
+	i2c_set_bit(priv->virtbase + I2C_CR, I2C_CR_PE);
 
-	init_completion(&dev->xfer_complete);
+	init_completion(&priv->xfer_complete);
 
 	/* enable interrupts by setting the mask */
 	irq_mask = (I2C_IT_RXFNF | I2C_IT_RXFF |
 			I2C_IT_MAL | I2C_IT_BERR);
 
-	if (dev->stop || !dev->vendor->has_mtdws)
+	if (priv->stop || !priv->vendor->has_mtdws)
 		irq_mask |= I2C_IT_MTD;
 	else
 		irq_mask |= I2C_IT_MTDWS;
 
 	irq_mask = I2C_CLEAR_ALL_INTS & IRQ_MASK(irq_mask);
 
-	writel(readl(dev->virtbase + I2C_IMSCR) | irq_mask,
-			dev->virtbase + I2C_IMSCR);
+	writel(readl(priv->virtbase + I2C_IMSCR) | irq_mask,
+	       priv->virtbase + I2C_IMSCR);
 
 	timeout = wait_for_completion_timeout(
-		&dev->xfer_complete, dev->adap.timeout);
+		&priv->xfer_complete, priv->adap.timeout);
 
 	if (timeout == 0) {
 		/* Controller timed out */
-		dev_err(&dev->adev->dev, "read from slave 0x%x timed out\n",
-				dev->cli.slave_adr);
+		dev_err(&priv->adev->dev, "read from slave 0x%x timed out\n",
+			priv->cli.slave_adr);
 		status = -ETIMEDOUT;
 	}
 	return status;
 }
 
-static void fill_tx_fifo(struct nmk_i2c_dev *dev, int no_bytes)
+static void fill_tx_fifo(struct nmk_i2c_dev *priv, int no_bytes)
 {
 	int count;
 
 	for (count = (no_bytes - 2);
 			(count > 0) &&
-			(dev->cli.count != 0);
+			(priv->cli.count != 0);
 			count--) {
 		/* write to the Tx FIFO */
-		writeb(*dev->cli.buffer,
-			dev->virtbase + I2C_TFR);
-		dev->cli.buffer++;
-		dev->cli.count--;
-		dev->cli.xfer_bytes++;
+		writeb(*priv->cli.buffer, priv->virtbase + I2C_TFR);
+		priv->cli.buffer++;
+		priv->cli.count--;
+		priv->cli.xfer_bytes++;
 	}
 
 }
 
 /**
  * write_i2c() - Write data to I2C client.
- * @dev: private data of I2C Driver
+ * @priv: private data of I2C Driver
  * @flags: message flags
  *
  * This function writes data to I2C client
  */
-static int write_i2c(struct nmk_i2c_dev *dev, u16 flags)
+static int write_i2c(struct nmk_i2c_dev *priv, u16 flags)
 {
 	u32 status = 0;
 	u32 mcr, irq_mask;
 	unsigned long timeout;
 
-	mcr = load_i2c_mcr_reg(dev, flags);
+	mcr = load_i2c_mcr_reg(priv, flags);
 
-	writel(mcr, dev->virtbase + I2C_MCR);
+	writel(mcr, priv->virtbase + I2C_MCR);
 
 	/* load the current CR value */
-	writel(readl(dev->virtbase + I2C_CR) | DEFAULT_I2C_REG_CR,
-			dev->virtbase + I2C_CR);
+	writel(readl(priv->virtbase + I2C_CR) | DEFAULT_I2C_REG_CR,
+	       priv->virtbase + I2C_CR);
 
 	/* enable the controller */
-	i2c_set_bit(dev->virtbase + I2C_CR, I2C_CR_PE);
+	i2c_set_bit(priv->virtbase + I2C_CR, I2C_CR_PE);
 
-	init_completion(&dev->xfer_complete);
+	init_completion(&priv->xfer_complete);
 
 	/* enable interrupts by settings the masks */
 	irq_mask = (I2C_IT_TXFOVR | I2C_IT_MAL | I2C_IT_BERR);
 
 	/* Fill the TX FIFO with transmit data */
-	fill_tx_fifo(dev, MAX_I2C_FIFO_THRESHOLD);
+	fill_tx_fifo(priv, MAX_I2C_FIFO_THRESHOLD);
 
-	if (dev->cli.count != 0)
+	if (priv->cli.count != 0)
 		irq_mask |= I2C_IT_TXFNE;
 
 	/*
@@ -543,23 +542,23 @@ static int write_i2c(struct nmk_i2c_dev *dev, u16 flags)
 	 * set the MTDWS bit (Master Transaction Done Without Stop)
 	 * to start repeated start operation
 	 */
-	if (dev->stop || !dev->vendor->has_mtdws)
+	if (priv->stop || !priv->vendor->has_mtdws)
 		irq_mask |= I2C_IT_MTD;
 	else
 		irq_mask |= I2C_IT_MTDWS;
 
 	irq_mask = I2C_CLEAR_ALL_INTS & IRQ_MASK(irq_mask);
 
-	writel(readl(dev->virtbase + I2C_IMSCR) | irq_mask,
-			dev->virtbase + I2C_IMSCR);
+	writel(readl(priv->virtbase + I2C_IMSCR) | irq_mask,
+	       priv->virtbase + I2C_IMSCR);
 
 	timeout = wait_for_completion_timeout(
-		&dev->xfer_complete, dev->adap.timeout);
+		&priv->xfer_complete, priv->adap.timeout);
 
 	if (timeout == 0) {
 		/* Controller timed out */
-		dev_err(&dev->adev->dev, "write to slave 0x%x timed out\n",
-				dev->cli.slave_adr);
+		dev_err(&priv->adev->dev, "write to slave 0x%x timed out\n",
+			priv->cli.slave_adr);
 		status = -ETIMEDOUT;
 	}
 
@@ -568,28 +567,28 @@ static int write_i2c(struct nmk_i2c_dev *dev, u16 flags)
 
 /**
  * nmk_i2c_xfer_one() - transmit a single I2C message
- * @dev: device with a message encoded into it
+ * @priv: device with a message encoded into it
  * @flags: message flags
  */
-static int nmk_i2c_xfer_one(struct nmk_i2c_dev *dev, u16 flags)
+static int nmk_i2c_xfer_one(struct nmk_i2c_dev *priv, u16 flags)
 {
 	int status;
 
 	if (flags & I2C_M_RD) {
 		/* read operation */
-		dev->cli.operation = I2C_READ;
-		status = read_i2c(dev, flags);
+		priv->cli.operation = I2C_READ;
+		status = read_i2c(priv, flags);
 	} else {
 		/* write operation */
-		dev->cli.operation = I2C_WRITE;
-		status = write_i2c(dev, flags);
+		priv->cli.operation = I2C_WRITE;
+		status = write_i2c(priv, flags);
 	}
 
-	if (status || (dev->result)) {
+	if (status || priv->result) {
 		u32 i2c_sr;
 		u32 cause;
 
-		i2c_sr = readl(dev->virtbase + I2C_SR);
+		i2c_sr = readl(priv->virtbase + I2C_SR);
 		/*
 		 * Check if the controller I2C operation status
 		 * is set to ABORT(11b).
@@ -597,15 +596,15 @@ static int nmk_i2c_xfer_one(struct nmk_i2c_dev *dev, u16 flags)
 		if (((i2c_sr >> 2) & 0x3) == 0x3) {
 			/* get the abort cause */
 			cause =	(i2c_sr >> 4) & 0x7;
-			dev_err(&dev->adev->dev, "%s\n",
+			dev_err(&priv->adev->dev, "%s\n",
 				cause >= ARRAY_SIZE(abort_causes) ?
 				"unknown reason" :
 				abort_causes[cause]);
 		}
 
-		(void) init_hw(dev);
+		init_hw(priv);
 
-		status = status ? status : dev->result;
+		status = status ? status : priv->result;
 	}
 
 	return status;
@@ -663,24 +662,24 @@ static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap,
 {
 	int status = 0;
 	int i;
-	struct nmk_i2c_dev *dev = i2c_get_adapdata(i2c_adap);
+	struct nmk_i2c_dev *priv = i2c_get_adapdata(i2c_adap);
 	int j;
 
-	pm_runtime_get_sync(&dev->adev->dev);
+	pm_runtime_get_sync(&priv->adev->dev);
 
 	/* Attempt three times to send the message queue */
 	for (j = 0; j < 3; j++) {
 		/* setup the i2c controller */
-		setup_i2c_controller(dev);
+		setup_i2c_controller(priv);
 
 		for (i = 0; i < num_msgs; i++) {
-			dev->cli.slave_adr	= msgs[i].addr;
-			dev->cli.buffer		= msgs[i].buf;
-			dev->cli.count		= msgs[i].len;
-			dev->stop = (i < (num_msgs - 1)) ? 0 : 1;
-			dev->result = 0;
+			priv->cli.slave_adr	= msgs[i].addr;
+			priv->cli.buffer		= msgs[i].buf;
+			priv->cli.count		= msgs[i].len;
+			priv->stop = (i < (num_msgs - 1)) ? 0 : 1;
+			priv->result = 0;
 
-			status = nmk_i2c_xfer_one(dev, msgs[i].flags);
+			status = nmk_i2c_xfer_one(priv, msgs[i].flags);
 			if (status != 0)
 				break;
 		}
@@ -688,7 +687,7 @@ static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap,
 			break;
 	}
 
-	pm_runtime_put_sync(&dev->adev->dev);
+	pm_runtime_put_sync(&priv->adev->dev);
 
 	/* return the no. messages processed */
 	if (status)
@@ -699,14 +698,14 @@ static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap,
 
 /**
  * disable_interrupts() - disable the interrupts
- * @dev: private data of controller
+ * @priv: private data of controller
  * @irq: interrupt number
  */
-static int disable_interrupts(struct nmk_i2c_dev *dev, u32 irq)
+static int disable_interrupts(struct nmk_i2c_dev *priv, u32 irq)
 {
 	irq = IRQ_MASK(irq);
-	writel(readl(dev->virtbase + I2C_IMSCR) & ~(I2C_CLEAR_ALL_INTS & irq),
-			dev->virtbase + I2C_IMSCR);
+	writel(readl(priv->virtbase + I2C_IMSCR) & ~(I2C_CLEAR_ALL_INTS & irq),
+	       priv->virtbase + I2C_IMSCR);
 	return 0;
 }
 
@@ -723,17 +722,18 @@ static int disable_interrupts(struct nmk_i2c_dev *dev, u32 irq)
  */
 static irqreturn_t i2c_irq_handler(int irq, void *arg)
 {
-	struct nmk_i2c_dev *dev = arg;
+	struct nmk_i2c_dev *priv = arg;
+	struct device *dev = &priv->adev->dev;
 	u32 tft, rft;
 	u32 count;
 	u32 misr, src;
 
 	/* load Tx FIFO and Rx FIFO threshold values */
-	tft = readl(dev->virtbase + I2C_TFTR);
-	rft = readl(dev->virtbase + I2C_RFTR);
+	tft = readl(priv->virtbase + I2C_TFTR);
+	rft = readl(priv->virtbase + I2C_RFTR);
 
 	/* read interrupt status register */
-	misr = readl(dev->virtbase + I2C_MISR);
+	misr = readl(priv->virtbase + I2C_MISR);
 
 	src = __ffs(misr);
 	switch ((1 << src)) {
@@ -741,20 +741,20 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
 	/* Transmit FIFO nearly empty interrupt */
 	case I2C_IT_TXFNE:
 	{
-		if (dev->cli.operation == I2C_READ) {
+		if (priv->cli.operation == I2C_READ) {
 			/*
 			 * in read operation why do we care for writing?
 			 * so disable the Transmit FIFO interrupt
 			 */
-			disable_interrupts(dev, I2C_IT_TXFNE);
+			disable_interrupts(priv, I2C_IT_TXFNE);
 		} else {
-			fill_tx_fifo(dev, (MAX_I2C_FIFO_THRESHOLD - tft));
+			fill_tx_fifo(priv, (MAX_I2C_FIFO_THRESHOLD - tft));
 			/*
 			 * if done, close the transfer by disabling the
 			 * corresponding TXFNE interrupt
 			 */
-			if (dev->cli.count == 0)
-				disable_interrupts(dev,	I2C_IT_TXFNE);
+			if (priv->cli.count == 0)
+				disable_interrupts(priv,	I2C_IT_TXFNE);
 		}
 	}
 	break;
@@ -768,60 +768,59 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
 	case I2C_IT_RXFNF:
 		for (count = rft; count > 0; count--) {
 			/* Read the Rx FIFO */
-			*dev->cli.buffer = readb(dev->virtbase + I2C_RFR);
-			dev->cli.buffer++;
+			*priv->cli.buffer = readb(priv->virtbase + I2C_RFR);
+			priv->cli.buffer++;
 		}
-		dev->cli.count -= rft;
-		dev->cli.xfer_bytes += rft;
+		priv->cli.count -= rft;
+		priv->cli.xfer_bytes += rft;
 		break;
 
 	/* Rx FIFO full */
 	case I2C_IT_RXFF:
 		for (count = MAX_I2C_FIFO_THRESHOLD; count > 0; count--) {
-			*dev->cli.buffer = readb(dev->virtbase + I2C_RFR);
-			dev->cli.buffer++;
+			*priv->cli.buffer = readb(priv->virtbase + I2C_RFR);
+			priv->cli.buffer++;
 		}
-		dev->cli.count -= MAX_I2C_FIFO_THRESHOLD;
-		dev->cli.xfer_bytes += MAX_I2C_FIFO_THRESHOLD;
+		priv->cli.count -= MAX_I2C_FIFO_THRESHOLD;
+		priv->cli.xfer_bytes += MAX_I2C_FIFO_THRESHOLD;
 		break;
 
 	/* Master Transaction Done with/without stop */
 	case I2C_IT_MTD:
 	case I2C_IT_MTDWS:
-		if (dev->cli.operation == I2C_READ) {
-			while (!(readl(dev->virtbase + I2C_RISR)
+		if (priv->cli.operation == I2C_READ) {
+			while (!(readl(priv->virtbase + I2C_RISR)
 				 & I2C_IT_RXFE)) {
-				if (dev->cli.count == 0)
+				if (priv->cli.count == 0)
 					break;
-				*dev->cli.buffer =
-					readb(dev->virtbase + I2C_RFR);
-				dev->cli.buffer++;
-				dev->cli.count--;
-				dev->cli.xfer_bytes++;
+				*priv->cli.buffer =
+					readb(priv->virtbase + I2C_RFR);
+				priv->cli.buffer++;
+				priv->cli.count--;
+				priv->cli.xfer_bytes++;
 			}
 		}
 
-		disable_all_interrupts(dev);
-		clear_all_interrupts(dev);
+		disable_all_interrupts(priv);
+		clear_all_interrupts(priv);
 
-		if (dev->cli.count) {
-			dev->result = -EIO;
-			dev_err(&dev->adev->dev,
-				"%lu bytes still remain to be xfered\n",
-				dev->cli.count);
-			(void) init_hw(dev);
+		if (priv->cli.count) {
+			priv->result = -EIO;
+			dev_err(dev, "%lu bytes still remain to be xfered\n",
+				priv->cli.count);
+			init_hw(priv);
 		}
-		complete(&dev->xfer_complete);
+		complete(&priv->xfer_complete);
 
 		break;
 
 	/* Master Arbitration lost interrupt */
 	case I2C_IT_MAL:
-		dev->result = -EIO;
-		(void) init_hw(dev);
+		priv->result = -EIO;
+		init_hw(priv);
 
-		i2c_set_bit(dev->virtbase + I2C_ICR, I2C_IT_MAL);
-		complete(&dev->xfer_complete);
+		i2c_set_bit(priv->virtbase + I2C_ICR, I2C_IT_MAL);
+		complete(&priv->xfer_complete);
 
 		break;
 
@@ -831,13 +830,13 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
 	 * during the transaction.
 	 */
 	case I2C_IT_BERR:
-		dev->result = -EIO;
+		priv->result = -EIO;
 		/* get the status */
-		if (((readl(dev->virtbase + I2C_SR) >> 2) & 0x3) == I2C_ABORT)
-			(void) init_hw(dev);
+		if (((readl(priv->virtbase + I2C_SR) >> 2) & 0x3) == I2C_ABORT)
+			init_hw(priv);
 
-		i2c_set_bit(dev->virtbase + I2C_ICR, I2C_IT_BERR);
-		complete(&dev->xfer_complete);
+		i2c_set_bit(priv->virtbase + I2C_ICR, I2C_IT_BERR);
+		complete(&priv->xfer_complete);
 
 		break;
 
@@ -847,11 +846,11 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
 	 * the Tx FIFO is full.
 	 */
 	case I2C_IT_TXFOVR:
-		dev->result = -EIO;
-		(void) init_hw(dev);
+		priv->result = -EIO;
+		init_hw(priv);
 
-		dev_err(&dev->adev->dev, "Tx Fifo Over run\n");
-		complete(&dev->xfer_complete);
+		dev_err(dev, "Tx Fifo Over run\n");
+		complete(&priv->xfer_complete);
 
 		break;
 
@@ -863,10 +862,10 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
 	case I2C_IT_RFSE:
 	case I2C_IT_WTSR:
 	case I2C_IT_STD:
-		dev_err(&dev->adev->dev, "unhandled Interrupt\n");
+		dev_err(dev, "unhandled Interrupt\n");
 		break;
 	default:
-		dev_err(&dev->adev->dev, "spurious Interrupt..\n");
+		dev_err(dev, "spurious Interrupt..\n");
 		break;
 	}
 
@@ -893,9 +892,9 @@ static int nmk_i2c_resume_early(struct device *dev)
 static int nmk_i2c_runtime_suspend(struct device *dev)
 {
 	struct amba_device *adev = to_amba_device(dev);
-	struct nmk_i2c_dev *nmk_i2c = amba_get_drvdata(adev);
+	struct nmk_i2c_dev *priv = amba_get_drvdata(adev);
 
-	clk_disable_unprepare(nmk_i2c->clk);
+	clk_disable_unprepare(priv->clk);
 	pinctrl_pm_select_idle_state(dev);
 	return 0;
 }
@@ -903,10 +902,10 @@ static int nmk_i2c_runtime_suspend(struct device *dev)
 static int nmk_i2c_runtime_resume(struct device *dev)
 {
 	struct amba_device *adev = to_amba_device(dev);
-	struct nmk_i2c_dev *nmk_i2c = amba_get_drvdata(adev);
+	struct nmk_i2c_dev *priv = amba_get_drvdata(adev);
 	int ret;
 
-	ret = clk_prepare_enable(nmk_i2c->clk);
+	ret = clk_prepare_enable(priv->clk);
 	if (ret) {
 		dev_err(dev, "can't prepare_enable clock\n");
 		return ret;
@@ -914,9 +913,9 @@ static int nmk_i2c_runtime_resume(struct device *dev)
 
 	pinctrl_pm_select_default_state(dev);
 
-	ret = init_hw(nmk_i2c);
+	ret = init_hw(priv);
 	if (ret) {
-		clk_disable_unprepare(nmk_i2c->clk);
+		clk_disable_unprepare(priv->clk);
 		pinctrl_pm_select_idle_state(dev);
 	}
 
@@ -939,107 +938,108 @@ static const struct i2c_algorithm nmk_i2c_algo = {
 };
 
 static void nmk_i2c_of_probe(struct device_node *np,
-			     struct nmk_i2c_dev *nmk)
+			     struct nmk_i2c_dev *priv)
 {
 	/* Default to 100 kHz if no frequency is given in the node */
-	if (of_property_read_u32(np, "clock-frequency", &nmk->clk_freq))
-		nmk->clk_freq = I2C_MAX_STANDARD_MODE_FREQ;
+	if (of_property_read_u32(np, "clock-frequency", &priv->clk_freq))
+		priv->clk_freq = I2C_MAX_STANDARD_MODE_FREQ;
 
 	/* This driver only supports 'standard' and 'fast' modes of operation. */
-	if (nmk->clk_freq <= I2C_MAX_STANDARD_MODE_FREQ)
-		nmk->sm = I2C_FREQ_MODE_STANDARD;
+	if (priv->clk_freq <= I2C_MAX_STANDARD_MODE_FREQ)
+		priv->sm = I2C_FREQ_MODE_STANDARD;
 	else
-		nmk->sm = I2C_FREQ_MODE_FAST;
-	nmk->tft = 1; /* Tx FIFO threshold */
-	nmk->rft = 8; /* Rx FIFO threshold */
-	nmk->timeout = 200; /* Slave response timeout(ms) */
+		priv->sm = I2C_FREQ_MODE_FAST;
+	priv->tft = 1; /* Tx FIFO threshold */
+	priv->rft = 8; /* Rx FIFO threshold */
+	priv->timeout = 200; /* Slave response timeout(ms) */
 }
 
 static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
 {
 	int ret = 0;
+	struct nmk_i2c_dev *priv;
 	struct device_node *np = adev->dev.of_node;
-	struct nmk_i2c_dev	*dev;
+	struct device *dev = &adev->dev;
 	struct i2c_adapter *adap;
 	struct i2c_vendor_data *vendor = id->data;
 	u32 max_fifo_threshold = (vendor->fifodepth / 2) - 1;
 
-	dev = devm_kzalloc(&adev->dev, sizeof(*dev), GFP_KERNEL);
-	if (!dev)
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
 		return -ENOMEM;
 
-	dev->vendor = vendor;
-	dev->adev = adev;
-	nmk_i2c_of_probe(np, dev);
+	priv->vendor = vendor;
+	priv->adev = adev;
+	nmk_i2c_of_probe(np, priv);
 
-	if (dev->tft > max_fifo_threshold) {
-		dev_warn(&adev->dev, "requested TX FIFO threshold %u, adjusted down to %u\n",
-			 dev->tft, max_fifo_threshold);
-		dev->tft = max_fifo_threshold;
+	if (priv->tft > max_fifo_threshold) {
+		dev_warn(dev, "requested TX FIFO threshold %u, adjusted down to %u\n",
+			 priv->tft, max_fifo_threshold);
+		priv->tft = max_fifo_threshold;
 	}
 
-	if (dev->rft > max_fifo_threshold) {
-		dev_warn(&adev->dev, "requested RX FIFO threshold %u, adjusted down to %u\n",
-			dev->rft, max_fifo_threshold);
-		dev->rft = max_fifo_threshold;
+	if (priv->rft > max_fifo_threshold) {
+		dev_warn(dev, "requested RX FIFO threshold %u, adjusted down to %u\n",
+			 priv->rft, max_fifo_threshold);
+		priv->rft = max_fifo_threshold;
 	}
 
-	amba_set_drvdata(adev, dev);
+	amba_set_drvdata(adev, priv);
 
-	dev->virtbase = devm_ioremap(&adev->dev, adev->res.start,
-				resource_size(&adev->res));
-	if (!dev->virtbase)
+	priv->virtbase = devm_ioremap(dev, adev->res.start,
+				      resource_size(&adev->res));
+	if (!priv->virtbase)
 		return -ENOMEM;
 
-	dev->irq = adev->irq[0];
-	ret = devm_request_irq(&adev->dev, dev->irq, i2c_irq_handler, 0,
-				DRIVER_NAME, dev);
+	priv->irq = adev->irq[0];
+	ret = devm_request_irq(dev, priv->irq, i2c_irq_handler, 0,
+			       DRIVER_NAME, priv);
 	if (ret)
-		return dev_err_probe(&adev->dev, ret,
-				     "cannot claim the irq %d\n", dev->irq);
+		return dev_err_probe(dev, ret,
+				     "cannot claim the irq %d\n", priv->irq);
 
-	dev->clk = devm_clk_get_enabled(&adev->dev, NULL);
-	if (IS_ERR(dev->clk))
-		return dev_err_probe(&adev->dev, PTR_ERR(dev->clk),
+	priv->clk = devm_clk_get_enabled(dev, NULL);
+	if (IS_ERR(priv->clk))
+		return dev_err_probe(dev, PTR_ERR(priv->clk),
 				     "could enable i2c clock\n");
 
-	init_hw(dev);
+	init_hw(priv);
 
-	adap = &dev->adap;
+	adap = &priv->adap;
 	adap->dev.of_node = np;
-	adap->dev.parent = &adev->dev;
+	adap->dev.parent = dev;
 	adap->owner = THIS_MODULE;
 	adap->class = I2C_CLASS_DEPRECATED;
 	adap->algo = &nmk_i2c_algo;
-	adap->timeout = msecs_to_jiffies(dev->timeout);
+	adap->timeout = msecs_to_jiffies(priv->timeout);
 	snprintf(adap->name, sizeof(adap->name),
 		 "Nomadik I2C at %pR", &adev->res);
 
-	i2c_set_adapdata(adap, dev);
+	i2c_set_adapdata(adap, priv);
 
-	dev_info(&adev->dev,
+	dev_info(dev,
 		 "initialize %s on virtual base %p\n",
-		 adap->name, dev->virtbase);
+		 adap->name, priv->virtbase);
 
 	ret = i2c_add_adapter(adap);
 	if (ret)
 		return ret;
 
-	pm_runtime_put(&adev->dev);
+	pm_runtime_put(dev);
 
 	return 0;
 }
 
 static void nmk_i2c_remove(struct amba_device *adev)
 {
-	struct nmk_i2c_dev *dev = amba_get_drvdata(adev);
+	struct nmk_i2c_dev *priv = amba_get_drvdata(adev);
 
-	i2c_del_adapter(&dev->adap);
-	flush_i2c_fifo(dev);
-	disable_all_interrupts(dev);
-	clear_all_interrupts(dev);
+	i2c_del_adapter(&priv->adap);
+	flush_i2c_fifo(priv);
+	disable_all_interrupts(priv);
+	clear_all_interrupts(priv);
 	/* disable the controller */
-	i2c_clr_bit(dev->virtbase + I2C_CR, I2C_CR_PE);
+	i2c_clr_bit(priv->virtbase + I2C_CR, I2C_CR_PE);
 }
 
 static struct i2c_vendor_data vendor_stn8815 = {

-- 
2.43.1

Re: [PATCH 04/13] i2c: nomadik: rename private struct pointers from dev to priv

[Linus Walleij]

On Thu, Feb 15, 2024 at 5:52 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote:

> Disambiguate the usage of dev as a variable name; it is usually best to
> keep it reserved for struct device pointers. Avoid having multiple
> names for the same struct pointer (previously: dev, nmk, nmk_i2c).
>
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>

Fair enough, it's more readable like this.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 05/13] i2c: nomadik: simplify IRQ masking logic

[Théo Lebrun]

IRQ_MASK and I2C_CLEAR_ALL_INTS are redundant. One masks the top three
bits off as reserved, the other one masks the reserved IRQs inside the
u32. Get rid of IRQ_MASK and only use the most restrictive mask.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/i2c/busses/i2c-nomadik.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index cd511c884f99..80bdf7e42613 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -94,9 +94,6 @@
 /* some bits in ICR are reserved */
 #define I2C_CLEAR_ALL_INTS	0x131f007f
 
-/* first three msb bits are reserved */
-#define IRQ_MASK(mask)		(mask & 0x1fffffff)
-
 /* maximum threshold value */
 #define MAX_I2C_FIFO_THRESHOLD	15
 
@@ -249,8 +246,7 @@ static int flush_i2c_fifo(struct nmk_i2c_dev *priv)
  */
 static void disable_all_interrupts(struct nmk_i2c_dev *priv)
 {
-	u32 mask = IRQ_MASK(0);
-	writel(mask, priv->virtbase + I2C_IMSCR);
+	writel(0, priv->virtbase + I2C_IMSCR);
 }
 
 /**
@@ -259,9 +255,7 @@ static void disable_all_interrupts(struct nmk_i2c_dev *priv)
  */
 static void clear_all_interrupts(struct nmk_i2c_dev *priv)
 {
-	u32 mask;
-	mask = IRQ_MASK(I2C_CLEAR_ALL_INTS);
-	writel(mask, priv->virtbase + I2C_ICR);
+	writel(I2C_CLEAR_ALL_INTS, priv->virtbase + I2C_ICR);
 }
 
 /**
@@ -468,7 +462,7 @@ static int read_i2c(struct nmk_i2c_dev *priv, u16 flags)
 	else
 		irq_mask |= I2C_IT_MTDWS;
 
-	irq_mask = I2C_CLEAR_ALL_INTS & IRQ_MASK(irq_mask);
+	irq_mask &= I2C_CLEAR_ALL_INTS;
 
 	writel(readl(priv->virtbase + I2C_IMSCR) | irq_mask,
 	       priv->virtbase + I2C_IMSCR);
@@ -547,7 +541,7 @@ static int write_i2c(struct nmk_i2c_dev *priv, u16 flags)
 	else
 		irq_mask |= I2C_IT_MTDWS;
 
-	irq_mask = I2C_CLEAR_ALL_INTS & IRQ_MASK(irq_mask);
+	irq_mask &= I2C_CLEAR_ALL_INTS;
 
 	writel(readl(priv->virtbase + I2C_IMSCR) | irq_mask,
 	       priv->virtbase + I2C_IMSCR);
@@ -703,8 +697,8 @@ static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap,
  */
 static int disable_interrupts(struct nmk_i2c_dev *priv, u32 irq)
 {
-	irq = IRQ_MASK(irq);
-	writel(readl(priv->virtbase + I2C_IMSCR) & ~(I2C_CLEAR_ALL_INTS & irq),
+	irq &= I2C_CLEAR_ALL_INTS;
+	writel(readl(priv->virtbase + I2C_IMSCR) & ~irq,
 	       priv->virtbase + I2C_IMSCR);
 	return 0;
 }

-- 
2.43.1

Re: [PATCH 05/13] i2c: nomadik: simplify IRQ masking logic

[Linus Walleij]

On Thu, Feb 15, 2024 at 5:52 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote:

> IRQ_MASK and I2C_CLEAR_ALL_INTS are redundant. One masks the top three
> bits off as reserved, the other one masks the reserved IRQs inside the
> u32. Get rid of IRQ_MASK and only use the most restrictive mask.
>
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>

Yeah, more readable like this definitely.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 06/13] i2c: nomadik: use bitops helpers

[Théo Lebrun]

Constant register bit fields are declared using hardcoded hex values;
replace them by calls to BIT() and GENMASK(). Replace custom GEN_MASK()
macro by the generic FIELD_PREP(). Replace manual bit manipulations by
the generic FIELD_GET() macro.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/i2c/busses/i2c-nomadik.c | 150 ++++++++++++++++++++-------------------
 1 file changed, 77 insertions(+), 73 deletions(-)

diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index 80bdf7e42613..aa68ab402b10 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -9,6 +9,7 @@
  * Author: Srinidhi Kasagar <srinidhi.kasagar@stericsson.com>
  * Author: Sachin Verma <sachin.verma@st.com>
  */
+#include <linux/bitfield.h>
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/amba/bus.h>
@@ -42,54 +43,59 @@
 #define I2C_ICR		(0x038)
 
 /* Control registers */
-#define I2C_CR_PE		(0x1 << 0)	/* Peripheral Enable */
-#define I2C_CR_OM		(0x3 << 1)	/* Operating mode */
-#define I2C_CR_SAM		(0x1 << 3)	/* Slave addressing mode */
-#define I2C_CR_SM		(0x3 << 4)	/* Speed mode */
-#define I2C_CR_SGCM		(0x1 << 6)	/* Slave general call mode */
-#define I2C_CR_FTX		(0x1 << 7)	/* Flush Transmit */
-#define I2C_CR_FRX		(0x1 << 8)	/* Flush Receive */
-#define I2C_CR_DMA_TX_EN	(0x1 << 9)	/* DMA Tx enable */
-#define I2C_CR_DMA_RX_EN	(0x1 << 10)	/* DMA Rx Enable */
-#define I2C_CR_DMA_SLE		(0x1 << 11)	/* DMA sync. logic enable */
-#define I2C_CR_LM		(0x1 << 12)	/* Loopback mode */
-#define I2C_CR_FON		(0x3 << 13)	/* Filtering on */
-#define I2C_CR_FS		(0x3 << 15)	/* Force stop enable */
+#define I2C_CR_PE		BIT(0)		/* Peripheral Enable */
+#define I2C_CR_OM		GENMASK(2, 1)	/* Operating mode */
+#define I2C_CR_SAM		BIT(3)		/* Slave addressing mode */
+#define I2C_CR_SM		GENMASK(5, 4)	/* Speed mode */
+#define I2C_CR_SGCM		BIT(6)		/* Slave general call mode */
+#define I2C_CR_FTX		BIT(7)		/* Flush Transmit */
+#define I2C_CR_FRX		BIT(8)		/* Flush Receive */
+#define I2C_CR_DMA_TX_EN	BIT(9)		/* DMA Tx enable */
+#define I2C_CR_DMA_RX_EN	BIT(10)		/* DMA Rx Enable */
+#define I2C_CR_DMA_SLE		BIT(11)		/* DMA sync. logic enable */
+#define I2C_CR_LM		BIT(12)		/* Loopback mode */
+#define I2C_CR_FON		GENMASK(14, 13)	/* Filtering on */
+#define I2C_CR_FS		GENMASK(16, 15)	/* Force stop enable */
+
+/* Slave control register (SCR) */
+#define I2C_SCR_SLSU		GENMASK(31, 16)	/* Slave data setup time */
 
 /* Master controller (MCR) register */
-#define I2C_MCR_OP		(0x1 << 0)	/* Operation */
-#define I2C_MCR_A7		(0x7f << 1)	/* 7-bit address */
-#define I2C_MCR_EA10		(0x7 << 8)	/* 10-bit Extended address */
-#define I2C_MCR_SB		(0x1 << 11)	/* Extended address */
-#define I2C_MCR_AM		(0x3 << 12)	/* Address type */
-#define I2C_MCR_STOP		(0x1 << 14)	/* Stop condition */
-#define I2C_MCR_LENGTH		(0x7ff << 15)	/* Transaction length */
+#define I2C_MCR_OP		BIT(0)		/* Operation */
+#define I2C_MCR_A7		GENMASK(7, 1)	/* 7-bit address */
+#define I2C_MCR_EA10		GENMASK(10, 8)	/* 10-bit Extended address */
+#define I2C_MCR_SB		BIT(11)		/* Extended address */
+#define I2C_MCR_AM		GENMASK(13, 12)	/* Address type */
+#define I2C_MCR_STOP		BIT(14)		/* Stop condition */
+#define I2C_MCR_LENGTH		GENMASK(25, 15)	/* Transaction length */
 
 /* Status register (SR) */
-#define I2C_SR_OP		(0x3 << 0)	/* Operation */
-#define I2C_SR_STATUS		(0x3 << 2)	/* controller status */
-#define I2C_SR_CAUSE		(0x7 << 4)	/* Abort cause */
-#define I2C_SR_TYPE		(0x3 << 7)	/* Receive type */
-#define I2C_SR_LENGTH		(0x7ff << 9)	/* Transfer length */
+#define I2C_SR_OP		GENMASK(1, 0)	/* Operation */
+#define I2C_SR_STATUS		GENMASK(3, 2)	/* controller status */
+#define I2C_SR_CAUSE		GENMASK(6, 4)	/* Abort cause */
+#define I2C_SR_TYPE		GENMASK(8, 7)	/* Receive type */
+#define I2C_SR_LENGTH		GENMASK(19, 9)	/* Transfer length */
+
+/* Baud-rate counter register (BRCR) */
+#define I2C_BRCR_BRCNT1		GENMASK(31, 16)	/* Baud-rate counter 1 */
+#define I2C_BRCR_BRCNT2		GENMASK(15, 0)	/* Baud-rate counter 2 */
 
 /* Interrupt mask set/clear (IMSCR) bits */
-#define I2C_IT_TXFE		(0x1 << 0)
-#define I2C_IT_TXFNE		(0x1 << 1)
-#define I2C_IT_TXFF		(0x1 << 2)
-#define I2C_IT_TXFOVR		(0x1 << 3)
-#define I2C_IT_RXFE		(0x1 << 4)
-#define I2C_IT_RXFNF		(0x1 << 5)
-#define I2C_IT_RXFF		(0x1 << 6)
-#define I2C_IT_RFSR		(0x1 << 16)
-#define I2C_IT_RFSE		(0x1 << 17)
-#define I2C_IT_WTSR		(0x1 << 18)
-#define I2C_IT_MTD		(0x1 << 19)
-#define I2C_IT_STD		(0x1 << 20)
-#define I2C_IT_MAL		(0x1 << 24)
-#define I2C_IT_BERR		(0x1 << 25)
-#define I2C_IT_MTDWS		(0x1 << 28)
-
-#define GEN_MASK(val, mask, sb)  (((val) << (sb)) & (mask))
+#define I2C_IT_TXFE		BIT(0)
+#define I2C_IT_TXFNE		BIT(1)
+#define I2C_IT_TXFF		BIT(2)
+#define I2C_IT_TXFOVR		BIT(3)
+#define I2C_IT_RXFE		BIT(4)
+#define I2C_IT_RXFNF		BIT(5)
+#define I2C_IT_RXFF		BIT(6)
+#define I2C_IT_RFSR		BIT(16)
+#define I2C_IT_RFSE		BIT(17)
+#define I2C_IT_WTSR		BIT(18)
+#define I2C_IT_MTD		BIT(19)
+#define I2C_IT_STD		BIT(20)
+#define I2C_IT_MAL		BIT(24)
+#define I2C_IT_BERR		BIT(25)
+#define I2C_IT_MTDWS		BIT(28)
 
 /* some bits in ICR are reserved */
 #define I2C_CLEAR_ALL_INTS	0x131f007f
@@ -284,7 +290,7 @@ static int init_hw(struct nmk_i2c_dev *priv)
 }
 
 /* enable peripheral, master mode operation */
-#define DEFAULT_I2C_REG_CR	((1 << 1) | I2C_CR_PE)
+#define DEFAULT_I2C_REG_CR	(FIELD_PREP(I2C_CR_OM, 0b01) | I2C_CR_PE)
 
 /**
  * load_i2c_mcr_reg() - load the MCR register
@@ -296,41 +302,42 @@ static u32 load_i2c_mcr_reg(struct nmk_i2c_dev *priv, u16 flags)
 	u32 mcr = 0;
 	unsigned short slave_adr_3msb_bits;
 
-	mcr |= GEN_MASK(priv->cli.slave_adr, I2C_MCR_A7, 1);
+	mcr |= FIELD_PREP(I2C_MCR_A7, priv->cli.slave_adr);
 
 	if (unlikely(flags & I2C_M_TEN)) {
 		/* 10-bit address transaction */
-		mcr |= GEN_MASK(2, I2C_MCR_AM, 12);
+		mcr |= FIELD_PREP(I2C_MCR_AM, 2);
 		/*
 		 * Get the top 3 bits.
 		 * EA10 represents extended address in MCR. This includes
 		 * the extension (MSB bits) of the 7 bit address loaded
 		 * in A7
 		 */
-		slave_adr_3msb_bits = (priv->cli.slave_adr >> 7) & 0x7;
+		slave_adr_3msb_bits = FIELD_GET(GENMASK(9, 7),
+						priv->cli.slave_adr);
 
-		mcr |= GEN_MASK(slave_adr_3msb_bits, I2C_MCR_EA10, 8);
+		mcr |= FIELD_PREP(I2C_MCR_EA10, slave_adr_3msb_bits);
 	} else {
 		/* 7-bit address transaction */
-		mcr |= GEN_MASK(1, I2C_MCR_AM, 12);
+		mcr |= FIELD_PREP(I2C_MCR_AM, 1);
 	}
 
 	/* start byte procedure not applied */
-	mcr |= GEN_MASK(0, I2C_MCR_SB, 11);
+	mcr |= FIELD_PREP(I2C_MCR_SB, 0);
 
 	/* check the operation, master read/write? */
 	if (priv->cli.operation == I2C_WRITE)
-		mcr |= GEN_MASK(I2C_WRITE, I2C_MCR_OP, 0);
+		mcr |= FIELD_PREP(I2C_MCR_OP, I2C_WRITE);
 	else
-		mcr |= GEN_MASK(I2C_READ, I2C_MCR_OP, 0);
+		mcr |= FIELD_PREP(I2C_MCR_OP, I2C_READ);
 
 	/* stop or repeated start? */
 	if (priv->stop)
-		mcr |= GEN_MASK(1, I2C_MCR_STOP, 14);
+		mcr |= FIELD_PREP(I2C_MCR_STOP, 1);
 	else
-		mcr &= ~(GEN_MASK(1, I2C_MCR_STOP, 14));
+		mcr &= ~FIELD_PREP(I2C_MCR_STOP, 1);
 
-	mcr |= GEN_MASK(priv->cli.count, I2C_MCR_LENGTH, 15);
+	mcr |= FIELD_PREP(I2C_MCR_LENGTH, priv->cli.count);
 
 	return mcr;
 }
@@ -383,7 +390,7 @@ static void setup_i2c_controller(struct nmk_i2c_dev *priv)
 	slsu += 1;
 
 	dev_dbg(&priv->adev->dev, "calculated SLSU = %04x\n", slsu);
-	writel(slsu << 16, priv->virtbase + I2C_SCR);
+	writel(FIELD_PREP(I2C_SCR_SLSU, slsu), priv->virtbase + I2C_SCR);
 
 	/*
 	 * The spec says, in case of std. mode the divider is
@@ -399,8 +406,8 @@ static void setup_i2c_controller(struct nmk_i2c_dev *priv)
 	 * plus operation. Currently we do not supprt high speed mode
 	 * so set brcr1 to 0.
 	 */
-	brcr1 = 0 << 16;
-	brcr2 = (i2c_clk / (priv->clk_freq * div)) & 0xffff;
+	brcr1 = FIELD_PREP(I2C_BRCR_BRCNT1, 0);
+	brcr2 = FIELD_PREP(I2C_BRCR_BRCNT2, i2c_clk / (priv->clk_freq * div));
 
 	/* set the baud rate counter register */
 	writel((brcr1 | brcr2), priv->virtbase + I2C_BRCR);
@@ -414,12 +421,13 @@ static void setup_i2c_controller(struct nmk_i2c_dev *priv)
 	if (priv->sm > I2C_FREQ_MODE_FAST) {
 		dev_err(&priv->adev->dev,
 			"do not support this mode defaulting to std. mode\n");
-		brcr2 = i2c_clk / (I2C_MAX_STANDARD_MODE_FREQ * 2) & 0xffff;
+		brcr2 = FIELD_PREP(I2C_BRCR_BRCNT2,
+				   i2c_clk / (I2C_MAX_STANDARD_MODE_FREQ * 2));
 		writel((brcr1 | brcr2), priv->virtbase + I2C_BRCR);
-		writel(I2C_FREQ_MODE_STANDARD << 4,
-				priv->virtbase + I2C_CR);
+		writel(FIELD_PREP(I2C_CR_SM, I2C_FREQ_MODE_STANDARD),
+		       priv->virtbase + I2C_CR);
 	}
-	writel(priv->sm << 4, priv->virtbase + I2C_CR);
+	writel(FIELD_PREP(I2C_CR_SM, priv->sm), priv->virtbase + I2C_CR);
 
 	/* set the Tx and Rx FIFO threshold */
 	writel(priv->tft, priv->virtbase + I2C_TFTR);
@@ -583,13 +591,8 @@ static int nmk_i2c_xfer_one(struct nmk_i2c_dev *priv, u16 flags)
 		u32 cause;
 
 		i2c_sr = readl(priv->virtbase + I2C_SR);
-		/*
-		 * Check if the controller I2C operation status
-		 * is set to ABORT(11b).
-		 */
-		if (((i2c_sr >> 2) & 0x3) == 0x3) {
-			/* get the abort cause */
-			cause =	(i2c_sr >> 4) & 0x7;
+		if (FIELD_GET(I2C_SR_STATUS, i2c_sr) == I2C_ABORT) {
+			cause = FIELD_GET(I2C_SR_CAUSE, i2c_sr);
 			dev_err(&priv->adev->dev, "%s\n",
 				cause >= ARRAY_SIZE(abort_causes) ?
 				"unknown reason" :
@@ -730,7 +733,7 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
 	misr = readl(priv->virtbase + I2C_MISR);
 
 	src = __ffs(misr);
-	switch ((1 << src)) {
+	switch (BIT(src)) {
 
 	/* Transmit FIFO nearly empty interrupt */
 	case I2C_IT_TXFNE:
@@ -824,15 +827,16 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
 	 * during the transaction.
 	 */
 	case I2C_IT_BERR:
+	{
+		u32 sr = readl(priv->virtbase + I2C_SR);
 		priv->result = -EIO;
-		/* get the status */
-		if (((readl(priv->virtbase + I2C_SR) >> 2) & 0x3) == I2C_ABORT)
+		if (FIELD_GET(I2C_SR_STATUS, sr) == I2C_ABORT)
 			init_hw(priv);
 
 		i2c_set_bit(priv->virtbase + I2C_ICR, I2C_IT_BERR);
 		complete(&priv->xfer_complete);
-
-		break;
+	}
+	break;
 
 	/*
 	 * Tx FIFO overrun interrupt.

-- 
2.43.1

Re: [PATCH 06/13] i2c: nomadik: use bitops helpers

[Linus Walleij]

On Thu, Feb 15, 2024 at 5:52 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote:

> Constant register bit fields are declared using hardcoded hex values;
> replace them by calls to BIT() and GENMASK(). Replace custom GEN_MASK()
> macro by the generic FIELD_PREP(). Replace manual bit manipulations by
> the generic FIELD_GET() macro.
>
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>

I'm a fan of this style.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 07/13] i2c: nomadik: support short xfer timeouts using waitqueue & hrtimer

[Théo Lebrun]

Replace the completion by a waitqueue for synchronization from IRQ
handler to task. For short timeouts, use hrtimers, else use timers.
Usecase: avoid blocking the I2C bus for too long when an issue occurs.

The threshold picked is one jiffy: if timeout is below that, use
hrtimers. This threshold is NOT configurable.

Implement behavior but do NOT change fetching of timeout. This means the
timeout is unchanged (200ms) and the hrtimer case will never trigger.

A waitqueue is used because it supports both desired timeout approaches.
See wait_event_timeout() and wait_event_hrtimeout(). An atomic boolean
serves as synchronization condition.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/i2c/busses/i2c-nomadik.c | 70 +++++++++++++++++++++++++++-------------
 1 file changed, 48 insertions(+), 22 deletions(-)

diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index aa68ab402b10..e68b8e0d7919 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -162,10 +162,11 @@ struct i2c_nmk_client {
  * @clk_freq: clock frequency for the operation mode
  * @tft: Tx FIFO Threshold in bytes
  * @rft: Rx FIFO Threshold in bytes
- * @timeout: Slave response timeout (ms)
+ * @timeout_usecs: Slave response timeout
  * @sm: speed mode
  * @stop: stop condition.
- * @xfer_complete: acknowledge completion for a I2C message.
+ * @xfer_wq: xfer done wait queue.
+ * @xfer_done: xfer done boolean.
  * @result: controller propogated result.
  */
 struct nmk_i2c_dev {
@@ -179,10 +180,11 @@ struct nmk_i2c_dev {
 	u32				clk_freq;
 	unsigned char			tft;
 	unsigned char			rft;
-	int				timeout;
+	int				timeout_usecs;
 	enum i2c_freq_mode		sm;
 	int				stop;
-	struct completion		xfer_complete;
+	struct wait_queue_head		xfer_wq;
+	bool				xfer_done;
 	int				result;
 };
 
@@ -434,6 +436,22 @@ static void setup_i2c_controller(struct nmk_i2c_dev *priv)
 	writel(priv->rft, priv->virtbase + I2C_RFTR);
 }
 
+static bool nmk_i2c_wait_xfer_done(struct nmk_i2c_dev *priv)
+{
+	if (priv->timeout_usecs < jiffies_to_usecs(1)) {
+		unsigned long timeout_usecs = priv->timeout_usecs;
+		ktime_t timeout = ktime_set(0, timeout_usecs * NSEC_PER_USEC);
+
+		wait_event_hrtimeout(priv->xfer_wq, priv->xfer_done, timeout);
+	} else {
+		unsigned long timeout = usecs_to_jiffies(priv->timeout_usecs);
+
+		wait_event_timeout(priv->xfer_wq, priv->xfer_done, timeout);
+	}
+
+	return priv->xfer_done;
+}
+
 /**
  * read_i2c() - Read from I2C client device
  * @priv: private data of I2C Driver
@@ -445,9 +463,9 @@ static void setup_i2c_controller(struct nmk_i2c_dev *priv)
  */
 static int read_i2c(struct nmk_i2c_dev *priv, u16 flags)
 {
-	int status = 0;
 	u32 mcr, irq_mask;
-	unsigned long timeout;
+	int status = 0;
+	bool xfer_done;
 
 	mcr = load_i2c_mcr_reg(priv, flags);
 	writel(mcr, priv->virtbase + I2C_MCR);
@@ -459,7 +477,8 @@ static int read_i2c(struct nmk_i2c_dev *priv, u16 flags)
 	/* enable the controller */
 	i2c_set_bit(priv->virtbase + I2C_CR, I2C_CR_PE);
 
-	init_completion(&priv->xfer_complete);
+	init_waitqueue_head(&priv->xfer_wq);
+	priv->xfer_done = false;
 
 	/* enable interrupts by setting the mask */
 	irq_mask = (I2C_IT_RXFNF | I2C_IT_RXFF |
@@ -475,10 +494,9 @@ static int read_i2c(struct nmk_i2c_dev *priv, u16 flags)
 	writel(readl(priv->virtbase + I2C_IMSCR) | irq_mask,
 	       priv->virtbase + I2C_IMSCR);
 
-	timeout = wait_for_completion_timeout(
-		&priv->xfer_complete, priv->adap.timeout);
+	xfer_done = nmk_i2c_wait_xfer_done(priv);
 
-	if (timeout == 0) {
+	if (!xfer_done) {
 		/* Controller timed out */
 		dev_err(&priv->adev->dev, "read from slave 0x%x timed out\n",
 			priv->cli.slave_adr);
@@ -513,9 +531,9 @@ static void fill_tx_fifo(struct nmk_i2c_dev *priv, int no_bytes)
  */
 static int write_i2c(struct nmk_i2c_dev *priv, u16 flags)
 {
-	u32 status = 0;
 	u32 mcr, irq_mask;
-	unsigned long timeout;
+	u32 status = 0;
+	bool xfer_done;
 
 	mcr = load_i2c_mcr_reg(priv, flags);
 
@@ -528,7 +546,8 @@ static int write_i2c(struct nmk_i2c_dev *priv, u16 flags)
 	/* enable the controller */
 	i2c_set_bit(priv->virtbase + I2C_CR, I2C_CR_PE);
 
-	init_completion(&priv->xfer_complete);
+	init_waitqueue_head(&priv->xfer_wq);
+	priv->xfer_done = false;
 
 	/* enable interrupts by settings the masks */
 	irq_mask = (I2C_IT_TXFOVR | I2C_IT_MAL | I2C_IT_BERR);
@@ -554,10 +573,9 @@ static int write_i2c(struct nmk_i2c_dev *priv, u16 flags)
 	writel(readl(priv->virtbase + I2C_IMSCR) | irq_mask,
 	       priv->virtbase + I2C_IMSCR);
 
-	timeout = wait_for_completion_timeout(
-		&priv->xfer_complete, priv->adap.timeout);
+	xfer_done = nmk_i2c_wait_xfer_done(priv);
 
-	if (timeout == 0) {
+	if (!xfer_done) {
 		/* Controller timed out */
 		dev_err(&priv->adev->dev, "write to slave 0x%x timed out\n",
 			priv->cli.slave_adr);
@@ -807,7 +825,9 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
 				priv->cli.count);
 			init_hw(priv);
 		}
-		complete(&priv->xfer_complete);
+		priv->xfer_done = true;
+		wake_up(&priv->xfer_wq);
+
 
 		break;
 
@@ -817,7 +837,9 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
 		init_hw(priv);
 
 		i2c_set_bit(priv->virtbase + I2C_ICR, I2C_IT_MAL);
-		complete(&priv->xfer_complete);
+		priv->xfer_done = true;
+		wake_up(&priv->xfer_wq);
+
 
 		break;
 
@@ -834,7 +856,9 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
 			init_hw(priv);
 
 		i2c_set_bit(priv->virtbase + I2C_ICR, I2C_IT_BERR);
-		complete(&priv->xfer_complete);
+		priv->xfer_done = true;
+		wake_up(&priv->xfer_wq);
+
 	}
 	break;
 
@@ -848,7 +872,9 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
 		init_hw(priv);
 
 		dev_err(dev, "Tx Fifo Over run\n");
-		complete(&priv->xfer_complete);
+		priv->xfer_done = true;
+		wake_up(&priv->xfer_wq);
+
 
 		break;
 
@@ -949,7 +975,7 @@ static void nmk_i2c_of_probe(struct device_node *np,
 		priv->sm = I2C_FREQ_MODE_FAST;
 	priv->tft = 1; /* Tx FIFO threshold */
 	priv->rft = 8; /* Rx FIFO threshold */
-	priv->timeout = 200; /* Slave response timeout(ms) */
+	priv->timeout_usecs = 200 * USEC_PER_MSEC; /* Slave response timeout */
 }
 
 static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
@@ -1009,7 +1035,7 @@ static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
 	adap->owner = THIS_MODULE;
 	adap->class = I2C_CLASS_DEPRECATED;
 	adap->algo = &nmk_i2c_algo;
-	adap->timeout = msecs_to_jiffies(priv->timeout);
+	adap->timeout = usecs_to_jiffies(priv->timeout_usecs);
 	snprintf(adap->name, sizeof(adap->name),
 		 "Nomadik I2C at %pR", &adev->res);
 

-- 
2.43.1

Re: [PATCH 07/13] i2c: nomadik: support short xfer timeouts using waitqueue & hrtimer

[Linus Walleij]

On Thu, Feb 15, 2024 at 5:52 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote:

> Replace the completion by a waitqueue for synchronization from IRQ
> handler to task. For short timeouts, use hrtimers, else use timers.
> Usecase: avoid blocking the I2C bus for too long when an issue occurs.
>
> The threshold picked is one jiffy: if timeout is below that, use
> hrtimers. This threshold is NOT configurable.
>
> Implement behavior but do NOT change fetching of timeout. This means the
> timeout is unchanged (200ms) and the hrtimer case will never trigger.
>
> A waitqueue is used because it supports both desired timeout approaches.
> See wait_event_timeout() and wait_event_hrtimeout(). An atomic boolean
> serves as synchronization condition.
>
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>

Part of me want to go and fix completions to handle hrtimer timeouts
for submicrosecond timeouts, BUT I realized that this is a bit thick
request for a simple driver, so just a suggestion for something we could
do one day. This is fine with me.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 07/13] i2c: nomadik: support short xfer timeouts using waitqueue & hrtimer

[Théo Lebrun]

Hello,

On Mon Feb 19, 2024 at 3:19 PM CET, Linus Walleij wrote:
> On Thu, Feb 15, 2024 at 5:52 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote:
>
> > Replace the completion by a waitqueue for synchronization from IRQ
> > handler to task. For short timeouts, use hrtimers, else use timers.
> > Usecase: avoid blocking the I2C bus for too long when an issue occurs.
> >
> > The threshold picked is one jiffy: if timeout is below that, use
> > hrtimers. This threshold is NOT configurable.
> >
> > Implement behavior but do NOT change fetching of timeout. This means the
> > timeout is unchanged (200ms) and the hrtimer case will never trigger.
> >
> > A waitqueue is used because it supports both desired timeout approaches.
> > See wait_event_timeout() and wait_event_hrtimeout(). An atomic boolean
> > serves as synchronization condition.
> >
> > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
>
> Part of me want to go and fix completions to handle hrtimer timeouts
> for submicrosecond timeouts, BUT I realized that this is a bit thick
> request for a simple driver, so just a suggestion for something we could
> do one day. This is fine with me.
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Indeed having to switch to another abstraction because we desire another
timeout method is nonsensical. Completion supporting hrtimeouts would
make sense. As you said though, this is too much for a simple driver.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

------------------------------------------------------------------------

[PATCH 08/13] i2c: nomadik: replace jiffies by ktime for FIFO flushing timeout

[Théo Lebrun]

The FIFO flush function uses a jiffies amount to detect timeouts as the
flushing is async. Replace with ktime to get more accurate precision
and support short timeouts.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/i2c/busses/i2c-nomadik.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index e68b8e0d7919..afd54999bbbb 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -219,8 +219,8 @@ static inline void i2c_clr_bit(void __iomem *reg, u32 mask)
 static int flush_i2c_fifo(struct nmk_i2c_dev *priv)
 {
 #define LOOP_ATTEMPTS 10
+	ktime_t timeout;
 	int i;
-	unsigned long timeout;
 
 	/*
 	 * flush the transmit and receive FIFO. The flushing
@@ -232,9 +232,9 @@ static int flush_i2c_fifo(struct nmk_i2c_dev *priv)
 	writel((I2C_CR_FTX | I2C_CR_FRX), priv->virtbase + I2C_CR);
 
 	for (i = 0; i < LOOP_ATTEMPTS; i++) {
-		timeout = jiffies + priv->adap.timeout;
+		timeout = ktime_add_us(ktime_get(), priv->timeout_usecs);
 
-		while (!time_after(jiffies, timeout)) {
+		while (ktime_after(timeout, ktime_get())) {
 			if ((readl(priv->virtbase + I2C_CR) &
 				(I2C_CR_FTX | I2C_CR_FRX)) == 0)
 				return 0;

-- 
2.43.1

Re: [PATCH 08/13] i2c: nomadik: replace jiffies by ktime for FIFO flushing timeout

[Linus Walleij]

On Thu, Feb 15, 2024 at 5:52 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote:

> The FIFO flush function uses a jiffies amount to detect timeouts as the
> flushing is async. Replace with ktime to get more accurate precision
> and support short timeouts.
>
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>

Excellent patch. Thanks.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 08/13] i2c: nomadik: replace jiffies by ktime for FIFO flushing timeout

[Théo Lebrun]

Hello,

On Mon Feb 19, 2024 at 3:21 PM CET, Linus Walleij wrote:
> On Thu, Feb 15, 2024 at 5:52 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote:
>
> > The FIFO flush function uses a jiffies amount to detect timeouts as the
> > flushing is async. Replace with ktime to get more accurate precision
> > and support short timeouts.
> >
> > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
>
> Excellent patch. Thanks.
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Somewhat related to this patch: while writing it, I noticed the total
timeout of flush_i2c_fifo() is 10 times the timeout. Without this
series, this means 10*200ms of busywaiting!

If you have an opinion on a more sensible option for this I could add a
patch to my V2. I don't know enough to pick a sensible value.

I'm unsure if it makes sense that the timeout of flush_i2c_fifo() is a
multiple of the transfer timeout. Does it make sense that those two
timeouts are correlated?

Big thanks for your review,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 08/13] i2c: nomadik: replace jiffies by ktime for FIFO flushing timeout

[Linus Walleij]

On Mon, Feb 19, 2024 at 3:38 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote:

> Somewhat related to this patch: while writing it, I noticed the total
> timeout of flush_i2c_fifo() is 10 times the timeout. Without this
> series, this means 10*200ms of busywaiting!
>
> If you have an opinion on a more sensible option for this I could add a
> patch to my V2. I don't know enough to pick a sensible value.
>
> I'm unsure if it makes sense that the timeout of flush_i2c_fifo() is a
> multiple of the transfer timeout. Does it make sense that those two
> timeouts are correlated?

I have a *vague* memory of the timeouts for flushing needing to be longer
but I might be mistaken. This is probably a Srinidhi or even Sachin question...
Sadly I don't have their current mail addresses.

Yours,
Linus Walleij

[PATCH 09/13] i2c: nomadik: fetch timeout-usecs property from devicetree

[Théo Lebrun]

Allow overriding the default timeout value (200ms) from devicetree,
using the timeout-usecs property.

The i2c_adapter->timeout field is an unaccurate jiffies amount;
i2c-nomadik uses hrtimers for timeouts below one jiffy.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/i2c/busses/i2c-nomadik.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index afd54999bbbb..23e12c570457 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -964,6 +964,8 @@ static const struct i2c_algorithm nmk_i2c_algo = {
 static void nmk_i2c_of_probe(struct device_node *np,
 			     struct nmk_i2c_dev *priv)
 {
+	u32 timeout_usecs;
+
 	/* Default to 100 kHz if no frequency is given in the node */
 	if (of_property_read_u32(np, "clock-frequency", &priv->clk_freq))
 		priv->clk_freq = I2C_MAX_STANDARD_MODE_FREQ;
@@ -975,7 +977,12 @@ static void nmk_i2c_of_probe(struct device_node *np,
 		priv->sm = I2C_FREQ_MODE_FAST;
 	priv->tft = 1; /* Tx FIFO threshold */
 	priv->rft = 8; /* Rx FIFO threshold */
-	priv->timeout_usecs = 200 * USEC_PER_MSEC; /* Slave response timeout */
+
+	/* Slave response timeout */
+	if (!of_property_read_u32(np, "timeout-usecs", &timeout_usecs))
+		priv->timeout_usecs = timeout_usecs;
+	else
+		priv->timeout_usecs = 200 * USEC_PER_MSEC;
 }
 
 static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)

-- 
2.43.1

Re: [PATCH 09/13] i2c: nomadik: fetch timeout-usecs property from devicetree

[Linus Walleij]

On Thu, Feb 15, 2024 at 5:52 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote:

> Allow overriding the default timeout value (200ms) from devicetree,
> using the timeout-usecs property.
>
> The i2c_adapter->timeout field is an unaccurate jiffies amount;
> i2c-nomadik uses hrtimers for timeouts below one jiffy.
>
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>

Once we agree on the binding name:

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 09/13] i2c: nomadik: fetch timeout-usecs property from devicetree

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 416 bytes --]


> +	/* Slave response timeout */
> +	if (!of_property_read_u32(np, "timeout-usecs", &timeout_usecs))
> +		priv->timeout_usecs = timeout_usecs;
> +	else
> +		priv->timeout_usecs = 200 * USEC_PER_MSEC;

I could imagine to add 'transfer_timeout_us' to struct i2c_timings.
Then, you could use 'i2c_parse_fw_timings' to obtain the value. What
values/value range do you use here? I can't find them in the DTS
additions.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 09/13] i2c: nomadik: fetch timeout-usecs property from devicetree

[Théo Lebrun]

Hello,

On Tue Feb 27, 2024 at 1:14 PM CET, Wolfram Sang wrote:
> > +	/* Slave response timeout */
> > +	if (!of_property_read_u32(np, "timeout-usecs", &timeout_usecs))
> > +		priv->timeout_usecs = timeout_usecs;
> > +	else
> > +		priv->timeout_usecs = 200 * USEC_PER_MSEC;
>
> I could imagine to add 'transfer_timeout_us' to struct i2c_timings.
> Then, you could use 'i2c_parse_fw_timings' to obtain the value. What
> values/value range do you use here? I can't find them in the DTS
> additions.

That sounds good. I have not used this prop in the DTS as it does not
make much sense for an eval board. The target is production boards.

An order of magnitude is a few transfers every 15ms. It means a timeout
of 15ms divided by "a few". I don't have more precise values, but I
could if you consider it useful.

I've done some testing at 50~100µs timeouts and it works as expected. At
those values timerslack is important to consider (default of 50µs).
This is at 400kHz clock frequency. Keep in mind the controllers support
up to 3.4MHz (not yet upstreamed) so timeouts could in theory go
lower if required by the usecase.

My upcoming question is how to move forward on this series. I can do the
patch to i2c_parse_fw_timings() in the next revision. That way it gets
added alongside the first user of this feature. Would it work for you?

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 09/13] i2c: nomadik: fetch timeout-usecs property from devicetree

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 753 bytes --]

Hi Théo,

> That sounds good. I have not used this prop in the DTS as it does not
> make much sense for an eval board. The target is production boards.

...

> My upcoming question is how to move forward on this series. I can do the
> patch to i2c_parse_fw_timings() in the next revision. That way it gets
> added alongside the first user of this feature. Would it work for you?

Hmmm, to be honest I have a bit of an issue with the 'no user' problem.
There is a driver which uses this feature, okay. But there is no
upstream hardware which uses this driver with this new feature. This
makes maintaining harder ("Who uses this feature?" - "Someone" - "How do
they use it? Can we modify it?" - "Dunno").

Kind regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 09/13] i2c: nomadik: fetch timeout-usecs property from devicetree

[Théo Lebrun]

Hello Wolfram,

On Wed Feb 28, 2024 at 11:49 AM CET, Wolfram Sang wrote:
> > That sounds good. I have not used this prop in the DTS as it does not
> > make much sense for an eval board. The target is production boards.
>
> ...
>
> > My upcoming question is how to move forward on this series. I can do the
> > patch to i2c_parse_fw_timings() in the next revision. That way it gets
> > added alongside the first user of this feature. Would it work for you?
>
> Hmmm, to be honest I have a bit of an issue with the 'no user' problem.
> There is a driver which uses this feature, okay. But there is no
> upstream hardware which uses this driver with this new feature. This
> makes maintaining harder ("Who uses this feature?" - "Someone" - "How do
> they use it? Can we modify it?" - "Dunno").

The alternative is that I keep going with a new revision of i2c-nomadik
that manually parses the prop. It'll be refactored if/when the I2C core
provides a better way to access the value. Is that OK?

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 09/13] i2c: nomadik: fetch timeout-usecs property from devicetree

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 490 bytes --]


> The alternative is that I keep going with a new revision of i2c-nomadik
> that manually parses the prop. It'll be refactored if/when the I2C core
> provides a better way to access the value. Is that OK?

That wouldn't have helped because there is still no user in-kernel of
that property, i.e. no DTS file with that property. But I just realized
that I need to convert i2c-mpc to avoid a deprecated binding, so we have
a user there. Lucky you ;)

I'll try to get the series done today.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 09/13] i2c: nomadik: fetch timeout-usecs property from devicetree

[Théo Lebrun]

Hello,

On Thu Feb 29, 2024 at 10:25 AM CET, Wolfram Sang wrote:
> > The alternative is that I keep going with a new revision of i2c-nomadik
> > that manually parses the prop. It'll be refactored if/when the I2C core
> > provides a better way to access the value. Is that OK?
>
> That wouldn't have helped because there is still no user in-kernel of
> that property, i.e. no DTS file with that property. But I just realized
> that I need to convert i2c-mpc to avoid a deprecated binding, so we have
> a user there. Lucky you ;)
>
> I'll try to get the series done today.

Lucky me indeed!

I just thought about it but I don't mind using the property in the eval
board DTS. The default 200ms timeout makes no sense for us. Using a
default of a few ms would be more sensible and make us the second user
of the prop.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH 10/13] i2c: nomadik: support Mobileye EyeQ5 I2C controller

[Théo Lebrun]

Add compatible for the integration of the same DB8500 IP block into the
Mobileye EyeQ5 platform. Two quirks are present:

 - The memory bus only supports 32-bit accesses. One writeb() is done to
   fill the Tx FIFO which we replace with a writel().

 - A register must be configured for the I2C speed mode; it is located
   in a shared register region called OLB. We access that memory region
   using a syscon & regmap that gets passed as a phandle (mobileye,olb).

   A two-bit enum per controller is written into the register; that
   requires us to know the global index of the I2C
   controller (mobileye,id).

We add #include <linux/mfd/syscon.h> and <linux/regmap.h> and sort
headers.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/i2c/busses/i2c-nomadik.c | 79 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 71 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index 23e12c570457..eb076419dfa8 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -6,22 +6,30 @@
  * I2C master mode controller driver, used in Nomadik 8815
  * and Ux500 platforms.
  *
+ * The Mobileye EyeQ5 platform is also supported; it uses
+ * the same Ux500/DB8500 IP block with two quirks:
+ *  - The memory bus only supports 32-bit accesses.
+ *  - A register must be configured for the I2C speed mode;
+ *    it is located in a shared register region called OLB.
+ *
  * Author: Srinidhi Kasagar <srinidhi.kasagar@stericsson.com>
  * Author: Sachin Verma <sachin.verma@st.com>
  */
+#include <linux/amba/bus.h>
 #include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
 #include <linux/init.h>
-#include <linux/module.h>
-#include <linux/amba/bus.h>
-#include <linux/slab.h>
 #include <linux/interrupt.h>
-#include <linux/i2c.h>
-#include <linux/err.h>
-#include <linux/clk.h>
 #include <linux/io.h>
-#include <linux/pm_runtime.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
 #include <linux/of.h>
 #include <linux/pinctrl/consumer.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
 
 #define DRIVER_NAME "nmk-i2c"
 
@@ -110,6 +118,10 @@ enum i2c_freq_mode {
 	I2C_FREQ_MODE_FAST_PLUS,	/* up to 1 Mb/s */
 };
 
+/* Mobileye EyeQ5 offset into a shared register region (called OLB) */
+#define NMK_I2C_EYEQ5_OLB_IOCR2			0x0B8
+#define NMK_I2C_EYEQ5_OLB_IOCR2_SHIFT(id)	(4 + 2 * (id))
+
 /**
  * struct i2c_vendor_data - per-vendor variations
  * @has_mtdws: variant has the MTDWS bit
@@ -168,6 +180,7 @@ struct i2c_nmk_client {
  * @xfer_wq: xfer done wait queue.
  * @xfer_done: xfer done boolean.
  * @result: controller propogated result.
+ * @has_32b_bus: controller is on a bus that only supports 32-bit accesses.
  */
 struct nmk_i2c_dev {
 	struct i2c_vendor_data		*vendor;
@@ -186,6 +199,7 @@ struct nmk_i2c_dev {
 	struct wait_queue_head		xfer_wq;
 	bool				xfer_done;
 	int				result;
+	bool				has_32b_bus;
 };
 
 /* controller's abort causes */
@@ -514,7 +528,10 @@ static void fill_tx_fifo(struct nmk_i2c_dev *priv, int no_bytes)
 			(priv->cli.count != 0);
 			count--) {
 		/* write to the Tx FIFO */
-		writeb(*priv->cli.buffer, priv->virtbase + I2C_TFR);
+		if (priv->has_32b_bus)
+			writel(*priv->cli.buffer, priv->virtbase + I2C_TFR);
+		else
+			writeb(*priv->cli.buffer, priv->virtbase + I2C_TFR);
 		priv->cli.buffer++;
 		priv->cli.count--;
 		priv->cli.xfer_bytes++;
@@ -985,6 +1002,43 @@ static void nmk_i2c_of_probe(struct device_node *np,
 		priv->timeout_usecs = 200 * USEC_PER_MSEC;
 }
 
+static int nmk_i2c_eyeq5_probe(struct nmk_i2c_dev *priv)
+{
+	struct device *dev = &priv->adev->dev;
+	struct device_node *np = dev->of_node;
+	unsigned int shift, speed_mode;
+	struct regmap *olb;
+	int ret;
+	u32 id;
+
+	priv->has_32b_bus = true;
+
+	olb = syscon_regmap_lookup_by_phandle(np, "mobileye,olb");
+	if (IS_ERR_OR_NULL(olb)) {
+		if (!olb)
+			olb = ERR_PTR(-ENOENT);
+		return dev_err_probe(dev, PTR_ERR(olb),
+				     "failed OLB lookup: %lu\n", PTR_ERR(olb));
+	}
+
+	ret = of_property_read_u32(np, "mobileye,id", &id);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed ID lookup: %d\n", ret);
+
+	if (priv->clk_freq <= 400000)
+		speed_mode = 0b00;
+	else if (priv->clk_freq <= 1000000)
+		speed_mode = 0b01;
+	else
+		speed_mode = 0b10;
+
+	shift = NMK_I2C_EYEQ5_OLB_IOCR2_SHIFT(id);
+	regmap_update_bits(olb, NMK_I2C_EYEQ5_OLB_IOCR2,
+			   0b11 << shift, speed_mode << shift);
+
+	return 0;
+}
+
 static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
 {
 	int ret = 0;
@@ -1001,8 +1055,17 @@ static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
 
 	priv->vendor = vendor;
 	priv->adev = adev;
+	priv->has_32b_bus = false;
 	nmk_i2c_of_probe(np, priv);
 
+	if (of_device_is_compatible(np, "mobileye,eyeq5-i2c")) {
+		ret = nmk_i2c_eyeq5_probe(priv);
+		if (ret) {
+			dev_info(dev, "%s: %d: %d\n", __func__, __LINE__, ret);
+			return ret;
+		}
+	}
+
 	if (priv->tft > max_fifo_threshold) {
 		dev_warn(dev, "requested TX FIFO threshold %u, adjusted down to %u\n",
 			 priv->tft, max_fifo_threshold);

-- 
2.43.1

Re: [PATCH 10/13] i2c: nomadik: support Mobileye EyeQ5 I2C controller

[Linus Walleij]

On Thu, Feb 15, 2024 at 5:52 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote:

> Add compatible for the integration of the same DB8500 IP block into the
> Mobileye EyeQ5 platform. Two quirks are present:
>
>  - The memory bus only supports 32-bit accesses. One writeb() is done to
>    fill the Tx FIFO which we replace with a writel().
>
>  - A register must be configured for the I2C speed mode; it is located
>    in a shared register region called OLB. We access that memory region
>    using a syscon & regmap that gets passed as a phandle (mobileye,olb).
>
>    A two-bit enum per controller is written into the register; that
>    requires us to know the global index of the I2C
>    controller (mobileye,id).
>
> We add #include <linux/mfd/syscon.h> and <linux/regmap.h> and sort
> headers.
>
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>

(...)

> -               writeb(*priv->cli.buffer, priv->virtbase + I2C_TFR);
> +               if (priv->has_32b_bus)
> +                       writel(*priv->cli.buffer, priv->virtbase + I2C_TFR);
> +               else
> +                       writeb(*priv->cli.buffer, priv->virtbase + I2C_TFR);

Are the other byte accessors working flawlessly? I get the shivers.
If it's needed in one place I bet the others prefer 32bit access too.

Further the MIPS is big-endian is it not? It feels that this just happens
to work because of byte order access? writel() is little-endian by
definition.

What happens if you replace all writeb():s with something like

static void nmk_write_reg(struct nmk_i2c_dev *priv, u32 reg, u8 val)
{
    if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
        writeb(val, priv->virtbase + reg + 3);
        // if this doesn't work then use writeb((u32)val,
priv->virtbase + reg) I guess
   else
        writeb(val, priv->virtbase + reg);
}

and conversely for readb()?

Other accessors such as iowrite* are perhaps viable in this case, I'm not sure.

Yours,
Linus Walleij

Re: [PATCH 10/13] i2c: nomadik: support Mobileye EyeQ5 I2C controller

[Théo Lebrun]

Hello,

On Mon Feb 19, 2024 at 3:35 PM CET, Linus Walleij wrote:
> On Thu, Feb 15, 2024 at 5:52 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote:
>
> > Add compatible for the integration of the same DB8500 IP block into the
> > Mobileye EyeQ5 platform. Two quirks are present:
> >
> >  - The memory bus only supports 32-bit accesses. One writeb() is done to
> >    fill the Tx FIFO which we replace with a writel().
> >
> >  - A register must be configured for the I2C speed mode; it is located
> >    in a shared register region called OLB. We access that memory region
> >    using a syscon & regmap that gets passed as a phandle (mobileye,olb).
> >
> >    A two-bit enum per controller is written into the register; that
> >    requires us to know the global index of the I2C
> >    controller (mobileye,id).
> >
> > We add #include <linux/mfd/syscon.h> and <linux/regmap.h> and sort
> > headers.
> >
> > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
>
> (...)
>
> > -               writeb(*priv->cli.buffer, priv->virtbase + I2C_TFR);
> > +               if (priv->has_32b_bus)
> > +                       writel(*priv->cli.buffer, priv->virtbase + I2C_TFR);
> > +               else
> > +                       writeb(*priv->cli.buffer, priv->virtbase + I2C_TFR);
>
> Are the other byte accessors working flawlessly? I get the shivers.
> If it's needed in one place I bet the others prefer 32bit access too.

I see where your shivers come from; I'll investigate as I don't remember
my conclusion from the time when I worked on this driver (a few months
ago).

> Further the MIPS is big-endian is it not? It feels that this just happens
> to work because of byte order access? writel() is little-endian by
> definition.

Actually, no. Our platform is little-endian.

The full story, summarised: the endianness of our cores in kernel and
hypervisor mode is defined by a pin read at reset. User mode can toggle
the endianness at runtime I believe, but that is not of our concern.
Our endianness in kernel mode is little-endian because the pin in
question is hardwired to the value meaning little-endian.

> What happens if you replace all writeb():s with something like
>
> static void nmk_write_reg(struct nmk_i2c_dev *priv, u32 reg, u8 val)
> {
>     if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
>         writeb(val, priv->virtbase + reg + 3);
>         // if this doesn't work then use writeb((u32)val,
> priv->virtbase + reg) I guess
>    else
>         writeb(val, priv->virtbase + reg);
> }
>
> and conversely for readb()?

As mentionned above, big endian isn't the worry for us. I'll be checking
the readb() calls found in i2c_irq_handler() though.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 10/13] i2c: nomadik: support Mobileye EyeQ5 I2C controller

[Théo Lebrun]

Hello,

On Mon Feb 19, 2024 at 3:52 PM CET, Théo Lebrun wrote:
> On Mon Feb 19, 2024 at 3:35 PM CET, Linus Walleij wrote:
> > On Thu, Feb 15, 2024 at 5:52 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote:
> >
> > > Add compatible for the integration of the same DB8500 IP block into the
> > > Mobileye EyeQ5 platform. Two quirks are present:
> > >
> > >  - The memory bus only supports 32-bit accesses. One writeb() is done to
> > >    fill the Tx FIFO which we replace with a writel().
> > >
> > >  - A register must be configured for the I2C speed mode; it is located
> > >    in a shared register region called OLB. We access that memory region
> > >    using a syscon & regmap that gets passed as a phandle (mobileye,olb).
> > >
> > >    A two-bit enum per controller is written into the register; that
> > >    requires us to know the global index of the I2C
> > >    controller (mobileye,id).
> > >
> > > We add #include <linux/mfd/syscon.h> and <linux/regmap.h> and sort
> > > headers.
> > >
> > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> >
> > (...)
> >
> > > -               writeb(*priv->cli.buffer, priv->virtbase + I2C_TFR);
> > > +               if (priv->has_32b_bus)
> > > +                       writel(*priv->cli.buffer, priv->virtbase + I2C_TFR);
> > > +               else
> > > +                       writeb(*priv->cli.buffer, priv->virtbase + I2C_TFR);
> >
> > Are the other byte accessors working flawlessly? I get the shivers.
> > If it's needed in one place I bet the others prefer 32bit access too.
>
> I see where your shivers come from; I'll investigate as I don't remember
> my conclusion from the time when I worked on this driver (a few months
> ago).
>
> > Further the MIPS is big-endian is it not? It feels that this just happens
> > to work because of byte order access? writel() is little-endian by
> > definition.
>
> Actually, no. Our platform is little-endian.
>
> The full story, summarised: the endianness of our cores in kernel and
> hypervisor mode is defined by a pin read at reset. User mode can toggle
> the endianness at runtime I believe, but that is not of our concern.
> Our endianness in kernel mode is little-endian because the pin in
> question is hardwired to the value meaning little-endian.
>
> > What happens if you replace all writeb():s with something like
> >
> > static void nmk_write_reg(struct nmk_i2c_dev *priv, u32 reg, u8 val)
> > {
> >     if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
> >         writeb(val, priv->virtbase + reg + 3);
> >         // if this doesn't work then use writeb((u32)val,
> > priv->virtbase + reg) I guess
> >    else
> >         writeb(val, priv->virtbase + reg);
> > }
> >
> > and conversely for readb()?
>
> As mentionned above, big endian isn't the worry for us. I'll be checking
> the readb() calls found in i2c_irq_handler() though.

Follow up on this. It was working by luck.

 - writeb() are generating Store Byte (sb) instructions which are
   unsupported on the memory bus.

 - readb() are generating Load Doubleword (ld) instructions and not the
   expected Load Byte (lb). It explains why readb() are working.

To be safe I'll make sure to use readl() and writel() everywhere for our
compatible. There is one writeb() and three readb(). Only the writeb()
was covered by this V1.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH 11/13] MIPS: mobileye: eyeq5: add 5 I2C controller nodes

[Théo Lebrun]

Add the SoC I2C controller nodes to the platform devicetree. Use a
default bus frequency of 400kHz. They are AMBA devices that are matched
on PeriphID.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 arch/mips/boot/dts/mobileye/eyeq5.dtsi | 70 ++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/arch/mips/boot/dts/mobileye/eyeq5.dtsi b/arch/mips/boot/dts/mobileye/eyeq5.dtsi
index 8d4f65ec912d..d27e164f0fc1 100644
--- a/arch/mips/boot/dts/mobileye/eyeq5.dtsi
+++ b/arch/mips/boot/dts/mobileye/eyeq5.dtsi
@@ -70,6 +70,76 @@ soc: soc {
 		ranges;
 		compatible = "simple-bus";
 
+		i2c0: i2c@300000 {
+			compatible = "mobileye,eyeq5-i2c", "arm,primecell";
+			reg = <0 0x300000 0x0 0x1000>;
+			interrupt-parent = <&gic>;
+			interrupts = <GIC_SHARED 1 IRQ_TYPE_LEVEL_HIGH>;
+			clock-frequency = <400000>; /* Fast mode */
+			#address-cells = <1>;
+			#size-cells = <0>;
+			clocks = <&i2c_ser_clk>, <&i2c_clk>;
+			clock-names = "i2cclk", "apb_pclk";
+			mobileye,olb = <&olb>;
+			mobileye,id = <0>;
+		};
+
+		i2c1: i2c@400000 {
+			compatible = "mobileye,eyeq5-i2c", "arm,primecell";
+			reg = <0 0x400000 0x0 0x1000>;
+			interrupt-parent = <&gic>;
+			interrupts = <GIC_SHARED 2 IRQ_TYPE_LEVEL_HIGH>;
+			clock-frequency = <400000>; /* Fast mode */
+			#address-cells = <1>;
+			#size-cells = <0>;
+			clocks = <&i2c_ser_clk>, <&i2c_clk>;
+			clock-names = "i2cclk", "apb_pclk";
+			mobileye,olb = <&olb>;
+			mobileye,id = <1>;
+		};
+
+		i2c2: i2c@500000 {
+			compatible = "mobileye,eyeq5-i2c", "arm,primecell";
+			reg = <0 0x500000 0x0 0x1000>;
+			interrupt-parent = <&gic>;
+			interrupts = <GIC_SHARED 3 IRQ_TYPE_LEVEL_HIGH>;
+			clock-frequency = <400000>; /* Fast mode */
+			#address-cells = <1>;
+			#size-cells = <0>;
+			clocks = <&i2c_ser_clk>, <&i2c_clk>;
+			clock-names = "i2cclk", "apb_pclk";
+			mobileye,olb = <&olb>;
+			mobileye,id = <2>;
+		};
+
+		i2c3: i2c@600000 {
+			compatible = "mobileye,eyeq5-i2c", "arm,primecell";
+			reg = <0 0x600000 0x0 0x1000>;
+			interrupt-parent = <&gic>;
+			interrupts = <GIC_SHARED 4 IRQ_TYPE_LEVEL_HIGH>;
+			clock-frequency = <400000>; /* Fast mode */
+			#address-cells = <1>;
+			#size-cells = <0>;
+			clocks = <&i2c_ser_clk>, <&i2c_clk>;
+			clock-names = "i2cclk", "apb_pclk";
+			mobileye,olb = <&olb>;
+			mobileye,id = <3>;
+		};
+
+		i2c4: i2c@700000 {
+			compatible = "mobileye,eyeq5-i2c", "arm,primecell";
+			reg = <0 0x700000 0x0 0x1000>;
+			interrupt-parent = <&gic>;
+			interrupts = <GIC_SHARED 5 IRQ_TYPE_LEVEL_HIGH>;
+			clock-frequency = <400000>; /* Fast mode */
+			#address-cells = <1>;
+			#size-cells = <0>;
+			clocks = <&i2c_ser_clk>, <&i2c_clk>;
+			clock-names = "i2cclk", "apb_pclk";
+			mobileye,olb = <&olb>;
+			mobileye,id = <4>;
+		};
+
 		uart0: serial@800000 {
 			compatible = "arm,pl011", "arm,primecell";
 			reg = <0 0x800000 0x0 0x1000>;

-- 
2.43.1

[PATCH 12/13] MIPS: mobileye: eyeq5: add evaluation board I2C temp sensor

[Théo Lebrun]

Declare the temperature sensor on I2C bus 2. Its label is the schematics
identifier.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 arch/mips/boot/dts/mobileye/eyeq5-epm5.dts | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/mips/boot/dts/mobileye/eyeq5-epm5.dts b/arch/mips/boot/dts/mobileye/eyeq5-epm5.dts
index 6898b2d8267d..1f8549acd40d 100644
--- a/arch/mips/boot/dts/mobileye/eyeq5-epm5.dts
+++ b/arch/mips/boot/dts/mobileye/eyeq5-epm5.dts
@@ -21,3 +21,11 @@ memory@0 {
 		      <0x8 0x02000000 0x0 0x7E000000>;
 	};
 };
+
+&i2c2 {
+	tmp112@48 {
+		compatible = "ti,tmp112";
+		reg = <0x48>;
+		label = "U60";
+	};
+};

-- 
2.43.1

Re: [PATCH 12/13] MIPS: mobileye: eyeq5: add evaluation board I2C temp sensor

[Krzysztof Kozlowski]

On 15/02/2024 17:52, Théo Lebrun wrote:
> Declare the temperature sensor on I2C bus 2. Its label is the schematics
> identifier.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  arch/mips/boot/dts/mobileye/eyeq5-epm5.dts | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/mips/boot/dts/mobileye/eyeq5-epm5.dts b/arch/mips/boot/dts/mobileye/eyeq5-epm5.dts
> index 6898b2d8267d..1f8549acd40d 100644
> --- a/arch/mips/boot/dts/mobileye/eyeq5-epm5.dts
> +++ b/arch/mips/boot/dts/mobileye/eyeq5-epm5.dts
> @@ -21,3 +21,11 @@ memory@0 {
>  		      <0x8 0x02000000 0x0 0x7E000000>;
>  	};
>  };
> +
> +&i2c2 {
> +	tmp112@48 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation


temperature-sensor, power-sensor etc.

Best regards,
Krzysztof

[PATCH 13/13] MIPS: mobileye: eyeq5: add resets to I2C controllers

[Théo Lebrun]

Add resets properties to each I2C controller. This depends on the
reset-eyeq5 platform reset controller driver.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 arch/mips/boot/dts/mobileye/eyeq5.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/mips/boot/dts/mobileye/eyeq5.dtsi b/arch/mips/boot/dts/mobileye/eyeq5.dtsi
index d27e164f0fc1..c0842836fcc8 100644
--- a/arch/mips/boot/dts/mobileye/eyeq5.dtsi
+++ b/arch/mips/boot/dts/mobileye/eyeq5.dtsi
@@ -80,6 +80,7 @@ i2c0: i2c@300000 {
 			#size-cells = <0>;
 			clocks = <&i2c_ser_clk>, <&i2c_clk>;
 			clock-names = "i2cclk", "apb_pclk";
+			resets = <&reset 0 13>;
 			mobileye,olb = <&olb>;
 			mobileye,id = <0>;
 		};
@@ -94,6 +95,7 @@ i2c1: i2c@400000 {
 			#size-cells = <0>;
 			clocks = <&i2c_ser_clk>, <&i2c_clk>;
 			clock-names = "i2cclk", "apb_pclk";
+			resets = <&reset 0 14>;
 			mobileye,olb = <&olb>;
 			mobileye,id = <1>;
 		};
@@ -108,6 +110,7 @@ i2c2: i2c@500000 {
 			#size-cells = <0>;
 			clocks = <&i2c_ser_clk>, <&i2c_clk>;
 			clock-names = "i2cclk", "apb_pclk";
+			resets = <&reset 0 15>;
 			mobileye,olb = <&olb>;
 			mobileye,id = <2>;
 		};
@@ -122,6 +125,7 @@ i2c3: i2c@600000 {
 			#size-cells = <0>;
 			clocks = <&i2c_ser_clk>, <&i2c_clk>;
 			clock-names = "i2cclk", "apb_pclk";
+			resets = <&reset 0 16>;
 			mobileye,olb = <&olb>;
 			mobileye,id = <3>;
 		};
@@ -136,6 +140,7 @@ i2c4: i2c@700000 {
 			#size-cells = <0>;
 			clocks = <&i2c_ser_clk>, <&i2c_clk>;
 			clock-names = "i2cclk", "apb_pclk";
+			resets = <&reset 0 17>;
 			mobileye,olb = <&olb>;
 			mobileye,id = <4>;
 		};

-- 
2.43.1

Re: [PATCH 13/13] MIPS: mobileye: eyeq5: add resets to I2C controllers

[Krzysztof Kozlowski]

On 15/02/2024 17:52, Théo Lebrun wrote:
> Add resets properties to each I2C controller. This depends on the
> reset-eyeq5 platform reset controller driver.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---

This should be squashed with previous patch adding i2c controllers.
Don't add incomplete nodes just to fix them in next patch.

Best regards,
Krzysztof

Re: [PATCH 13/13] MIPS: mobileye: eyeq5: add resets to I2C controllers

[Théo Lebrun]

Hello,

On Fri Feb 16, 2024 at 8:59 AM CET, Krzysztof Kozlowski wrote:
> On 15/02/2024 17:52, Théo Lebrun wrote:
> > Add resets properties to each I2C controller. This depends on the
> > reset-eyeq5 platform reset controller driver.
> > 
> > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> > ---
>
> This should be squashed with previous patch adding i2c controllers.
> Don't add incomplete nodes just to fix them in next patch.

The goal was to isolate reset phandles to a single patch. The series
with this patch dropped works because resets in their default state are
deasserted, so this isn't a fix. And it allows testing the series on
hardware with only the base platform series, which I found useful.

Noted, I'll be squashed for next revision.

Regards,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 13/13] MIPS: mobileye: eyeq5: add resets to I2C controllers

[Krzysztof Kozlowski]

On 16/02/2024 10:05, Théo Lebrun wrote:
> Hello,
> 
> On Fri Feb 16, 2024 at 8:59 AM CET, Krzysztof Kozlowski wrote:
>> On 15/02/2024 17:52, Théo Lebrun wrote:
>>> Add resets properties to each I2C controller. This depends on the
>>> reset-eyeq5 platform reset controller driver.
>>>
>>> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
>>> ---
>>
>> This should be squashed with previous patch adding i2c controllers.
>> Don't add incomplete nodes just to fix them in next patch.
> 
> The goal was to isolate reset phandles to a single patch. The series

That was what you did, not the goal. If that's the goal, then it is
clearly wrong.

> with this patch dropped works because resets in their default state are
> deasserted, so this isn't a fix. And it allows testing the series on
> hardware with only the base platform series, which I found useful.

Series or half-of-series? Anyway, commits must be logical chunks, so one
chunk is to add I2C controllers, not "part of I2C controllers". DTS is
also independent of drivers (and it will go via different trees!), so
whatever dependency you think of, it does not exist.

Best regards,
Krzysztof

Re: [PATCH 13/13] MIPS: mobileye: eyeq5: add resets to I2C controllers

[Théo Lebrun]

Hello,

On Fri Feb 16, 2024 at 10:17 AM CET, Krzysztof Kozlowski wrote:
> On 16/02/2024 10:05, Théo Lebrun wrote:
> > Hello,
> > 
> > On Fri Feb 16, 2024 at 8:59 AM CET, Krzysztof Kozlowski wrote:
> >> On 15/02/2024 17:52, Théo Lebrun wrote:
> >>> Add resets properties to each I2C controller. This depends on the
> >>> reset-eyeq5 platform reset controller driver.
> >>>
> >>> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> >>> ---
> >>
> >> This should be squashed with previous patch adding i2c controllers.
> >> Don't add incomplete nodes just to fix them in next patch.
> > 
> > The goal was to isolate reset phandles to a single patch. The series
>
> That was what you did, not the goal. If that's the goal, then it is
> clearly wrong.
>
> > with this patch dropped works because resets in their default state are
> > deasserted, so this isn't a fix. And it allows testing the series on
> > hardware with only the base platform series, which I found useful.
>
> Series or half-of-series? Anyway, commits must be logical chunks, so one
> chunk is to add I2C controllers, not "part of I2C controllers". DTS is
> also independent of drivers (and it will go via different trees!), so
> whatever dependency you think of, it does not exist.

My reasoning was focused on my point-of-view as a contributor and tester
of the series. Your explanation makes sense; I had never thought this
through from the maintainer's POV.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 00/13] Add Mobileye EyeQ5 support to the Nomadik I2C controller & use hrtimers for timeouts

[Linus Walleij]

On Thu, Feb 15, 2024 at 5:52 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote:

>  - Add a new compatible to support Mobileye EyeQ5 which uses the same IP
>    block as Nomadik.

Sweet! I'm amazed ST Micro licensed this "ARM PrimeCell" to Mobileye, but
it's a well tested IP and used in eg ST automotive SoC:s so it's a solid
product.

It feels worth it for all the time I have put into maintaining it, finally some
real users again! :)

Yours,
Linus Walleij

Re: [PATCH 00/13] Add Mobileye EyeQ5 support to the Nomadik I2C controller & use hrtimers for timeouts

[Théo Lebrun]

Hello,

On Mon Feb 19, 2024 at 3:11 PM CET, Linus Walleij wrote:
> On Thu, Feb 15, 2024 at 5:52 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote:
>
> >  - Add a new compatible to support Mobileye EyeQ5 which uses the same IP
> >    block as Nomadik.
>
> Sweet! I'm amazed ST Micro licensed this "ARM PrimeCell" to Mobileye, but
> it's a well tested IP and used in eg ST automotive SoC:s so it's a solid
> product.
>
> It feels worth it for all the time I have put into maintaining it, finally some
> real users again! :)

Using the existing Nomadik drivers with the AMBA bus infrastructure on a
non-ARM platform and having it work as-is made our day here at Bootlin.
We are indeed grateful for your work maintaining this platform!

Regards,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

------------------------------------------------------------------------