[PATCH v3 0/2] i2c-mux-gpmux: Support settle-time-us property

[PATCH v3 0/2] i2c-mux-gpmux: Support settle-time-us property

[Horatiu Vultur]

Add support for settle-time-us property. If this is defined in device
tree then add this delay to mux APIs.

v2->v3:
 - move the binding changes into i2c-mux.yaml
 - read at a later point the DT property 'settle-time-us'

v1->v2:
 - add the changes to i2c-mux-gpmux instead of i2c-mux-gpio to be able
   to use mux_control_select_delay

Horatiu Vultur (2):
  dt-bindings: i2c-mux: Add property for settle time
  i2c-mux-gpmux: Support settle-time-us property

 Documentation/devicetree/bindings/i2c/i2c-mux.yaml | 6 ++++++
 drivers/i2c/muxes/i2c-mux-gpmux.c                  | 4 +++-
 2 files changed, 9 insertions(+), 1 deletion(-)

-- 
2.33.0

[PATCH v3 1/2] dt-bindings: i2c-mux: Add property for settle time

[Horatiu Vultur]

Some HW requires some time for the signals to settle after the muxing is
changed. Allow this time to be specified in device tree.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 Documentation/devicetree/bindings/i2c/i2c-mux.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux.yaml b/Documentation/devicetree/bindings/i2c/i2c-mux.yaml
index 24cac36037f5..4628ff6340c1 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mux.yaml
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux.yaml
@@ -29,6 +29,12 @@ properties:
   '#size-cells':
     const: 0
 
+  settle-time-us:
+    default: 0
+    description:
+      The time required for the signals to settle. Currently only the
+      i2c-mux-gpmux driver supports this optional binding.
+
 patternProperties:
   '^i2c@[0-9a-f]+$':
     $ref: /schemas/i2c/i2c-controller.yaml
-- 
2.33.0

Re: [PATCH v3 1/2] dt-bindings: i2c-mux: Add property for settle time

[Peter Rosin]

On 2021-11-01 13:25, Horatiu Vultur wrote:
> Some HW requires some time for the signals to settle after the muxing is
> changed. Allow this time to be specified in device tree.
> 
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---
>  Documentation/devicetree/bindings/i2c/i2c-mux.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux.yaml b/Documentation/devicetree/bindings/i2c/i2c-mux.yaml
> index 24cac36037f5..4628ff6340c1 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-mux.yaml
> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux.yaml
> @@ -29,6 +29,12 @@ properties:
>    '#size-cells':
>      const: 0
>  
> +  settle-time-us:
> +    default: 0
> +    description:
> +      The time required for the signals to settle. Currently only the
> +      i2c-mux-gpmux driver supports this optional binding.

The information about how i2c-mux-gpmux is special is bound to go stale,
and I don't think we should mention such specific details in the binding.
What I meant was a generic warnings about optional bindings perhaps not
being supported by all drivers, along the lines of this from i2c.txt:

"These properties may not be supported by all drivers. However, if a driver
 wants to support one of the below features, it should adapt these bindings."

However, I now notice that this sentence makes no sense. It looks like it
should be s/adapt/adopt/.

And, in the i2c-mux.yaml case it can simply say "Optional properties"
instead of "These properites" (which refers to a subset of properties
immediately below the text) since with a yaml binding it is always
clear which properties are optional and which are required. Lastly, I
guess this warning belongs in the description.

> +
>  patternProperties:
>    '^i2c@[0-9a-f]+$':
>      $ref: /schemas/i2c/i2c-controller.yaml
> 

Since this is the first optional property, you now need to specify what
properties are required, which is everything but settle-time-us. If you
don't, all properties are required. Which is not what we want...

Something like this should do it, I think:

required:
  - compatible
  - '#address-cells'
  - '#size-cells'

Cheers,
Peter

Re: [PATCH v3 1/2] dt-bindings: i2c-mux: Add property for settle time

[Horatiu Vultur]

The 11/01/2021 15:32, Peter Rosin wrote:

Hi Peter,

