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

[Boris BREZILLON <boris.brezillon@free-electrons.com>]

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.

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

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

Best Regards,

Boris

[1]https://lkml.org/lkml/2014/7/22/300

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com