[PATCH v14 04/10] imx-drm: use defines for clock polarity settings

2014-06-26 Thread Russell King - ARM Linux 
On Wed, Jun 25, 2014 at 11:44:47AM +0200, Denis Carikli wrote:
> On 06/25/2014 06:48 AM, Sascha Hauer wrote:
>>> +#define ENABLE_POL_LOW 0
>>> +#define ENABLE_POL_HIGH1
>>
>> Adding defines without a proper namespace (IPU_) outside a driver
>> private header file is not nice. Anyway, instead of adding the
>> defines ...
> Fixed in "imx-drm: use defines for clock polarity settings" and in  
> "imx-drm: Use drm_display_mode timings flags.".

Denis, can you send just this one updated patch, so I can update the
one I have here with this change.  Once you've done that, I'll send
the first four off to Greg.

Thanks.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

[PATCH v14 04/10] imx-drm: use defines for clock polarity settings

2014-06-25 Thread Sascha Hauer 
On Wed, Jun 25, 2014 at 09:43:27AM +0100, Russell King - ARM Linux wrote:
> On Wed, Jun 25, 2014 at 06:48:45AM +0200, Sascha Hauer wrote:
> > On Mon, Jun 16, 2014 at 12:11:18PM +0200, Denis Carikli wrote:
> > > +
> > >  /*
> > >   * Bitfield of Display Interface signal polarities.
> > >   */
> > > @@ -37,7 +43,7 @@ struct ipu_di_signal_cfg {
> > >   unsigned clksel_en:1;
> > >   unsigned clkidle_en:1;
> > >   unsigned data_pol:1;/* true = inverted */
> > > - unsigned clk_pol:1; /* true = rising edge */
> > > + unsigned clk_pol:1;
> > >   unsigned enable_pol:1;
> > >   unsigned Hsync_pol:1;   /* true = active high */
> > >   unsigned Vsync_pol:1;
> > 
> > ...can we rename the flags to more meaningful names instead?
> > 
> > unsigned clk_pol_rising_edge:1;
> > unsigned enable_pol_high:1;
> > unsigned hsync_active_high:1;
> > unsigned vsync_active_high:1;
> 
> Now look at patch 7, where these become tri-state:
> - don't change
> - rising edge/active high
> - falling edge/active low
> 
> So your suggestion is not a good idea.

Hm, you're right.

Still I think we should add a prefix to make the context of the flags
clear.

Sascha

-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |

[PATCH v14 04/10] imx-drm: use defines for clock polarity settings

2014-06-25 Thread Denis Carikli 
On 06/25/2014 06:48 AM, Sascha Hauer wrote:
>> +#define ENABLE_POL_LOW  0
>> +#define ENABLE_POL_HIGH 1
>
> Adding defines without a proper namespace (IPU_) outside a driver
> private header file is not nice. Anyway, instead of adding the
> defines ...
Fixed in "imx-drm: use defines for clock polarity settings" and in 
"imx-drm: Use drm_display_mode timings flags.".

Denis.

[PATCH v14 04/10] imx-drm: use defines for clock polarity settings

2014-06-25 Thread Russell King - ARM Linux 
On Wed, Jun 25, 2014 at 06:48:45AM +0200, Sascha Hauer wrote:
> On Mon, Jun 16, 2014 at 12:11:18PM +0200, Denis Carikli wrote:
> > +
> >  /*
> >   * Bitfield of Display Interface signal polarities.
> >   */
> > @@ -37,7 +43,7 @@ struct ipu_di_signal_cfg {
> > unsigned clksel_en:1;
> > unsigned clkidle_en:1;
> > unsigned data_pol:1;/* true = inverted */
> > -   unsigned clk_pol:1; /* true = rising edge */
> > +   unsigned clk_pol:1;
> > unsigned enable_pol:1;
> > unsigned Hsync_pol:1;   /* true = active high */
> > unsigned Vsync_pol:1;
> 
> ...can we rename the flags to more meaningful names instead?
> 
>   unsigned clk_pol_rising_edge:1;
>   unsigned enable_pol_high:1;
>   unsigned hsync_active_high:1;
>   unsigned vsync_active_high:1;

Now look at patch 7, where these become tri-state:
- don't change
- rising edge/active high
- falling edge/active low

So your suggestion is not a good idea.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

[PATCH v14 04/10] imx-drm: use defines for clock polarity settings

2014-06-25 Thread Sascha Hauer 
On Mon, Jun 16, 2014 at 12:11:18PM +0200, Denis Carikli wrote:
> Signed-off-by: Denis Carikli 
> ---
> ChangeLog v13->v14:
> - Rebased
> ChangeLog 12->v13:
> - No changes
> ChangeLog 11->v12:
> - Improved the define names to match the hardware:
>   ENABLE_POL is not a clock signal but instead an enable signal.
> 
> ChangeLog v9->v10:
> - New patch which was splitted out from:
>   "staging: imx-drm: Use de-active and pixelclk-active display-timings.".
> - Fixes many issues in "staging: imx-drm: Use de-active and pixelclk-active
>   display-timings.":
>   - More clear meaning of the polarity settings.
>   - The SET_CLK_POL and SET_DE_POL masks are not
> needed anymore.
> ---
>  drivers/gpu/ipu-v3/ipu-di.c  |4 ++--
>  drivers/staging/imx-drm/ipuv3-crtc.c |4 ++--
>  include/video/imx-ipu-v3.h   |8 +++-
>  3 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/ipu-v3/ipu-di.c b/drivers/gpu/ipu-v3/ipu-di.c
> index c490ba4..d00f357 100644
> --- a/drivers/gpu/ipu-v3/ipu-di.c
> +++ b/drivers/gpu/ipu-v3/ipu-di.c
> @@ -595,7 +595,7 @@ int ipu_di_init_sync_panel(struct ipu_di *di, struct 
> ipu_di_signal_cfg *sig)
>   }
>   }
>  
> - if (sig->clk_pol)
> + if (sig->clk_pol == CLK_POL_POSEDGE)
>   di_gen |= DI_GEN_POLARITY_DISP_CLK;
>  
>   ipu_di_write(di, di_gen, DI_GENERAL);
> @@ -606,7 +606,7 @@ int ipu_di_init_sync_panel(struct ipu_di *di, struct 
> ipu_di_signal_cfg *sig)
>   reg = ipu_di_read(di, DI_POL);
>   reg &= ~(DI_POL_DRDY_DATA_POLARITY | DI_POL_DRDY_POLARITY_15);
>  
> - if (sig->enable_pol)
> + if (sig->enable_pol == ENABLE_POL_HIGH)
>   reg |= DI_POL_DRDY_POLARITY_15;
>   if (sig->data_pol)
>   reg |= DI_POL_DRDY_DATA_POLARITY;
> diff --git a/drivers/staging/imx-drm/ipuv3-crtc.c 
> b/drivers/staging/imx-drm/ipuv3-crtc.c
> index 720868b..7fec438 100644
> --- a/drivers/staging/imx-drm/ipuv3-crtc.c
> +++ b/drivers/staging/imx-drm/ipuv3-crtc.c
> @@ -165,8 +165,8 @@ 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 = 0;
> + sig_cfg.enable_pol = ENABLE_POL_HIGH;
> + sig_cfg.clk_pol = CLK_POL_NEGEDGE;
>   sig_cfg.width = mode->hdisplay;
>   sig_cfg.height = mode->vdisplay;
>   sig_cfg.pixel_fmt = out_pixel_fmt;
> diff --git a/include/video/imx-ipu-v3.h b/include/video/imx-ipu-v3.h
> index 3e43e22..305 100644
> --- a/include/video/imx-ipu-v3.h
> +++ b/include/video/imx-ipu-v3.h
> @@ -27,6 +27,12 @@ enum ipuv3_type {
>  
>  #define IPU_PIX_FMT_GBR24v4l2_fourcc('G', 'B', 'R', '3')
>  
> +#define CLK_POL_NEGEDGE  0
> +#define CLK_POL_POSEDGE  1
> +
> +#define ENABLE_POL_LOW   0
> +#define ENABLE_POL_HIGH  1

Adding defines without a proper namespace (IPU_) outside a driver
private header file is not nice. Anyway, instead of adding the
defines ...

> +
>  /*
>   * Bitfield of Display Interface signal polarities.
>   */
> @@ -37,7 +43,7 @@ struct ipu_di_signal_cfg {
>   unsigned clksel_en:1;
>   unsigned clkidle_en:1;
>   unsigned data_pol:1;/* true = inverted */
> - unsigned clk_pol:1; /* true = rising edge */
> + unsigned clk_pol:1;
>   unsigned enable_pol:1;
>   unsigned Hsync_pol:1;   /* true = active high */
>   unsigned Vsync_pol:1;

...can we rename the flags to more meaningful names instead?

unsigned clk_pol_rising_edge:1;
unsigned enable_pol_high:1;
unsigned hsync_active_high:1;
unsigned vsync_active_high:1;

Sascha

-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |

[PATCH v14 04/10] imx-drm: use defines for clock polarity settings

2014-06-24 Thread Denis Carikli 
On 06/24/2014 05:13 PM, Russell King - ARM Linux wrote:
[...]
> If you'd like to send me better commit messages for
> these patches, I'll add them to what I already have:

>   imx-drm: use defines for clock polarity settings
The comment of the clk_pol field of the ipu_di_signal_cfg struct was 
inverted.
Instead of merely inverting the comment, the values of clk_pol were defined.

>   imx-drm: add RGB666 support for parallel display.
This permits to drive parallel displays that expect the RGB666 color format.
>
> It may also be worth describing the RGB666 format in the commit message
> for:
>
>   v4l2: add new V4L2_PIX_FMT_RGB666 pixel format.
The RGB666 color format encodes 6 bits for each color(red, green and 
blue), linearly.
It looks like this in memory:
017
RRGGBB

Denis.

[PATCH v14 04/10] imx-drm: use defines for clock polarity settings

2014-06-24 Thread Russell King - ARM Linux 
On Tue, Jun 24, 2014 at 06:25:19PM +0200, Denis Carikli wrote:
> On 06/24/2014 05:13 PM, Russell King - ARM Linux wrote:
> [...]
>> If you'd like to send me better commit messages for
>> these patches, I'll add them to what I already have:
>
>>  imx-drm: use defines for clock polarity settings
> The comment of the clk_pol field of the ipu_di_signal_cfg struct was  
> inverted.
> Instead of merely inverting the comment, the values of clk_pol were defined.

s/inverting/fixing/

>
>>  imx-drm: add RGB666 support for parallel display.
> This permits to drive parallel displays that expect the RGB666 color format.

This allows imx-drm to drive ...

>>
>> It may also be worth describing the RGB666 format in the commit message
>> for:
>>
>>  v4l2: add new V4L2_PIX_FMT_RGB666 pixel format.
> The RGB666 color format encodes 6 bits for each color(red, green and  
> blue), linearly.
> It looks like this in memory:
> 017
> RRGGBB

Thanks! I've tweaked them very slightly as detailed above so they read a
bit better.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

[PATCH v14 04/10] imx-drm: use defines for clock polarity settings

2014-06-24 Thread Russell King - ARM Linux 
On Mon, Jun 16, 2014 at 12:11:18PM +0200, Denis Carikli wrote:
> Signed-off-by: Denis Carikli 

It would be nice to have a little more explanation in the commit messages
for these patches.  If you'd like to send me better commit messages for
these patches, I'll add them to what I already have:

imx-drm: use defines for clock polarity settings

imx-drm: add RGB666 support for parallel display.

It may also be worth describing the RGB666 format in the commit message
for:

v4l2: add new V4L2_PIX_FMT_RGB666 pixel format.

And... getting some more acks for these patches would be very useful,
I think I'd like to see Sascha's ack for these... Sascha?

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

[PATCH v14 04/10] imx-drm: use defines for clock polarity settings

2014-06-16 Thread Denis Carikli 
Signed-off-by: Denis Carikli 
---
ChangeLog v13->v14:
- Rebased
ChangeLog 12->v13:
- No changes
ChangeLog 11->v12:
- Improved the define names to match the hardware:
  ENABLE_POL is not a clock signal but instead an enable signal.

ChangeLog v9->v10:
- New patch which was splitted out from:
  "staging: imx-drm: Use de-active and pixelclk-active display-timings.".
- Fixes many issues in "staging: imx-drm: Use de-active and pixelclk-active
  display-timings.":
  - More clear meaning of the polarity settings.
  - The SET_CLK_POL and SET_DE_POL masks are not
needed anymore.
---
 drivers/gpu/ipu-v3/ipu-di.c  |4 ++--
 drivers/staging/imx-drm/ipuv3-crtc.c |4 ++--
 include/video/imx-ipu-v3.h   |8 +++-
 3 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/ipu-v3/ipu-di.c b/drivers/gpu/ipu-v3/ipu-di.c
index c490ba4..d00f357 100644
--- a/drivers/gpu/ipu-v3/ipu-di.c
+++ b/drivers/gpu/ipu-v3/ipu-di.c
@@ -595,7 +595,7 @@ int ipu_di_init_sync_panel(struct ipu_di *di, struct 
ipu_di_signal_cfg *sig)
}
}

