Messages in this thread | | | Date | Sun, 20 Aug 2023 22:16:12 +0530 | Subject | Re: [PATCH v3 1/4] dt-bindings: media: Add bindings for Imagination E5010 JPEG Encoder driver | From | Devarsh Thakkar <> |
| |
Hi Krzysztof,
Thanks for the review.
On 19/08/23 19:30, Krzysztof Kozlowski wrote: > On 16/08/2023 17:22, Devarsh Thakkar wrote: >> Add dt-bindings for Imagination E5010 JPEG Encoder driver which is >> implemented as stateful V4L2 M2M driver. >> >> Co-developed-by: David Huang <d-huang@ti.com> >> Signed-off-by: David Huang <d-huang@ti.com> >> Signed-off-by: Devarsh Thakkar <devarsht@ti.com> >> --- >> V2: No change >> V3: >> - Add vendor specific compatible >> - Fix commit title and message >> - Update reg names >> - Update clocks to 1 >> - Fix dts example with proper naming > > I do not see improvements in the subject. >
Sorry, Will correct in v4.
>> >> .../bindings/media/img,e5010-jpeg-enc.yaml | 81 +++++++++++++++++++ >> MAINTAINERS | 5 ++ >> 2 files changed, 86 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/media/img,e5010-jpeg-enc.yaml >> >> diff --git a/Documentation/devicetree/bindings/media/img,e5010-jpeg-enc.yaml b/Documentation/devicetree/bindings/media/img,e5010-jpeg-enc.yaml >> new file mode 100644 >> index 000000000000..d105a71ee2ea >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/media/img,e5010-jpeg-enc.yaml >> @@ -0,0 +1,81 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/media/img,e5010-jpeg-enc.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Imagination E5010 JPEG Encoder >> + >> +maintainers: >> + - Devarsh Thakkar <devarsht@ti.com> >> + >> +description: | >> + The E5010 is a JPEG encoder from Imagination Technologies implemented on >> + TI's AM62A SoC. It is capable of real time encoding of YUV420 and YUV422 >> + inputs to JPEG and M-JPEG. It supports baseline JPEG Encoding up to >> + 8Kx8K resolution. >> + >> +properties: >> + compatible: >> + oneOf: >> + - items: >> + - const: ti,e5010-jpeg-enc > > TI did not make e5010. Use SoC-based compatible. > >> + - const: img,e5010-jpeg-enc >> + - const: img,e5010-jpeg-enc > > img,e5010-jpeg-enc cannot be compatible with img,e5010-jpeg-enc. It does > not make sense. I guess I did not expect you are going to use what you > wrote in v1 directly... I thought it is just about syntax. >
Sorry but I did not understand this fully, the possible compatibles are:
1) "ti,am62a-jpeg-enc", "img,e5010-jpeg-enc" or 2) "img,e5010-jpeg-enc"
anything else will not comply during dtbs_check as shown below :
For e.g. If I use below compatible : "img,e5010-jpeg-enc", "img,e5010-jpeg-enc"
and run dtbs_check, it throw below error :
make CHECK_DTBS=y DT_SCHEMA_FILES=media/img,e5010-jpeg-enc.yaml ti/k3-am62a7-sk.dtb LINT Documentation/devicetree/bindings CHKDT Documentation/devicetree/bindings/processed-schema.json SCHEMA Documentation/devicetree/bindings/processed-schema.json DTC_CHK arch/arm64/boot/dts/ti/k3-am62a7-sk.dtb /home/devarsht/ti/linux-next2/linux-next/arch/arm64/boot/dts/ti/k3-am62a7-sk.dtb: jpeg-encoder@fd20000: compatible: 'oneOf' conditional failed, one must be fixed: ['img,e5010-jpeg-enc', 'img,e5010-jpeg-enc'] is too long 'ti,am62a-jpeg-enc' was expected From schema: /home/devarsht/ti/linux-next2/linux-next/Documentation/devicetree/bindings/media/img,e5010-jpeg-enc.yaml
Similarly, if I use below compatible :
"ti,am62a-jpeg-enc", It throw below error :
make CHECK_DTBS=y DT_SCHEMA_FILES=media/img,e5010-jpeg-enc.yaml ti/k3-am62a7-sk.dtb DTC_CHK arch/arm64/boot/dts/ti/k3-am62a7-sk.dtb /home/devarsht/ti/linux-next2/linux-next/arch/arm64/boot/dts/ti/k3-am62a7-sk.dtb: jpeg-encoder@fd20000: compatible: 'oneOf' conditional failed, one must be fixed: ['ti,am62a-jpeg-enc'] is too short 'img,e5010-jpeg-enc' was expected From schema: /home/devarsht/ti/linux-next2/linux-next/Documentation/devicetree/bindings/media/img,e5010-jpeg-enc.yaml
But If I use either 1) or 2) it does not throw any error. Please let me know if I missed to understand your point.
>> + >> + reg: >> + items: >> + - description: The E5010 core register region >> + - description: The E5010 mmu register region >> + >> + reg-names: >> + items: >> + - const: core >> + - const: mmu >> + >> + power-domains: >> + maxItems: 1 >> + >> + resets: >> + maxItems: 1 >> + >> + clocks: >> + maxItems: 1 >> + >> + clock-names: >> + items: >> + - const: core_clk > > Drop _clk or even drop clock-names. It brings little benefit for > one-entry list. >
Agreed, will drop clock-names altogether.
Regards Devarsh
> > Best regards, > Krzysztof >
| |