Re: [PATCH 7/7] ARM: dts: at91: sama5d4: add vdec0 component

From: Nicolas Ferre <nicolas.ferre@microchip.com>
To: Emil Velikov <emil.l.velikov@gmail.com>,
	Ezequiel Garcia <ezequiel@collabora.com>
Cc: <kernel@collabora.com>, Philipp Zabel <p.zabel@pengutronix.de>,
	<linux-media@vger.kernel.org>,
	linux-rockchip <linux-rockchip@lists.infradead.org>,
	Nicolas Dufresne <nicolas.dufresne@collabora.com>,
	Benjamin Gaignard <benjamin.gaignard@collabora.com>
Subject: Re: [PATCH 7/7] ARM: dts: at91: sama5d4: add vdec0 component
Date: Mon, 8 Mar 2021 14:21:11 +0100	[thread overview]
Message-ID: <2e921a53-def3-ed3a-6240-b81ea3ddf946@microchip.com> (raw)
In-Reply-To: <CACvgo51peuKsuqx-NwZSWU4Ys1q5MuXb=BRx7GLo3tkWH+vb0w@mail.gmail.com>

Hi Emil,

So nice to see this support! Thank you so much for handling that.

Little comments below...

On 08/03/2021 at 14:07, Emil Velikov wrote:
>   Hi Ezequiel,
> 
> Thanks for the prompt reply
> 
> On Sat, 6 Mar 2021 at 11:25, Ezequiel Garcia <ezequiel@collabora.com> wrote:
>>
>> (adding another Nicolas)
>>
>> Hi Emil,
>>
>> Thanks a lot for the patch.
>>
>> On Fri, 2021-03-05 at 18:39 +0000, Emil Velikov wrote:
>>> From: Emil Velikov <emil.velikov@collabora.com>
>>>
>>> The SoC features a Hantro G1 compatible video decoder.
>>>
>>> Cc: Ezequiel Garcia <ezequiel@collabora.com>
>>> Cc: Philipp Zabel <p.zabel@pengutronix.de>
>>> Cc: linux-media@vger.kernel.org
>>> Cc: linux-rockchip@lists.infradead.org
>>> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
>>> ---
>>>   arch/arm/boot/dts/sama5d4.dtsi                |   9 ++
>>
>> Usually device-tree changes go to their own patch.
>>
>> The new compatible string "atmel,sama5d4-vdec" needs

Nitpicking: I would use "microchip,sama5d4-vdec". We tend to use the 
microchip name for new DT bidings and compatibility strings.

>> an update to the DT bindings, see Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
>> for an example.
>>
>> DT bindings change should also go to a separate
>> patch, see Documentation/devicetree/bindings/submitting-patches.rst.
>>
> Will do. Thanks
> 
>>>   arch/arm/configs/sama5_defconfig              |   3 +
>>
>> Better if config changes are a separate patch.
>>
>> But also, the driver is in staging and we haven't enabled
>> in ARM64 defconfig. Let's leave that decision to the machine
>> maintainer to decide.
>>
> Makes sense. Will keep it separate patch for completeness sake, with
> explicit note.
> ARM/Microchip (AT91) SoC maintainers will be in CC list and will defer
> the decision to them.

I'm fine with having a "staging" component. Maybe add the hantro vdec as 
a module instead.

Best regards,
-- 
Nicolas Ferre