[PATCH dt-schema 0/3] schemas: i2c: fix "smbus_alert" handling

[PATCH dt-schema 0/3] schemas: i2c: fix "smbus_alert" handling

[Wolfram Sang]

"smbus_alert" is currently listed as an interrupt for targets. This is
wrong, it is an interrupt for the controller. The mistake (mea culpa!)
was already in the originating "i2c.txt" file in the Linux Kernel. Time
to fix it. Make small changes to descriptions to use inclusive language
while we are here.

Sadly, not tested with 'dt-doc-validate'. I tried to install my local
dtschema repo with

 $ ~/.venv/bin/pip3 install -e .

and got:

 error: Multiple top-level packages discovered in a flat-layout: ['patches', 'dtschema'].

I am afraid I am not enough of a Python guy to fix this on my own.

Looking forward to comments...


Wolfram Sang (3):
  schemas: i2c: reword descriptions for inclusive language
  schemas: i2c: clean up interrupt descriptions for I2C targets
  schemas: i2c: add generic interrupt name for I2C controllers

 dtschema/schemas/i2c/i2c-controller.yaml | 36 +++++++++++++++++-------
 1 file changed, 26 insertions(+), 10 deletions(-)

-- 
2.43.0

[PATCH dt-schema 1/3] schemas: i2c: reword descriptions for inclusive language

[Wolfram Sang]

Changing bindings is hard, changing descriptions is easy. Let's start
with the low-hanging fruits and use the official I2C terminology (as of
specs v7) in the descriptions. Drop a superfluous 'a' from the
description of 'reg' for targets.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 dtschema/schemas/i2c/i2c-controller.yaml | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/dtschema/schemas/i2c/i2c-controller.yaml b/dtschema/schemas/i2c/i2c-controller.yaml
index 9670082..e475ead 100644
--- a/dtschema/schemas/i2c/i2c-controller.yaml
+++ b/dtschema/schemas/i2c/i2c-controller.yaml
@@ -99,7 +99,7 @@ properties:
   multi-master:
     type: boolean
     description:
-      States that there is another master active on this bus. The OS can use
+      States that there is another controller active on this bus. The OS can use
       this information to adapt power management to keep the arbitration awake
       all the time, for example. Can not be combined with 'single-master'.
 
@@ -116,9 +116,9 @@ properties:
   single-master:
     type: boolean
     description:
-      States that there is no other master active on this bus. The OS can use
-      this information to detect a stalled bus more reliably, for example. Can
-      not be combined with 'multi-master'.
+      States that there is no other controller active on this bus. The OS can
+      use this information to detect a stalled bus more reliably, for example.
+      Can not be combined with 'multi-master'.
 
   smbus:
     type: boolean
@@ -155,7 +155,7 @@ patternProperties:
                 - minimum: 0xc0000000
                   maximum: 0xc00003ff
         description: |
-          One or many I2C slave addresses. These are usually a 7 bit addresses.
+          One or many I2C target addresses. These are usually 7 bit addresses.
           However, flags can be attached to an address. I2C_TEN_BIT_ADDRESS is
           used to mark a 10 bit address. It is needed to avoid the ambiguity
           between e.g. a 7 bit address of 0x50 and a 10 bit address of 0x050
@@ -172,7 +172,7 @@ patternProperties:
       interrupts:
         description:
           I2C core will treat "irq" interrupt (or the very first interrupt if
-          not using interrupt names) as primary interrupt for the slave.
+          not using interrupt names) as primary interrupt for the target.
 
       interrupt-names:
         anyOf:
-- 
2.43.0

[PATCH dt-schema 2/3] schemas: i2c: clean up interrupt descriptions for I2C targets

[Wolfram Sang]

