Am Freitag, den 06.12.2013, 14:14 +0100 schrieb Thierry Reding:

On Thu, Dec 05, 2013 at 07:28:06PM +0100, Denis Carikli wrote: [...]

...

diff --git a/drivers/staging/imx-drm/ipu-v3/ipu-dc.c b/drivers/staging/imx-drm/ipu-v3/ipu-dc.c

[...]

...

@@ -155,6 +156,8 @@ static int ipu_pixfmt_to_map(u32 fmt) return IPU_DC_MAP_BGR666; case V4L2_PIX_FMT_BGR24: return IPU_DC_MAP_BGR24;

• case V4L2_PIX_FMT_RGB666:

• return IPU_DC_MAP_RGB666;
  

Why is this DRM driver even using V4L2 pixel formats in the first place?

Because imx-drm is actually a misnomer. The i.MX IPU is a multifunction device, which as one part has the display controllers, but also camera interfaces and mem-to-mem scaler devices, which are hooked up via the V4L2 interface.

The generic IPU part, which is used for example for programming the DMA channels is using V4L2 pixel formats as a common base. We have patches to split this out and make this fact more visible. (The IPU core will be placed aside the Tegra host1x driver)

Regards, Lucas

On Thu, Dec 05, 2013 at 07:28:07PM +0100, Denis Carikli wrote: [...]

