Re: [V2 1/7] video: mmp: rb swap setting update for new LCD driver

[Daniel Drake <dsd@laptop.org>]

On Mon, Jun 24, 2013 at 8:23 PM, jett zhou <jett.zhou@gmail.com> wrote:
>> What if I'm working with a display that doesn't need or want RB
>> swapping? Lets say I am working with format PIXFMT_RGB565, and running
>> your patch. dmafetch_set_fmt() gets called, and fmt_to_reg() sets
>> rbswap to 1.
>> This means that dmafetch_set_fmt() writes a '1' into the appropriate
>> RB-swapping bit in the LCD_PN_CTRL0 register, and this triggers the
>> "DMA input" swapping that you mentioned. But I never asked for RB
>> swapping...
>
> Yes, if you configure it as PIXFMT_RGB565, it will set rbswap in "DMA
> input" part.
> So, for your case, you need to use PIXFMT_BGR565 instead of PIXFMT_RGB565.

So let me get this straight. I have a display that wants RGB565
format, no RB swapping. I don't do anything special in link_config to
affect any swapping.

After your patch, I must request BGR565 format in order to get RGB565?
That sounds backwards to me.

>> Your comment above suggests that this RB-swapping behaviour is
>> something that is imposed by the output device. In which case, this
>> should be a configuration parameter on the panel, not on the path
>> structure.
>>
>>>            TTC_dkb does not support dsi, the link_config is no used anymore.
>>
>> Then you should fix up ttc_dkb before submitting this patch.
>
> After we add one new field for this output rbswap setting based on dsi
> interface, it can be used by new stepping of mmp display controller,
> ttc_dkb platform just leave and not touch it, it will be tranparent
> for ttc_dkb, does not need to nothing for platform configuration for
> ttc_dkb usage.
> It means , ttc_dkb can only configure rbswap in "dma input" part, not
> support rbswap in dsi interface part.
> What do you think?

The point I am trying to make is that your patch is changing behaviour
for ttc_dkb, so you need to address that at the same time.

Right now ttc_dkb does this:

#define CFG_GRA_SWAPRB(x)      (x << 0) /* 1: rbswap enabled */
.link_config = CFG_DUMBMODE(0x2) | CFG_GRA_SWAPRB(0x1),

i.e. SWAPRB requested through bit 0 in link_config

And this is obeyed by the existing code in fmt_to_reg:

   u32 link_config = path_to_path_plat(overlay->path)->link_config;
   rbswap = link_config & 0x1;

Your patch removes the handling of link_config bit 0, without fixing
up its only user (even if that user was incorrect). That is not good
practice.

Another question: why is this change needed?
We can request rb swapping either in DMA input or in the output
interface. I can understand the driver maybe supporting one option or
the other. But after your patch, it seems like both are supported: RB
swapping could be enabled either through choice of input format, or
through configuration of output parameters.

Daniel