Schemas should be OS agnostic, so don't mention what the "I2C core" will
do because this only applies to Linux. Also, drop the generic
"smbus_alert" naming because this belongs to controllers not targets, so
we don't want to describe it here.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 dtschema/schemas/i2c/i2c-controller.yaml | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/dtschema/schemas/i2c/i2c-controller.yaml b/dtschema/schemas/i2c/i2c-controller.yaml
index e475ead..7eb6b0f 100644
--- a/dtschema/schemas/i2c/i2c-controller.yaml
+++ b/dtschema/schemas/i2c/i2c-controller.yaml
@@ -171,8 +171,8 @@ patternProperties:
 
       interrupts:
         description:
-          I2C core will treat "irq" interrupt (or the very first interrupt if
-          not using interrupt names) as primary interrupt for the target.
+          If not using interrupt-names, the first interrupt will be treated as
+          the primary interrupt for the target.
 
       interrupt-names:
         anyOf:
@@ -181,10 +181,10 @@ patternProperties:
               enum:
                 - irq
                 - wakeup
-                - smbus_alert
             description:
-              Names which are recognized by I2C core, other names are left to
-              individual bindings.
+              Generic names are "irq" for the primary interrupt and "wakeup"
+              for the wakeup interrupt. Other names are left to individual
+              bindings.
 
       wakeup-source:
         description:
-- 
2.43.0

[PATCH dt-schema 3/3] schemas: i2c: add generic interrupt name for I2C controllers

[Wolfram Sang]

"smbus_alert" is a generic interrupt name for controllers, so document
it. Introduce also "irq" for the primary interrupt.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 dtschema/schemas/i2c/i2c-controller.yaml | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/dtschema/schemas/i2c/i2c-controller.yaml b/dtschema/schemas/i2c/i2c-controller.yaml
index 7eb6b0f..da277a2 100644
--- a/dtschema/schemas/i2c/i2c-controller.yaml
+++ b/dtschema/schemas/i2c/i2c-controller.yaml
@@ -90,6 +90,22 @@ properties:
       low-pass analog filter). Typical value should be above the normal i2c bus
       clock frequency (clock-frequency). Specified in Hz.
 
+  interrupts:
+    description:
+      If not using interrupt-names, the first interrupt will be treated as the
+      primary interrupt for the controller.
+
+  interrupt-names:
+    anyOf:
+      - {} # Any name is allowed.
+      - items:
+          enum:
+            - irq
+            - smbus_alert
+        description:
+          Generic names are "irq" for a primary interrupt and "smbus_alert" for
+          the SMBusAlert signal. Other names are left to individual bindings.
+
   mctp-controller:
     type: boolean
     description:
-- 
2.43.0

Re: [PATCH dt-schema 3/3] schemas: i2c: add generic interrupt name for I2C controllers

[Rob Herring]

On Thu, Aug 15, 2024 at 6:09 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> "smbus_alert" is a generic interrupt name for controllers, so document
> it. Introduce also "irq" for the primary interrupt.

I wouldn't add "irq". It's kind of odd in that we have it for I2C
devices in the first place and we don't try to do that anywhere else.

> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  dtschema/schemas/i2c/i2c-controller.yaml | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/dtschema/schemas/i2c/i2c-controller.yaml b/dtschema/schemas/i2c/i2c-controller.yaml
> index 7eb6b0f..da277a2 100644
> --- a/dtschema/schemas/i2c/i2c-controller.yaml
> +++ b/dtschema/schemas/i2c/i2c-controller.yaml
> @@ -90,6 +90,22 @@ properties:
>        low-pass analog filter). Typical value should be above the normal i2c bus
>        clock frequency (clock-frequency). Specified in Hz.
>
> +  interrupts:
> +    description:
> +      If not using interrupt-names, the first interrupt will be treated as the
> +      primary interrupt for the controller.
> +
> +  interrupt-names:
> +    anyOf:
> +      - {} # Any name is allowed.
> +      - items:
> +          enum:
> +            - irq
> +            - smbus_alert

It doesn't really matter with the {}, but the 2nd entry is only true
if interrupt-names only contains these 2 names. I'd use 'contains'
rather than 'items' here.

> +        description:
> +          Generic names are "irq" for a primary interrupt and "smbus_alert" for
> +          the SMBusAlert signal. Other names are left to individual bindings.
> +
>    mctp-controller:
>      type: boolean
>      description:
> --
> 2.43.0
>
>

