linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Sam Ravnborg <sam@ravnborg.org>
To: Bogdan Togorean <bogdan.togorean@analog.com>
Cc: dri-devel@lists.freedesktop.org, devicetree@vger.kernel.org,
	Thomas Zimmermann <tzimmermann@suse.de>,
	Mike Looijmans <mike.looijmans@topic.nl>,
	linux-kernel@vger.kernel.org, David Airlie <airlied@linux.ie>,
	Rob Herring <robh+dt@kernel.org>,
	Alexandru Ardelean <alexandru.ardelean@analog.com>
Subject: Re: [PATCH 1/2] drm/adi: axi-hdmi-tx: Add support for AXI HDMI TX IP core
Date: Fri, 16 Oct 2020 16:55:00 +0200	[thread overview]
Message-ID: <20201016145500.GA1325536@ravnborg.org> (raw)
In-Reply-To: <20201005141226.180655-1-bogdan.togorean@analog.com>

Hi Bogdan

On Mon, Oct 05, 2020 at 05:12:08PM +0300, Bogdan Togorean wrote:
> From: Lars-Peter Clausen <lars@metafoo.de>
> 
> 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



  parent reply	other threads:[~2020-10-16 14:55 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-05 14:12 [PATCH 1/2] drm/adi: axi-hdmi-tx: Add support for AXI HDMI TX IP core Bogdan Togorean
2020-10-05 14:12 ` [PATCH 2/2] drm: dt-bindings: adi: axi-hdmi-tx: Add DT bindings for axi-hdmi-tx Bogdan Togorean
2020-10-06 22:00   ` Rob Herring
2020-10-16 14:55 ` Sam Ravnborg [this message]
2020-10-23  8:51   ` [PATCH 1/2] drm/adi: axi-hdmi-tx: Add support for AXI HDMI TX IP core Togorean, Bogdan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201016145500.GA1325536@ravnborg.org \
    --to=sam@ravnborg.org \
    --cc=airlied@linux.ie \
    --cc=alexandru.ardelean@analog.com \
    --cc=bogdan.togorean@analog.com \
    --cc=devicetree@vger.kernel.org \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mike.looijmans@topic.nl \
    --cc=robh+dt@kernel.org \
    --cc=tzimmermann@suse.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).