Date: Fri, 1 Mar 2024 18:06:11 +0100
From: Sebastian Fricke <sebastian.fricke@...labora.com>
To: Devarsh Thakkar <devarsht@...com>
Cc: mchehab@...nel.org, robh@...nel.org, krzysztof.kozlowski+dt@...aro.org,
	conor+dt@...nel.org, hverkuil-cisco@...all.nl,
	linux-media@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, benjamin.gaignard@...labora.com,
	laurent.pinchart@...asonboard.com, praneeth@...com, nm@...com,
	vigneshr@...com, a-bhatia1@...com, j-luthra@...com, b-brnich@...com,
	detheridge@...com, p-mantena@...com, vijayp@...com,
	andrzej.p@...labora.com, nicolas@...fresne.ca, afd@...com,
	milkfafa@...il.com
Subject: Re: [PATCH v6 1/3] media: dt-bindings: Add Imagination E5010 JPEG
 Encoder

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@...com>
>>>>> Signed-off-by: David Huang <d-huang@...com>
>>>>> Signed-off-by: Devarsh Thakkar <devarsht@...com>
>>>>> Reviewed-by: Rob Herring <robh@...nel.org>
>>>>
>>>> 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@...labora.com`
>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.

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@kernel.org/
>>> [1] : https://lore.kernel.org/all/20240228141140.3530612-4-devarsht@ti.com/
>>>
>>>
>>> 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@...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,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@...0000 {
>>>>> +          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@...com>
>>>>> +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
>>>>>

Powered by blists - more mailing lists