Re: [PATCH v6 1/3] media: dt-bindings: Add Imagination E5010 JPEG Encoder

[Devarsh Thakkar]

Hi Sebastian,

On 01/03/24 22:36, Sebastian Fricke wrote:
> Hey Devarsh,
> 
> On 01.03.2024 22:02, Devarsh Thakkar wrote:
>> Hi Sebastian,
>>
>> On 29/02/24 19:00, Sebastian Fricke wrote:
>>> Hey Devarsh,
>>>
>>> On 29.02.2024 16:50, Devarsh Thakkar wrote:
>>>> Hi Sebastian,
>>>>
>>>> Thanks for the review.
>>>>
>>>> On 29/02/24 15:56, Sebastian Fricke wrote:
>>>>> Hey Devarsh,
>>>>>
>>>>> On 28.02.2024 19:41, Devarsh Thakkar wrote:
>>>>>> Add dt-bindings for Imagination E5010 JPEG Encoder [1] which is implemented
>>>>>> as stateful V4L2 M2M driver.
>>>>>>
>>>>>> The device supports baseline encoding with two different quantization
>>>>>> tables and compression ratio as demanded.
>>>>>>
>>>>>> Minimum resolution supported is 64x64 and Maximum resolution supported is
>>>>>> 8192x8192.
>>>>>>
>>>>>> [1]:  AM62A TRM (Section 7.6 is for JPEG Encoder)
>>>>>> Link: https://www.ti.com/lit/pdf/spruj16
>>>>>>
>>>>>> Co-developed-by: David Huang <d-huang@xxxxxx>
>>>>>> Signed-off-by: David Huang <d-huang@xxxxxx>
>>>>>> Signed-off-by: Devarsh Thakkar <devarsht@xxxxxx>
>>>>>> Reviewed-by: Rob Herring <robh@xxxxxxxxxx>
>>>>>
>>>>> hmmm when did Rob give his reviewed by on this patch? (As this is not a
>>>>> DT binding I find that odd)
>>>>
>>>> [PATCH v6 1/3] media: dt-bindings: Add Imagination E5010 JPEG Encoder : This
>>>> is indeed the dt-binding patch. Also As shared in version history it is at V4
>>>> where Rob Herring added a Reviewed-By as seen here [0]
>>>>
>>>>> And where is the Reviewed by tag from Benjamin that he provided on V5?
>>>>>
>>>>
>>>> As captured in patch version history here [1] I thought to remove the
>>>> Reviewed-By since the Reviewed-By tag was on V5 and with V6 the driver got
>>>> updated with some changes to handle reported sparse warnings and so I have
>>>> asked Benjamin to check the range-diff and help with a quick review again if
>>>> possible.
>>>>
>>>> Kindly let me know if I missed something or anything needs to be done from
>>>> my end.
>>>
>>> Yes thanks I was a bit too swift to write here, sorry for the noise.
>>> We'll have a look.
>>>
>>
>> Sorry for the back and forth, but on the hindsight and re-looking at the
>> kernel patch guidelines [0] they suggest that Reviewed-By tag should only be
>> removed if substantial changes were made in further revisions.
>>
>> So looks to me in-fact it was a mistake on my part to remove the Reviewed-by
>> considering the change made in the following patch series was not a
>> substantial one as seen in the range-diff [1].
>>
>> Considering this, just wanted to check with you if it's possible for you to
>> consider the Reviewed-by tag :
>> `Reviewed-by: Benjamin Gaignard <benjamin.gaignard@xxxxxxxxxxxxx`
>> if it helps consolidate things faster to get this series in given we are close
>> to final RC's ?
> 
> Yup I think we can keep it as the changes are very minor. Otherwise the
> series is pretty much good to go, I'll prepare the PR asap.
> 

Thanks, so I assume this could still make it to 6.9 merge window if it gets
pulled in before 6.8 rc8 ? Also do you think it would be possible to pull in
https://lore.kernel.org/all/20240305160529.4152865-1-devarsht@xxxxxx/ as here
too nothing much has changed w.r.t initial posting apart from commit message.

Regards
Devarsh

