LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Jerome Brunet <jbrunet@baylibre.com>
To: Maxime Jourdan <maxi.jourdan@wanadoo.fr>
Cc: Neil Armstrong <narmstrong@baylibre.com>,
	Kevin Hilman <khilman@baylibre.com>,
	linux-arm-kernel@lists.infradead.org,
	linux-amlogic <linux-amlogic@lists.infradead.org>,
	linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
	devicetree@vger.kernel.org
Subject: Re: [PATCH 4/4] drm/meson: convert to the new canvas module
Date: Thu, 02 Aug 2018 15:01:41 +0200
Message-ID: <843e41de9fa7d5f87309ff4e0db2ed84a1153a4c.camel@baylibre.com> (raw)
In-Reply-To: <CAHStOZ6B-R7=5utGqb49OsFBsUsMVsn=WMrzNK8Wk-zcgbnPuA@mail.gmail.com>

On Thu, 2018-08-02 at 14:34 +0200, Maxime Jourdan wrote:
> Hi Jerome,
> 
> 2018-08-02 10:39 GMT+02:00 Jerome Brunet <jbrunet@baylibre.com>:
> > I looks like the consumer of your 'canvas' devices must know how the canvas
> > device is organized internally. Maybe something better can be done ?
> > 
> > Your canvas driver could provide a consumer API, for example:
> > meson_canvas_get(): to translate for struct device_node to whatever abstract
> > pointer you would need.
> > meson_canvas_alloc(), setup(), etc ...
> > 
> > ... This is just adding a bit of indirection but it would help hide the plumbing
> > of your canvas driver from the consumers (and repeat this code in each). This
> > might be usefull if you ever to make this canvas driver evolve.
> 
> Overall the inner workings are hidden as there is an ops struct
> instead of public functions.

What I don't like is precisely that you need to expose this 'struct ops' to the
consumer. I would prefer an API for this (it can be a 1:1 mapping). The canvas
should remain some abstract object you get from DT.

IMO, this is the same as a reset, a syscon or whatever other phandle you get
from DT. The consumer should not have to 'care' how it works (how data are
organized), it should just use it.

Whatever you need to do to deal with your canvas phandle should (IMO) reside in
your soc/amlogic/meson-canvas.c, and not be spread in the consumers.

> 
> I agree that the "fetch the node" boilerplate code could be put behind
> a helper, but at the same time this code helps remind the developer
> that there needs to be a canvas node in the dts, and that it has to be
> linked in your own device node.

This is why we have a DT documentation.

And, as far as I understand amlogic's display and decoder stuff, you won't get
very far w/o a canvas, so 'the developer' will be reminded fairly quickly ;)

> 
> I would like to keep it that way if that is okay with you.

It's just my opinion, I'm not the one merging it ... :P



  reply index

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-01 18:51 [PATCH 0/4] soc: amlogic: add meson-canvas driver Maxime Jourdan
2018-08-01 18:51 ` [PATCH 1/4] " Maxime Jourdan
2018-08-02  8:38   ` Neil Armstrong
2018-08-02  8:52   ` Neil Armstrong
2018-08-02 13:14     ` Maxime Jourdan
2018-08-03 14:14   ` Yixun Lan
2018-08-03 21:47     ` Maxime Jourdan
2018-08-01 18:51 ` [PATCH 2/4] dt-bindings: soc: amlogic: add meson-canvas documentation Maxime Jourdan
2018-08-01 18:51 ` [PATCH 3/4] ARM64: dts: meson-gx: add dmcbus and canvas nodes Maxime Jourdan
2018-08-03 13:50   ` Yixun Lan
2018-08-04 20:02     ` Maxime Jourdan
2018-08-07  1:29       ` Yixun Lan
2018-08-01 18:51 ` [PATCH 4/4] drm/meson: convert to the new canvas module Maxime Jourdan
2018-08-02  8:39   ` Jerome Brunet
2018-08-02 12:34     ` Maxime Jourdan
2018-08-02 13:01       ` Jerome Brunet [this message]
2018-08-02 13:09         ` Maxime Jourdan
     [not found]   ` <5b6cc316.1c69fb81.682d3.1216@mx.google.com>
2018-08-10  6:35     ` Maxime Jourdan
2018-08-10  7:49       ` Neil Armstrong

Reply instructions:

You may reply publically 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=843e41de9fa7d5f87309ff4e0db2ed84a1153a4c.camel@baylibre.com \
    --to=jbrunet@baylibre.com \
    --cc=devicetree@vger.kernel.org \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=khilman@baylibre.com \
    --cc=linux-amlogic@lists.infradead.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=maxi.jourdan@wanadoo.fr \
    --cc=narmstrong@baylibre.com \
    /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

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org linux-kernel@archiver.kernel.org
	public-inbox-index lkml


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox