[PATCH 1/2] spi: dt-bindings: spi-peripheral-props: Document spi-cpha and spi-cpol

[PATCH 1/2] spi: dt-bindings: spi-peripheral-props: Document spi-cpha and spi-cpol

[Fabio Estevam]

The 'spi-cpha' and 'spi-cpol' are commonly used SPI peripheral
properties that indicate the device clock phase and polarity.

Document these properties.

Signed-off-by: Fabio Estevam <festevam@gmail.com>
---
 .../devicetree/bindings/spi/spi-peripheral-props.yaml  | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
index 0bb443b8decd..b2e2717f3619 100644
--- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
+++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
@@ -29,6 +29,16 @@ properties:
     description:
       Chip select used by the device.
 
+  spi-cpha:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      The device data is sampled on trailing (last) edge of the SPI clock.
+
+  spi-cpol:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      The device clock has a falling lead (first) edge.
+
   spi-cs-high:
     $ref: /schemas/types.yaml#/definitions/flag
     description:
-- 
2.34.1

[PATCH 2/2] dt-bindings: trivial-devices: Add a reference to spi-peripheral-props.yaml

[Fabio Estevam]

There may be cases where a trivial-device needs to describe
the SPI clock polarity and phase via spi-cpol and spi-cpha
properties.

Add a reference to spi-peripheral-props.yaml to allow that.

Suggested-by: Rob Herring <robh@kernel.org>
Signed-off-by: Fabio Estevam <festevam@gmail.com>
---
 Documentation/devicetree/bindings/trivial-devices.yaml | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
index 75a5fad08c44..356b89ade6a8 100644
--- a/Documentation/devicetree/bindings/trivial-devices.yaml
+++ b/Documentation/devicetree/bindings/trivial-devices.yaml
@@ -424,6 +424,9 @@ required:
   - compatible
   - reg
 
-additionalProperties: false
+allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+unevaluatedProperties: false
 
 ...
-- 
2.34.1

Re: [PATCH 1/2] spi: dt-bindings: spi-peripheral-props: Document spi-cpha and spi-cpol

[Conor Dooley]

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

On Thu, Aug 29, 2024 at 05:13:14PM -0300, Fabio Estevam wrote:
> The 'spi-cpha' and 'spi-cpol' are commonly used SPI peripheral
> properties that indicate the device clock phase and polarity.
> 
> Document these properties.
> 
> Signed-off-by: Fabio Estevam <festevam@gmail.com>
> ---
>  .../devicetree/bindings/spi/spi-peripheral-props.yaml  | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> index 0bb443b8decd..b2e2717f3619 100644
> --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> @@ -29,6 +29,16 @@ properties:
>      description:
>        Chip select used by the device.
>  
> +  spi-cpha:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      The device data is sampled on trailing (last) edge of the SPI clock.
> +
> +  spi-cpol:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      The device clock has a falling lead (first) edge.

These two should just be usable with "spi-cpha: true", they're applied
by the controller schema to it's child nodes, so you don't need to
redefine their type here.

> +
>    spi-cs-high:
>      $ref: /schemas/types.yaml#/definitions/flag
>      description:
> -- 
> 2.34.1
> 

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

Re: [PATCH 2/2] dt-bindings: trivial-devices: Add a reference to spi-peripheral-props.yaml

[Conor Dooley]

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

On Thu, Aug 29, 2024 at 05:13:15PM -0300, Fabio Estevam wrote:
> There may be cases where a trivial-device needs to describe
> the SPI clock polarity and phase via spi-cpol and spi-cpha
> properties.

Since those don't come from spi-peripheral-props, not really the correct
justification (although why they don't, I'm not sure). If you still saw
dtbs_check complaints after the first patch, I maybe the controller
schema is missing a reference to spi-controller.yaml?
That said, I think the reference is useful for other the properties,
like spi-max-frequency, so the diff here is sound.

