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

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2313 bytes --]

On Tue, Sep 23, 2014 at 09:24:36AM +0200, Boris BREZILLON wrote:
> Hi Thierry,
> 
> On Tue, 23 Sep 2014 08:32:33 +0200
> Thierry Reding <thierry.reding@gmail.com> wrote:
> 
> > On Mon, Sep 22, 2014 at 09:18:11PM +0200, Boris BREZILLON wrote:
> > > On Mon,  8 Sep 2014 10:43:36 +0200 Boris BREZILLON <boris.brezillon@free-electrons.com> wrote:
> > [...]
> > > > 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...
> > > 
> > > 
> > > > +	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.

> 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.

> 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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]