Hi Krzysztof, On 27/07/23 19:58, Devarsh Thakkar wrote: > Hi Krzysztof, > > Thanks for the quick review. > > On 26/07/23 22:03, Krzysztof Kozlowski wrote: >> On 26/07/2023 18:26, 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@xxxxxx> >>> Signed-off-by: David Huang <d-huang@xxxxxx> >> >> A nit, subject: drop second/last, redundant "bindings for". The >> "dt-bindings" prefix is already stating that these are bindings. >> >> Drop also "driver". Bindings are for hardware, not drivers. >> >> Prefix starts with media and then dt-bindings. >> > > Agreed. >> >>> Signed-off-by: Devarsh Thakkar <devarsht@xxxxxx> >>> --- >>> .../bindings/media/img,e5010-jpeg-enc.yaml | 79 +++++++++++++++++++ >>> MAINTAINERS | 5 ++ >>> 2 files changed, 84 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..0060373eace7 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/media/img,e5010-jpeg-enc.yaml >>> @@ -0,0 +1,79 @@ >>> +# 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@xxxxxx> >>> + >>> +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: >>> + const: img,e5010-jpeg-enc >> >> Your description suggests that this is part of TI SoC. Pretty often >> licensed blocks cannot be used on their own and need some >> customizations. Are you sure your block does not need any customization >> thus no dedicated compatible is needed? >> > > There is a wrapper for interfacing this core with TI SoC, I will recheck this > interfacing but I believe nothing changes from programming perspective as > there is 1-to-1 maintained between the clocks and signals w.r.t actual E5010 > core. > Just to add to above, on a second thought we think it would be better to still have a separate compatible for TI as you suggested (since we have a wrapper) so that it allows any customization needed for future. So compatible enum would look like : oneOf: - items: - const: ti,e5010-jpeg-enc - const: img,e5010-jpeg-enc - const: img,e5010-jpeg-enc Thanks for the suggestion. Regards Devarsh >>> + >>> + reg: >>> + items: >>> + - description: The E5010 main register region >>> + - description: The E5010 mmu register region >>> + >>> + reg-names: >>> + items: >>> + - const: regjasper >>> + - const: regmmu >>> + >> >> Drop reg from both >> > > Agreed. > >>> + power-domains: >>> + maxItems: 1 >>> + >>> + resets: >>> + maxItems: 1 >>> + >>> + clocks: >>> + minItems: 1 >>> + maxItems: 2 >> >> You need to specify the items. Also, no variable number of clocks. Why >> would they vary if block is strictly defined? >> > > Agreed, I believe this version of E5010 core only supports single clock, so we > can get rid of maxItems: 2. > >>> + >>> + clock-names: >>> + minItems: 1 >>> + maxItems: 2 >> >> Instead list the names. >> > > Agreed. > >>> + >>> + interrupts: >>> + maxItems: 1 >>> + >>> +required: >>> + - compatible >>> + - reg >>> + - reg-names >>> + - interrupts >>> + - clocks >>> + - clock-names >>> + - power-domains >>> + >>> +additionalProperties: false >>> + >>> +examples: >>> + - | >>> + #include <dt-bindings/soc/ti,sci_pm_domain.h> >>> + #include <dt-bindings/interrupt-controller/arm-gic.h> >>> + #include <dt-bindings/interrupt-controller/irq.h> >>> + >>> + cbass_main { >> >> That's some weird name. Probably you meant soc. Anyway, underscores are >> not allowed. > > Yes, I think I can put soc. cbass_main is specific to TI (soc interconnect bus). > >> >>> + #address-cells = <2>; >>> + #size-cells = <2>; >>> + e5010: e5010@fd20000 { >> >> Node names should be generic. See also an explanation and list of >> examples (not exhaustive) in DT specification: >> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation >> > > Yes, video-codec is the nearest one, but this is not really a codec as it only > supports encoding, is it fine to name node as jpeg-enc ? > >> >> Drop the label. >> > > Agreed. > > Best Regards, > Devarsh > >>> + compatible = "img,e5010-jpeg-enc"; >>> + reg = <0x00 0xfd20000 0x00 0x100>, >>> + <0x00 0xfd20200 0x00 0x200>; >>> + reg-names = "regjasper", "regmmu"; >>> + clocks = <&k3_clks 201 0>; >>> + clock-names = "core_clk"; >>> + power-domains = <&k3_pds 201 TI_SCI_PD_EXCLUSIVE>; >>> + interrupts = <GIC_SPI 98 IRQ_TYPE_LEVEL_HIGH>; >>> + }; >>> + }; >> >> >> Best regards, >> Krzysztof >>
