| Message ID | 20240321-dp-live-fmt-v3-8-d5090d796b7e@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Setting live video input format for ZynqMP DPSUB | expand |
On Thu, 21 Mar 2024 13:43:46 -0700, Anatoliy Klymenko wrote: > DO NOT MERGE. REFERENCE ONLY. > > Add binding for AMD/Xilinx Video Timing Controller and Test Pattern > Generator. > > Copy media-bus-formats.h into dt-bindings/media to suplement TPG DT node. > > Signed-off-by: Anatoliy Klymenko <anatoliy.klymenko@amd.com> > --- > .../bindings/display/xlnx/xlnx,v-tpg.yaml | 87 ++++++++++ > .../devicetree/bindings/display/xlnx/xlnx,vtc.yaml | 65 ++++++++ > include/dt-bindings/media/media-bus-format.h | 177 +++++++++++++++++++++ > 3 files changed, 329 insertions(+) > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: ./Documentation/devicetree/bindings/display/xlnx/xlnx,v-tpg.yaml:35:4: [warning] wrong indentation: expected 4 but found 3 (indentation) ./Documentation/devicetree/bindings/display/xlnx/xlnx,v-tpg.yaml:45:8: [warning] wrong indentation: expected 8 but found 7 (indentation) ./Documentation/devicetree/bindings/display/xlnx/xlnx,v-tpg.yaml:49:8: [warning] wrong indentation: expected 8 but found 7 (indentation) dtschema/dtc warnings/errors: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/display/xlnx/xlnx,v-tpg.yaml: bus-format: missing type definition /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/display/xlnx/xlnx,v-tpg.yaml: xlnx,bridge: missing type definition /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/display/xlnx/xlnx,vtc.yaml: xlnx,pixels-per-clock: missing type definition doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240321-dp-live-fmt-v3-8-d5090d796b7e@amd.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
On 21/03/2024 21:43, Anatoliy Klymenko wrote: > DO NOT MERGE. REFERENCE ONLY. Why? What are you doing here and why nothing about this is explained? > > Add binding for AMD/Xilinx Video Timing Controller and Test Pattern > Generator. > > Copy media-bus-formats.h into dt-bindings/media to suplement TPG DT node. Still not tested. Do not send untested code to the lists. NAK Best regards, Krzysztof
On 21/03/2024 21:43, Anatoliy Klymenko wrote: > diff --git a/include/dt-bindings/media/media-bus-format.h b/include/dt-bindings/media/media-bus-format.h > new file mode 100644 > index 000000000000..60fc6e11dabc > --- /dev/null > +++ b/include/dt-bindings/media/media-bus-format.h > @@ -0,0 +1,177 @@ > +/* SPDX-License-Identifier: (GPL-2.0-only OR MIT) */ > +/* > + * Media Bus API header > + * > + * Copyright (C) 2009, Guennadi Liakhovetski <g.liakhovetski@gmx.de> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. That's not true. Your SPDX tells something entirely different. Anyway, you did not explain why you need to copy anything anywhere. Specifically, random hex values *are not bindings*. Best regards, Krzysztof
On Fri, Mar 22, 2024 at 06:59:18AM +0100, Krzysztof Kozlowski wrote: > On 21/03/2024 21:43, Anatoliy Klymenko wrote: > > diff --git a/include/dt-bindings/media/media-bus-format.h b/include/dt-bindings/media/media-bus-format.h > > new file mode 100644 > > index 000000000000..60fc6e11dabc > > --- /dev/null > > +++ b/include/dt-bindings/media/media-bus-format.h > > @@ -0,0 +1,177 @@ > > +/* SPDX-License-Identifier: (GPL-2.0-only OR MIT) */ > > +/* > > + * Media Bus API header > > + * > > + * Copyright (C) 2009, Guennadi Liakhovetski <g.liakhovetski@gmx.de> > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License version 2 as > > + * published by the Free Software Foundation. > > That's not true. Your SPDX tells something entirely different. > > Anyway, you did not explain why you need to copy anything anywhere. I assume by "copy anything anywhere" you mean "why did you copy a linux uapi header into the bindings? > Specifically, random hex values *are not bindings*. > > Best regards, > Krzysztof >
Hi Krzysztof, Thanks a lot for the review. > -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Thursday, March 21, 2024 10:59 PM > To: Klymenko, Anatoliy <Anatoliy.Klymenko@amd.com>; Laurent Pinchart > <laurent.pinchart@ideasonboard.com>; Maarten Lankhorst > <maarten.lankhorst@linux.intel.com>; Maxime Ripard <mripard@kernel.org>; > Thomas Zimmermann <tzimmermann@suse.de>; David Airlie > <airlied@gmail.com>; Daniel Vetter <daniel@ffwll.ch>; Simek, Michal > <michal.simek@amd.com>; Andrzej Hajda <andrzej.hajda@intel.com>; Neil > Armstrong <neil.armstrong@linaro.org>; Robert Foss <rfoss@kernel.org>; Jonas > Karlman <jonas@kwiboo.se>; Jernej Skrabec <jernej.skrabec@gmail.com>; Rob > Herring <robh+dt@kernel.org>; Krzysztof Kozlowski > <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; > Mauro Carvalho Chehab <mchehab@kernel.org> > Cc: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>; dri- > devel@lists.freedesktop.org; linux-arm-kernel@lists.infradead.org; linux- > kernel@vger.kernel.org; devicetree@vger.kernel.org; linux- > media@vger.kernel.org > Subject: Re: [PATCH v3 8/9] dt-bindings: xlnx: Add VTC and TPG bindings > > Caution: This message originated from an External Source. Use proper caution > when opening attachments, clicking links, or responding. > > > On 21/03/2024 21:43, Anatoliy Klymenko wrote: > > diff --git a/include/dt-bindings/media/media-bus-format.h b/include/dt- > bindings/media/media-bus-format.h > > new file mode 100644 > > index 000000000000..60fc6e11dabc > > --- /dev/null > > +++ b/include/dt-bindings/media/media-bus-format.h > > @@ -0,0 +1,177 @@ > > +/* SPDX-License-Identifier: (GPL-2.0-only OR MIT) */ > > +/* > > + * Media Bus API header > > + * > > + * Copyright (C) 2009, Guennadi Liakhovetski <g.liakhovetski@gmx.de> > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License version 2 as > > + * published by the Free Software Foundation. > > That's not true. Your SPDX tells something entirely different. > Thank you - I'll see how to fix it. > Anyway, you did not explain why you need to copy anything anywhere. > > Specifically, random hex values *are not bindings*. > The same media bus format values are being used by the reference driver in patch #9. And, as far as I know, we cannot use headers from Linux API headers directly (at least I noticed the same pattern in ../dt-bindings/sdtv-standarts.h for instance). What would be the best approach to reusing the same defines on DT and driver sides from your point of view? Symlink maybe? > Best regards, > Krzysztof Thank you, Anatoliy
On 22/03/2024 20:12, Klymenko, Anatoliy wrote: > Hi Krzysztof, > > Thanks a lot for the review. > >> -----Original Message----- >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> Sent: Thursday, March 21, 2024 10:59 PM >> To: Klymenko, Anatoliy <Anatoliy.Klymenko@amd.com>; Laurent Pinchart >> <laurent.pinchart@ideasonboard.com>; Maarten Lankhorst >> <maarten.lankhorst@linux.intel.com>; Maxime Ripard <mripard@kernel.org>; >> Thomas Zimmermann <tzimmermann@suse.de>; David Airlie >> <airlied@gmail.com>; Daniel Vetter <daniel@ffwll.ch>; Simek, Michal >> <michal.simek@amd.com>; Andrzej Hajda <andrzej.hajda@intel.com>; Neil >> Armstrong <neil.armstrong@linaro.org>; Robert Foss <rfoss@kernel.org>; Jonas >> Karlman <jonas@kwiboo.se>; Jernej Skrabec <jernej.skrabec@gmail.com>; Rob >> Herring <robh+dt@kernel.org>; Krzysztof Kozlowski >> <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; >> Mauro Carvalho Chehab <mchehab@kernel.org> >> Cc: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>; dri- >> devel@lists.freedesktop.org; linux-arm-kernel@lists.infradead.org; linux- >> kernel@vger.kernel.org; devicetree@vger.kernel.org; linux- >> media@vger.kernel.org >> Subject: Re: [PATCH v3 8/9] dt-bindings: xlnx: Add VTC and TPG bindings >> >> Caution: This message originated from an External Source. Use proper caution >> when opening attachments, clicking links, or responding. >> >> >> On 21/03/2024 21:43, Anatoliy Klymenko wrote: >>> diff --git a/include/dt-bindings/media/media-bus-format.h b/include/dt- >> bindings/media/media-bus-format.h >>> new file mode 100644 >>> index 000000000000..60fc6e11dabc >>> --- /dev/null >>> +++ b/include/dt-bindings/media/media-bus-format.h >>> @@ -0,0 +1,177 @@ >>> +/* SPDX-License-Identifier: (GPL-2.0-only OR MIT) */ >>> +/* >>> + * Media Bus API header >>> + * >>> + * Copyright (C) 2009, Guennadi Liakhovetski <g.liakhovetski@gmx.de> >>> + * >>> + * This program is free software; you can redistribute it and/or modify >>> + * it under the terms of the GNU General Public License version 2 as >>> + * published by the Free Software Foundation. >> >> That's not true. Your SPDX tells something entirely different. >> > > Thank you - I'll see how to fix it. > >> Anyway, you did not explain why you need to copy anything anywhere. >> >> Specifically, random hex values *are not bindings*. >> > > The same media bus format values are being used by the reference driver in patch #9. And, as far as I know, we cannot use headers from Linux API headers directly (at least I I don't understand what does it mean. You can use in your driver whatever headers you wish, I don't care about them. noticed the same pattern in ../dt-bindings/sdtv-standarts.h for instance). What would be the best approach to reusing the same defines on DT and driver sides from your point of view? Symlink maybe? > Wrap your messages to match mailing list discussion style. There are no defines used in DT. If there are, show me them in *THIS* or other *upstreamed* (being upstreamed) patchset. Whatever you have out of tree or "DO NOT MERGE" does not matter and does not justify anything. Best regards, Krzysztof
On 22/03/2024 19:05, Conor Dooley wrote: > On Fri, Mar 22, 2024 at 06:59:18AM +0100, Krzysztof Kozlowski wrote: >> On 21/03/2024 21:43, Anatoliy Klymenko wrote: >>> diff --git a/include/dt-bindings/media/media-bus-format.h b/include/dt-bindings/media/media-bus-format.h >>> new file mode 100644 >>> index 000000000000..60fc6e11dabc >>> --- /dev/null >>> +++ b/include/dt-bindings/media/media-bus-format.h >>> @@ -0,0 +1,177 @@ >>> +/* SPDX-License-Identifier: (GPL-2.0-only OR MIT) */ >>> +/* >>> + * Media Bus API header >>> + * >>> + * Copyright (C) 2009, Guennadi Liakhovetski <g.liakhovetski@gmx.de> >>> + * >>> + * This program is free software; you can redistribute it and/or modify >>> + * it under the terms of the GNU General Public License version 2 as >>> + * published by the Free Software Foundation. >> >> That's not true. Your SPDX tells something entirely different. >> >> Anyway, you did not explain why you need to copy anything anywhere. > > I assume by "copy anything anywhere" you mean "why did you copy a linux > uapi header into the bindings? Yes, I trimmed context too much. The reasoning of copying some UAPI and claiming it is a binding was: "Copy media-bus-formats.h into dt-bindings/media to suplement TPG DT node." so as seen *there is no reason*. Commit msg should explain why we are doing things. Best regards, Krzysztof
On Sat, Mar 23, 2024 at 11:22:22AM +0100, Krzysztof Kozlowski wrote: > On 22/03/2024 19:05, Conor Dooley wrote: > > On Fri, Mar 22, 2024 at 06:59:18AM +0100, Krzysztof Kozlowski wrote: > >> On 21/03/2024 21:43, Anatoliy Klymenko wrote: > >>> diff --git a/include/dt-bindings/media/media-bus-format.h b/include/dt-bindings/media/media-bus-format.h > >>> new file mode 100644 > >>> index 000000000000..60fc6e11dabc > >>> --- /dev/null > >>> +++ b/include/dt-bindings/media/media-bus-format.h > >>> @@ -0,0 +1,177 @@ > >>> +/* SPDX-License-Identifier: (GPL-2.0-only OR MIT) */ > >>> +/* > >>> + * Media Bus API header > >>> + * > >>> + * Copyright (C) 2009, Guennadi Liakhovetski <g.liakhovetski@gmx.de> > >>> + * > >>> + * This program is free software; you can redistribute it and/or modify > >>> + * it under the terms of the GNU General Public License version 2 as > >>> + * published by the Free Software Foundation. > >> > >> That's not true. Your SPDX tells something entirely different. > >> > >> Anyway, you did not explain why you need to copy anything anywhere. > > > > I assume by "copy anything anywhere" you mean "why did you copy a linux > > uapi header into the bindings? > > Yes, I trimmed context too much. > > The reasoning of copying some UAPI and claiming it is a binding was: > "Copy media-bus-formats.h into dt-bindings/media to suplement TPG DT node." > so as seen *there is no reason*. > > Commit msg should explain why we are doing things. Oh for sure. I was just wondering if you were complaining about the UAPI header or if that comment was about the copyright notice. If it had been the latter I was gonna point out the former :)
Hi Krzysztof, Thank you for the feedback. > -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Saturday, March 23, 2024 3:21 AM > To: Klymenko, Anatoliy <Anatoliy.Klymenko@amd.com>; Laurent Pinchart > <laurent.pinchart@ideasonboard.com>; Maarten Lankhorst > <maarten.lankhorst@linux.intel.com>; Maxime Ripard > <mripard@kernel.org>; Thomas Zimmermann <tzimmermann@suse.de>; > David Airlie <airlied@gmail.com>; Daniel Vetter <daniel@ffwll.ch>; Simek, > Michal <michal.simek@amd.com>; Andrzej Hajda > <andrzej.hajda@intel.com>; Neil Armstrong <neil.armstrong@linaro.org>; > Robert Foss <rfoss@kernel.org>; Jonas Karlman <jonas@kwiboo.se>; Jernej > Skrabec <jernej.skrabec@gmail.com>; Rob Herring <robh+dt@kernel.org>; > Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley > <conor+dt@kernel.org>; Mauro Carvalho Chehab <mchehab@kernel.org> > Cc: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>; dri- > devel@lists.freedesktop.org; linux-arm-kernel@lists.infradead.org; linux- > kernel@vger.kernel.org; devicetree@vger.kernel.org; linux- > media@vger.kernel.org > Subject: Re: [PATCH v3 8/9] dt-bindings: xlnx: Add VTC and TPG bindings > > Caution: This message originated from an External Source. Use proper > caution when opening attachments, clicking links, or responding. > > > On 22/03/2024 20:12, Klymenko, Anatoliy wrote: > > Hi Krzysztof, > > > > Thanks a lot for the review. > > > >> -----Original Message----- > >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > >> Sent: Thursday, March 21, 2024 10:59 PM > >> To: Klymenko, Anatoliy <Anatoliy.Klymenko@amd.com>; Laurent Pinchart > >> <laurent.pinchart@ideasonboard.com>; Maarten Lankhorst > >> <maarten.lankhorst@linux.intel.com>; Maxime Ripard > >> <mripard@kernel.org>; Thomas Zimmermann <tzimmermann@suse.de>; > David > >> Airlie <airlied@gmail.com>; Daniel Vetter <daniel@ffwll.ch>; Simek, > >> Michal <michal.simek@amd.com>; Andrzej Hajda > >> <andrzej.hajda@intel.com>; Neil Armstrong > >> <neil.armstrong@linaro.org>; Robert Foss <rfoss@kernel.org>; Jonas > >> Karlman <jonas@kwiboo.se>; Jernej Skrabec > <jernej.skrabec@gmail.com>; > >> Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski > >> <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley > >> <conor+dt@kernel.org>; Mauro Carvalho Chehab <mchehab@kernel.org> > >> Cc: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>; dri- > >> devel@lists.freedesktop.org; linux-arm-kernel@lists.infradead.org; > >> linux- kernel@vger.kernel.org; devicetree@vger.kernel.org; linux- > >> media@vger.kernel.org > >> Subject: Re: [PATCH v3 8/9] dt-bindings: xlnx: Add VTC and TPG > >> bindings > >> > >> Caution: This message originated from an External Source. Use proper > >> caution when opening attachments, clicking links, or responding. > >> > >> > >> On 21/03/2024 21:43, Anatoliy Klymenko wrote: > >>> diff --git a/include/dt-bindings/media/media-bus-format.h > >>> b/include/dt- > >> bindings/media/media-bus-format.h > >>> new file mode 100644 > >>> index 000000000000..60fc6e11dabc > >>> --- /dev/null > >>> +++ b/include/dt-bindings/media/media-bus-format.h > >>> @@ -0,0 +1,177 @@ > >>> +/* SPDX-License-Identifier: (GPL-2.0-only OR MIT) */ > >>> +/* > >>> + * Media Bus API header > >>> + * > >>> + * Copyright (C) 2009, Guennadi Liakhovetski > >>> +<g.liakhovetski@gmx.de> > >>> + * > >>> + * This program is free software; you can redistribute it and/or > >>> +modify > >>> + * it under the terms of the GNU General Public License version 2 > >>> +as > >>> + * published by the Free Software Foundation. > >> > >> That's not true. Your SPDX tells something entirely different. > >> > > > > Thank you - I'll see how to fix it. > > > >> Anyway, you did not explain why you need to copy anything anywhere. > >> > >> Specifically, random hex values *are not bindings*. > >> > > > > The same media bus format values are being used by the reference > > driver in patch #9. And, as far as I know, we cannot use headers from > > Linux API headers directly (at least I > > I don't understand what does it mean. You can use in your driver whatever > headers you wish, I don't care about them. > > > noticed the same pattern in ../dt-bindings/sdtv-standarts.h for instance). > What would be the best approach to reusing the same defines on DT and > driver sides from your point of view? Symlink maybe? > > > > Wrap your messages to match mailing list discussion style. There are no > defines used in DT. If there are, show me them in *THIS* or other > *upstreamed* (being upstreamed) patchset. > Sorry, I didn't explain properly what I'm trying to achieve. I need to create a DT node property that represents video signal format, one of MEDIA_BUS_FMT_* from include/uapi/linux/media-bus-format.h. It would be nice to reuse the same symbolic values in the device tree. What is the best approach here? Should I create a separate header in include/dt-bindings with the same or similar (to avoid multiple definition errors) defines, or is it better to create a symlink to media-bus-format.h like include/dt-bindings/linux-event-codes.h? > Whatever you have out of tree or "DO NOT MERGE" does not matter and > does not justify anything. > > > Best regards, > Krzysztof Thank you, Anatoliy
On Fri, Mar 29, 2024 at 12:38:33AM +0000, Klymenko, Anatoliy wrote: > Thank you for the feedback. > > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > Subject: Re: [PATCH v3 8/9] dt-bindings: xlnx: Add VTC and TPG bindings > > On 22/03/2024 20:12, Klymenko, Anatoliy wrote: > > >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > >> On 21/03/2024 21:43, Anatoliy Klymenko wrote: > > >>> diff --git a/include/dt-bindings/media/media-bus-format.h > > >>> b/include/dt- > > >> bindings/media/media-bus-format.h > > >>> new file mode 100644 > > >>> index 000000000000..60fc6e11dabc > > >>> --- /dev/null > > >>> +++ b/include/dt-bindings/media/media-bus-format.h > > >>> @@ -0,0 +1,177 @@ > > >>> +/* SPDX-License-Identifier: (GPL-2.0-only OR MIT) */ > > >>> +/* > > >>> + * Media Bus API header > > >>> + * > > >>> + * Copyright (C) 2009, Guennadi Liakhovetski > > >>> +<g.liakhovetski@gmx.de> > > >>> + * > > >>> + * This program is free software; you can redistribute it and/or > > >>> +modify > > >>> + * it under the terms of the GNU General Public License version 2 > > >>> +as > > >>> + * published by the Free Software Foundation. > > >> > > >> That's not true. Your SPDX tells something entirely different. > > >> > > > > > > Thank you - I'll see how to fix it. > > > > > >> Anyway, you did not explain why you need to copy anything anywhere. > > >> > > >> Specifically, random hex values *are not bindings*. > > >> > > > > > > The same media bus format values are being used by the reference > > > driver in patch #9. And, as far as I know, we cannot use headers from > > > Linux API headers directly (at least I > > > > I don't understand what does it mean. You can use in your driver whatever > > headers you wish, I don't care about them. > > > > > > noticed the same pattern in ../dt-bindings/sdtv-standarts.h for instance). > > What would be the best approach to reusing the same defines on DT and > > driver sides from your point of view? Symlink maybe? > > > > > > > Wrap your messages to match mailing list discussion style. There are no > > defines used in DT. If there are, show me them in *THIS* or other > > *upstreamed* (being upstreamed) patchset. > > > > Sorry, I didn't explain properly what I'm trying to achieve. I need to > create a DT node property that represents video signal format, one of > MEDIA_BUS_FMT_* from include/uapi/linux/media-bus-format.h. It would be > nice to reuse the same symbolic values in the device tree. What is the > best approach here? Should I create a separate header in > include/dt-bindings with the same or similar (to avoid multiple > definition errors) defines, or is it better to create a symlink to > media-bus-format.h like include/dt-bindings/linux-event-codes.h? Isn't there already a property for this, described in Documentation/devicetree/bindings/media/xilinx/video.txt ?
> -----Original Message----- > From: Conor Dooley <conor@kernel.org> > Sent: Friday, March 29, 2024 8:47 AM > To: Klymenko, Anatoliy <Anatoliy.Klymenko@amd.com> > Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>; Laurent Pinchart > <laurent.pinchart@ideasonboard.com>; Maarten Lankhorst > <maarten.lankhorst@linux.intel.com>; Maxime Ripard > <mripard@kernel.org>; Thomas Zimmermann <tzimmermann@suse.de>; > David Airlie <airlied@gmail.com>; Daniel Vetter <daniel@ffwll.ch>; Simek, > Michal <michal.simek@amd.com>; Andrzej Hajda > <andrzej.hajda@intel.com>; Neil Armstrong <neil.armstrong@linaro.org>; > Robert Foss <rfoss@kernel.org>; Jonas Karlman <jonas@kwiboo.se>; Jernej > Skrabec <jernej.skrabec@gmail.com>; Rob Herring <robh+dt@kernel.org>; > Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley > <conor+dt@kernel.org>; Mauro Carvalho Chehab <mchehab@kernel.org>; > Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>; dri- > devel@lists.freedesktop.org; linux-arm-kernel@lists.infradead.org; linux- > kernel@vger.kernel.org; devicetree@vger.kernel.org; linux- > media@vger.kernel.org > Subject: Re: [PATCH v3 8/9] dt-bindings: xlnx: Add VTC and TPG bindings > > On Fri, Mar 29, 2024 at 12:38:33AM +0000, Klymenko, Anatoliy wrote: > > Thank you for the feedback. > > > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > > Subject: Re: [PATCH v3 8/9] dt-bindings: xlnx: Add VTC and TPG bindings > > > On 22/03/2024 20:12, Klymenko, Anatoliy wrote: > > > >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > > >> On 21/03/2024 21:43, Anatoliy Klymenko wrote: > > > >>> diff --git a/include/dt-bindings/media/media-bus-format.h > > > >>> b/include/dt- > > > >> bindings/media/media-bus-format.h > > > >>> new file mode 100644 > > > >>> index 000000000000..60fc6e11dabc > > > >>> --- /dev/null > > > >>> +++ b/include/dt-bindings/media/media-bus-format.h > > > >>> @@ -0,0 +1,177 @@ > > > >>> +/* SPDX-License-Identifier: (GPL-2.0-only OR MIT) */ > > > >>> +/* > > > >>> + * Media Bus API header > > > >>> + * > > > >>> + * Copyright (C) 2009, Guennadi Liakhovetski > > > >>> +<g.liakhovetski@gmx.de> > > > >>> + * > > > >>> + * This program is free software; you can redistribute it and/or > > > >>> +modify > > > >>> + * it under the terms of the GNU General Public License version 2 > > > >>> +as > > > >>> + * published by the Free Software Foundation. > > > >> > > > >> That's not true. Your SPDX tells something entirely different. > > > >> > > > > > > > > Thank you - I'll see how to fix it. > > > > > > > >> Anyway, you did not explain why you need to copy anything anywhere. > > > >> > > > >> Specifically, random hex values *are not bindings*. > > > >> > > > > > > > > The same media bus format values are being used by the reference > > > > driver in patch #9. And, as far as I know, we cannot use headers from > > > > Linux API headers directly (at least I > > > > > > I don't understand what does it mean. You can use in your driver > whatever > > > headers you wish, I don't care about them. > > > > > > > > > noticed the same pattern in ../dt-bindings/sdtv-standarts.h for instance). > > > What would be the best approach to reusing the same defines on DT and > > > driver sides from your point of view? Symlink maybe? > > > > > > > > > > Wrap your messages to match mailing list discussion style. There are no > > > defines used in DT. If there are, show me them in *THIS* or other > > > *upstreamed* (being upstreamed) patchset. > > > > > > > Sorry, I didn't explain properly what I'm trying to achieve. I need to > > create a DT node property that represents video signal format, one of > > MEDIA_BUS_FMT_* from include/uapi/linux/media-bus-format.h. It would > be > > nice to reuse the same symbolic values in the device tree. What is the > > best approach here? Should I create a separate header in > > include/dt-bindings with the same or similar (to avoid multiple > > definition errors) defines, or is it better to create a symlink to > > media-bus-format.h like include/dt-bindings/linux-event-codes.h? > > Isn't there already a property for this, described in > Documentation/devicetree/bindings/media/xilinx/video.txt > ? Those properties are very similar, indeed but not exactly the same. The one you noticed maps Xilinx video data format on AXI4-Stream transport that covers color space and chroma subsampling only. The rest of the signal attributes are either conventional or left out. MEDIA_BUS_FMT_* collection is more specific and embeds additional information about video signals, like bits per component and component ordering.
On 30/03/2024 03:02, Klymenko, Anatoliy wrote: >>>> >>> >>> Sorry, I didn't explain properly what I'm trying to achieve. I need to >>> create a DT node property that represents video signal format, one of >>> MEDIA_BUS_FMT_* from include/uapi/linux/media-bus-format.h. It would >> be >>> nice to reuse the same symbolic values in the device tree. What is the >>> best approach here? Should I create a separate header in There is no user of this new header, so I don't agree. Please send either full work or link your other upstreamed patchset. Anything sent as "DO NOT MERGE" does not count because it is not an user. Without the DTS user I claim that you do not bind here anything... >>> include/dt-bindings with the same or similar (to avoid multiple >>> definition errors) defines, or is it better to create a symlink to >>> media-bus-format.h like include/dt-bindings/linux-event-codes.h? Copying or symlinking entire header into bindings does not help us to understand what is exactly a binding here. For example, maybe you encode runtime information into DT (don't do this) and that's why you need these defines... Or maybe your block has some capabilities. Dunno, patch was not tested, is defined as do not merge and is not explaining any of these. Therefore, please provide complete set of users ready to be merged, test your patches, provide rationale why this is supposed to be a binding and why do you think it represents hardware configuration, not OS policy or runtime configuration. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/display/xlnx/xlnx,v-tpg.yaml b/Documentation/devicetree/bindings/display/xlnx/xlnx,v-tpg.yaml new file mode 100644 index 000000000000..df5ee5b547cf --- /dev/null +++ b/Documentation/devicetree/bindings/display/xlnx/xlnx,v-tpg.yaml @@ -0,0 +1,87 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/xlnx/xlnx,v-tpg.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Xilinx Video Test Pattern Generator (TPG) + +description: + AMD/Xilinx Video Test Pattern Generator IP is a soft IP designed to + generate test video signal in AXI4-Stream Video format. + https://docs.xilinx.com/r/en-US/pg103-v-tpg + +maintainers: + - Anatoliy Klymenko <anatoliy.klymenko@amd.com> + +properties: + compatible: + description: + TPG versions backward-compatible with previous versions should list all + compatible versions in the newer to older order. + enum: ["xlnx,v-tpg-8.0", "xlnx,v-tpg-8.2"] + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + bus-format: + description: Output media bus format, one of MEDIA_BUS_FMT_* + maxItems: 1 + + xlnx,bridge: + description: Reference to Video Timing Controller + maxItems: 1 + + ports: + $ref: /schemas/graph.yaml#/properties/ports + description: + Connections from and to external components in the video pipeline. + + properties: + port@0: + $ref: /schemas/graph.yaml#/properties/port + description: Sink port connected to downstream video IP. + + port@1: + $ref: /schemas/graph.yaml#/properties/port + description: Source port to connect to optional video signal source. + + required: + - port@0 + +required: + - compatible + - reg + - interrupts + - bus-format + - xlnx,bridge + - ports + +additionalProperties: false + +examples: + - | + #include <dt-bindings/media/media-bus-format.h> + + tpg_0: tpg@40050000 { + compatible = "xlnx,v-tpg-8.0"; + reg = <0x40050000 0x10000>; + interrupts = <0 89 4>; + xlnx,bridge = <&vtc_3>; + bus-format = <MEDIA_BUS_FMT_UYVY8_1X16>; + ports { + #address-cells = <1>; + #size-cells = <0>; + port@0 { + reg = <0>; + tpg_out: endpoint { + remote-endpoint = <&dp_encoder>; + }; + }; + }; + }; + +... diff --git a/Documentation/devicetree/bindings/display/xlnx/xlnx,vtc.yaml b/Documentation/devicetree/bindings/display/xlnx/xlnx,vtc.yaml new file mode 100644 index 000000000000..51eb12cdcfdc --- /dev/null +++ b/Documentation/devicetree/bindings/display/xlnx/xlnx,vtc.yaml @@ -0,0 +1,65 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/xlnx/xlnx,vtc.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Device-Tree for Xilinx Video Timing Controller(VTC) + +description: + Xilinx VTC is a general purpose video timing generator and detector. + The input side of this core automatically detects horizontal and + vertical synchronization, pulses, polarity, blanking timing and active pixels. + While on the output, it generates the horizontal and vertical blanking and + synchronization pulses used with a standard video system including support + for programmable pulse polarity. + + The core is commonly used with Video in to AXI4-Stream core to detect the + format and timing of incoming video data or with AXI4-Stream to Video out core + to generate outgoing video timing for downstream sinks like a video monitor. + + For details please refer to + https://docs.xilinx.com/r/en-US/pg016_v_tc + +maintainers: + - Sagar Vishal <sagar.vishal.com> + +properties: + compatible: + const: "xlnx,bridge-v-tc-6.1" + + reg: + maxItems: 1 + + xlnx,pixels-per-clock: + description: Pixels per clock of the stream. + enum: [1, 2, 4] + + clocks: + minItems: 2 + + clock-names: + items: + - const: clk + - const: s_axi_aclk + +required: + - compatible + - reg + - xlnx,pixels-per-clock + - clocks + - clock-names + +additionalProperties: false + +examples: + - | + v_tc_0: v_tc@80010000 { + clock-names = "clk", "s_axi_aclk"; + clocks = <&clk_wiz_0>, <&misc_clk_0>; + compatible = "xlnx,bridge-v-tc-6.1"; + xlnx,pixels-per-clock = <1>; + reg = <0x80010000 0x10000>; + }; + +... diff --git a/include/dt-bindings/media/media-bus-format.h b/include/dt-bindings/media/media-bus-format.h new file mode 100644 index 000000000000..60fc6e11dabc --- /dev/null +++ b/include/dt-bindings/media/media-bus-format.h @@ -0,0 +1,177 @@ +/* SPDX-License-Identifier: (GPL-2.0-only OR MIT) */ +/* + * Media Bus API header + * + * Copyright (C) 2009, Guennadi Liakhovetski <g.liakhovetski@gmx.de> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#ifndef __LINUX_MEDIA_BUS_FORMAT_H +#define __LINUX_MEDIA_BUS_FORMAT_H + +/* + * These bus formats uniquely identify data formats on the data bus. Format 0 + * is reserved, MEDIA_BUS_FMT_FIXED shall be used by host-client pairs, where + * the data format is fixed. Additionally, "2X8" means that one pixel is + * transferred in two 8-bit samples, "BE" or "LE" specify in which order those + * samples are transferred over the bus: "LE" means that the least significant + * bits are transferred first, "BE" means that the most significant bits are + * transferred first, and "PADHI" and "PADLO" define which bits - low or high, + * in the incomplete high byte, are filled with padding bits. + * + * The bus formats are grouped by type, bus_width, bits per component, samples + * per pixel and order of subsamples. Numerical values are sorted using generic + * numerical sort order (8 thus comes before 10). + * + * As their value can't change when a new bus format is inserted in the + * enumeration, the bus formats are explicitly given a numerical value. The next + * free values for each category are listed below, update them when inserting + * new pixel codes. + */ + +#define MEDIA_BUS_FMT_FIXED 0x0001 + +/* RGB - next is 0x1026 */ +#define MEDIA_BUS_FMT_RGB444_1X12 0x1016 +#define MEDIA_BUS_FMT_RGB444_2X8_PADHI_BE 0x1001 +#define MEDIA_BUS_FMT_RGB444_2X8_PADHI_LE 0x1002 +#define MEDIA_BUS_FMT_RGB555_2X8_PADHI_BE 0x1003 +#define MEDIA_BUS_FMT_RGB555_2X8_PADHI_LE 0x1004 +#define MEDIA_BUS_FMT_RGB565_1X16 0x1017 +#define MEDIA_BUS_FMT_BGR565_2X8_BE 0x1005 +#define MEDIA_BUS_FMT_BGR565_2X8_LE 0x1006 +#define MEDIA_BUS_FMT_RGB565_2X8_BE 0x1007 +#define MEDIA_BUS_FMT_RGB565_2X8_LE 0x1008 +#define MEDIA_BUS_FMT_RGB666_1X18 0x1009 +#define MEDIA_BUS_FMT_RGB666_2X9_BE 0x1025 +#define MEDIA_BUS_FMT_BGR666_1X18 0x1023 +#define MEDIA_BUS_FMT_RBG888_1X24 0x100e +#define MEDIA_BUS_FMT_RGB666_1X24_CPADHI 0x1015 +#define MEDIA_BUS_FMT_BGR666_1X24_CPADHI 0x1024 +#define MEDIA_BUS_FMT_RGB565_1X24_CPADHI 0x1022 +#define MEDIA_BUS_FMT_RGB666_1X7X3_SPWG 0x1010 +#define MEDIA_BUS_FMT_BGR888_1X24 0x1013 +#define MEDIA_BUS_FMT_BGR888_3X8 0x101b +#define MEDIA_BUS_FMT_GBR888_1X24 0x1014 +#define MEDIA_BUS_FMT_RGB888_1X24 0x100a +#define MEDIA_BUS_FMT_RGB888_2X12_BE 0x100b +#define MEDIA_BUS_FMT_RGB888_2X12_LE 0x100c +#define MEDIA_BUS_FMT_RGB888_3X8 0x101c +#define MEDIA_BUS_FMT_RGB888_3X8_DELTA 0x101d +#define MEDIA_BUS_FMT_RGB888_1X7X4_SPWG 0x1011 +#define MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA 0x1012 +#define MEDIA_BUS_FMT_RGB666_1X30_CPADLO 0x101e +#define MEDIA_BUS_FMT_RGB888_1X30_CPADLO 0x101f +#define MEDIA_BUS_FMT_ARGB8888_1X32 0x100d +#define MEDIA_BUS_FMT_RGB888_1X32_PADHI 0x100f +#define MEDIA_BUS_FMT_RGB101010_1X30 0x1018 +#define MEDIA_BUS_FMT_RGB666_1X36_CPADLO 0x1020 +#define MEDIA_BUS_FMT_RGB888_1X36_CPADLO 0x1021 +#define MEDIA_BUS_FMT_RGB121212_1X36 0x1019 +#define MEDIA_BUS_FMT_RGB161616_1X48 0x101a + +/* YUV (including grey) - next is 0x202f */ +#define MEDIA_BUS_FMT_Y8_1X8 0x2001 +#define MEDIA_BUS_FMT_UV8_1X8 0x2015 +#define MEDIA_BUS_FMT_UYVY8_1_5X8 0x2002 +#define MEDIA_BUS_FMT_VYUY8_1_5X8 0x2003 +#define MEDIA_BUS_FMT_YUYV8_1_5X8 0x2004 +#define MEDIA_BUS_FMT_YVYU8_1_5X8 0x2005 +#define MEDIA_BUS_FMT_UYVY8_2X8 0x2006 +#define MEDIA_BUS_FMT_VYUY8_2X8 0x2007 +#define MEDIA_BUS_FMT_YUYV8_2X8 0x2008 +#define MEDIA_BUS_FMT_YVYU8_2X8 0x2009 +#define MEDIA_BUS_FMT_Y10_1X10 0x200a +#define MEDIA_BUS_FMT_Y10_2X8_PADHI_LE 0x202c +#define MEDIA_BUS_FMT_UYVY10_2X10 0x2018 +#define MEDIA_BUS_FMT_VYUY10_2X10 0x2019 +#define MEDIA_BUS_FMT_YUYV10_2X10 0x200b +#define MEDIA_BUS_FMT_YVYU10_2X10 0x200c +#define MEDIA_BUS_FMT_Y12_1X12 0x2013 +#define MEDIA_BUS_FMT_UYVY12_2X12 0x201c +#define MEDIA_BUS_FMT_VYUY12_2X12 0x201d +#define MEDIA_BUS_FMT_YUYV12_2X12 0x201e +#define MEDIA_BUS_FMT_YVYU12_2X12 0x201f +#define MEDIA_BUS_FMT_Y14_1X14 0x202d +#define MEDIA_BUS_FMT_Y16_1X16 0x202e +#define MEDIA_BUS_FMT_UYVY8_1X16 0x200f +#define MEDIA_BUS_FMT_VYUY8_1X16 0x2010 +#define MEDIA_BUS_FMT_YUYV8_1X16 0x2011 +#define MEDIA_BUS_FMT_YVYU8_1X16 0x2012 +#define MEDIA_BUS_FMT_YDYUYDYV8_1X16 0x2014 +#define MEDIA_BUS_FMT_UYVY10_1X20 0x201a +#define MEDIA_BUS_FMT_VYUY10_1X20 0x201b +#define MEDIA_BUS_FMT_YUYV10_1X20 0x200d +#define MEDIA_BUS_FMT_YVYU10_1X20 0x200e +#define MEDIA_BUS_FMT_VUY8_1X24 0x2024 +#define MEDIA_BUS_FMT_YUV8_1X24 0x2025 +#define MEDIA_BUS_FMT_UYYVYY8_0_5X24 0x2026 +#define MEDIA_BUS_FMT_UYVY12_1X24 0x2020 +#define MEDIA_BUS_FMT_VYUY12_1X24 0x2021 +#define MEDIA_BUS_FMT_YUYV12_1X24 0x2022 +#define MEDIA_BUS_FMT_YVYU12_1X24 0x2023 +#define MEDIA_BUS_FMT_YUV10_1X30 0x2016 +#define MEDIA_BUS_FMT_UYYVYY10_0_5X30 0x2027 +#define MEDIA_BUS_FMT_AYUV8_1X32 0x2017 +#define MEDIA_BUS_FMT_UYYVYY12_0_5X36 0x2028 +#define MEDIA_BUS_FMT_YUV12_1X36 0x2029 +#define MEDIA_BUS_FMT_YUV16_1X48 0x202a +#define MEDIA_BUS_FMT_UYYVYY16_0_5X48 0x202b + +/* Bayer - next is 0x3021 */ +#define MEDIA_BUS_FMT_SBGGR8_1X8 0x3001 +#define MEDIA_BUS_FMT_SGBRG8_1X8 0x3013 +#define MEDIA_BUS_FMT_SGRBG8_1X8 0x3002 +#define MEDIA_BUS_FMT_SRGGB8_1X8 0x3014 +#define MEDIA_BUS_FMT_SBGGR10_ALAW8_1X8 0x3015 +#define MEDIA_BUS_FMT_SGBRG10_ALAW8_1X8 0x3016 +#define MEDIA_BUS_FMT_SGRBG10_ALAW8_1X8 0x3017 +#define MEDIA_BUS_FMT_SRGGB10_ALAW8_1X8 0x3018 +#define MEDIA_BUS_FMT_SBGGR10_DPCM8_1X8 0x300b +#define MEDIA_BUS_FMT_SGBRG10_DPCM8_1X8 0x300c +#define MEDIA_BUS_FMT_SGRBG10_DPCM8_1X8 0x3009 +#define MEDIA_BUS_FMT_SRGGB10_DPCM8_1X8 0x300d +#define MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_BE 0x3003 +#define MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_LE 0x3004 +#define MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_BE 0x3005 +#define MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_LE 0x3006 +#define MEDIA_BUS_FMT_SBGGR10_1X10 0x3007 +#define MEDIA_BUS_FMT_SGBRG10_1X10 0x300e +#define MEDIA_BUS_FMT_SGRBG10_1X10 0x300a +#define MEDIA_BUS_FMT_SRGGB10_1X10 0x300f +#define MEDIA_BUS_FMT_SBGGR12_1X12 0x3008 +#define MEDIA_BUS_FMT_SGBRG12_1X12 0x3010 +#define MEDIA_BUS_FMT_SGRBG12_1X12 0x3011 +#define MEDIA_BUS_FMT_SRGGB12_1X12 0x3012 +#define MEDIA_BUS_FMT_SBGGR14_1X14 0x3019 +#define MEDIA_BUS_FMT_SGBRG14_1X14 0x301a +#define MEDIA_BUS_FMT_SGRBG14_1X14 0x301b +#define MEDIA_BUS_FMT_SRGGB14_1X14 0x301c +#define MEDIA_BUS_FMT_SBGGR16_1X16 0x301d +#define MEDIA_BUS_FMT_SGBRG16_1X16 0x301e +#define MEDIA_BUS_FMT_SGRBG16_1X16 0x301f +#define MEDIA_BUS_FMT_SRGGB16_1X16 0x3020 + +/* JPEG compressed formats - next is 0x4002 */ +#define MEDIA_BUS_FMT_JPEG_1X8 0x4001 + +/* Vendor specific formats - next is 0x5002 */ + +/* S5C73M3 sensor specific interleaved UYVY and JPEG */ +#define MEDIA_BUS_FMT_S5C_UYVY_JPEG_1X8 0x5001 + +/* HSV - next is 0x6002 */ +#define MEDIA_BUS_FMT_AHSV8888_1X32 0x6001 + +/* + * This format should be used when the same driver handles + * both sides of the link and the bus format is a fixed + * metadata format that is not configurable from userspace. + * Width and height will be set to 0 for this format. + */ +#define MEDIA_BUS_FMT_METADATA_FIXED 0x7001 + +#endif /* __LINUX_MEDIA_BUS_FORMAT_H */
DO NOT MERGE. REFERENCE ONLY. Add binding for AMD/Xilinx Video Timing Controller and Test Pattern Generator. Copy media-bus-formats.h into dt-bindings/media to suplement TPG DT node. Signed-off-by: Anatoliy Klymenko <anatoliy.klymenko@amd.com> --- .../bindings/display/xlnx/xlnx,v-tpg.yaml | 87 ++++++++++ .../devicetree/bindings/display/xlnx/xlnx,vtc.yaml | 65 ++++++++ include/dt-bindings/media/media-bus-format.h | 177 +++++++++++++++++++++ 3 files changed, 329 insertions(+)