Re: [PATCH V2 3/9] drm/panel: Add driver for exynos_dp based panels

From: Thierry Reding <thierry.reding@gmail.com>
To: Ajay kumar <ajaynumb@gmail.com>
Cc: "linux-samsung-soc@vger.kernel.org"
	<linux-samsung-soc@vger.kernel.org>,
	Sean Paul <seanpaul@google.com>, sunil joshi <joshi@samsung.com>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	Prashanth G <prashanth.g@samsung.com>,
	Ajay Kumar <ajaykumar.rs@samsung.com>
Subject: Re: [PATCH V2 3/9] drm/panel: Add driver for exynos_dp based panels
Date: Tue, 22 Apr 2014 17:16:06 +0200	[thread overview]
Message-ID: <20140422151604.GA30048@ulmo> (raw)
In-Reply-To: <CAEC9eQPnmx+EDk3FK7V+gbf6eP9SXyBHQVOxHUGHG82iyK2zbA@mail.gmail.com>

[-- Attachment #1.1: Type: text/plain, Size: 2963 bytes --]

On Tue, Apr 22, 2014 at 08:23:03PM +0530, Ajay kumar wrote:
> Hi Thierry,
> 
> 
> On Tue, Apr 22, 2014 at 1:56 PM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> > On Tue, Apr 22, 2014 at 04:09:12AM +0530, Ajay Kumar wrote:
> >> This patch adds a simple driver to handle all the LCD and LED
> >> powerup/down routines needed to support eDP/eDP-LVDS panels
> >> supported on exynos boards.
> >>
> >> The LCD and LED units are usually powered up via regulators,
> >> and almost on all boards, we will have a BL_EN pin to enable/
> >> disable the backlight. Sometimes, we can have LCD_EN switches
> >> as well. The routines in this driver can be used to control
> >> panel power sequence on such boards.
> >>
> >> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
> >> ---
> >> Changes since V1:
> >>       Added routine for post_disable, removed few unwanted headers.
> >>       Refactored a lot of code.
> >>
> >>  .../devicetree/bindings/panel/exynos-dp-panel.txt  |   45 ++++
> >>  drivers/gpu/drm/panel/Kconfig                      |    9 +
> >>  drivers/gpu/drm/panel/Makefile                     |    1 +
> >>  drivers/gpu/drm/panel/panel-exynos-dp.c            |  251 ++++++++++++++++++++
> >>  4 files changed, 306 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/panel/exynos-dp-panel.txt
> >>  create mode 100644 drivers/gpu/drm/panel/panel-exynos-dp.c
> >
> > What this patch does is in no way Exynos specific. It is also not eDP
> > specific.
> >
> Right. This is not at all writing into any "exynos" specific register,
> but can't this be just a placeholder for all the panel controls
> which can serve boards which use exynos_dp?

No, it shouldn't. Like I said, if I have a panel that happens to be used
on an Exynos board but I use it on a different board, then I don't want
to have to know that the panel might be supported by Exynos so that I
know which driver to use.

So ideally there should be one driver per panel. panel-simple was
introduced because of the five panels that I had access to at the time,
five panels could be supported using the same code.

> > This conflates panel drivers with board drivers in a strange way. Panel
> > drivers should be for *panels*, not for a given SoC or specific outputs
> > on that SoC.
> >
> Right. But for that matter, even the "panel-simple" driver is doing the same,
> which is just a placeholder for "generic" panel controls which serves
> multiple boards.

panel-simple is meant for panels that require only the parameters that
are specified for those. Anything that needs more is by definition no
longer "simple".

And the difference here is that panel-simple is a true wildcard, but it
isn't specific to one SoC. And the name doesn't imply that either. Also
each panel is still identified by the specific compatible value, which
makes it easier to find out which driver supports the panel.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel