On Mon, Jun 24, 2013 at 8:23 PM, jett zhou <jett.zhou@xxxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
