RE: [PATCH 1/2] drm/adi: axi-hdmi-tx: Add support for AXI HDMI TX IP core

[Togorean, Bogdan]

Thank you Sam for your review,
I'll send now V2 implementing all your remarks.

Best regards,
Bogdan

> 
> Hi Bogdan
> 
> On Mon, Oct 05, 2020 at 05:12:08PM +0300, Bogdan Togorean wrote:
> > From: Lars-Peter Clausen <lars@xxxxxxxxxx>
> >
> > The AXI HDMI HDL driver is the driver for the HDL graphics core which is
> > used on various FPGA designs. It's mostly used to interface with the
> > ADV7511 driver on some Zynq boards (e.g. ZC702 & ZedBoard).
> >
> > Link: https://wiki.analog.com/resources/tools-software/linux-drivers/drm/hdl-
> axi-hdmi
> > Link: https://wiki.analog.com/resources/fpga/docs/axi_hdmi_tx
> 
> Thanks for submitting the driver - a few high level comments after
> browsing the driver:
> 
> - Use drmm_mode_config_init() to utilize new cleanup
> - Look at other uses of drm_driver - there is macros that makes this
>   much simpler / smaller
> - Use devm_drm_dev_alloc() to allocate axi_hdmi_tx_private
>   To do so embed drm_device in axi_hdmi_tx_private - which is the way to
>   do it today
> - Do not use ddev->dev_private, it is deprecated
> - Use dev_err_probe() when you risk to see a PROBE_DEFER
> - In all include blocks sort the include alphabetically
> - Use the new interface to drm_bridge_attach() - where display driver
>   creates the connector
> - See if the Kconfig selects can be trimmed - the framebuffer releated
>   selects looks wrong (others get it wrong too)
> - Check if you can use the simple encoder - see
>   drm_simple_encoder_init()
> 
> If this is a simple one plane, one crtc display driver then it should
> use the drm_simple_* support. Or the changelog should explain why not.
> 
> We want the drivers as simple as we can - and they shall use as much of
> the helper infrastructure as they can.
> 
> We continue to develop the helper infrastructure so it is expected that
> there is some lacking behind as is the case here.
> 
>         Sam
>