RE: [EXT] Re: [PATCH V2 1/2] bindings: fsl-imx-sdma: Document 'HDMI Audio' transfer

[Joy Zou]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> Sent: 2022年5月24日 17:47
> To: Joy Zou <joy.zou@xxxxxxx>; vkoul@xxxxxxxxxx
> Cc: S.J. Wang <shengjiu.wang@xxxxxxx>; robh+dt@xxxxxxxxxx;
> krzysztof.kozlowski+dt@xxxxxxxxxx; shawnguo@xxxxxxxxxx;
> s.hauer@xxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxx; festevam@xxxxxxxxx;
> dl-linux-imx <linux-imx@xxxxxxx>; dmaengine@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx
> Subject: [EXT] Re: [PATCH V2 1/2] bindings: fsl-imx-sdma: Document 'HDMI
> Audio' transfer
> 
> Caution: EXT Email
> 
> On 24/05/2022 10:03, Joy Zou wrote:
> > Add HDMI Audio transfer type.
> >
> > convert the sdma bindings txt into yaml in v2.
> >
> > Signed-off-by: Joy Zou <joy.zou@xxxxxxx>
> > ---
> > Changes since v1:
> > convert the sdma bindings txt into yaml in v2.
> > ---
> >  .../devicetree/bindings/dma/fsl-imx-sdma.yaml | 135
> > ++++++++++++++++++
> 
> There is no conversion here, only new file...
  I will modify the commit message in patch v3.
> 
> >  1 file changed, 135 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/dma/fsl-imx-sdma.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/dma/fsl-imx-sdma.yaml
> > b/Documentation/devicetree/bindings/dma/fsl-imx-sdma.yaml
> > new file mode 100644
> > index 000000000000..5b4f7a09a395
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/dma/fsl-imx-sdma.yaml
> 
> Filename: fsl,imx-sdma.yaml
  I will change filename fsl-imx-sdma.yaml into fsl,imx-sdma.yaml in patch v3.
> 
> > @@ -0,0 +1,135 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause %YAML 1.2
> > +---
> > +$id:
> > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> >
> +cetree.org%2Fschemas%2Fdma%2Ffsl-imx-sdma.yaml%23&amp;data=05%7C
> 01%7C
> >
> +joy.zou%40nxp.com%7C31558c585b2d4c4be66f08da3d6a4f36%7C686ea1d
> 3bc2b4c
> >
> +6fa92cd99c5c301635%7C0%7C0%7C637889824044352650%7CUnknown%7
> CTWFpbGZsb
> >
> +3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0
> %3D
> >
> +%7C3000%7C%7C%7C&amp;sdata=C%2Fkq7qtlS47iAEFK9vExXYc1JuGsnmB4
> 1lQknWkD
> > +i%2Bo%3D&amp;reserved=0
> > +$schema:
> > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> >
> +cetree.org%2Fmeta-schemas%2Fcore.yaml%23&amp;data=05%7C01%7Cjoy.z
> ou%4
> >
> +0nxp.com%7C31558c585b2d4c4be66f08da3d6a4f36%7C686ea1d3bc2b4c6f
> a92cd99
> >
> +c5c301635%7C0%7C0%7C637889824044352650%7CUnknown%7CTWFpbG
> Zsb3d8eyJWIj
> >
> +oiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3
> 000%7
> >
> +C%7C%7C&amp;sdata=cF6zgE9W1gom8rKa6RUmjWDaCq%2FWZhFXVJDugt
> Nx%2BzY%3D&
> > +amp;reserved=0
> > +
> > +title: Freescale Smart Direct Memory Access (SDMA) Controller for
> > +i.MX
> > +
> > +maintainers:
> > +  - Vinod Koul <vkoul@xxxxxxxxxx>
> 
> This should not be subsystem maintainer but someone closer to the actual
> device.
 I will modify maintainer in patch v3.
> 
> > +
> > +allOf:
> > +  - $ref: "dma-controller.yaml#"
> > +
> > +# Everything else is described in the common file
> 
> Skip the comment please.
  I will delete the comment in patch v3.
