[U-Boot] [PATCH] video: mxsfb: Support data-enable and pixclock polarity

[Michael Nazzareno Trimarchi]

Hi

On Thu, Jun 21, 2018 at 9:17 AM, Anatolij Gustschin <agust at denx.de> wrote:
> Hi Michael,
>
> On Wed, 20 Jun 2018 22:55:55 +0200
> Michael Trimarchi michael at amarulasolutions.com wrote:
>
>> Add extra feature to support data-enable and clock-polarity
>>
>> Signed-off-by: Michael Trimarchi <michael at amarulasolutions.com>
>> ---
>>  drivers/video/mxsfb.c | 21 ++++++++++++++++++---
>>  1 file changed, 18 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/video/mxsfb.c b/drivers/video/mxsfb.c
>> index 02fde05..4e3e3d7 100644
>> --- a/drivers/video/mxsfb.c
>> +++ b/drivers/video/mxsfb.c
>> @@ -20,6 +20,9 @@
>>
>>  #define      PS2KHZ(ps)      (1000000000UL / (ps))
>>
>> +#define FB_SYNC_DATA_ENABLE_HIGH_ACT 0x80000000
>> +#define FB_SYNC_DOTCLK_FALLING_ACT   0x40000000
>
> shouldn't these be defined in drivers/video/videomodes.h? I assume
> that some external code will set these flags in the mode struct, so
> these must be known (and included) elsewhere.

No, those are what we call host->flags and they can not land there.
Those mapping
is already used for those special host implementation and we can not
diverge from linux.
I found two different way to work with panel, video and display

>
>> @@ -50,7 +53,7 @@ static void mxs_lcd_init(GraphicDevice *panel,
>>                       struct ctfb_res_modes *mode, int bpp)
>>  {
>>       struct mxs_lcdif_regs *regs = (struct mxs_lcdif_regs *)MXS_LCDIF_BASE;
>> -     uint32_t word_len = 0, bus_width = 0;
>> +     uint32_t word_len = 0, bus_width = 0, flags = 0;
>>       uint8_t valid_data = 0;
>>
>>       /* Kick in the LCDIF clock */
>> @@ -94,10 +97,22 @@ static void mxs_lcd_init(GraphicDevice *panel,
>>       writel((mode->yres << LCDIF_TRANSFER_COUNT_V_COUNT_OFFSET) | mode->xres,
>>               &regs->hw_lcdif_transfer_count);
>>
>> -     writel(LCDIF_VDCTRL0_ENABLE_PRESENT | LCDIF_VDCTRL0_ENABLE_POL |
>> +     if (mode->sync & FB_SYNC_HOR_HIGH_ACT)
>> +             flags |= LCDIF_VDCTRL0_VSYNC_POL;
>
> you check for HSYNC high flag, but set VSYNC bit?
>
>> +     if (mode->sync & FB_SYNC_VERT_HIGH_ACT)
>> +             flags |= LCDIF_VDCTRL0_HSYNC_POL;
>
> you check for VSYNC high flag, but set HSYNC bit. Please fix.
>

Yes right

>> +
>> +     if (mode->sync & FB_SYNC_DATA_ENABLE_HIGH_ACT)
>> +             flags |= LCDIF_VDCTRL0_ENABLE_POL;
>
> previously data enable polarity bit was always set in the ctrl register,
> now it will be only set if requested by new flag in mode sync. This
> is going to break current users, I'm afraid. Who is going to request
> this for all driver users? Will it be supplied via environment mode
> variable "video=ctfb..." ? This should be compatible with existing
> video mode environments then (which we cannot easily update everywhere).

I have notice this one, but the previsius implementation was wrong.
I'm planning to move
this one in DM_VIDEO anyway later

Michael

>
> --
> Anatolij



-- 
| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |