| Message ID | 20240319-xilinx-dp-audio-v2-2-92d6d3a7ca7e@ideasonboard.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | drm: xlnx: zynqmp: Add DP audio support | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On Tue, Mar 19, 2024 at 10:22:37AM +0200, Tomi Valkeinen wrote: > The DP subsystem for ZynqMP support audio via two channels, and the DP > DMA has dma-engines for those channels. For some reason the DT binding > has not specified those channels, even if the picture included in > xlnx,zynqmp-dpsub.yaml shows "2 x aud" DMAs. New required entries is an ABI change. This message kind of indicates it was a mistake, but should be a lot more explicit. Are things broken without the entries? Need 'Fixes'? > > Add the two audio DMAs to the binding. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > .../devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml b/Documentation/devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml > index 554f9d5809d4..6b754d4f260e 100644 > --- a/Documentation/devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml > +++ b/Documentation/devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml > @@ -100,12 +100,16 @@ properties: > - description: Video layer, plane 1 (U/V or U) > - description: Video layer, plane 2 (V) > - description: Graphics layer > + - description: Audio channel 0 > + - description: Audio channel 1 > dma-names: > items: > - const: vid0 > - const: vid1 > - const: vid2 > - const: gfx0 > + - const: aud0 > + - const: aud1 > > phys: > description: PHYs for the DP data lanes > @@ -194,11 +198,13 @@ examples: > power-domains = <&pd_dp>; > resets = <&reset ZYNQMP_RESET_DP>; > > - dma-names = "vid0", "vid1", "vid2", "gfx0"; > + dma-names = "vid0", "vid1", "vid2", "gfx0", "aud0", "aud1"; > dmas = <&xlnx_dpdma 0>, > <&xlnx_dpdma 1>, > <&xlnx_dpdma 2>, > - <&xlnx_dpdma 3>; > + <&xlnx_dpdma 3>, > + <&xlnx_dpdma 4>, > + <&xlnx_dpdma 5>; > > phys = <&psgtr 1 PHY_TYPE_DP 0 3>, > <&psgtr 0 PHY_TYPE_DP 1 3>; > > -- > 2.34.1 >
On 20/03/2024 17:37, Rob Herring wrote: > On Tue, Mar 19, 2024 at 10:22:37AM +0200, Tomi Valkeinen wrote: >> The DP subsystem for ZynqMP support audio via two channels, and the DP >> DMA has dma-engines for those channels. For some reason the DT binding >> has not specified those channels, even if the picture included in >> xlnx,zynqmp-dpsub.yaml shows "2 x aud" DMAs. > > New required entries is an ABI change. This message kind of indicates it > was a mistake, but should be a lot more explicit. Are things broken > without the entries? Need 'Fixes'? I'll improve the desc for the next version. So, yes, it's an ABI change, and as far as I can guess (I can't figure out any other reason), the audio DMAs were left out by mistake or misunderstanding. The Linux driver has not supported audio, so this has not been an issue and nothing is broken. Now that this series adds the audio support, I had to add the audio DMAs. I considered making the DMAs optional in the DT, but that doesn't sound right, even if that would keep the ABI compatibility (wouldn't it?). The driver I add in this series does consider the audio DMAs as optional, though. If they're not present, the driver will continue without audio support. So, strictly speaking I think this is a fix to the original commit that adds the DT node, but as the driver using the audio DMAs comes in only now, I think there's no need for the 'Fixes' and backporting. I'm happy to change the approach if you think some other way is better. Tomi >> >> Add the two audio DMAs to the binding. >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> --- >> .../devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml | 10 ++++++++-- >> 1 file changed, 8 insertions(+), 2 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml b/Documentation/devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml >> index 554f9d5809d4..6b754d4f260e 100644 >> --- a/Documentation/devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml >> +++ b/Documentation/devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml >> @@ -100,12 +100,16 @@ properties: >> - description: Video layer, plane 1 (U/V or U) >> - description: Video layer, plane 2 (V) >> - description: Graphics layer >> + - description: Audio channel 0 >> + - description: Audio channel 1 >> dma-names: >> items: >> - const: vid0 >> - const: vid1 >> - const: vid2 >> - const: gfx0 >> + - const: aud0 >> + - const: aud1 >> >> phys: >> description: PHYs for the DP data lanes >> @@ -194,11 +198,13 @@ examples: >> power-domains = <&pd_dp>; >> resets = <&reset ZYNQMP_RESET_DP>; >> >> - dma-names = "vid0", "vid1", "vid2", "gfx0"; >> + dma-names = "vid0", "vid1", "vid2", "gfx0", "aud0", "aud1"; >> dmas = <&xlnx_dpdma 0>, >> <&xlnx_dpdma 1>, >> <&xlnx_dpdma 2>, >> - <&xlnx_dpdma 3>; >> + <&xlnx_dpdma 3>, >> + <&xlnx_dpdma 4>, >> + <&xlnx_dpdma 5>; >> >> phys = <&psgtr 1 PHY_TYPE_DP 0 3>, >> <&psgtr 0 PHY_TYPE_DP 1 3>; >> >> -- >> 2.34.1 >>
Hi Rob, On 21/03/2024 07:52, Tomi Valkeinen wrote: > On 20/03/2024 17:37, Rob Herring wrote: >> On Tue, Mar 19, 2024 at 10:22:37AM +0200, Tomi Valkeinen wrote: >>> The DP subsystem for ZynqMP support audio via two channels, and the DP >>> DMA has dma-engines for those channels. For some reason the DT binding >>> has not specified those channels, even if the picture included in >>> xlnx,zynqmp-dpsub.yaml shows "2 x aud" DMAs. >> >> New required entries is an ABI change. This message kind of indicates it >> was a mistake, but should be a lot more explicit. Are things broken >> without the entries? Need 'Fixes'? > > I'll improve the desc for the next version. > > So, yes, it's an ABI change, and as far as I can guess (I can't figure > out any other reason), the audio DMAs were left out by mistake or > misunderstanding. The Linux driver has not supported audio, so this has > not been an issue and nothing is broken. > > Now that this series adds the audio support, I had to add the audio > DMAs. I considered making the DMAs optional in the DT, but that doesn't > sound right, even if that would keep the ABI compatibility (wouldn't > it?). The driver I add in this series does consider the audio DMAs as > optional, though. If they're not present, the driver will continue > without audio support. > > So, strictly speaking I think this is a fix to the original commit that > adds the DT node, but as the driver using the audio DMAs comes in only > now, I think there's no need for the 'Fixes' and backporting. > > I'm happy to change the approach if you think some other way is better. Do you have any feedback on the above? Should I mark the audio DMAs as optional in the binding? Tomi > Tomi > >>> >>> Add the two audio DMAs to the binding. >>> >>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >>> --- >>> .../devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml | 10 >>> ++++++++-- >>> 1 file changed, 8 insertions(+), 2 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/display/xlnx/ >>> xlnx,zynqmp-dpsub.yaml b/Documentation/devicetree/bindings/display/ >>> xlnx/xlnx,zynqmp-dpsub.yaml >>> index 554f9d5809d4..6b754d4f260e 100644 >>> --- a/Documentation/devicetree/bindings/display/xlnx/xlnx,zynqmp- >>> dpsub.yaml >>> +++ b/Documentation/devicetree/bindings/display/xlnx/xlnx,zynqmp- >>> dpsub.yaml >>> @@ -100,12 +100,16 @@ properties: >>> - description: Video layer, plane 1 (U/V or U) >>> - description: Video layer, plane 2 (V) >>> - description: Graphics layer >>> + - description: Audio channel 0 >>> + - description: Audio channel 1 >>> dma-names: >>> items: >>> - const: vid0 >>> - const: vid1 >>> - const: vid2 >>> - const: gfx0 >>> + - const: aud0 >>> + - const: aud1 >>> phys: >>> description: PHYs for the DP data lanes >>> @@ -194,11 +198,13 @@ examples: >>> power-domains = <&pd_dp>; >>> resets = <&reset ZYNQMP_RESET_DP>; >>> - dma-names = "vid0", "vid1", "vid2", "gfx0"; >>> + dma-names = "vid0", "vid1", "vid2", "gfx0", "aud0", "aud1"; >>> dmas = <&xlnx_dpdma 0>, >>> <&xlnx_dpdma 1>, >>> <&xlnx_dpdma 2>, >>> - <&xlnx_dpdma 3>; >>> + <&xlnx_dpdma 3>, >>> + <&xlnx_dpdma 4>, >>> + <&xlnx_dpdma 5>; >>> phys = <&psgtr 1 PHY_TYPE_DP 0 3>, >>> <&psgtr 0 PHY_TYPE_DP 1 3>; >>> >>> -- >>> 2.34.1 >>> >
diff --git a/Documentation/devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml b/Documentation/devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml index 554f9d5809d4..6b754d4f260e 100644 --- a/Documentation/devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml +++ b/Documentation/devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml @@ -100,12 +100,16 @@ properties: - description: Video layer, plane 1 (U/V or U) - description: Video layer, plane 2 (V) - description: Graphics layer + - description: Audio channel 0 + - description: Audio channel 1 dma-names: items: - const: vid0 - const: vid1 - const: vid2 - const: gfx0 + - const: aud0 + - const: aud1 phys: description: PHYs for the DP data lanes @@ -194,11 +198,13 @@ examples: power-domains = <&pd_dp>; resets = <&reset ZYNQMP_RESET_DP>; - dma-names = "vid0", "vid1", "vid2", "gfx0"; + dma-names = "vid0", "vid1", "vid2", "gfx0", "aud0", "aud1"; dmas = <&xlnx_dpdma 0>, <&xlnx_dpdma 1>, <&xlnx_dpdma 2>, - <&xlnx_dpdma 3>; + <&xlnx_dpdma 3>, + <&xlnx_dpdma 4>, + <&xlnx_dpdma 5>; phys = <&psgtr 1 PHY_TYPE_DP 0 3>, <&psgtr 0 PHY_TYPE_DP 1 3>;
The DP subsystem for ZynqMP support audio via two channels, and the DP DMA has dma-engines for those channels. For some reason the DT binding has not specified those channels, even if the picture included in xlnx,zynqmp-dpsub.yaml shows "2 x aud" DMAs. Add the two audio DMAs to the binding. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> --- .../devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)