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

2014-09-28 Thread Rob Clark 
On Sat, Sep 27, 2014 at 4:12 PM, Boris BREZILLON
 wrote:
>> > +static int atmel_hlcdc_rgb_mode_valid(struct drm_connector *connector,
>> > + struct drm_display_mode *mode)
>> > +{
>> > +   return MODE_OK;
>> > +}
>>
>> your _mode_valid() should perhaps somehow check the constraints in
>> atmel_hlcdc_crtc_mode_set()?  This way invalid modes get filtered out
>> earlier..
>
> I'm not sure, the test done in atmel_hlcdc_crtc_mode_set are not
> connector related, but rather imposed by the display controller
> limitations.
> Anyway, let me know if you still think I should move those tests in the
> connector mode_valid implementation.


it gets a bit tricky if you have multiple crtc with different
constants which can be hooked to any connector (which I'm not sure if
that is the case here).. but for sure if you can eliminate modes that
cannot possibly work regardless of the crtc chosen, that is a good
thing.

So you may have to preserve the checks in the crtc.. but if you can
filter anything that is completely impossible in the connector, you
should.


BR,
-R

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

2014-09-27 Thread Boris BREZILLON 
Hi Rob,

On Fri, 26 Sep 2014 17:10:49 -0400
Rob Clark  wrote:

> On Mon, Sep 8, 2014 at 4:43 AM, Boris BREZILLON
>  wrote:
> > The Atmel HLCDC (HLCD Controller) IP available on some Atmel SoCs (i.e.
> > at91sam9n12, at91sam9x5 family or sama5d3 family) provides a display
> > controller device.
> >
> > This display controller supports at least one primary plane and might
> > provide several overlays and an hardware cursor depending on the IP
> > version.
> >
> > At the moment, this driver only implements an RGB connector to interface
> > with LCD panels, but support for other kind of external devices might be
> > added later.
> >
> > Signed-off-by: Boris BREZILLON 
> > Tested-by: Ludovic Desroches 
> 
> A few small comments below, but with those addressed it has my
> 
> Reviewed-by: Rob Clark 

Thanks for your review.

> 
> 
> > ---
> >  drivers/gpu/drm/Kconfig  |   2 +
> >  drivers/gpu/drm/Makefile |   1 +
> >  drivers/gpu/drm/atmel-hlcdc/Kconfig  |  13 +
> >  drivers/gpu/drm/atmel-hlcdc/Makefile |   7 +
> >  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c   | 286 
> >  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 488 +
> >  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h | 224 ++
> >  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_layer.c  | 656 ++
> >  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_layer.h  | 403 +++
> >  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c | 476 +
> >  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c  | 836 
> > +++
> >  11 files changed, 3392 insertions(+)
> >  create mode 100644 drivers/gpu/drm/atmel-hlcdc/Kconfig
> >  create mode 100644 drivers/gpu/drm/atmel-hlcdc/Makefile
> >  create mode 100644 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> >  create mode 100644 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> >  create mode 100644 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
> >  create mode 100644 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_layer.c
> >  create mode 100644 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_layer.h
> >  create mode 100644 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
> >  create mode 100644 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
> >
> 
> [snip]
> 
> > +
> > +/**
> > + * Atmel HLCDC plane rotation enum
> > + *
> > + * TODO: export DRM_ROTATE_XX macros defined by omap driver and use them
> > + * instead of defining this enum.
> > + */
> > +enum atmel_hlcdc_plane_rotation {
> > +   ATMEL_HLCDC_PLANE_NO_ROTATION,
> > +   ATMEL_HLCDC_PLANE_90DEG_ROTATION,
> > +   ATMEL_HLCDC_PLANE_180DEG_ROTATION,
> > +   ATMEL_HLCDC_PLANE_270DEG_ROTATION,
> > +};
> 
> DRM_ROTATE_* are already in drm_crtc.h
> 
> [snip]

Yep, I changed that, but won't be able to test until next week.

