| Message ID | 1402913484-25910-4-git-send-email-denis@eukrea.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jun 16, 2014 at 12:11:18PM +0200, Denis Carikli wrote:
> Signed-off-by: Denis Carikli <denis@eukrea.com>
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?
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: 0 17 RRRRRRGGGGGGBBBBBB Denis.
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: > 0 17 > RRRRRRGGGGGGBBBBBB Thanks! I've tweaked them very slightly as detailed above so they read a bit better.
On Mon, Jun 16, 2014 at 12:11:18PM +0200, Denis Carikli wrote: > Signed-off-by: Denis Carikli <denis@eukrea.com> > --- > 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..8888305 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_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
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.
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.
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
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_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, 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.
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..8888305 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_NEGEDGE 0 +#define CLK_POL_POSEDGE 1 + +#define ENABLE_POL_LOW 0 +#define ENABLE_POL_HIGH 1 + /* * 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;
Signed-off-by: Denis Carikli <denis@eukrea.com> --- 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(-)