Re: [PATCH v5 05/11] drm: add Atmel HLCDC Display Controller support

Hi Thierry,

On 23/09/2014 10:42, Thierry Reding :

On Tue, Sep 23, 2014 at 09:24:36AM +0200, Boris BREZILLON wrote:

quoted

Hi Thierry,

On Tue, 23 Sep 2014 08:32:33 +0200
Thierry Reding [off-list ref] wrote:

quoted

On Mon, Sep 22, 2014 at 09:18:11PM +0200, Boris BREZILLON wrote:

quoted

On Mon,  8 Sep 2014 10:43:36 +0200 Boris BREZILLON [off-list ref] wrote:

[...]

quoted

quoted

diff --git a/drivers/gpu/drm/atmel-hlcdc/Kconfig b/drivers/gpu/drm/atmel-hlcdc/Kconfig
new file mode 100644
index 0000000..6d0d785
--- /dev/null
+++ b/drivers/gpu/drm/atmel-hlcdc/Kconfig

@@ -0,0 +1,13 @@
+config DRM_ATMEL_HLCDC
+	tristate "DRM Support for ATMEL HLCDC Display Controller"
+	depends on DRM && OF && MFD_ATMEL_HLCDC && COMMON_CLK

I forgot to remove MFD_ATMEL_HLCDC dependency which is now selected...

quoted

+	select DRM_GEM_CMA_HELPER
+	select DRM_KMS_HELPER
+	select DRM_KMS_FB_HELPER
+	select DRM_KMS_CMA_HELPER
+	select DRM_PANEL
+	select MFD_ATMEL_HLCDC

here.

Maybe this was discussed earlier, but can you remind me why this was
done? The changelog says it was to "simplify", but there's been some
recent discussion that select should never be used for symbols that
are user-visible because that can lead to issues.

What were the problems having this as "depends on"?

I had several people complaining about the complexity introduced by
this dependency scheme: if one wants to select the HLCDC KMS driver, he
has to select the HLCDC MFD driver first, which is kind of tricky.

That's not at all tricky in my opinion. The MFD provides an LCDC as one
of the devices, so requiring it to be selected before you can enable any
of the subdrivers is very natural. It's also how the majority of MFD
devices work.

Well, that's me that suggested to Boris to change this: I had very hard
time to enable the LCD driver when first moving to this new HLCD driver.
Usually, when I want to enable a LCD driver, I go to the "Graphics
support" menu.

quoted

The MFD_ATMEL_HLCDC symbol is now hidden (see patch 1) which should
solve part of the issue.

I think that's wrong but I don't care enough to object.

Ok, me, I kind of like it: the MFD is something convenient to aggregate
two drivers using the same register set (LCD and Backlight) but has no
real "feature" interest in my opinion: it ease things.

quoted

Now that I have your attention :-), could you review this series [1] ?
The HLCDC KMS depends on those changes (which you and Laurent
suggested).

My attention span tends to be very short these days. No promises. =)

Thierry

Thanks, bye,
-- 
Nicolas Ferre