[PATCH v3 1/2] dt-bindings: dma: fsl-edma: remove 'clocks' from required

[PATCH v3 1/2] dt-bindings: dma: fsl-edma: remove 'clocks' from required

[Frank Li]

fsl,imx8qm-adma and fsl,imx8qm-edma don't require 'clocks'. Remove it from
required and add 'if' block for other compatible string to keep the same
restrictions.

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---

Notes:
    Change from v2 to v3
      - rebase to dmaengine/next
    Change from v1 to v2
      - add Krzysztof's ACK.

 .../devicetree/bindings/dma/fsl,edma.yaml       | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/dma/fsl,edma.yaml b/Documentation/devicetree/bindings/dma/fsl,edma.yaml
index 825f4715499e5..657a7d3ebf857 100644
--- a/Documentation/devicetree/bindings/dma/fsl,edma.yaml
+++ b/Documentation/devicetree/bindings/dma/fsl,edma.yaml
@@ -82,7 +82,6 @@ required:
   - compatible
   - reg
   - interrupts
-  - clocks
   - dma-channels
 
 allOf:
@@ -187,6 +186,22 @@ allOf:
         "#dma-cells":
           const: 3
 
+  - if:
+      properties:
+        compatible:
+	  contains:
+            enum:
+              - fsl,vf610-edma
+              - fsl,imx7ulp-edma
+              - fsl,imx93-edma3
+              - fsl,imx93-edma4
+              - fsl,imx95-edma5
+              - fsl,imx8ulp-edma
+              - fsl,ls1028a-edma
+    then:
+      required:
+        - clocks
+
 unevaluatedProperties: false
 
 examples:
-- 
2.34.1

[PATCH v3 2/2] dt-bindings: dma: fsl-edma: allow 'power-domains' property

[Frank Li]

Allow 'power-domains' property because i.MX8DXL i.MX8QM and i.MX8QXP need
it. EDMA supports each power-domain for each dma channel. So minItems and
maxItems align 'dma-channels'.

Change fsl,imx93-edma3 example to fsl,imx8qm-edma to affect this variants.

Fixed below DTB_CHECK warning:
  dma-controller@599f0000: Unevaluated properties are not allowed ('power-domains' was unexpected)

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---

Notes:
    Change from v2 to v3
    - set 'power-domains' false for other compatitble string
    - change imx93 example to 8qm example to affect this change according to
    Krzysztof Kozlowski's suggestion, choose least channel number edma
    instance to reduce code copy. max channel number is 64.
    
    - Rebase to latest dmaengine/next
    
    Change from v1 to v2
    - using maxitem: 64. Each channel have one power domain. Max 64 dmachannel.
    - add power-domains to 'required' when compatible string is fsl,imx8qm-adma
        or fsl,imx8qm-edma

 .../devicetree/bindings/dma/fsl,edma.yaml     | 79 ++++++++++---------
 1 file changed, 40 insertions(+), 39 deletions(-)

diff --git a/Documentation/devicetree/bindings/dma/fsl,edma.yaml b/Documentation/devicetree/bindings/dma/fsl,edma.yaml
index 657a7d3ebf857..012522612dc96 100644
--- a/Documentation/devicetree/bindings/dma/fsl,edma.yaml
+++ b/Documentation/devicetree/bindings/dma/fsl,edma.yaml
@@ -71,6 +71,10 @@ properties:
     minItems: 1
     maxItems: 33
 
+  power-domains:
+    minItems: 1
+    maxItems: 64
+
   big-endian:
     description: |
       If present registers and hardware scatter/gather descriptors of the
@@ -189,7 +193,7 @@ allOf:
   - if:
       properties:
         compatible:
-	  contains:
+          contains:
             enum:
               - fsl,vf610-edma
               - fsl,imx7ulp-edma
@@ -202,6 +206,20 @@ allOf:
       required:
         - clocks
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - fsl,imx8qm-adma
+              - fsl,imx8qm-edma
+    then:
+      required:
+        - power-domains
+    else:
+      properties:
+        power-domains: false
+
 unevaluatedProperties: false
 
 examples:
@@ -257,44 +275,27 @@ examples:
 
   - |
     #include <dt-bindings/interrupt-controller/arm-gic.h>
-    #include <dt-bindings/clock/imx93-clock.h>
+    #include <dt-bindings/firmware/imx/rsrc.h>
 
