Re: [PATCH 7/7] drm/fsl-dcu: use mode flags for hsync/vsync pixelclk polarity

[Stefan Agner]

On 2016-02-03 15:18, Stefan Agner wrote:
> On 2016-02-03 06:00, Thierry Reding wrote:
>> On Wed, Jan 27, 2016 at 06:46:50PM -0800, Stefan Agner wrote:
>> [...]
>>> > diff --git a/drivers/gpu/drm/panel/panel-simple.c
>>> > b/drivers/gpu/drm/panel/panel-simple.c
>>> > index f97b73e..fa68b56 100644
>>> > --- a/drivers/gpu/drm/panel/panel-simple.c
>>> > +++ b/drivers/gpu/drm/panel/panel-simple.c
>>> > @@ -960,6 +960,8 @@ static const struct drm_display_mode
>>> > nec_nl4827hc19_05b_mode = {
>>> >  	.vsync_end = 272 + 2 + 4,
>>> >  	.vtotal = 272 + 2 + 4 + 2,
>>> >  	.vrefresh = 74,
>>> > +	.flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC |
>>> > +		 DISPLAY_FLAGS_PIXDATA_POSEDGE,
>>
>> It doesn't seem like these two types of flags should be mixed because
>> they overlap. DISPLAY_FLAGS_PIXDATA_POSEDGE has the same value as the
>> DRM_MODE_FLAG_CSYNC define.
> 
> There are other entries such as hannstar_hsd100pxn1_timing which make
> use of DISPLAY_FLAGS too, hence I did not bother further. Having a
> conflict is definitely not what we want.

Oh, just realized, hannstar_hsd100pxn1_timing is actually a different
type of struct, and for this struct the DISPALY_FLAGS make sense...

> 
>> The definition of the DISPLAY_FLAGS_PIXDATA_POSEDGE is also not very
>> clear to me. I don't think we have an equivalent DRM_MODE_FLAG_* but
>> we could add one if there's really a need.
> 
> E.g. assuming a parallel video signal, the pixel data signals could be
> changing on positive or negative edge relative to the clock signal. Most
> displays _sample_ the data on rising edge, hence controllers should
> normally drive data on falling edge.
> 
> However, as always, there are exceptions to the rule, and one of the
> exception is Freescales default display for the Tower evaluation board.
> It samples data on falling edge, hence the controller should drive data
> on rising edge...
> 
> Yeah I also did not found an equivalent in DRM_MODE_FLAG_*. I have here
> also other displays where we would need this flag. The display-timings
> binds and enum display_flags support it too, hence I guess we should
> have it here too.
> 
> I will split out this patch from the patchset (I already applied the
> rest), add another patch to add the flag, make use of the flag in this
> patch and resend it as v2.

I realized that after this, there are only two flags which are not yet
in DRM_MODE_FLAG: DE_LOW/DE_HIGH.

Thierry, what do you think, should I add these remaining two flags too?

--
Stefan