> 
> On 2021-11-01 13:25, Horatiu Vultur wrote:
> > Some HW requires some time for the signals to settle after the muxing is
> > changed. Allow this time to be specified in device tree.
> >
> > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> > ---
> >  Documentation/devicetree/bindings/i2c/i2c-mux.yaml | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux.yaml b/Documentation/devicetree/bindings/i2c/i2c-mux.yaml
> > index 24cac36037f5..4628ff6340c1 100644
> > --- a/Documentation/devicetree/bindings/i2c/i2c-mux.yaml
> > +++ b/Documentation/devicetree/bindings/i2c/i2c-mux.yaml
> > @@ -29,6 +29,12 @@ properties:
> >    '#size-cells':
> >      const: 0
> >
> > +  settle-time-us:
> > +    default: 0
> > +    description:
> > +      The time required for the signals to settle. Currently only the
> > +      i2c-mux-gpmux driver supports this optional binding.
> 
> The information about how i2c-mux-gpmux is special is bound to go stale,
> and I don't think we should mention such specific details in the binding.
> What I meant was a generic warnings about optional bindings perhaps not
> being supported by all drivers, along the lines of this from i2c.txt:
> 
> "These properties may not be supported by all drivers. However, if a driver
>  wants to support one of the below features, it should adapt these bindings."
> 
> However, I now notice that this sentence makes no sense. It looks like it
> should be s/adapt/adopt/.
> 
> And, in the i2c-mux.yaml case it can simply say "Optional properties"
> instead of "These properites" (which refers to a subset of properties
> immediately below the text) since with a yaml binding it is always
> clear which properties are optional and which are required. Lastly, I
> guess this warning belongs in the description.
> 
> > +
> >  patternProperties:
> >    '^i2c@[0-9a-f]+$':
> >      $ref: /schemas/i2c/i2c-controller.yaml
> >
> 
> Since this is the first optional property, you now need to specify what
> properties are required, which is everything but settle-time-us. If you
> don't, all properties are required. Which is not what we want...
> 
> Something like this should do it, I think:
> 
> required:
>   - compatible
>   - '#address-cells'
>   - '#size-cells'

Thanks for a detail explanation but I am still struggling with these
bindings. Were you thinking to have something like this?

---
diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux.yaml b/Documentation/devicetree/bindings/i2c/i2c-mux.yaml
index 24cac36037f5..c9fde1bb0fea 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mux.yaml
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux.yaml
@@ -19,6 +19,9 @@ description: |+
   populating the i2c child busses.  If an 'i2c-mux' subnode is present, only
   subnodes of this will be considered as i2c child busses.

+  Optional properties may not be supported by all drivers. However, if a driver
+  wants to support one of the below features, it should adopt these bindings.
+
 properties:
   $nodename:
     pattern: '^(i2c-?)?mux'
@@ -29,6 +32,11 @@ properties:
   '#size-cells':
     const: 0

+  settle-time-us:
+    default: 0
+    description:
+      The time required for the signals to settle.
+
 patternProperties:
   '^i2c@[0-9a-f]+$':
     $ref: /schemas/i2c/i2c-controller.yaml
@@ -41,6 +49,11 @@ patternProperties:

 additionalProperties: true