-    dma-controller@44000000 {
-      compatible = "fsl,imx93-edma3";
-      reg = <0x44000000 0x200000>;
+    dma-controller@5a9f0000 {
+      compatible = "fsl,imx8qm-edma";
+      reg = <0x5a9f0000 0x90000>;
       #dma-cells = <3>;
-      dma-channels = <31>;
-      interrupts = <GIC_SPI 95 IRQ_TYPE_LEVEL_HIGH>,
-                   <GIC_SPI 96 IRQ_TYPE_LEVEL_HIGH>,
-                   <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH>,
-                   <GIC_SPI 98 IRQ_TYPE_LEVEL_HIGH>,
-                   <GIC_SPI 99 IRQ_TYPE_LEVEL_HIGH>,
-                   <GIC_SPI 100 IRQ_TYPE_LEVEL_HIGH>,
-                   <GIC_SPI 101 IRQ_TYPE_LEVEL_HIGH>,
-                   <GIC_SPI 102 IRQ_TYPE_LEVEL_HIGH>,
-                   <GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>,
-                   <GIC_SPI 104 IRQ_TYPE_LEVEL_HIGH>,
-                   <GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH>,
-                   <GIC_SPI 106 IRQ_TYPE_LEVEL_HIGH>,
-                   <GIC_SPI 107 IRQ_TYPE_LEVEL_HIGH>,
-                   <GIC_SPI 108 IRQ_TYPE_LEVEL_HIGH>,
-                   <GIC_SPI 109 IRQ_TYPE_LEVEL_HIGH>,
-                   <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>,
-                   <GIC_SPI 111 IRQ_TYPE_LEVEL_HIGH>,
-                   <GIC_SPI 112 IRQ_TYPE_LEVEL_HIGH>,
-                   <GIC_SPI 113 IRQ_TYPE_LEVEL_HIGH>,
-                   <GIC_SPI 114 IRQ_TYPE_LEVEL_HIGH>,
-                   <GIC_SPI 115 IRQ_TYPE_LEVEL_HIGH>,
-                   <GIC_SPI 116 IRQ_TYPE_LEVEL_HIGH>,
-                   <GIC_SPI 117 IRQ_TYPE_LEVEL_HIGH>,
-                   <GIC_SPI 118 IRQ_TYPE_LEVEL_HIGH>,
-                   <GIC_SPI 119 IRQ_TYPE_LEVEL_HIGH>,
-                   <GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>,
-                   <GIC_SPI 121 IRQ_TYPE_LEVEL_HIGH>,
-                   <GIC_SPI 122 IRQ_TYPE_LEVEL_HIGH>,
-                   <GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>,
-                   <GIC_SPI 124 IRQ_TYPE_LEVEL_HIGH>,
-                   <GIC_SPI 125 IRQ_TYPE_LEVEL_HIGH>;
-        clocks = <&clk IMX93_CLK_EDMA1_GATE>;
-        clock-names = "dma";
+      dma-channels = <8>;
+      interrupts = <GIC_SPI 424 IRQ_TYPE_LEVEL_HIGH>,
+                   <GIC_SPI 425 IRQ_TYPE_LEVEL_HIGH>,
+                   <GIC_SPI 426 IRQ_TYPE_LEVEL_HIGH>,
+                   <GIC_SPI 427 IRQ_TYPE_LEVEL_HIGH>,
+                   <GIC_SPI 428 IRQ_TYPE_LEVEL_HIGH>,
+                   <GIC_SPI 429 IRQ_TYPE_LEVEL_HIGH>,
+                   <GIC_SPI 430 IRQ_TYPE_LEVEL_HIGH>,
+                   <GIC_SPI 431 IRQ_TYPE_LEVEL_HIGH>;
+      power-domains = <&pd IMX_SC_R_DMA_3_CH0>,
+                      <&pd IMX_SC_R_DMA_3_CH1>,
+                      <&pd IMX_SC_R_DMA_3_CH2>,
+                      <&pd IMX_SC_R_DMA_3_CH3>,
+                      <&pd IMX_SC_R_DMA_3_CH4>,
+                      <&pd IMX_SC_R_DMA_3_CH5>,
+                      <&pd IMX_SC_R_DMA_3_CH6>,
+                      <&pd IMX_SC_R_DMA_3_CH7>;
     };
-- 
2.34.1

Re: [PATCH v3 1/2] dt-bindings: dma: fsl-edma: remove 'clocks' from required

[Krzysztof Kozlowski]

On 09/04/2024 20:54, Frank Li wrote:
> fsl,imx8qm-adma and fsl,imx8qm-edma don't require 'clocks'. Remove it from
> required and add 'if' block for other compatible string to keep the same
> restrictions.
> 
> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> 
> Notes:
>     Change from v2 to v3
>       - rebase to dmaengine/next

This fails...

> 
> diff --git a/Documentation/devicetree/bindings/dma/fsl,edma.yaml b/Documentation/devicetree/bindings/dma/fsl,edma.yaml
> index 825f4715499e5..657a7d3ebf857 100644
> --- a/Documentation/devicetree/bindings/dma/fsl,edma.yaml
> +++ b/Documentation/devicetree/bindings/dma/fsl,edma.yaml
> @@ -82,7 +82,6 @@ required:
>    - compatible
>    - reg
>    - interrupts
> -  - clocks
>    - dma-channels
>  
>  allOf:
> @@ -187,6 +186,22 @@ allOf:
>          "#dma-cells":
>            const: 3
>  
> +  - if:
> +      properties:
> +        compatible:
> +	  contains:

It does not look like you tested the bindings, at least after quick
look. Please run `make dt_binding_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).
Maybe you need to update your dtschema and yamllint.

Best regards,
Krzysztof

Re: [PATCH v3 1/2] dt-bindings: dma: fsl-edma: remove 'clocks' from required

[Frank Li]

On Tue, Apr 09, 2024 at 10:02:32PM +0200, Krzysztof Kozlowski wrote:
> On 09/04/2024 20:54, Frank Li wrote:
> > fsl,imx8qm-adma and fsl,imx8qm-edma don't require 'clocks'. Remove it from
> > required and add 'if' block for other compatible string to keep the same
> > restrictions.
> > 
> > Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> > 
> > Notes:
> >     Change from v2 to v3
> >       - rebase to dmaengine/next
> 
> This fails...

What's wrong? 

https://git.kernel.org/pub/scm/linux/kernel/git/vkoul/dmaengine.git/log/?h=next

> 
> > 
> > diff --git a/Documentation/devicetree/bindings/dma/fsl,edma.yaml b/Documentation/devicetree/bindings/dma/fsl,edma.yaml
> > index 825f4715499e5..657a7d3ebf857 100644
> > --- a/Documentation/devicetree/bindings/dma/fsl,edma.yaml
> > +++ b/Documentation/devicetree/bindings/dma/fsl,edma.yaml
> > @@ -82,7 +82,6 @@ required:
> >    - compatible
> >    - reg
> >    - interrupts
> > -  - clocks
> >    - dma-channels
> >  
> >  allOf:
> > @@ -187,6 +186,22 @@ allOf:
> >          "#dma-cells":
> >            const: 3
> >  
> > +  - if:
> > +      properties:
> > +        compatible:
> > +	  contains:
> 
> It does not look like you tested the bindings, at least after quick
> look. Please run `make dt_binding_check` (see
> Documentation/devicetree/bindings/writing-schema.rst for instructions).
> Maybe you need to update your dtschema and yamllint.

Strange, Test passed

make ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- -j8  dt_binding_check DT_SCHEMA_FILES=fsl,edma.yaml
  LINT    Documentation/devicetree/bindings
  DTEX    Documentation/devicetree/bindings/dma/fsl,edma.example.dts
  CHKDT   Documentation/devicetree/bindings/processed-schema.json
  SCHEMA  Documentation/devicetree/bindings/processed-schema.json
  DTC_CHK Documentation/devicetree/bindings/dma/fsl,edma.example.dtb


> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v3 1/2] dt-bindings: dma: fsl-edma: remove 'clocks' from required

[Krzysztof Kozlowski]

On 09/04/2024 23:09, Frank Li wrote:
> On Tue, Apr 09, 2024 at 10:02:32PM +0200, Krzysztof Kozlowski wrote:
>> On 09/04/2024 20:54, Frank Li wrote:
>>> fsl,imx8qm-adma and fsl,imx8qm-edma don't require 'clocks'. Remove it from
>>> required and add 'if' block for other compatible string to keep the same
>>> restrictions.
>>>
>>> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>> Signed-off-by: Frank Li <Frank.Li@nxp.com>
>>> ---
>>>
>>> Notes:
>>>     Change from v2 to v3
>>>       - rebase to dmaengine/next
>>
>> This fails...
> 
> What's wrong? 
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/vkoul/dmaengine.git/log/?h=next
> 
>>
>>>
>>> diff --git a/Documentation/devicetree/bindings/dma/fsl,edma.yaml b/Documentation/devicetree/bindings/dma/fsl,edma.yaml
>>> index 825f4715499e5..657a7d3ebf857 100644
>>> --- a/Documentation/devicetree/bindings/dma/fsl,edma.yaml
>>> +++ b/Documentation/devicetree/bindings/dma/fsl,edma.yaml
>>> @@ -82,7 +82,6 @@ required:
>>>    - compatible
>>>    - reg
>>>    - interrupts
>>> -  - clocks
>>>    - dma-channels
>>>  
>>>  allOf:
>>> @@ -187,6 +186,22 @@ allOf:
>>>          "#dma-cells":
>>>            const: 3
>>>  
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +	  contains:
>>
>> It does not look like you tested the bindings, at least after quick
>> look. Please run `make dt_binding_check` (see
>> Documentation/devicetree/bindings/writing-schema.rst for instructions).
>> Maybe you need to update your dtschema and yamllint.
> 
> Strange, Test passed
> 
> make ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- -j8  dt_binding_check DT_SCHEMA_FILES=fsl,edma.yaml
>   LINT    Documentation/devicetree/bindings
>   DTEX    Documentation/devicetree/bindings/dma/fsl,edma.example.dts
>   CHKDT   Documentation/devicetree/bindings/processed-schema.json
>   SCHEMA  Documentation/devicetree/bindings/processed-schema.json
>   DTC_CHK Documentation/devicetree/bindings/dma/fsl,edma.example.dtb

Nope, you tested other patch. Just look at your second patch for this.
When reviewer points to errors to your code, please investigate?

NAK, fix your patches.

Best regards,
Krzysztof

Re: [PATCH v3 1/2] dt-bindings: dma: fsl-edma: remove 'clocks' from required

[Krzysztof Kozlowski]

On 10/04/2024 08:30, Krzysztof Kozlowski wrote:
> On 09/04/2024 23:09, Frank Li wrote:
>> On Tue, Apr 09, 2024 at 10:02:32PM +0200, Krzysztof Kozlowski wrote:
>>> On 09/04/2024 20:54, Frank Li wrote:
>>>> fsl,imx8qm-adma and fsl,imx8qm-edma don't require 'clocks'. Remove it from
>>>> required and add 'if' block for other compatible string to keep the same
>>>> restrictions.
>>>>
>>>> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>>> Signed-off-by: Frank Li <Frank.Li@nxp.com>
>>>> ---
>>>>
>>>> Notes:
>>>>     Change from v2 to v3
>>>>       - rebase to dmaengine/next
>>>
>>> This fails...
>>
>> What's wrong? 
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/vkoul/dmaengine.git/log/?h=next
>>
>>>
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/dma/fsl,edma.yaml b/Documentation/devicetree/bindings/dma/fsl,edma.yaml
>>>> index 825f4715499e5..657a7d3ebf857 100644
>>>> --- a/Documentation/devicetree/bindings/dma/fsl,edma.yaml
>>>> +++ b/Documentation/devicetree/bindings/dma/fsl,edma.yaml
>>>> @@ -82,7 +82,6 @@ required:
>>>>    - compatible
>>>>    - reg
>>>>    - interrupts
>>>> -  - clocks
>>>>    - dma-channels
>>>>  
>>>>  allOf:
>>>> @@ -187,6 +186,22 @@ allOf:
>>>>          "#dma-cells":
>>>>            const: 3
>>>>  
>>>> +  - if:
>>>> +      properties:
>>>> +        compatible:
>>>> +	  contains:
>>>
>>> It does not look like you tested the bindings, at least after quick
>>> look. Please run `make dt_binding_check` (see
>>> Documentation/devicetree/bindings/writing-schema.rst for instructions).
>>> Maybe you need to update your dtschema and yamllint.
>>
>> Strange, Test passed
>>
>> make ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- -j8  dt_binding_check DT_SCHEMA_FILES=fsl,edma.yaml
>>   LINT    Documentation/devicetree/bindings
>>   DTEX    Documentation/devicetree/bindings/dma/fsl,edma.example.dts
>>   CHKDT   Documentation/devicetree/bindings/processed-schema.json
>>   SCHEMA  Documentation/devicetree/bindings/processed-schema.json
>>   DTC_CHK Documentation/devicetree/bindings/dma/fsl,edma.example.dtb
> 
> Nope, you tested other patch. Just look at your second patch for this.
> When reviewer points to errors to your code, please investigate?
> 
> NAK, fix your patches.

And to prove it, so you will stop wasting my time:
../Documentation/devicetree/bindings/dma/fsl,edma.yaml:192:1: found
character that cannot start any token

../Documentation/devicetree/bindings/dma/fsl,edma.yaml:192:1: [error]
syntax error: found character '\t' that cannot start any token (syntax)

../Documentation/devicetree/bindings/dma/fsl,edma.yaml:192:1: found
character that cannot start any token

Documentation/devicetree/bindings/dma/fsl,edma.yaml: ignoring, error
parsing file


Best regards,
Krzysztof

Re: [PATCH v3 1/2] dt-bindings: dma: fsl-edma: remove 'clocks' from required

[Krzysztof Kozlowski]

On 10/04/2024 08:32, Krzysztof Kozlowski wrote:
> On 10/04/2024 08:30, Krzysztof Kozlowski wrote:
>> On 09/04/2024 23:09, Frank Li wrote:
>>> On Tue, Apr 09, 2024 at 10:02:32PM +0200, Krzysztof Kozlowski wrote:
>>>> On 09/04/2024 20:54, Frank Li wrote:
>>>>> fsl,imx8qm-adma and fsl,imx8qm-edma don't require 'clocks'. Remove it from
>>>>> required and add 'if' block for other compatible string to keep the same
>>>>> restrictions.
>>>>>
>>>>> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>>>> Signed-off-by: Frank Li <Frank.Li@nxp.com>
>>>>> ---
>>>>>
>>>>> Notes:
>>>>>     Change from v2 to v3
>>>>>       - rebase to dmaengine/next
>>>>
>>>> This fails...
>>>
>>> What's wrong? 
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/vkoul/dmaengine.git/log/?h=next
>>>
>>>>
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/dma/fsl,edma.yaml b/Documentation/devicetree/bindings/dma/fsl,edma.yaml
>>>>> index 825f4715499e5..657a7d3ebf857 100644
>>>>> --- a/Documentation/devicetree/bindings/dma/fsl,edma.yaml
>>>>> +++ b/Documentation/devicetree/bindings/dma/fsl,edma.yaml
>>>>> @@ -82,7 +82,6 @@ required:
>>>>>    - compatible
>>>>>    - reg
>>>>>    - interrupts
>>>>> -  - clocks
>>>>>    - dma-channels
>>>>>  
>>>>>  allOf:
>>>>> @@ -187,6 +186,22 @@ allOf:
>>>>>          "#dma-cells":
>>>>>            const: 3
>>>>>  
>>>>> +  - if:
>>>>> +      properties:
>>>>> +        compatible:
>>>>> +	  contains:
>>>>
>>>> It does not look like you tested the bindings, at least after quick
>>>> look. Please run `make dt_binding_check` (see
>>>> Documentation/devicetree/bindings/writing-schema.rst for instructions).
>>>> Maybe you need to update your dtschema and yamllint.
>>>
>>> Strange, Test passed
>>>
>>> make ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- -j8  dt_binding_check DT_SCHEMA_FILES=fsl,edma.yaml
>>>   LINT    Documentation/devicetree/bindings
>>>   DTEX    Documentation/devicetree/bindings/dma/fsl,edma.example.dts
>>>   CHKDT   Documentation/devicetree/bindings/processed-schema.json
>>>   SCHEMA  Documentation/devicetree/bindings/processed-schema.json
>>>   DTC_CHK Documentation/devicetree/bindings/dma/fsl,edma.example.dtb
>>
>> Nope, you tested other patch. Just look at your second patch for this.
>> When reviewer points to errors to your code, please investigate?
>>
>> NAK, fix your patches.
> 
> And to prove it, so you will stop wasting my time:
> ../Documentation/devicetree/bindings/dma/fsl,edma.yaml:192:1: found
> character that cannot start any token
> 
> ../Documentation/devicetree/bindings/dma/fsl,edma.yaml:192:1: [error]
> syntax error: found character '\t' that cannot start any token (syntax)
> 
> ../Documentation/devicetree/bindings/dma/fsl,edma.yaml:192:1: found
> character that cannot start any token
> 
> Documentation/devicetree/bindings/dma/fsl,edma.yaml: ignoring, error
> parsing file

Dear NXP,

Quality of patches from NXP is terrible. Several of them are poorly
coded, not following coding style, their submission is not following the
process and requires a lot of effort from reviewers. I was already
complaining about this on mailing lists months ago.

Things did not improve much.

However another trouble is the quality of responses during review. In
many patchsets your responses to reviewers comments were half-baked, not
on actual topic or just with minimal effort to close the topic from your
side. That's not how it works.

If you receive comment, you must investigate. Don't respond immediately
"no, I don't see error" or "but I want something else", but be sure that
you fixed the problem.

Such responses of minimal effort or pushing your own patch is
significant effort on reviewers side. I was complaining about this as
well. This patch here, which does not even build/test yet you claim in
response that you test, is perfect example of it. You got comment from
reviewer and instead really investigating this, you respond that
everything is good on your side. Typical response with minimal effort on
your side, but pushing it to the community.

That's it, that's too much.

NXP, your contributions are poor quality and put too much effort on
community.

Please improve your process, e.g. by training people interacting with
community and using extensive internal review. You can also reach to
experienced community members for help in training and explaining
upstream work, like Denx, Pengutronix, Bootlin, Linaro, Baylibre,
Collabora and others.

Till the situation improves, I will be ignoring all patches from @nxp.com.

Best regards,
Krzysztof

Re: [PATCH v3 1/2] dt-bindings: dma: fsl-edma: remove 'clocks' from required

[Frank Li]

On Wed, Apr 10, 2024 at 08:47:00AM +0200, Krzysztof Kozlowski wrote:
> On 10/04/2024 08:32, Krzysztof Kozlowski wrote:
> > On 10/04/2024 08:30, Krzysztof Kozlowski wrote:
> >> On 09/04/2024 23:09, Frank Li wrote:
> >>> On Tue, Apr 09, 2024 at 10:02:32PM +0200, Krzysztof Kozlowski wrote:
> >>>> On 09/04/2024 20:54, Frank Li wrote:
> >>>>> fsl,imx8qm-adma and fsl,imx8qm-edma don't require 'clocks'. Remove it from
> >>>>> required and add 'if' block for other compatible string to keep the same
> >>>>> restrictions.
> >>>>>
> >>>>> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> >>>>> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> >>>>> ---
> >>>>>
> >>>>> Notes:
> >>>>>     Change from v2 to v3
> >>>>>       - rebase to dmaengine/next
> >>>>
> >>>> This fails...
> >>>
> >>> What's wrong? 
> >>>
> >>> https://git.kernel.org/pub/scm/linux/kernel/git/vkoul/dmaengine.git/log/?h=next
> >>>
> >>>>
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/dma/fsl,edma.yaml b/Documentation/devicetree/bindings/dma/fsl,edma.yaml
> >>>>> index 825f4715499e5..657a7d3ebf857 100644
> >>>>> --- a/Documentation/devicetree/bindings/dma/fsl,edma.yaml
> >>>>> +++ b/Documentation/devicetree/bindings/dma/fsl,edma.yaml
> >>>>> @@ -82,7 +82,6 @@ required:
> >>>>>    - compatible
> >>>>>    - reg
> >>>>>    - interrupts
> >>>>> -  - clocks
> >>>>>    - dma-channels
> >>>>>  
> >>>>>  allOf:
> >>>>> @@ -187,6 +186,22 @@ allOf:
> >>>>>          "#dma-cells":
> >>>>>            const: 3
> >>>>>  
> >>>>> +  - if:
> >>>>> +      properties:
> >>>>> +        compatible:
> >>>>> +	  contains:
> >>>>
> >>>> It does not look like you tested the bindings, at least after quick
> >>>> look. Please run `make dt_binding_check` (see
> >>>> Documentation/devicetree/bindings/writing-schema.rst for instructions).
> >>>> Maybe you need to update your dtschema and yamllint.
> >>>
> >>> Strange, Test passed
> >>>
> >>> make ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- -j8  dt_binding_check DT_SCHEMA_FILES=fsl,edma.yaml
> >>>   LINT    Documentation/devicetree/bindings
> >>>   DTEX    Documentation/devicetree/bindings/dma/fsl,edma.example.dts
> >>>   CHKDT   Documentation/devicetree/bindings/processed-schema.json
> >>>   SCHEMA  Documentation/devicetree/bindings/processed-schema.json
> >>>   DTC_CHK Documentation/devicetree/bindings/dma/fsl,edma.example.dtb
> >>
> >> Nope, you tested other patch. Just look at your second patch for this.
> >> When reviewer points to errors to your code, please investigate?
> >>
> >> NAK, fix your patches.
> > 
> > And to prove it, so you will stop wasting my time:
> > ../Documentation/devicetree/bindings/dma/fsl,edma.yaml:192:1: found
> > character that cannot start any token
> > 
> > ../Documentation/devicetree/bindings/dma/fsl,edma.yaml:192:1: [error]
> > syntax error: found character '\t' that cannot start any token (syntax)
> > 
> > ../Documentation/devicetree/bindings/dma/fsl,edma.yaml:192:1: found
> > character that cannot start any token
> > 
> > Documentation/devicetree/bindings/dma/fsl,edma.yaml: ignoring, error
> > parsing file
> 
> Dear NXP,
> 
> Quality of patches from NXP is terrible. Several of them are poorly
> coded, not following coding style, their submission is not following the
> process and requires a lot of effort from reviewers. I was already
> complaining about this on mailing lists months ago.
> 
> Things did not improve much.

I understand what's your concern. I just said I run dt_binding_check before
submit patch. Never said I will not take time to inversitage it. Actually
I found the problem yesterday and will fix next version. I also find what's
wrong why success in my side.

I also read many of your email about other thread in dt-binding. Most is
the similar question.

The words in your all email "It does not look like you tested the bindings"
is most frequency words regardless from nxp.com or NOT.

Yaml file look like as document, but actually NOT. Unlike C code, which
can compile and running.  And more people get well trainning at debug C
driver. Actually it takes more time handle binding yaml doc than debug
driver code. Some item is actually quite difficult to understand without
deep into json scheme. It is not simple as what look like. It may cause
many low level error because it it not simple as what look like and even
hard.

The whole reviewer team take great efforts to make dt-binding better. 

I think it should stop complain and start improve tools. 

For example: 

I try to add --check in b4.
https://lore.kernel.org/tools/20240319045332.2304950-1-Frank.Li@nxp.com/T/#t

Some self-auto-bot help filter low lever problem.

And from another point, it means more and more NXP engineer want to take
more time and efforts to do upstream works.

> 
> However another trouble is the quality of responses during review. In
> many patchsets your responses to reviewers comments were half-baked, not
> on actual topic or just with minimal effort to close the topic from your
> side. That's not how it works.

Some background is difference. There are many argument spaces at some side.
'minimal effort' is not exist. I bet the email reply to community is 10x
carefull more than other internal email.

> 
> If you receive comment, you must investigate. Don't respond immediately
> "no, I don't see error" or "but I want something else", but be sure that
> you fixed the problem.

My intention "strange, test pass" just said my suprised "why it success in
my side" and "will check tools and my environment", Not "skip run
dt-binding" before submit. But what your understand is , "my comment is
wrong, and you stop inverstiage it and close this topic".
  
> 
> Such responses of minimal effort or pushing your own patch is
> significant effort on reviewers side. I was complaining about this as
> well. This patch here, which does not even build/test yet you claim in
> response that you test, is perfect example of it. You got comment from
> reviewer and instead really investigating this, you respond that
> everything is good on your side. Typical response with minimal effort on
> your side, but pushing it to the community.
> 
> That's it, that's too much.
> 
> NXP, your contributions are poor quality and put too much effort on
> community.

Many time, I want to help filter out some low level problem for nxp patche.
A long-term illness makes a good doctor. Your team work are too efficency.
before I can do anything, you already provide professional feedback.

> 
> Please improve your process, e.g. by training people interacting with
> community and using extensive internal review. You can also reach to
> experienced community members for help in training and explaining
> upstream work, like Denx, Pengutronix, Bootlin, Linaro, Baylibre,
> Collabora and others.

If you can help do trainning for us, weclome!!

> 
> Till the situation improves, I will be ignoring all patches from @nxp.com.
> 
> Best regards,
> Krzysztof
>