Hi Rob,
On Fri, 26 Sep 2014 17:10:49 -0400
Rob Clark <robdclark@xxxxxxxxx> wrote:
> On Mon, Sep 8, 2014 at 4:43 AM, Boris BREZILLON
> <boris.brezillon@xxxxxxxxxxxxxxxxxx> 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 <boris.brezillon@xxxxxxxxxxxxxxxxxx>
> > Tested-by: Ludovic Desroches <ludovic.desroches@xxxxxxxxx>
>
> A few small comments below, but with those addressed it has my
>
> Reviewed-by: Rob Clark <robdclark@xxxxxxxxx>
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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