Re: [PATCH dt-schema 3/3] schemas: i2c: add generic interrupt name for I2C controllers

[Wolfram Sang]

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

Hi Rob,

thanks for the super-fast review!

> > "smbus_alert" is a generic interrupt name for controllers, so document
> > it. Introduce also "irq" for the primary interrupt.
> 
> I wouldn't add "irq". It's kind of odd in that we have it for I2C
> devices in the first place and we don't try to do that anywhere else.

Yes, I am actually happy to remove "irq". I only added it for
consistency.

> > +  interrupt-names:
> > +    anyOf:
> > +      - {} # Any name is allowed.
> > +      - items:
> > +          enum:
> > +            - irq
> > +            - smbus_alert
> 
> It doesn't really matter with the {}, but the 2nd entry is only true
> if interrupt-names only contains these 2 names. I'd use 'contains'
> rather than 'items' here.

So, just replace "items:" with "contains:", right? Should I also change
this for I2C targets and add this chunk to patch 2?

All the best,

   Wolfram


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

Re: [PATCH dt-schema 3/3] schemas: i2c: add generic interrupt name for I2C controllers

[Rob Herring]

On Thu, Aug 15, 2024 at 12:43 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> Hi Rob,
>
> thanks for the super-fast review!
>
> > > "smbus_alert" is a generic interrupt name for controllers, so document
> > > it. Introduce also "irq" for the primary interrupt.
> >
> > I wouldn't add "irq". It's kind of odd in that we have it for I2C
> > devices in the first place and we don't try to do that anywhere else.
>
> Yes, I am actually happy to remove "irq". I only added it for
> consistency.
>
> > > +  interrupt-names:
> > > +    anyOf:
> > > +      - {} # Any name is allowed.
> > > +      - items:
> > > +          enum:
> > > +            - irq
> > > +            - smbus_alert
> >
> > It doesn't really matter with the {}, but the 2nd entry is only true
> > if interrupt-names only contains these 2 names. I'd use 'contains'
> > rather than 'items' here.
>
> So, just replace "items:" with "contains:", right? Should I also change
> this for I2C targets and add this chunk to patch 2?

Oh, I missed that you just copied that, but yes, if you can update
that one too, it would be good.

Rob

Re: [PATCH dt-schema 0/3] schemas: i2c: fix "smbus_alert" handling

[Rob Herring]

On Thu, Aug 15, 2024 at 6:09 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> "smbus_alert" is currently listed as an interrupt for targets. This is
> wrong, it is an interrupt for the controller. The mistake (mea culpa!)
> was already in the originating "i2c.txt" file in the Linux Kernel. Time
> to fix it. Make small changes to descriptions to use inclusive language
> while we are here.
>
> Sadly, not tested with 'dt-doc-validate'. I tried to install my local
> dtschema repo with
>
>  $ ~/.venv/bin/pip3 install -e .
>
> and got:
>
>  error: Multiple top-level packages discovered in a flat-layout: ['patches', 'dtschema'].

Do you have a 'patches' directory? Not sure why that's a problem other
than python having specific ideas on directory structures.

Rob

Re: [PATCH dt-schema 0/3] schemas: i2c: fix "smbus_alert" handling

[Wolfram Sang]

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


> >  error: Multiple top-level packages discovered in a flat-layout: ['patches', 'dtschema'].
> 
> Do you have a 'patches' directory? Not sure why that's a problem other
> than python having specific ideas on directory structures.