Thanks,
Conor.
> 
> Add a reference to spi-peripheral-props.yaml to allow that.
> 
> Suggested-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Fabio Estevam <festevam@gmail.com>
> ---
>  Documentation/devicetree/bindings/trivial-devices.yaml | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
> index 75a5fad08c44..356b89ade6a8 100644
> --- a/Documentation/devicetree/bindings/trivial-devices.yaml
> +++ b/Documentation/devicetree/bindings/trivial-devices.yaml
> @@ -424,6 +424,9 @@ required:
>    - compatible
>    - reg
>  
> -additionalProperties: false
> +allOf:
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +unevaluatedProperties: false
>  
>  ...
> -- 
> 2.34.1
> 

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

Re: [PATCH 2/2] dt-bindings: trivial-devices: Add a reference to spi-peripheral-props.yaml

[Fabio Estevam]

Hi Conor,

On Fri, Aug 30, 2024 at 11:14 AM Conor Dooley <conor@kernel.org> wrote:

> Since those don't come from spi-peripheral-props, not really the correct
> justification (although why they don't, I'm not sure). If you still saw
> dtbs_check complaints after the first patch, I maybe the controller
> schema is missing a reference to spi-controller.yaml?

I changed the first patch as suggested:

--- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
+++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
@@ -29,6 +29,10 @@ properties:
     description:
       Chip select used by the device.

+  spi-cpha: true
+
+  spi-cpol: true
+
   spi-cs-high:
     $ref: /schemas/types.yaml#/definitions/flag
     description:

spi-rockchip.yaml does reference spi-controller.yaml, but I still get
dtbs_check complaints after the first patch.

$ make CHECK_DTBS=y rockchip/rv1108-elgin-r1.dtb -j12
  UPD     include/config/kernel.release
  SCHEMA  Documentation/devicetree/bindings/processed-schema.json
  DTC [C] arch/arm/boot/dts/rockchip/rv1108-elgin-r1.dtb
/home/fabio/linux-next/arch/arm/boot/dts/rockchip/rv1108-elgin-r1.dtb:
display@0: 'spi-cpha', 'spi-cpol' do not match any of the regexes:
'pinctrl-[0-9]+'
from schema $id: http://devicetree.org/schemas/trivial-devices.yaml#

I would appreciate some suggestions on how to fix this warning.

Thanks

Re: [PATCH 2/2] dt-bindings: trivial-devices: Add a reference to spi-peripheral-props.yaml

[Conor Dooley]

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

On Fri, Aug 30, 2024 at 12:05:20PM -0300, Fabio Estevam wrote:
> Hi Conor,
> 
> On Fri, Aug 30, 2024 at 11:14 AM Conor Dooley <conor@kernel.org> wrote:
> 
> > Since those don't come from spi-peripheral-props, not really the correct
> > justification (although why they don't, I'm not sure). If you still saw
> > dtbs_check complaints after the first patch, I maybe the controller
> > schema is missing a reference to spi-controller.yaml?
> 
> I changed the first patch as suggested:
> 
> --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> @@ -29,6 +29,10 @@ properties:
>      description:
>        Chip select used by the device.
> 
> +  spi-cpha: true
> +
> +  spi-cpol: true
> +
>    spi-cs-high:
>      $ref: /schemas/types.yaml#/definitions/flag
>      description:
> 
> spi-rockchip.yaml does reference spi-controller.yaml, but I still get
> dtbs_check complaints after the first patch.
> 
> $ make CHECK_DTBS=y rockchip/rv1108-elgin-r1.dtb -j12
>   UPD     include/config/kernel.release
>   SCHEMA  Documentation/devicetree/bindings/processed-schema.json
>   DTC [C] arch/arm/boot/dts/rockchip/rv1108-elgin-r1.dtb
> /home/fabio/linux-next/arch/arm/boot/dts/rockchip/rv1108-elgin-r1.dtb:
> display@0: 'spi-cpha', 'spi-cpol' do not match any of the regexes:
> 'pinctrl-[0-9]+'
> from schema $id: http://devicetree.org/schemas/trivial-devices.yaml#
> 
> I would appreciate some suggestions on how to fix this warning.

