| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 8 Aug 2023 15:50:23 +0530
From: Devarsh Thakkar <devarsht@...com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
<mchehab@...nel.org>, <robh+dt@...nel.org>,
<krzysztof.kozlowski+dt@...aro.org>, <conor+dt@...nel.org>,
<linux-media@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>
CC: <praneeth@...com>, <nm@...com>, <vigneshr@...com>,
<a-bhatia1@...com>, <j-luthra@...com>, <b-brnich@...com>,
<detheridge@...com>, <p-mantena@...com>, <vijayp@...com>
Subject: Re: [PATCH] dt-bindings: media: Add bindings for Imagination E5010
JPEG Encoder driver
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@...com>
>>> Signed-off-by: David Huang <d-huang@...com>
>>
>> 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@...com>
>>> ---
>>> .../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@...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:
>>> + 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@...0000 {
>>
>> 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
>>
Powered by blists - more mailing lists