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

[Nicolas Ferre]

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 at 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 at collabora.com>
>>>
>>> The SoC features a Hantro G1 compatible video decoder.
>>>
>>> Cc: Ezequiel Garcia <ezequiel at collabora.com>
>>> Cc: Philipp Zabel <p.zabel at pengutronix.de>
>>> Cc: linux-media at vger.kernel.org
>>> Cc: linux-rockchip at lists.infradead.org
>>> Signed-off-by: Emil Velikov <emil.velikov at 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