Ah, I think I suggested something garbage, because I misread the diff,
as my quoted mail evidences. I was really trying to suggest putting
spi-cpha: true
spi-cpol: true
in trivial-devices.yaml, but I didn't notice that the patch was to
spi-peripheral-props rather than trivial-devices. These properties are
defined (for reasons I don't quite understand) in spi-controller.yaml
and applied to children of the controller node by that binding and I
wanted to avoid the redefinition.

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

Re: [PATCH 2/2] dt-bindings: trivial-devices: Add a reference to spi-peripheral-props.yaml

[Rob Herring]

On Fri, Aug 30, 2024 at 04:17:02PM +0100, Conor Dooley wrote:
> On Fri, Aug 30, 2024 at 12:05:20PM -0300, Fabio Estevam wrote:
> > Hi Conor,
> > 
> > On Fri, Aug 30, 2024 at 11:14 AM Conor Dooley <conor@kernel.org> wrote:
> > 
> > > Since those don't come from spi-peripheral-props, not really the correct
> > > justification (although why they don't, I'm not sure). If you still saw
> > > dtbs_check complaints after the first patch, I maybe the controller
> > > schema is missing a reference to spi-controller.yaml?
> > 
> > I changed the first patch as suggested:
> > 
> > --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> > +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> > @@ -29,6 +29,10 @@ properties:
> >      description:
> >        Chip select used by the device.
> > 
> > +  spi-cpha: true
> > +
> > +  spi-cpol: true
> > +
> >    spi-cs-high:
> >      $ref: /schemas/types.yaml#/definitions/flag
> >      description:
> > 
> > spi-rockchip.yaml does reference spi-controller.yaml, but I still get
> > dtbs_check complaints after the first patch.
> > 
> > $ make CHECK_DTBS=y rockchip/rv1108-elgin-r1.dtb -j12
> >   UPD     include/config/kernel.release
> >   SCHEMA  Documentation/devicetree/bindings/processed-schema.json
> >   DTC [C] arch/arm/boot/dts/rockchip/rv1108-elgin-r1.dtb
> > /home/fabio/linux-next/arch/arm/boot/dts/rockchip/rv1108-elgin-r1.dtb:
> > display@0: 'spi-cpha', 'spi-cpol' do not match any of the regexes:
> > 'pinctrl-[0-9]+'
> > from schema $id: http://devicetree.org/schemas/trivial-devices.yaml#
> > 
> > I would appreciate some suggestions on how to fix this warning.
> 
> Ah, I think I suggested something garbage, because I misread the diff,
> as my quoted mail evidences. I was really trying to suggest putting
> spi-cpha: true
> spi-cpol: true
> in trivial-devices.yaml, but I didn't notice that the patch was to
> spi-peripheral-props rather than trivial-devices. These properties are
> defined (for reasons I don't quite understand) in spi-controller.yaml
> and applied to children of the controller node by that binding and I
> wanted to avoid the redefinition.

I steered Fabio wrong...

I think we originally had these in spi-peripheral-props, but then 
decided they are properties of the device, not the controller. These 
properties should really only be needed if the device supports different 
modes. If what a device supports is fixed, then that can be implicit.

There's one other case I see with "dh,dhcom-board". So I guess add 
spi-cpha and spi-cpol directly to trivial-devices.yaml.

Rob

Re: [PATCH 2/2] dt-bindings: trivial-devices: Add a reference to spi-peripheral-props.yaml

[Conor Dooley]

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

On Fri, Aug 30, 2024 at 01:05:09PM -0500, Rob Herring wrote:
> On Fri, Aug 30, 2024 at 04:17:02PM +0100, Conor Dooley wrote:
> > On Fri, Aug 30, 2024 at 12:05:20PM -0300, Fabio Estevam wrote:
> > > Hi Conor,
> > > 
> > > On Fri, Aug 30, 2024 at 11:14 AM Conor Dooley <conor@kernel.org> wrote:
> > > 
> > > > Since those don't come from spi-peripheral-props, not really the correct
> > > > justification (although why they don't, I'm not sure). If you still saw
> > > > dtbs_check complaints after the first patch, I maybe the controller
> > > > schema is missing a reference to spi-controller.yaml?
> > > 
> > > I changed the first patch as suggested:
> > > 
> > > --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> > > +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> > > @@ -29,6 +29,10 @@ properties:
> > >      description:
> > >        Chip select used by the device.
> > > 
> > > +  spi-cpha: true
> > > +
> > > +  spi-cpol: true
> > > +
> > >    spi-cs-high:
> > >      $ref: /schemas/types.yaml#/definitions/flag
> > >      description:
> > > 
> > > spi-rockchip.yaml does reference spi-controller.yaml, but I still get
> > > dtbs_check complaints after the first patch.
> > > 
> > > $ make CHECK_DTBS=y rockchip/rv1108-elgin-r1.dtb -j12
> > >   UPD     include/config/kernel.release
> > >   SCHEMA  Documentation/devicetree/bindings/processed-schema.json
> > >   DTC [C] arch/arm/boot/dts/rockchip/rv1108-elgin-r1.dtb
> > > /home/fabio/linux-next/arch/arm/boot/dts/rockchip/rv1108-elgin-r1.dtb:
> > > display@0: 'spi-cpha', 'spi-cpol' do not match any of the regexes:
> > > 'pinctrl-[0-9]+'
> > > from schema $id: http://devicetree.org/schemas/trivial-devices.yaml#
> > > 
> > > I would appreciate some suggestions on how to fix this warning.
> > 
> > Ah, I think I suggested something garbage, because I misread the diff,
> > as my quoted mail evidences. I was really trying to suggest putting
> > spi-cpha: true
> > spi-cpol: true
> > in trivial-devices.yaml, but I didn't notice that the patch was to
> > spi-peripheral-props rather than trivial-devices. These properties are
> > defined (for reasons I don't quite understand) in spi-controller.yaml
> > and applied to children of the controller node by that binding and I
> > wanted to avoid the redefinition.
> 
> I steered Fabio wrong...
> 
> I think we originally had these in spi-peripheral-props, but then 
> decided they are properties of the device, not the controller.

I don't follow, how would them being properties of the "device", make them
unsuitable for spi-peripheral-props? Is the differentiation supposed to
be that the things in spi-peripheral-props are actually there to do
per-"device" tweaks for special controller features and the things
applied by spi-controller to child nodes of SPI buses are the ones that
describe requirements of the device?

Even if that is a rather WTF responsibility distribution between files
(partly that's down to naming), the usage does make sense.
spi-peripheral-props can be unconditionally included by all SPI devices,
since the controller determines what properties are relevant, and spa-cpha
etc only get permitted when explicitly set as "true".

> These 
> properties should really only be needed if the device supports different 
> modes. If what a device supports is fixed, then that can be implicit.

Right. That's very inconsistently done though, even if it makes sense.
I'd wager there are very few devices that actually support both
configurations but conversely very few drivers for active-high-only
devices that don't rely on the spi-cs-active-high (or w/e it is)
property to function correctly.

I should send a patch for pcf2123 to make it required, because that is
the one I know for sure requires active high off the top of my head.

> 
> There's one other case I see with "dh,dhcom-board". So I guess add 
> spi-cpha and spi-cpol directly to trivial-devices.yaml.
> 
> Rob

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

Re: [PATCH 1/2] spi: dt-bindings: spi-peripheral-props: Document spi-cpha and spi-cpol

[Krzysztof Kozlowski]

On 29/08/2024 22:13, Fabio Estevam wrote:
> The 'spi-cpha' and 'spi-cpol' are commonly used SPI peripheral
> properties that indicate the device clock phase and polarity.
> 
> Document these properties.
> 
> Signed-off-by: Fabio Estevam <festevam@gmail.com>
> ---
>  .../devicetree/bindings/spi/spi-peripheral-props.yaml  | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> index 0bb443b8decd..b2e2717f3619 100644
> --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> @@ -29,6 +29,16 @@ properties:
>      description:
>        Chip select used by the device.
>  
> +  spi-cpha:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      The device data is sampled on trailing (last) edge of the SPI clock.
> +
> +  spi-cpol:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      The device clock has a falling lead (first) edge.

That would be a revert of earlier commit without explanation why such
revert is needed. :(

Best regards,
Krzysztof

Re: [PATCH 2/2] dt-bindings: trivial-devices: Add a reference to spi-peripheral-props.yaml

[Krzysztof Kozlowski]

On 30/08/2024 20:05, Rob Herring wrote:
> On Fri, Aug 30, 2024 at 04:17:02PM +0100, Conor Dooley wrote:
>> On Fri, Aug 30, 2024 at 12:05:20PM -0300, Fabio Estevam wrote:
>>> Hi Conor,
>>>
>>> On Fri, Aug 30, 2024 at 11:14 AM Conor Dooley <conor@kernel.org> wrote:
>>>
>>>> Since those don't come from spi-peripheral-props, not really the correct
>>>> justification (although why they don't, I'm not sure). If you still saw
>>>> dtbs_check complaints after the first patch, I maybe the controller
>>>> schema is missing a reference to spi-controller.yaml?
>>>
>>> I changed the first patch as suggested:
>>>
>>> --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
>>> +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
>>> @@ -29,6 +29,10 @@ properties:
>>>      description:
>>>        Chip select used by the device.
>>>
>>> +  spi-cpha: true
>>> +
>>> +  spi-cpol: true
>>> +
>>>    spi-cs-high:
>>>      $ref: /schemas/types.yaml#/definitions/flag
>>>      description:
>>>
>>> spi-rockchip.yaml does reference spi-controller.yaml, but I still get
>>> dtbs_check complaints after the first patch.
>>>
>>> $ make CHECK_DTBS=y rockchip/rv1108-elgin-r1.dtb -j12
>>>   UPD     include/config/kernel.release
>>>   SCHEMA  Documentation/devicetree/bindings/processed-schema.json
>>>   DTC [C] arch/arm/boot/dts/rockchip/rv1108-elgin-r1.dtb
>>> /home/fabio/linux-next/arch/arm/boot/dts/rockchip/rv1108-elgin-r1.dtb:
>>> display@0: 'spi-cpha', 'spi-cpol' do not match any of the regexes:
>>> 'pinctrl-[0-9]+'
>>> from schema $id: http://devicetree.org/schemas/trivial-devices.yaml#
>>>
>>> I would appreciate some suggestions on how to fix this warning.
>>
>> Ah, I think I suggested something garbage, because I misread the diff,
>> as my quoted mail evidences. I was really trying to suggest putting
>> spi-cpha: true
>> spi-cpol: true
>> in trivial-devices.yaml, but I didn't notice that the patch was to
>> spi-peripheral-props rather than trivial-devices. These properties are
>> defined (for reasons I don't quite understand) in spi-controller.yaml
>> and applied to children of the controller node by that binding and I
>> wanted to avoid the redefinition.
> 
> I steered Fabio wrong...
> 
> I think we originally had these in spi-peripheral-props, but then 
> decided they are properties of the device, not the controller. These 
> properties should really only be needed if the device supports different 
> modes. If what a device supports is fixed, then that can be implicit.
> 
> There's one other case I see with "dh,dhcom-board". So I guess add 
> spi-cpha and spi-cpol directly to trivial-devices.yaml.

I responded to v2 before reaching here. Are you sure that's intended?
It's almost equal to the patch here: all trivial devices will have it,
even if they do not need it. For a device requiring CPOL/CPHA, not
having it (or the reverse) is a bug in DTS and basically we would miss
such check for all trivial devices.

Best regards,
Krzysztof