+required:
+  - compatible
+  - '#address-cells'
+  - '#size-cells'
+
 examples:
   - |
     /*
---

If I have this then my problem is with the required properties because then I
start to get new warnings once I run:

make ARCH=arm CROSS_COMPILE=arm-linux- dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/i2c/i2c-mux.yaml

For example, one of new the warnings is this:

/home/hvultur/linux/arch/arm/boot/dts/am335x-icev2.dt.yaml: mux-mii-hog: 'compatible' is a required property
	From schema: /home/hvultur/linux/Documentation/devicetree/bindings/i2c/i2c-mux.yaml
/home/hvultur/linux/arch/arm/boot/dts/am335x-icev2.dt.yaml: mux-mii-hog: '#address-cells' is a required property
	From schema: /home/hvultur/linux/Documentation/devicetree/bindings/i2c/i2c-mux.yaml
/home/hvultur/linux/arch/arm/boot/dts/am335x-icev2.dt.yaml: mux-mii-hog: '#size-cells' is a required property
	From schema: /home/hvultur/linux/Documentation/devicetree/bindings/i2c/i2c-mux.yaml

If I don't have the required properties then I don't see these new warnings.

Does it mean that actually the properties are optional by default?

> 
> Cheers,
> Peter

-- 
/Horatiu

Re: [PATCH v3 1/2] dt-bindings: i2c-mux: Add property for settle time

[Rob Herring]

On Mon, Nov 01, 2021 at 10:32:01PM +0100, Horatiu Vultur wrote:
> The 11/01/2021 15:32, Peter Rosin wrote:
> 
> Hi Peter,
> 
> > 
> > On 2021-11-01 13:25, Horatiu Vultur wrote:
> > > Some HW requires some time for the signals to settle after the muxing is
> > > changed. Allow this time to be specified in device tree.
> > >
> > > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> > > ---
> > >  Documentation/devicetree/bindings/i2c/i2c-mux.yaml | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux.yaml b/Documentation/devicetree/bindings/i2c/i2c-mux.yaml
> > > index 24cac36037f5..4628ff6340c1 100644
> > > --- a/Documentation/devicetree/bindings/i2c/i2c-mux.yaml
> > > +++ b/Documentation/devicetree/bindings/i2c/i2c-mux.yaml
> > > @@ -29,6 +29,12 @@ properties:
> > >    '#size-cells':
> > >      const: 0
> > >
> > > +  settle-time-us:
> > > +    default: 0
> > > +    description:
> > > +      The time required for the signals to settle. Currently only the
> > > +      i2c-mux-gpmux driver supports this optional binding.
> > 
> > The information about how i2c-mux-gpmux is special is bound to go stale,
> > and I don't think we should mention such specific details in the binding.
> > What I meant was a generic warnings about optional bindings perhaps not
> > being supported by all drivers, along the lines of this from i2c.txt:
> > 
> > "These properties may not be supported by all drivers. However, if a driver
> >  wants to support one of the below features, it should adapt these bindings."
> > 
> > However, I now notice that this sentence makes no sense. It looks like it
> > should be s/adapt/adopt/.
> > 
> > And, in the i2c-mux.yaml case it can simply say "Optional properties"
> > instead of "These properites" (which refers to a subset of properties
> > immediately below the text) since with a yaml binding it is always
> > clear which properties are optional and which are required. Lastly, I
> > guess this warning belongs in the description.
> > 
> > > +
> > >  patternProperties:
> > >    '^i2c@[0-9a-f]+$':
> > >      $ref: /schemas/i2c/i2c-controller.yaml
> > >
> > 
> > Since this is the first optional property, you now need to specify what
> > properties are required, which is everything but settle-time-us. If you
> > don't, all properties are required. Which is not what we want...
> > 
> > Something like this should do it, I think:
> > 
> > required:
> >   - compatible
> >   - '#address-cells'
> >   - '#size-cells'
> 
> Thanks for a detail explanation but I am still struggling with these
> bindings. Were you thinking to have something like this?
> 
> ---
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux.yaml b/Documentation/devicetree/bindings/i2c/i2c-mux.yaml
> index 24cac36037f5..c9fde1bb0fea 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-mux.yaml
> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux.yaml
> @@ -19,6 +19,9 @@ description: |+
>    populating the i2c child busses.  If an 'i2c-mux' subnode is present, only
>    subnodes of this will be considered as i2c child busses.
> 
> +  Optional properties may not be supported by all drivers. However, if a driver
> +  wants to support one of the below features, it should adopt these bindings.
> +
>  properties:
>    $nodename:
>      pattern: '^(i2c-?)?mux'
> @@ -29,6 +32,11 @@ properties:
>    '#size-cells':
>      const: 0
> 
> +  settle-time-us:
> +    default: 0
> +    description:
> +      The time required for the signals to settle.
> +
>  patternProperties:
>    '^i2c@[0-9a-f]+$':
>      $ref: /schemas/i2c/i2c-controller.yaml
> @@ -41,6 +49,11 @@ patternProperties:
> 
>  additionalProperties: true
> 
> +required:
> +  - compatible

compatible should not be required here.

> +  - '#address-cells'
> +  - '#size-cells'
> +
>  examples:
>    - |
>      /*
> ---
> 
> If I have this then my problem is with the required properties because then I
> start to get new warnings once I run:
> 
> make ARCH=arm CROSS_COMPILE=arm-linux- dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/i2c/i2c-mux.yaml
> 
> For example, one of new the warnings is this:
> 
> /home/hvultur/linux/arch/arm/boot/dts/am335x-icev2.dt.yaml: mux-mii-hog: 'compatible' is a required property
> 	From schema: /home/hvultur/linux/Documentation/devicetree/bindings/i2c/i2c-mux.yaml
> /home/hvultur/linux/arch/arm/boot/dts/am335x-icev2.dt.yaml: mux-mii-hog: '#address-cells' is a required property
> 	From schema: /home/hvultur/linux/Documentation/devicetree/bindings/i2c/i2c-mux.yaml
> /home/hvultur/linux/arch/arm/boot/dts/am335x-icev2.dt.yaml: mux-mii-hog: '#size-cells' is a required property
> 	From schema: /home/hvultur/linux/Documentation/devicetree/bindings/i2c/i2c-mux.yaml

This is because of the $nodename pattern being pretty lax and matches 
on mux-mii-hog by mistake. We have 2 options. Change the nodename 
pattern to '^(i2c-?)?mux(@.*)?$' or add 'select: false'. The former 
would still match on 'mux' or 'mux@.*' which might still have problems. 
For the latter, we just need to make sure all the i2c-mux schemas have a 
$ref to this schema. Also, with that change we'd stop checking 'i2c-mux' 
nodes that don't yet have a specific schema. That said, I do lean toward 
the latter option.

Rob

Re: [PATCH v3 1/2] dt-bindings: i2c-mux: Add property for settle time

[Horatiu Vultur]

The 11/02/2021 13:37, Rob Herring wrote:

Hi Rob,

> 
> On Mon, Nov 01, 2021 at 10:32:01PM +0100, Horatiu Vultur wrote:
> > The 11/01/2021 15:32, Peter Rosin wrote:
> >
> > Hi Peter,
> >
> > >
> > > On 2021-11-01 13:25, Horatiu Vultur wrote:
> > > > Some HW requires some time for the signals to settle after the muxing is
> > > > changed. Allow this time to be specified in device tree.
> > > >
> > > > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> > > > ---
> > > >  Documentation/devicetree/bindings/i2c/i2c-mux.yaml | 6 ++++++
> > > >  1 file changed, 6 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux.yaml b/Documentation/devicetree/bindings/i2c/i2c-mux.yaml
> > > > index 24cac36037f5..4628ff6340c1 100644
> > > > --- a/Documentation/devicetree/bindings/i2c/i2c-mux.yaml
> > > > +++ b/Documentation/devicetree/bindings/i2c/i2c-mux.yaml
> > > > @@ -29,6 +29,12 @@ properties:
> > > >    '#size-cells':
> > > >      const: 0
> > > >
> > > > +  settle-time-us:
> > > > +    default: 0
> > > > +    description:
> > > > +      The time required for the signals to settle. Currently only the
> > > > +      i2c-mux-gpmux driver supports this optional binding.
> > >
> > > The information about how i2c-mux-gpmux is special is bound to go stale,
> > > and I don't think we should mention such specific details in the binding.
> > > What I meant was a generic warnings about optional bindings perhaps not
> > > being supported by all drivers, along the lines of this from i2c.txt:
> > >
> > > "These properties may not be supported by all drivers. However, if a driver
> > >  wants to support one of the below features, it should adapt these bindings."
> > >
> > > However, I now notice that this sentence makes no sense. It looks like it
> > > should be s/adapt/adopt/.
> > >
> > > And, in the i2c-mux.yaml case it can simply say "Optional properties"
> > > instead of "These properites" (which refers to a subset of properties
> > > immediately below the text) since with a yaml binding it is always
> > > clear which properties are optional and which are required. Lastly, I
> > > guess this warning belongs in the description.
> > >
> > > > +
> > > >  patternProperties:
> > > >    '^i2c@[0-9a-f]+$':
> > > >      $ref: /schemas/i2c/i2c-controller.yaml
> > > >
> > >
> > > Since this is the first optional property, you now need to specify what
> > > properties are required, which is everything but settle-time-us. If you
> > > don't, all properties are required. Which is not what we want...
> > >
> > > Something like this should do it, I think:
> > >
> > > required:
> > >   - compatible
> > >   - '#address-cells'
> > >   - '#size-cells'
> >
> > Thanks for a detail explanation but I am still struggling with these
> > bindings. Were you thinking to have something like this?
> >
> > ---
> > diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux.yaml b/Documentation/devicetree/bindings/i2c/i2c-mux.yaml
> > index 24cac36037f5..c9fde1bb0fea 100644
> > --- a/Documentation/devicetree/bindings/i2c/i2c-mux.yaml
> > +++ b/Documentation/devicetree/bindings/i2c/i2c-mux.yaml
> > @@ -19,6 +19,9 @@ description: |+
> >    populating the i2c child busses.  If an 'i2c-mux' subnode is present, only
> >    subnodes of this will be considered as i2c child busses.
> >
> > +  Optional properties may not be supported by all drivers. However, if a driver
> > +  wants to support one of the below features, it should adopt these bindings.
> > +
> >  properties:
> >    $nodename:
> >      pattern: '^(i2c-?)?mux'
> > @@ -29,6 +32,11 @@ properties:
> >    '#size-cells':
> >      const: 0
> >
> > +  settle-time-us:
> > +    default: 0
> > +    description:
> > +      The time required for the signals to settle.
> > +
> >  patternProperties:
> >    '^i2c@[0-9a-f]+$':
> >      $ref: /schemas/i2c/i2c-controller.yaml
> > @@ -41,6 +49,11 @@ patternProperties:
> >
> >  additionalProperties: true
> >
> > +required:
> > +  - compatible
> 
> compatible should not be required here.
> 
> > +  - '#address-cells'
> > +  - '#size-cells'
> > +
> >  examples:
> >    - |
> >      /*
> > ---
> >
> > If I have this then my problem is with the required properties because then I
> > start to get new warnings once I run:
> >
> > make ARCH=arm CROSS_COMPILE=arm-linux- dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/i2c/i2c-mux.yaml
> >
> > For example, one of new the warnings is this:
> >
> > /home/hvultur/linux/arch/arm/boot/dts/am335x-icev2.dt.yaml: mux-mii-hog: 'compatible' is a required property
> >       From schema: /home/hvultur/linux/Documentation/devicetree/bindings/i2c/i2c-mux.yaml
> > /home/hvultur/linux/arch/arm/boot/dts/am335x-icev2.dt.yaml: mux-mii-hog: '#address-cells' is a required property
> >       From schema: /home/hvultur/linux/Documentation/devicetree/bindings/i2c/i2c-mux.yaml
> > /home/hvultur/linux/arch/arm/boot/dts/am335x-icev2.dt.yaml: mux-mii-hog: '#size-cells' is a required property
> >       From schema: /home/hvultur/linux/Documentation/devicetree/bindings/i2c/i2c-mux.yaml
> 
> This is because of the $nodename pattern being pretty lax and matches
> on mux-mii-hog by mistake. We have 2 options. Change the nodename
> pattern to '^(i2c-?)?mux(@.*)?$' or add 'select: false'. The former
> would still match on 'mux' or 'mux@.*' which might still have problems.
> For the latter, we just need to make sure all the i2c-mux schemas have a
> $ref to this schema. Also, with that change we'd stop checking 'i2c-mux'
> nodes that don't yet have a specific schema. That said, I do lean toward
> the latter option.

From what I can see there are only two i2c-mux schemas and both of them
have a $ref to this schema [1][2]

[1] https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/i2c/i2c-mux-gpmux.yaml#L33
[2] https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml#L16

> 
> Rob

-- 
/Horatiu

Re: [PATCH v3 1/2] dt-bindings: i2c-mux: Add property for settle time

[Peter Rosin]

On 2021-11-02 23:27, Horatiu Vultur wrote:
> The 11/02/2021 13:37, Rob Herring wrote:
>> On Mon, Nov 01, 2021 at 10:32:01PM +0100, Horatiu Vultur wrote:
>>> The 11/01/2021 15:32, Peter Rosin wrote:

*snip*

>>>
>>> +required:
>>> +  - compatible
>>
>> compatible should not be required here.
>>
>>> +  - '#address-cells'
>>> +  - '#size-cells'
>>> +
>>>  examples:
>>>    - |
>>>      /*
>>> ---
>>>
>>> If I have this then my problem is with the required properties because then I
>>> start to get new warnings once I run:
>>>
>>> make ARCH=arm CROSS_COMPILE=arm-linux- dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/i2c/i2c-mux.yaml
>>>
>>> For example, one of new the warnings is this:
>>>
>>> /home/hvultur/linux/arch/arm/boot/dts/am335x-icev2.dt.yaml: mux-mii-hog: 'compatible' is a required property
>>>       From schema: /home/hvultur/linux/Documentation/devicetree/bindings/i2c/i2c-mux.yaml
>>> /home/hvultur/linux/arch/arm/boot/dts/am335x-icev2.dt.yaml: mux-mii-hog: '#address-cells' is a required property
>>>       From schema: /home/hvultur/linux/Documentation/devicetree/bindings/i2c/i2c-mux.yaml
>>> /home/hvultur/linux/arch/arm/boot/dts/am335x-icev2.dt.yaml: mux-mii-hog: '#size-cells' is a required property
>>>       From schema: /home/hvultur/linux/Documentation/devicetree/bindings/i2c/i2c-mux.yaml
>>
>> This is because of the $nodename pattern being pretty lax and matches
>> on mux-mii-hog by mistake. We have 2 options. Change the nodename
>> pattern to '^(i2c-?)?mux(@.*)?$' or add 'select: false'. The former
>> would still match on 'mux' or 'mux@.*' which might still have problems.
>> For the latter, we just need to make sure all the i2c-mux schemas have a
>> $ref to this schema. Also, with that change we'd stop checking 'i2c-mux'
>> nodes that don't yet have a specific schema. That said, I do lean toward
>> the latter option.
> 
> From what I can see there are only two i2c-mux schemas and both of them
> have a $ref to this schema [1][2]
> 
> [1] https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/i2c/i2c-mux-gpmux.yaml#L33
> [2] https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml#L16

I'm a relative yaml bindings newbie, but I assume adding "select: false" will
have the side effect of not enforcing this i2c-mux schema on i2c-muxes that
have not yet been converted to yaml? E.g. i2c-mux-gpio.txt, i2c-mux-pinctrl.txt
etc etc. But there are not too many of those. Is it a prerequisite to update
those bindings to yaml before doing "select: false"? Looking further I think
there's a total of about 15-20 drivers doing i2c-muxing (or arbing/gating),
and some of those exist outside the "i2c umbrella".

I wonder if e.g. this one [1] should really reference i2c-controller.yaml as
it is currently doing, or if i2c-mux.yaml is correct?

[1] Documentation/devicetree/bindings/power/supply/sbs,sbs-manager.yaml

Maybe i2c-mux.yaml didn't work in that case because the node names were
"wrong" and did not match the pattern and then someone stuck
i2c-controller.yaml in there simply because that was close enough, and
also happened to work?

Cheers,
Peter

Re: [PATCH v3 1/2] dt-bindings: i2c-mux: Add property for settle time

[Rob Herring]

On Wed, Nov 3, 2021 at 6:45 AM Peter Rosin <peda@axentia.se> wrote:
>
> On 2021-11-02 23:27, Horatiu Vultur wrote:
> > The 11/02/2021 13:37, Rob Herring wrote:
> >> On Mon, Nov 01, 2021 at 10:32:01PM +0100, Horatiu Vultur wrote:
> >>> The 11/01/2021 15:32, Peter Rosin wrote:
>
> *snip*
>
> >>>
> >>> +required:
> >>> +  - compatible
> >>
> >> compatible should not be required here.
> >>
> >>> +  - '#address-cells'
> >>> +  - '#size-cells'
> >>> +
> >>>  examples:
> >>>    - |
> >>>      /*
> >>> ---
> >>>
> >>> If I have this then my problem is with the required properties because then I
> >>> start to get new warnings once I run:
> >>>
> >>> make ARCH=arm CROSS_COMPILE=arm-linux- dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/i2c/i2c-mux.yaml
> >>>
> >>> For example, one of new the warnings is this:
> >>>
> >>> /home/hvultur/linux/arch/arm/boot/dts/am335x-icev2.dt.yaml: mux-mii-hog: 'compatible' is a required property
> >>>       From schema: /home/hvultur/linux/Documentation/devicetree/bindings/i2c/i2c-mux.yaml
> >>> /home/hvultur/linux/arch/arm/boot/dts/am335x-icev2.dt.yaml: mux-mii-hog: '#address-cells' is a required property
> >>>       From schema: /home/hvultur/linux/Documentation/devicetree/bindings/i2c/i2c-mux.yaml
> >>> /home/hvultur/linux/arch/arm/boot/dts/am335x-icev2.dt.yaml: mux-mii-hog: '#size-cells' is a required property
> >>>       From schema: /home/hvultur/linux/Documentation/devicetree/bindings/i2c/i2c-mux.yaml

There's actually a ton of 'mux' nodes that should be causing warnings too.

> >> This is because of the $nodename pattern being pretty lax and matches
> >> on mux-mii-hog by mistake. We have 2 options. Change the nodename
> >> pattern to '^(i2c-?)?mux(@.*)?$' or add 'select: false'. The former
> >> would still match on 'mux' or 'mux@.*' which might still have problems.
> >> For the latter, we just need to make sure all the i2c-mux schemas have a
> >> $ref to this schema. Also, with that change we'd stop checking 'i2c-mux'
> >> nodes that don't yet have a specific schema. That said, I do lean toward
> >> the latter option.
> >
> > From what I can see there are only two i2c-mux schemas and both of them
> > have a $ref to this schema [1][2]
> >
> > [1] https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/i2c/i2c-mux-gpmux.yaml#L33
> > [2] https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml#L16
>
> I'm a relative yaml bindings newbie, but I assume adding "select: false" will
> have the side effect of not enforcing this i2c-mux schema on i2c-muxes that
> have not yet been converted to yaml? E.g. i2c-mux-gpio.txt, i2c-mux-pinctrl.txt
> etc etc. But there are not too many of those. Is it a prerequisite to update
> those bindings to yaml before doing "select: false"?

No. We may be losing some checks temporarily, but we've got plenty of
other warnings to keep busy. And most cases in tree seem to be pca954x
anyways.

> Looking further I think
> there's a total of about 15-20 drivers doing i2c-muxing (or arbing/gating),
> and some of those exist outside the "i2c umbrella".
>
> I wonder if e.g. this one [1] should really reference i2c-controller.yaml as
> it is currently doing, or if i2c-mux.yaml is correct?
>
> [1] Documentation/devicetree/bindings/power/supply/sbs,sbs-manager.yaml
>
> Maybe i2c-mux.yaml didn't work in that case because the node names were
> "wrong" and did not match the pattern and then someone stuck
> i2c-controller.yaml in there simply because that was close enough, and
> also happened to work?

While the device does have muxing capability, it does much more and
the use is rather specific. So I think it is fine.

Rob

[PATCH v3 2/2] i2c-mux-gpmux: Support settle-time-us property

[Horatiu Vultur]

Add support for settle-time-us property. If this is defined in device
tree then add this delay to mux APIs.

Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>
Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 drivers/i2c/muxes/i2c-mux-gpmux.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/muxes/i2c-mux-gpmux.c b/drivers/i2c/muxes/i2c-mux-gpmux.c
index d3acd8d66c32..f64a4b6034df 100644
--- a/drivers/i2c/muxes/i2c-mux-gpmux.c
+++ b/drivers/i2c/muxes/i2c-mux-gpmux.c
@@ -16,6 +16,7 @@
 
 struct mux {
 	struct mux_control *control;
+	u32 delay_us;
 
 	bool do_not_deselect;
 };
@@ -25,7 +26,7 @@ static int i2c_mux_select(struct i2c_mux_core *muxc, u32 chan)
 	struct mux *mux = i2c_mux_priv(muxc);
 	int ret;
 
-	ret = mux_control_select(mux->control, chan);
+	ret = mux_control_select_delay(mux->control, chan, mux->delay_us);
 	mux->do_not_deselect = ret < 0;
 
 	return ret;
@@ -106,6 +107,7 @@ static int i2c_mux_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, muxc);
 
+	of_property_read_u32(np, "settle-time-us", &mux->delay_us);
 	muxc->mux_locked = of_property_read_bool(np, "mux-locked");
 
 	for_each_child_of_node(np, child) {
-- 
2.33.0