-   if (sig->clk_pol)
+   if (sig->clk_pol == CLK_POL_POSEDGE)
di_gen |= DI_GEN_POLARITY_DISP_CLK;

ipu_di_write(di, di_gen, DI_GENERAL);
@@ -606,7 +606,7 @@ int ipu_di_init_sync_panel(struct ipu_di *di, struct 
ipu_di_signal_cfg *sig)
reg = ipu_di_read(di, DI_POL);
reg &= ~(DI_POL_DRDY_DATA_POLARITY | DI_POL_DRDY_POLARITY_15);

-   if (sig->enable_pol)
+   if (sig->enable_pol == ENABLE_POL_HIGH)
reg |= DI_POL_DRDY_POLARITY_15;
if (sig->data_pol)
reg |= DI_POL_DRDY_DATA_POLARITY;
diff --git a/drivers/staging/imx-drm/ipuv3-crtc.c 
b/drivers/staging/imx-drm/ipuv3-crtc.c
index 720868b..7fec438 100644
--- a/drivers/staging/imx-drm/ipuv3-crtc.c
+++ b/drivers/staging/imx-drm/ipuv3-crtc.c
@@ -165,8 +165,8 @@ 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 = 0;
+   sig_cfg.enable_pol = ENABLE_POL_HIGH;
+   sig_cfg.clk_pol = CLK_POL_NEGEDGE;
sig_cfg.width = mode->hdisplay;
sig_cfg.height = mode->vdisplay;
sig_cfg.pixel_fmt = out_pixel_fmt;
diff --git a/include/video/imx-ipu-v3.h b/include/video/imx-ipu-v3.h
index 3e43e22..305 100644
--- a/include/video/imx-ipu-v3.h
+++ b/include/video/imx-ipu-v3.h
@@ -27,6 +27,12 @@ enum ipuv3_type {

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

+#define CLK_POL_NEGEDGE0
+#define CLK_POL_POSEDGE1
+
+#define ENABLE_POL_LOW 0
+#define ENABLE_POL_HIGH1
+
 /*
  * Bitfield of Display Interface signal polarities.
  */
@@ -37,7 +43,7 @@ struct ipu_di_signal_cfg {
unsigned clksel_en:1;
unsigned clkidle_en:1;
unsigned data_pol:1;/* true = inverted */
-   unsigned clk_pol:1; /* true = rising edge */
+   unsigned clk_pol:1;
unsigned enable_pol:1;
unsigned Hsync_pol:1;   /* true = active high */
unsigned Vsync_pol:1;
-- 
1.7.9.5