> Greetings,
> Sebastian
> 
>>
>> [0]:
>> https://docs.kernel.org/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes:~:text=changed%20substantially
>>
>> [1]: https://gist.github.com/devarsht/c89180ac2b0d2814614f2b59d0705c19
>>
>> Regards
>> Devarsh
>>
>>>
>>> Greetings,
>>> Sebastian
>>>
>>>>
>>>> [0] :
>>>> https://lore.kernel.org/all/170716378412.295212.11603162949482063011.robh@xxxxxxxxxx/
>>>> [1] : https://lore.kernel.org/all/20240228141140.3530612-4-devarsht@xxxxxx/
>>>>
>>>>
>>>> Regards
>>>> Devarsh
>>>>>> ---
>>>>>> V2: No change
>>>>>> V3:
>>>>>> - Add vendor specific compatible
>>>>>> - Update reg names
>>>>>> - Update clocks to 1
>>>>>> - Fix dts example with proper naming
>>>>>> V4:
>>>>>> - Use ti-specific compatible ti,am62a-jpeg-enc as secondary one
>>>>>> - Update commit message and title
>>>>>> - Remove clock-names as only single clock
>>>>>> V5:
>>>>>> - Add Reviewed-By tag
>>>>>> V6:
>>>>>> - No change
>>>>>>
>>>>>> .../bindings/media/img,e5010-jpeg-enc.yaml    | 75 +++++++++++++++++++
>>>>>> MAINTAINERS                                   |  5 ++
>>>>>> 2 files changed, 80 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..085020cb9e61
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/media/img,e5010-jpeg-enc.yaml
>>>>>> @@ -0,0 +1,75 @@
>>>>>> +# 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:
>>>>>> +    oneOf:
>>>>>> +      - items:
>>>>>> +          - const: ti,am62a-jpeg-enc
>>>>>> +          - const: img,e5010-jpeg-enc
>>>>>> +      - const: img,e5010-jpeg-enc
>>>>>> +
>>>>>> +  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
>>>>>> +
>>>>>> +  interrupts:
>>>>>> +    maxItems: 1
>>>>>> +
>>>>>> +required:
>>>>>> +  - compatible
>>>>>> +  - reg
>>>>>> +  - reg-names
>>>>>> +  - interrupts
>>>>>> +  - clocks
>>>>>> +
>>>>>> +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>
>>>>>> +
>>>>>> +    soc {
>>>>>> +      #address-cells = <2>;
>>>>>> +      #size-cells = <2>;
>>>>>> +      jpeg-encoder@fd20000 {
>>>>>> +          compatible = "img,e5010-jpeg-enc";
>>>>>> +          reg = <0x00 0xfd20000 0x00 0x100>,
>>>>>> +                <0x00 0xfd20200 0x00 0x200>;
>>>>>> +          reg-names = "core", "mmu";
>>>>>> +          clocks = <&k3_clks 201 0>;
>>>>>> +          power-domains = <&k3_pds 201 TI_SCI_PD_EXCLUSIVE>;
>>>>>> +          interrupts = <GIC_SPI 98 IRQ_TYPE_LEVEL_HIGH>;
>>>>>> +      };
>>>>>> +    };
>>>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>>>> index e1475ca38ff2..6b34ee8d92b5 100644
>>>>>> --- a/MAINTAINERS
>>>>>> +++ b/MAINTAINERS
>>>>>> @@ -10572,6 +10572,11 @@ S:    Maintained
>>>>>> F:    Documentation/devicetree/bindings/auxdisplay/img,ascii-lcd.yaml
>>>>>> F:    drivers/auxdisplay/img-ascii-lcd.c
>>>>>>
>>>>>> +IMGTEC JPEG ENCODER DRIVER
>>>>>> +M:    Devarsh Thakkar <devarsht@xxxxxx>
>>>>>> +S:    Supported
>>>>>> +F:    Documentation/devicetree/bindings/media/img,e5010-jpeg-enc.yaml
>>>>>> +
>>>>>> IMGTEC IR DECODER DRIVER
>>>>>> S:    Orphan
>>>>>> F:    drivers/media/rc/img-ir/
>>>>>> -- 
>>>>>> 2.39.1
>>>>>>