> On 03/01/2011 06:25 PM, Maurus Cuelenaere wrote:
>>> 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.
<snip>
>>>> +{
>>>> + 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.
>>>
> I've seen that you are writing quite a few registers at once in the G240400RTSW
> driver. So it might makes sense to have a single function which takes an array
> of uint32_t, with all these values instead of switching back and forth between
> the framebuffer and panel driver.
> You could encode whether a certain word is a command or data through the most
> upper bit as it is done in the actual register as well.
>
> Something like
> void jz4740_fb_slcd_send_cmd(struct device *dev, const uint32_t *data, size_t num)
> {
> struct jzfb *jzfb = dev_get_drvdata(dev);
> uint32_t d;
>
> mutex_lock(&jzfb->lock);
>
> if (!jzfb->is_enabled)
> clk_enable(jzfb->ldclk);
>
> for(; num; --num, ++data) {
> d = *data;
> if (d & JZ_SLCD_DATA_RS_CMD)
> d = jz4740_fb_slcd_panel_cmd(d);
> else
> d = jz4740_fb_slcd_panel_daya(d)
> jzfb_slcd_wait(jzfb);
> writel(d, jzfb->base + JZ_REG_SLCD_DATA);
> }
>
> if (!jzfb->is_enabled) {
> jzfb_slcd_wait(jzfb);
> clk_disable(jzfb->ldclk);
> }
>
> mutex_unlock(&jzfb->lock);
> }
>
Right, now if this would also do mdelay (e.g. (BIT(30) | 5) means mdelay(5)), I
could do away with jz4740_fb_slcd_{en,dis}able_transfer() :)
That probably shouldn't be done in the framebuffer driver though, so I'll just
stick to something like this:
struct panel_platform_data {
void (*lock)(void *);
void (*unlock)(void *);
void (*write)(const uint32_t *data, size_t num, void *);
void *platform_data;
};
>> Me neither, but the only "good" way forward I see is emulating an SPI interface
>> over this..
>> Do you think this is acceptable?
>>
> Since the driver is not actually talking SPI over the bus I don't think this is
> an option.
> What might be an option is to provide a generic callback structure for the
> panel driver.
>
> struct panel_platform_data {
> void (*panel_write)(void *data, const uint32_t *data, size_t num);
> void *panel_data;
> };
>
> So that the panel driver doesn't reference the jzfb driver directly.
>
>
>
>
Right, but that still doesn't make the LCD panel driver any less tightly coupled
with the Jz4740 SLCD format.
Looking at the Renesas R61509 datasheet, they call this "80-system 8/9/16/18-bit
interface", an x-bit parallel interface with CS, WR, RD and RS (register select).
So SPI doesn't fit the bill, but I can't seem to find any other Linux driver bus
better-suited to do this (apart from creating a new one).
I guess using a generic callback is the best solution, so next patch iteration
will include this unless someone else comes up with something better.
--
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
