> On 03/01/2011 01:06 PM, Maurus Cuelenaere wrote: >> This patch adds SLCD support to the Ingenic Jz4740 framebuffer driver. >> Besides adding the new LCD types to the platform data, this adds chip select >> and register select active low support (SLCD only). Also, this exports some >> functions related to starting and stopping the SLCD image transfer and sending >> commands. >> >> A lot of this code was based on work by Maarten ter Huurne: >> https://github.com/mthuurne/opendingux-kernel/commit/3dba5b5e7a868b1067acae8d1b5a19121b77bc6a >> >> Signed-off-by: Maurus Cuelenaere <mcuelenaere@xxxxxxxxx> > Looks mostly good :) > >> --- >> arch/mips/include/asm/mach-jz4740/jz4740_fb.h | 33 ++ >> drivers/video/jz4740_fb.c | 435 ++++++++++++++++++++++--- >> 2 files changed, 429 insertions(+), 39 deletions(-) >> >> diff --git a/arch/mips/include/asm/mach-jz4740/jz4740_fb.h b/arch/mips/include/asm/mach-jz4740/jz4740_fb.h >> index 6a50e6f..eaaac43 100644 >> --- a/arch/mips/include/asm/mach-jz4740/jz4740_fb.h >> +++ b/arch/mips/include/asm/mach-jz4740/jz4740_fb.h >> @@ -30,6 +30,13 @@ enum jz4740_fb_lcd_type { >> JZ_LCD_TYPE_DUAL_COLOR_STN = 10, >> JZ_LCD_TYPE_DUAL_MONOCHROME_STN = 11, >> JZ_LCD_TYPE_8BIT_SERIAL = 12, >> + >> + JZ_SLCD_TYPE_PARALLEL_8_BIT = 1 | (1 << 5), >> + JZ_SLCD_TYPE_PARALLEL_16_BIT = 0 | (1 << 5), >> + JZ_SLCD_TYPE_PARALLEL_18_BIT = 2 | (1 << 5), >> + JZ_SLCD_TYPE_SERIAL_8_BIT = 1 | (3 << 5), >> + JZ_SLCD_TYPE_SERIAL_16_BIT = 0 | (3 << 5), >> + JZ_SLCD_TYPE_SERIAL_18_BIT = 2 | (3 << 5), > I would prefer > JZ_LCD_TYPE_SLCD_... Ok. >> }; > Maybe add > #define JZ_LCD_TYPE_SLCD (1 << 5) > #define JZ_LCD_TYPE_SLCD_SERIAL (JZ_LCD_TYPE_SLCD | (1 << 6)) Ok. > >> >> #define JZ4740_FB_SPECIAL_TFT_CONFIG(start, stop) (((start) << 16) | (stop)) >> @@ -62,6 +69,32 @@ struct jz4740_fb_platform_data { >> >> unsigned pixclk_falling_edge:1; >> unsigned date_enable_active_low:1; >> + unsigned chip_select_active_low:1; >> + unsigned register_select_active_low:1; >> }; >> >> +struct platform_device; >> + >> +/* >> + * JzFB SLCD related functions >> + * >> + * jz4740_fb_slcd_disable_transfer: disables the current image transfer going >> + * on between memory and the LCD controller >> + * jz4740_fb_slcd_enable_transfer: the reverse operation of the above >> + * jz4740_fb_slcd_send_cmd: sends a command without any data to the LCD >> + * controller >> + * jz4740_fb_slcd_send_cmd_data: sends a command with a data argument to the LCD >> + * controller >> + * >> + * These functions can sleep and thus should not be called from an atomic >> + * context. Also, make sure you disable the SLCD image transfer *before* sending >> + * any commands and do not forget to re-enable it. >> + */ >> +extern void jz4740_fb_slcd_disable_transfer(struct platform_device *pdev); >> +extern void jz4740_fb_slcd_enable_transfer(struct platform_device *pdev); >> +extern void jz4740_fb_slcd_send_cmd_data(struct platform_device *pdev, >> + unsigned int cmd, unsigned int data); >> +extern void jz4740_fb_slcd_send_cmd(struct platform_device *pdev, >> + unsigned int cmd); >> + >> #endif >> diff --git a/drivers/video/jz4740_fb.c b/drivers/video/jz4740_fb.c >> index 2f3ea57..4064812 100644 >> --- a/drivers/video/jz4740_fb.c >> +++ b/drivers/video/jz4740_fb.c >> @@ -26,6 +26,7 @@ >> >> #include <linux/dma-mapping.h> >> >> +#include <asm/mach-jz4740/dma.h> >> #include <asm/mach-jz4740/jz4740_fb.h> >> #include <asm/mach-jz4740/gpio.h> >> >> @@ -107,6 +108,40 @@ >> >> #define JZ_LCD_STATE_DISABLED BIT(0) >> >> +#define JZ_REG_SLCD_CFG 0xA0 >> +#define JZ_REG_SLCD_CTRL 0xA4 >> +#define JZ_REG_SLCD_STATE 0xA8 >> +#define JZ_REG_SLCD_DATA 0xAC >> +#define JZ_REG_SLCD_FIFO 0xB0 >> + >> +#define JZ_SLCD_CFG_BURST_8_WORD BIT(14) >> +#define JZ_SLCD_CFG_DWIDTH_MASK (7 << 10) >> +#define JZ_SLCD_CFG_DWIDTH_18 (0 << 10) >> +#define JZ_SLCD_CFG_DWIDTH_16 (1 << 10) >> +#define JZ_SLCD_CFG_DWIDTH_8_x3 (2 << 10) >> +#define JZ_SLCD_CFG_DWIDTH_8_x2 (3 << 10) >> +#define JZ_SLCD_CFG_DWIDTH_8_x1 (4 << 10) >> +#define JZ_SLCD_CFG_DWIDTH_9_x2 (7 << 10) >> +#define JZ_SLCD_CFG_CWIDTH_MASK (3 << 8) >> +#define JZ_SLCD_CFG_CWIDTH(n) ((n) << 8) >> +#define JZ_SLCD_CFG_CWIDTH_16BIT (0 << 8) >> +#define JZ_SLCD_CFG_CWIDTH_8BIT (1 << 8) >> +#define JZ_SLCD_CFG_CWIDTH_18BIT (2 << 8) >> +#define JZ_SLCD_CFG_CS_ACTIVE_HIGH BIT(4) >> +#define JZ_SLCD_CFG_RS_CMD_HIGH BIT(3) >> +#define JZ_SLCD_CFG_CLK_ACTIVE_RISING BIT(1) >> +#define JZ_SLCD_CFG_TYPE_SERIAL BIT(0) >> + >> +#define JZ_SLCD_CTRL_DMA_EN (1 << 0) >> + >> +#define JZ_SLCD_STATE_BUSY (1 << 0) >> + >> +#define JZ_SLCD_DATA_RS_DATA (0 << 31) >> +#define JZ_SLCD_DATA_RS_COMMAND (1 << 31) >> + >> +#define JZ_SLCD_FIFO_RS_DATA (0 << 31) >> +#define JZ_SLCD_FIFO_RS_COMMAND (1 << 31) >> + >> struct jzfb_framedesc { >> uint32_t next; >> uint32_t addr; >> @@ -118,6 +153,7 @@ struct jzfb { >> struct fb_info *fb; >> struct platform_device *pdev; >> void __iomem *base; >> + phys_t phys_base; >> struct resource *mem; >> struct jz4740_fb_platform_data *pdata; >> >> @@ -126,6 +162,7 @@ struct jzfb { >> dma_addr_t vidmem_phys; >> struct jzfb_framedesc *framedesc; >> dma_addr_t framedesc_phys; >> + struct jz4740_dma_chan *slcd_dma; >> >> struct clk *ldclk; >> struct clk *lpclk; >> @@ -136,6 +173,9 @@ struct jzfb { >> uint32_t pseudo_palette[16]; >> }; >> >> +#define JZFB_IS_SLCD(jzfb) ((jzfb)->pdata->lcd_type & (1 << 5)) >> +#define JZFB_IS_SLCD_SERIAL_TYPE(jzfb) ((jzfb)->pdata->lcd_type & (2 << 5)) > These two should be static inline bool. Ok. >> +static void send_panel_command(struct jzfb *jzfb, u32 cmd) > You've been using u{16,32} throughout the whole patch, since the existing code > uses uint_{16,32}t you should stick to them. Ok, probably a restant from the original opendingux patch. >> +{ >> + u16 slcd_cfg = readw(jzfb->base + JZ_REG_SLCD_CFG); >> + >> + jzfb_slcd_wait(jzfb); >> + >> + switch (slcd_cfg & JZ_SLCD_CFG_CWIDTH_MASK) { >> + case JZ_SLCD_CFG_CWIDTH_8BIT: >> + writel(JZ_SLCD_DATA_RS_COMMAND | ((cmd & 0xff00) >> 8), >> + jzfb->base + JZ_REG_SLCD_DATA); >> + jzfb_slcd_wait(jzfb); >> + writel(JZ_SLCD_DATA_RS_COMMAND | (cmd & 0xff), >> + jzfb->base + JZ_REG_SLCD_DATA); >> + break; >> + >> + case JZ_SLCD_CFG_CWIDTH_16BIT: >> + writel(JZ_SLCD_DATA_RS_COMMAND | (cmd & 0xffff), >> + jzfb->base + JZ_REG_SLCD_DATA); >> + break; >> + >> + case JZ_SLCD_CFG_CWIDTH_18BIT: >> + writel(JZ_SLCD_DATA_RS_COMMAND | ((cmd & 0xff00) << 2) | >> + ((cmd & 0xff) << 1), jzfb->base + JZ_REG_SLCD_DATA); >> + break; >> + } >> +} >> + >> +static void send_panel_data(struct jzfb *jzfb, u32 data) >> +{ >> + u16 slcd_cfg = readw(jzfb->base + JZ_REG_SLCD_CFG); >> + >> + switch (slcd_cfg & JZ_SLCD_CFG_DWIDTH_MASK) { >> + case JZ_SLCD_CFG_DWIDTH_18: >> + case JZ_SLCD_CFG_DWIDTH_9_x2: >> + data = ((data & 0xff) << 1) | ((data & 0xff00) << 2); >> + data = ((data << 6) & 0xfc0000) | ((data << 4) & 0xfc00) | >> + ((data << 2) & 0x0000fc); >> + break; >> + >> + case JZ_SLCD_CFG_DWIDTH_16: >> + default: >> + data &= 0xffff; >> + break; >> + } >> + >> + jzfb_slcd_wait(jzfb); >> + writel(JZ_SLCD_DATA_RS_DATA | data, jzfb->base + JZ_REG_SLCD_DATA); >> +} >> + >> +void jz4740_fb_slcd_disable_transfer(struct platform_device *pdev) >> +{ >> + struct jzfb *jzfb = platform_get_drvdata(pdev); >> + >> + mutex_lock(&jzfb->lock); >> + >> + if (jzfb->is_enabled) { >> + jz4740_dma_disable(jzfb->slcd_dma); >> + jzfb_slcd_wait(jzfb); >> + } >> + >> + mutex_unlock(&jzfb->lock); >> +} >> +EXPORT_SYMBOL_GPL(jz4740_fb_slcd_disable_transfer); >> + >> +void jz4740_fb_slcd_enable_transfer(struct platform_device *pdev) >> +{ >> + struct jzfb *jzfb = platform_get_drvdata(pdev); >> + >> + mutex_lock(&jzfb->lock); >> + >> + if (jzfb->is_enabled) >> + jzfb_slcd_start_dma(jzfb); >> + >> + mutex_unlock(&jzfb->lock); >> +} >> +EXPORT_SYMBOL_GPL(jz4740_fb_slcd_enable_transfer); >> + >> +void jz4740_fb_slcd_send_cmd_data(struct platform_device *pdev, >> + unsigned int cmd, unsigned int data) >> +{ >> + struct jzfb *jzfb = platform_get_drvdata(pdev); >> + >> + mutex_lock(&jzfb->lock); >> + >> + if (!jzfb->is_enabled) >> + clk_enable(jzfb->ldclk); >> + >> + send_panel_command(jzfb, cmd); >> + send_panel_data(jzfb, data); >> + >> + if (!jzfb->is_enabled) { >> + jzfb_slcd_wait(jzfb); >> + clk_disable(jzfb->ldclk); >> + } >> + >> + mutex_unlock(&jzfb->lock); >> +} >> +EXPORT_SYMBOL_GPL(jz4740_fb_slcd_send_cmd_data); >> + >> +void jz4740_fb_slcd_send_cmd(struct platform_device *pdev, unsigned int cmd) >> +{ >> + struct jzfb *jzfb = platform_get_drvdata(pdev); >> + >> + mutex_lock(&jzfb->lock); >> + >> + if (!jzfb->is_enabled) >> + clk_enable(jzfb->ldclk); >> + >> + send_panel_command(jzfb, cmd); >> + >> + if (!jzfb->is_enabled) { >> + jzfb_slcd_wait(jzfb); >> + clk_disable(jzfb->ldclk); >> + } >> + >> + mutex_unlock(&jzfb->lock); >> +} >> +EXPORT_SYMBOL_GPL(jz4740_fb_slcd_send_cmd); > jz4740_fb_slcd_send_cmd_data and jz4740_fb_slcd_send_cmd are almost identical > it might makes sense to merge them. Something like void jz4740_fb_slcd_send_cmd(struct platform_device *pdev, unsigned int cmd, unsigned int data, bool with_data); ? You can't use data == 0 as "do not send data" as this could be a valid parameter. > I don't like the idea of adding a jzfb specific interface for talking to the > lcd panels, because it will tightly couple the panel driver to the framebuffer > driver and it won't be possible to reuse for some board using the same panel > but a different SoC. > Me neither, but the only "good" way forward I see is emulating an SPI interface over this.. Do you think this is acceptable? -- Maurus Cuelenaere -- 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