> 
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +          - fsl,imx25-sdma
> > +          - fsl,imx31-sdma
> > +          - fsl,imx31-to1-sdma
> > +          - fsl,imx31-to2-sdma
> > +          - fsl,imx35-to1-sdma
> > +          - fsl,imx35-to2-sdma
> > +          - fsl,imx51-sdma
> > +          - fsl,imx53-sdma
> > +          - fsl,imx6q-sdma
> > +          - fsl,imx7d-sdma
> > +          - fsl,imx6sx-sdma
> > +          - fsl,imx6ul-sdma
> > +          - fsl,imx8mm-sdma
> > +          - fsl,imx8mn-sdma
> > +          - fsl,imx8mp-sdma
> > +      - enum:
> > +          - fsl,imx35-sdma
> > +          - fsl,imx8mq-sdma
> 
> No, fallback cannot be variable. I doubt that fsl,imx25-sdma+fsl,imx8mq-sdma
> makes any sense!
> 
> Additionally, this does not match existing DTS. Please run `make dtbs_check`.
 I will change it and run make dtbs_check and dt_binding_check in patch v3.
> 
> > +
> > +  reg:
> > +    description: Should contain SDMA registers location and length
> 
> Skip description. Uou need to add maxItems
  I will delete the description and add maxItems in patch v3.
> 
> > +
> > +  interrupts:
> > +    description: Should contain SDMA interrupt
> 
> Skip description. Uou need to add maxItems
 I will delete the description and add maxItems in patch v3.
> 
> 
> > +
> > +  fsl,sdma-ram-script-name:
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +    description: Should contain the full path of SDMA RAM scripts
> firmware.
> > +
> > +  "#dma-cells":
> > +    const: 3
> > +    description: |
> > +      The first cell: request/event ID
> > +
> > +      The second cell: peripheral types ID
> > +        enum:
> > +          - MCU domain SSI: 0
> > +          - Shared SSI: 1
> > +          - MMC: 2
> > +          - SDHC: 3
> > +          - MCU domain UART: 4
> > +          - Shared UART: 5
> > +          - FIRI: 6
> > +          - MCU domain CSPI: 7
> > +          - Shared CSPI: 8
> > +          - SIM: 9
> > +          - ATA: 10
> > +          - CCM: 11
> > +          - External peripheral: 12
> > +          - Memory Stick Host Controller: 13
> > +          - Shared Memory Stick Host Controller: 14
> > +          - DSP: 15
> > +          - Memory: 16
> > +          - FIFO type Memory: 17
> > +          - SPDIF: 18
> > +          - IPU Memory: 19
> > +          - ASRC: 20
> > +          - ESAI: 21
> > +          - SSI Dual FIFO: 22
> > +              description: needs firmware more than ver 2> +
> - Shared ASRC: 23
> > +          - SAI: 24
> > +          - HDMI Audio: 25
> > +
> > +       The third cell: transfer priority ID
> > +         enum:
> > +           - High: 0
> > +           - Medium: 1
> > +           - Low: 2
> > +
> > +  gpr:
> > +    description: The phandle to the General Purpose Register (GPR)
> > + node
> 
> type/ref needed
  I will add it in patch v3.
> 
> > +
> > +  fsl,sdma-event-remap:
> > +    $ref: /schemas/types.yaml#/definitions/uint32-array
> > +    description: |
> > +      Register bits of sdma event remap, the format is <reg shift val>.
> > +      - reg: the GPR register offset
> > +      - shift: the bit position inside the GPR register
> > +      - val: the value of the bit (0 or 1)
> 
> Need maxItems or items with description.
 I will add items with description in patch v3.
> 
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - fsl,sdma-ram-script-name
> > +  - "#dma-cells"
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    sdma: dma-controller@83fb0000 {
> > +      compatible = "fsl,imx51-sdma", "fsl,imx35-sdma";
> > +      reg = <0x83fb0000 0x4000>;
> > +      interrupts = <6>;
> > +      #dma-cells = <3>;
> > +      fsl,sdma-ram-script-name = "sdma-imx51.bin";
> > +    };
> > +
> > +#DMA clients connected to the i.MX SDMA controller must use the
> > +format #described in the dma-controller.yaml file.
> > +  - |
> > +    ssi2: ssi@70014000 {
> 
> Skip consumer example, it's obvious.
 I will delete consumer example in patch v3.
 It is my honor to receive your comments. Thank you very much for your effort to provide valuable 
 and important comments on my patch.
BR
Joy Zou
> 
> > +      compatible = "fsl,imx51-ssi", "fsl,imx21-ssi";
> > +      reg = <0x70014000 0x4000>;
> > +      interrupts = <30>;
> > +      clocks = <&clks 49>;
> > +      dmas = <&sdma 24 1 0>,
> > +             <&sdma 25 1 0>;
> > +      dma-names = "rx", "tx";
> > +      fsl,fifo-depth = <15>;
> > +    };
> > +
> > +...
> 
> 
> Best regards,
> Krzysztof