Oh, wow, yes, I had a 'patches' dir and that was really an issue. Weird.
Thanks for helping out. I got one step further, now I get:

  × Building wheel for pylibfdt (pyproject.toml) did not run successfully.
  │ exit code: 1
  ╰─> [14 lines of output]
      WARNING setuptools_scm.pyproject_reading toml section missing 'pyproject.toml does not contain a tool.setuptools_scm section'
      Traceback (most recent call last):
        File "/tmp/pip-build-env-5v1x378l/normal/lib/python3.11/site-packages/setuptools_scm/_integration/pyproject_reading.py", line 36, in read_pyproject
          section = defn.get("tool", {})[tool_name]
                    ~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^
      KeyError: 'setuptools_scm'
      running bdist_wheel
      running build
      running build_py
      running build_ext
      building '_libfdt' extension
      swigging libfdt/libfdt.i to libfdt/libfdt_wrap.c
      swig -python -Ilibfdt -o libfdt/libfdt_wrap.c libfdt/libfdt.i
      error: command 'swig' failed: No such file or directory
      [end of output]


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

Re: [PATCH dt-schema 0/3] schemas: i2c: fix "smbus_alert" handling

[Rob Herring]

On Thu, Aug 15, 2024 at 12:36 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
>
> > >  error: Multiple top-level packages discovered in a flat-layout: ['patches', 'dtschema'].
> >
> > Do you have a 'patches' directory? Not sure why that's a problem other
> > than python having specific ideas on directory structures.
>
> Oh, wow, yes, I had a 'patches' dir and that was really an issue. Weird.
> Thanks for helping out. I got one step further, now I get:
>
>   × Building wheel for pylibfdt (pyproject.toml) did not run successfully.
>   │ exit code: 1
>   ╰─> [14 lines of output]
>       WARNING setuptools_scm.pyproject_reading toml section missing 'pyproject.toml does not contain a tool.setuptools_scm section'
>       Traceback (most recent call last):
>         File "/tmp/pip-build-env-5v1x378l/normal/lib/python3.11/site-packages/setuptools_scm/_integration/pyproject_reading.py", line 36, in read_pyproject
>           section = defn.get("tool", {})[tool_name]
>                     ~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^
>       KeyError: 'setuptools_scm'
>       running bdist_wheel
>       running build
>       running build_py
>       running build_ext
>       building '_libfdt' extension
>       swigging libfdt/libfdt.i to libfdt/libfdt_wrap.c
>       swig -python -Ilibfdt -o libfdt/libfdt_wrap.c libfdt/libfdt.i
>       error: command 'swig' failed: No such file or directory

You have to install 'swig' which is a C to python binding thing
pylibfdt uses. You should have a distro package for it. There's also a
pip package 'swig' that will download and build it. There's actually a
MR to make that a dependency I need to go look at.

Rob

Re: [PATCH dt-schema 0/3] schemas: i2c: fix "smbus_alert" handling

[Wolfram Sang]

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


> >   × Building wheel for pylibfdt (pyproject.toml) did not run successfully.
> >   │ exit code: 1
> >   ╰─> [14 lines of output]
> >       WARNING setuptools_scm.pyproject_reading toml section missing 'pyproject.toml does not contain a tool.setuptools_scm section'

Thanks for bearing with me. I have to admit I stopped reading after
this...

> >       Traceback (most recent call last):
> >         File "/tmp/pip-build-env-5v1x378l/normal/lib/python3.11/site-packages/setuptools_scm/_integration/pyproject_reading.py", line 36, in read_pyproject
> >           section = defn.get("tool", {})[tool_name]
> >                     ~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^

... and this ...

> >       KeyError: 'setuptools_scm'
> >       running bdist_wheel
> >       running build
> >       running build_py
> >       running build_ext
> >       building '_libfdt' extension
> >       swigging libfdt/libfdt.i to libfdt/libfdt_wrap.c
> >       swig -python -Ilibfdt -o libfdt/libfdt_wrap.c libfdt/libfdt.i
> >       error: command 'swig' failed: No such file or directory

... so I totally missed this. It is even documented in README. Sorry!

> You have to install 'swig' which is a C to python binding thing
> pylibfdt uses. You should have a distro package for it. There's also a

It works now, thank you!

> pip package 'swig' that will download and build it. There's actually a
> MR to make that a dependency I need to go look at.

Sounds like a good idea to save you from support like here ;)

I will send out v2 in a few minutes.


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