diff --git a/arch/arm/boot/dts/imx51-apf51dev.dts b/arch/arm/boot/dts/imx51-apf51dev.dts index f36a3aa..3b6de6a 100644 --- a/arch/arm/boot/dts/imx51-apf51dev.dts +++ b/arch/arm/boot/dts/imx51-apf51dev.dts @@ -19,7 +19,7 @@ display@di1 {

^^^^

I know this isn't introduced by your patch, but WTF???

Thierry

On Thursday, December 05, 2013 at 07:28:08 PM, Denis Carikli wrote:

If de-active and/or pixelclk-active properties were set in the display-timings DT node, they were not used.

Instead the data-enable and the pixel data clock polarity were hardcoded.

This change is needed for making the eukrea-cpuimx51 QVGA display work.

Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: driverdev-devel@linuxdriverproject.org Cc: Philipp Zabel p.zabel@pengutronix.de Cc: Sascha Hauer kernel@pengutronix.de Cc: Shawn Guo shawn.guo@linaro.org Cc: linux-arm-kernel@lists.infradead.org Cc: David Airlie airlied@linux.ie Cc: dri-devel@lists.freedesktop.org Cc: Eric Bénard eric@eukrea.com Signed-off-by: Denis Carikli denis@eukrea.com

---

ChangeLog v3->v4:

• The old patch was named "staging: imx-drm: ipuv3-crtc: don't harcode some

mode". - Reworked the patch entierly: we now takes the mode flags from the device tree.

ChangeLog v2->v3:

• Added some interested people in the Cc list.
• Ajusted the flags to match the changes in "drm: Add the lacking DRM_MODE_FLAG_* for matching the DISPLAY_FLAGS_*"

---

drivers/staging/imx-drm/imx-drm.h | 3 +++ drivers/staging/imx-drm/ipuv3-crtc.c | 11 +++++++++-- drivers/staging/imx-drm/parallel-display.c | 28 ++++++++++++++++++++++++++++ 3 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/imx-drm/imx-drm.h b/drivers/staging/imx-drm/imx-drm.h index ae90c9c..dfdc180 100644 --- a/drivers/staging/imx-drm/imx-drm.h +++ b/drivers/staging/imx-drm/imx-drm.h @@ -5,6 +5,9 @@

#define IPU_PIX_FMT_GBR24 v4l2_fourcc('G', 'B', 'R', '3')

+#define IMXDRM_MODE_FLAG_DE_HIGH (1<<0) +#define IMXDRM_MODE_FLAG_PIXDATA_POSEDGE (1<<1)

struct drm_crtc; struct drm_connector; struct drm_device; diff --git a/drivers/staging/imx-drm/ipuv3-crtc.c b/drivers/staging/imx-drm/ipuv3-crtc.c index ce6ba98..f3d2cae 100644 --- a/drivers/staging/imx-drm/ipuv3-crtc.c +++ b/drivers/staging/imx-drm/ipuv3-crtc.c @@ -156,8 +156,15 @@ static int ipu_crtc_mode_set(struct drm_crtc *crtc, if (mode->flags & DRM_MODE_FLAG_PVSYNC) sig_cfg.Vsync_pol = 1;

• sig_cfg.enable_pol = 1;
• sig_cfg.clk_pol = 1;

• /* Such flags are not availables in the DRM modes header,

• * and we don't want to export them to userspace.
  

• */
  

The multiline comment here is not correct.

• if (mode->private_flags & IMXDRM_MODE_FLAG_DE_HIGH)

• sig_cfg.enable_pol = 1;
  

• if (mode->private_flags & IMXDRM_MODE_FLAG_PIXDATA_POSEDGE)

• sig_cfg.clk_pol = 1;
  

• sig_cfg.width = mode->hdisplay; sig_cfg.height = mode->vdisplay; sig_cfg.pixel_fmt = out_pixel_fmt;

diff --git a/drivers/staging/imx-drm/parallel-display.c b/drivers/staging/imx-drm/parallel-display.c index bb71d6d..65d0c18 100644 --- a/drivers/staging/imx-drm/parallel-display.c +++ b/drivers/staging/imx-drm/parallel-display.c @@ -74,7 +74,35 @@ static int imx_pd_connector_get_modes(struct drm_connector *connector)

if (np) { struct drm_display_mode *mode = drm_mode_create(connector->dev);

• struct device_node *timings_np;
  

• struct device_node *mode_np;
  

• u32 val;
  

• of_get_drm_display_mode(np, &imxpd->mode, 0);

• /* Such flags are not availables in the DRM modes header,
  

•  * and we don't want to export them to userspace.
  

•  */
  

DTTO

• timings_np = of_get_child_by_name(np, "display-timings");
  

• if (timings_np) {
  

• 	/* get the display mode node */
  

• 	mode_np = of_parse_phandle(timings_np,
  

• 				   "native-mode", 0);
  

• 	if (!mode_np)
  

• 		mode_np = of_get_next_child(timings_np, NULL);
  

• 	/* set de-active to 1 if not set */
  

• 	of_property_read_u32(mode_np, "de-active", &val);
  

• 	if (!!val)
  

'if (val)' is enough.

• 		imxpd->mode.private_flags |=
  

• 			IMXDRM_MODE_FLAG_DE_HIGH;
  

• 	/* set pixelclk-active to 1 if not set */
  

• 	of_property_read_u32(mode_np, "pixelclk-active", &val);
  

• 	if (!!val)
  

DTTO, also please add {} around multiline expression.

• 		imxpd->mode.private_flags |=
  

• 			IMXDRM_MODE_FLAG_PIXDATA_POSEDGE;
  

• }
  

  drm_mode_copy(mode, &imxpd->mode); mode->type |= DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED, drm_mode_probed_add(connector, mode);

On Thursday, December 05, 2013 at 07:28:09 PM, Denis Carikli wrote:

Cc: Dan Carpenter dan.carpenter@oracle.com Cc: Rob Herring rob.herring@calxeda.com Cc: Pawel Moll pawel.moll@arm.com Cc: Mark Rutland mark.rutland@arm.com Cc: Stephen Warren swarren@wwwdotorg.org Cc: Ian Campbell ijc+devicetree@hellion.org.uk Cc: devicetree@vger.kernel.org Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: driverdev-devel@linuxdriverproject.org Cc: David Airlie airlied@linux.ie Cc: dri-devel@lists.freedesktop.org Cc: Sascha Hauer kernel@pengutronix.de Cc: Shawn Guo shawn.guo@linaro.org Cc: linux-arm-kernel@lists.infradead.org Cc: Eric Bénard eric@eukrea.com Signed-off-by: Denis Carikli denis@eukrea.com

---

ChangeLog v3->v5:

• Code clenaup.

ChangeLog v2->v3:

• Added some interested people in the Cc list.
• the lcd-supply is now called display-supply (not all display are LCD).
• The code and documentation was updated accordingly.
• regulator_is_enabled now guard the regulator enables/disables because regulator_disable does not check the regulator state.

---

.../bindings/staging/imx-drm/fsl-imx-drm.txt | 1 + drivers/staging/imx-drm/parallel-display.c | 22 ++++++++++++++++++++ 2 files changed, 23 insertions(+)

diff --git a/Documentation/devicetree/bindings/staging/imx-drm/fsl-imx-drm.txt b/Documentation/devicetree/bindings/staging/imx-drm/fsl-imx-drm.txt index 2d24425..4dd7ce5 100644 --- a/Documentation/devicetree/bindings/staging/imx-drm/fsl-imx-drm.txt +++ b/Documentation/devicetree/bindings/staging/imx-drm/fsl-imx-drm.txt @@ -28,6 +28,7 @@ Required properties:

• compatible: Should be "fsl,imx-parallel-display"
• crtc: the crtc this display is connected to, see below

Optional properties: +- display-supply : phandle to the regulator device tree node if needed.

• interface_pix_fmt: How this display is connected to the crtc. Currently supported types: "rgb24", "rgb565", "bgr666", "rgb666"
• edid: verbatim EDID data block describing attached display.

diff --git a/drivers/staging/imx-drm/parallel-display.c b/drivers/staging/imx-drm/parallel-display.c index 65d0c18..61c0aeb 100644 --- a/drivers/staging/imx-drm/parallel-display.c +++ b/drivers/staging/imx-drm/parallel-display.c @@ -22,6 +22,7 @@ #include <drm/drmP.h> #include <drm/drm_fb_helper.h> #include <drm/drm_crtc_helper.h> +#include <linux/regulator/consumer.h> #include <linux/videodev2.h>

#include "imx-drm.h" @@ -35,6 +36,7 @@ struct imx_parallel_display { struct drm_encoder encoder; struct imx_drm_encoder *imx_drm_encoder; struct device *dev;

• struct regulator *disp_reg; void *edid; int edid_len; u32 interface_pix_fmt;

@@ -141,6 +143,13 @@ static void imx_pd_encoder_prepare(struct drm_encoder *encoder) { struct imx_parallel_display *imxpd = enc_to_imxpd(encoder);

• if (!IS_ERR(imxpd->disp_reg) &&

•    !regulator_is_enabled(imxpd->disp_reg)) {
  

• if (regulator_enable(imxpd->disp_reg))
  

• 	dev_err(imxpd->dev,
  

• 		"Failed to enable regulator.\n");
  

I wonder, is this linebreak needed for this function call ? [...]