> 
> > +static int atmel_hlcdc_rgb_mode_valid(struct drm_connector *connector,
> > + struct drm_display_mode *mode)
> > +{
> > +   return MODE_OK;
> > +}
> 
> your _mode_valid() should perhaps somehow check the constraints in
> atmel_hlcdc_crtc_mode_set()?  This way invalid modes get filtered out
> earlier..

I'm not sure, the test done in atmel_hlcdc_crtc_mode_set are not
connector related, but rather imposed by the display controller
limitations.
Anyway, let me know if you still think I should move those tests in the
connector mode_valid implementation.

> 
> [snip]
> 
> > +static struct atmel_hlcdc_plane_properties *
> > +atmel_hlcdc_plane_create_properties(struct drm_device *dev)
> > +{
> > +   struct atmel_hlcdc_plane_properties *props;
> > +   const struct drm_prop_enum_list rotations[] = {
> > +   { ATMEL_HLCDC_PLANE_NO_ROTATION,   "rotate-0" },
> > +   { ATMEL_HLCDC_PLANE_90DEG_ROTATION,  "rotate-90" },
> > +   { ATMEL_HLCDC_PLANE_180DEG_ROTATION, "rotate-180" },
> > +   { ATMEL_HLCDC_PLANE_270DEG_ROTATION, "rotate-270" },
> > +   };
> > +
> 
> you could just use drm_mode_create_rotation_property() now
> 

Yep, I changed that too. Thanks for the tip.

Best Regards,

Boris

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

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

2014-09-26 Thread Rob Clark 
On Mon, Sep 8, 2014 at 4:43 AM, Boris BREZILLON
 wrote:
> The Atmel HLCDC (HLCD Controller) IP available on some Atmel SoCs (i.e.
> at91sam9n12, at91sam9x5 family or sama5d3 family) provides a display
> controller device.
>
> This display controller supports at least one primary plane and might
> provide several overlays and an hardware cursor depending on the IP
> version.
>
> At the moment, this driver only implements an RGB connector to interface
> with LCD panels, but support for other kind of external devices might be
> added later.
>
> Signed-off-by: Boris BREZILLON 
> Tested-by: Ludovic Desroches 

A few small comments below, but with those addressed it has my

Reviewed-by: Rob Clark 


> ---
>  drivers/gpu/drm/Kconfig  |   2 +
>  drivers/gpu/drm/Makefile |   1 +
>  drivers/gpu/drm/atmel-hlcdc/Kconfig  |  13 +
>  drivers/gpu/drm/atmel-hlcdc/Makefile |   7 +
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c   | 286 
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 488 +
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h | 224 ++
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_layer.c  | 656 ++
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_layer.h  | 403 +++
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c | 476 +
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c  | 836 
> +++
>  11 files changed, 3392 insertions(+)
>  create mode 100644 drivers/gpu/drm/atmel-hlcdc/Kconfig
>  create mode 100644 drivers/gpu/drm/atmel-hlcdc/Makefile
>  create mode 100644 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
>  create mode 100644 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
>  create mode 100644 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
>  create mode 100644 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_layer.c
>  create mode 100644 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_layer.h
>  create mode 100644 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
>  create mode 100644 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
>

[snip]

> +
> +/**
> + * Atmel HLCDC plane rotation enum
> + *
> + * TODO: export DRM_ROTATE_XX macros defined by omap driver and use them
> + * instead of defining this enum.
> + */
> +enum atmel_hlcdc_plane_rotation {
> +   ATMEL_HLCDC_PLANE_NO_ROTATION,
> +   ATMEL_HLCDC_PLANE_90DEG_ROTATION,
> +   ATMEL_HLCDC_PLANE_180DEG_ROTATION,
> +   ATMEL_HLCDC_PLANE_270DEG_ROTATION,
> +};

DRM_ROTATE_* are already in drm_crtc.h

[snip]

> +static int atmel_hlcdc_rgb_mode_valid(struct drm_connector *connector,
> + struct drm_display_mode *mode)
> +{
> +   return MODE_OK;
> +}

your _mode_valid() should perhaps somehow check the constraints in
atmel_hlcdc_crtc_mode_set()?  This way invalid modes get filtered out
earlier..

[snip]

> +static struct atmel_hlcdc_plane_properties *
> +atmel_hlcdc_plane_create_properties(struct drm_device *dev)
> +{
> +   struct atmel_hlcdc_plane_properties *props;
> +   const struct drm_prop_enum_list rotations[] = {
> +   { ATMEL_HLCDC_PLANE_NO_ROTATION,   "rotate-0" },
> +   { ATMEL_HLCDC_PLANE_90DEG_ROTATION,  "rotate-90" },
> +   { ATMEL_HLCDC_PLANE_180DEG_ROTATION, "rotate-180" },
> +   { ATMEL_HLCDC_PLANE_270DEG_ROTATION, "rotate-270" },
> +   };
> +

you could just use drm_mode_create_rotation_property() now


> +   props = devm_kzalloc(dev->dev, sizeof(*props), GFP_KERNEL);
> +   if (!props)
> +   return ERR_PTR(-ENOMEM);
> +
> +   props->alpha = drm_property_create_range(dev, 0, "alpha", 0, 255);
> +   if (!props->alpha)
> +   return ERR_PTR(-ENOMEM);
> +
> +   props->rotation = drm_property_create_enum(dev, 0, "rotation",
> +   rotations,
> +   ARRAY_SIZE(rotations));
> +   return props;
> +}
> +

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

2014-09-23 Thread Nicolas Ferre 
Hi Thierry,

On 23/09/2014 10:42, Thierry Reding :
> 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  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 >>> 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 000..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.

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.

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

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

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

2014-09-23 Thread Thierry Reding 
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  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  > > 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 000..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
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL:

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

2014-09-23 Thread Boris BREZILLON 
Hi Thierry,

On Tue, 23 Sep 2014 08:32:33 +0200
Thierry Reding  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  > 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 000..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

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

2014-09-23 Thread Thierry Reding 
On Mon, Sep 22, 2014 at 09:18:11PM +0200, Boris BREZILLON wrote:
> On Mon,  8 Sep 2014 10:43:36 +0200 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 000..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"?

Thierry
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL:

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

2014-09-22 Thread Boris BREZILLON 
On Mon,  8 Sep 2014 10:43:36 +0200
Boris BREZILLON  wrote:

> The Atmel HLCDC (HLCD Controller) IP available on some Atmel SoCs (i.e.
> at91sam9n12, at91sam9x5 family or sama5d3 family) provides a display
> controller device.
> 
> This display controller supports at least one primary plane and might
> provide several overlays and an hardware cursor depending on the IP
> version.
> 
> At the moment, this driver only implements an RGB connector to interface
> with LCD panels, but support for other kind of external devices might be
> added later.
> 
> Signed-off-by: Boris BREZILLON 
> Tested-by: Ludovic Desroches 
> ---
>  drivers/gpu/drm/Kconfig  |   2 +
>  drivers/gpu/drm/Makefile |   1 +
>  drivers/gpu/drm/atmel-hlcdc/Kconfig  |  13 +
>  drivers/gpu/drm/atmel-hlcdc/Makefile |   7 +
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c   | 286 
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 488 +
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h | 224 ++
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_layer.c  | 656 ++
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_layer.h  | 403 +++
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c | 476 +
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c  | 836 
> +++
>  11 files changed, 3392 insertions(+)
>  create mode 100644 drivers/gpu/drm/atmel-hlcdc/Kconfig
>  create mode 100644 drivers/gpu/drm/atmel-hlcdc/Makefile
>  create mode 100644 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
>  create mode 100644 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
>  create mode 100644 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
>  create mode 100644 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_layer.c
>  create mode 100644 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_layer.h
>  create mode 100644 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
>  create mode 100644 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index f512004..9183a78 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -184,6 +184,8 @@ source "drivers/gpu/drm/cirrus/Kconfig"
>  
>  source "drivers/gpu/drm/armada/Kconfig"
>  
> +source "drivers/gpu/drm/atmel-hlcdc/Kconfig"
> +
>  source "drivers/gpu/drm/rcar-du/Kconfig"
>  
>  source "drivers/gpu/drm/shmobile/Kconfig"
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index af9a609..07d388c 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -55,6 +55,7 @@ obj-$(CONFIG_DRM_GMA500) += gma500/
>  obj-$(CONFIG_DRM_UDL) += udl/
>  obj-$(CONFIG_DRM_AST) += ast/
>  obj-$(CONFIG_DRM_ARMADA) += armada/
> +obj-$(CONFIG_DRM_ATMEL_HLCDC)+= atmel-hlcdc/
>  obj-$(CONFIG_DRM_RCAR_DU) += rcar-du/
>  obj-$(CONFIG_DRM_SHMOBILE) +=shmobile/
>  obj-$(CONFIG_DRM_OMAP)   += omapdrm/
> diff --git a/drivers/gpu/drm/atmel-hlcdc/Kconfig 
> b/drivers/gpu/drm/atmel-hlcdc/Kconfig
> new file mode 100644
> index 000..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.

I'll fix that for the next version.

Best Regards,

Boris

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

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

2014-09-22 Thread Boris BREZILLON 
Hi David,

On Fri, 19 Sep 2014 15:10:02 +0200
David Herrmann  wrote:

> Hi
> 
> On Mon, Sep 8, 2014 at 10:43 AM, Boris BREZILLON
>  wrote:
> [snip]
> > +static int atmel_hlcdc_dc_drm_probe(struct platform_device *pdev)
> > +{
> > +   int ret;
> > +
> > +   ret = dma_set_coherent_mask(>dev, DMA_BIT_MASK(32));
> > +   if (ret)
> > +   return ret;
> > +
> > +   ret = drm_platform_init(_hlcdc_dc_driver, pdev);
> > +   if (ret)
> > +   return ret;
> 
> Please avoid any use of drm_platform_*(). Use drm_dev_alloc(),
> drm_dev_register() directly. See my response on
>   "[PATCH v3 1/5] drm/rockchip: Add basic drm driver":
> for details. Also have a look at the tegra driver how to do it.
> 
> > +
> > +   return 0;
> > +}
> > +
> > +static int atmel_hlcdc_dc_drm_remove(struct platform_device *pdev)
> > +{
> > +   drm_put_dev(platform_get_drvdata(pdev));
> 
> Same here: please use dev_dev_*() directly:
> 
> drm_dev_unregister(ddev);
> drm_dev_unref(ddev);


Sure, I'll change that.

Thanks,

Boris

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

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

2014-09-19 Thread David Herrmann 
Hi

On Mon, Sep 8, 2014 at 10:43 AM, Boris BREZILLON
 wrote:
[snip]
> +static int atmel_hlcdc_dc_drm_probe(struct platform_device *pdev)
> +{
> +   int ret;
> +
> +   ret = dma_set_coherent_mask(>dev, DMA_BIT_MASK(32));
> +   if (ret)
> +   return ret;
> +
> +   ret = drm_platform_init(_hlcdc_dc_driver, pdev);
> +   if (ret)
> +   return ret;

Please avoid any use of drm_platform_*(). Use drm_dev_alloc(),
drm_dev_register() directly. See my response on
  "[PATCH v3 1/5] drm/rockchip: Add basic drm driver":
for details. Also have a look at the tegra driver how to do it.

> +
> +   return 0;
> +}
> +
> +static int atmel_hlcdc_dc_drm_remove(struct platform_device *pdev)
> +{
> +   drm_put_dev(platform_get_drvdata(pdev));

Same here: please use dev_dev_*() directly:

drm_dev_unregister(ddev);
drm_dev_unref(ddev);


Thanks
David

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

2014-09-08 Thread Boris BREZILLON 
The Atmel HLCDC (HLCD Controller) IP available on some Atmel SoCs (i.e.
at91sam9n12, at91sam9x5 family or sama5d3 family) provides a display
controller device.

This display controller supports at least one primary plane and might
provide several overlays and an hardware cursor depending on the IP
version.

At the moment, this driver only implements an RGB connector to interface
with LCD panels, but support for other kind of external devices might be
added later.

Signed-off-by: Boris BREZILLON 
Tested-by: Ludovic Desroches 
---
 drivers/gpu/drm/Kconfig  |   2 +
 drivers/gpu/drm/Makefile |   1 +
 drivers/gpu/drm/atmel-hlcdc/Kconfig  |  13 +
 drivers/gpu/drm/atmel-hlcdc/Makefile |   7 +
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c   | 286 
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 488 +
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h | 224 ++
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_layer.c  | 656 ++
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_layer.h  | 403 +++
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c | 476 +
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c  | 836 +++
 11 files changed, 3392 insertions(+)
 create mode 100644 drivers/gpu/drm/atmel-hlcdc/Kconfig
 create mode 100644 drivers/gpu/drm/atmel-hlcdc/Makefile
 create mode 100644 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
 create mode 100644 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
 create mode 100644 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
 create mode 100644 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_layer.c
 create mode 100644 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_layer.h
 create mode 100644 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
 create mode 100644 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index f512004..9183a78 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -184,6 +184,8 @@ source "drivers/gpu/drm/cirrus/Kconfig"

 source "drivers/gpu/drm/armada/Kconfig"

+source "drivers/gpu/drm/atmel-hlcdc/Kconfig"
+
 source "drivers/gpu/drm/rcar-du/Kconfig"

 source "drivers/gpu/drm/shmobile/Kconfig"
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index af9a609..07d388c 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -55,6 +55,7 @@ obj-$(CONFIG_DRM_GMA500) += gma500/
 obj-$(CONFIG_DRM_UDL) += udl/
 obj-$(CONFIG_DRM_AST) += ast/
 obj-$(CONFIG_DRM_ARMADA) += armada/
+obj-$(CONFIG_DRM_ATMEL_HLCDC)  += atmel-hlcdc/
 obj-$(CONFIG_DRM_RCAR_DU) += rcar-du/
 obj-$(CONFIG_DRM_SHMOBILE) +=shmobile/
 obj-$(CONFIG_DRM_OMAP) += omapdrm/
diff --git a/drivers/gpu/drm/atmel-hlcdc/Kconfig 
b/drivers/gpu/drm/atmel-hlcdc/Kconfig
new file mode 100644
index 000..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
+   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
+   depends on OF
+   help
+ Choose this option if you have an ATMEL SoC with an HLCDC display
+ controller (i.e. at91sam9n12, at91sam9x5 family or sama5d3 family).
diff --git a/drivers/gpu/drm/atmel-hlcdc/Makefile 
b/drivers/gpu/drm/atmel-hlcdc/Makefile
new file mode 100644
index 000..10ae426
--- /dev/null
+++ b/drivers/gpu/drm/atmel-hlcdc/Makefile
@@ -0,0 +1,7 @@
+atmel-hlcdc-dc-y := atmel_hlcdc_crtc.o \
+   atmel_hlcdc_dc.o \
+   atmel_hlcdc_layer.o \
+   atmel_hlcdc_output.o \
+   atmel_hlcdc_plane.o
+
+obj-$(CONFIG_DRM_ATMEL_HLCDC)  += atmel-hlcdc-dc.o
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c 
b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
new file mode 100644
index 000..2186830
--- /dev/null
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
@@ -0,0 +1,286 @@
+/*
+ * Copyright (C) 2014 Traphandler
+ * Copyright (C) 2014 Free Electrons
+ *
+ * Author: Jean-Jacques Hiblot 
+ * Author: Boris BREZILLON 
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see .
+ */
+
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+
+#include 
+