On Thu, 5, Jan 2012 10:01, Andrew Morton wrote:
> On Wed, 21 Dec 2011 13:25:00 +0900
> Donghwa Lee <dh09.lee@xxxxxxxxxxx> wrote:
>
>>
>> This patch is amoled panel driver based MIPI DSI interface.
>> S6E8AX0 means it may includes many other ldi controllers, for example,
>> S6E8AA0, S6E8AB0, and so on.
>>
>> This patch can be modified depending on each panel properites. For example,
>> second parameter of panel condition register can be changed depending on
>> ldi controller or amoled type.
>>
>>
>> ...
>>
>> +static unsigned char s6e8ax0_22_gamma_30[] = {
>> + 0xFA, 0x01, 0x60, 0x10, 0x60, 0xF5, 0x00, 0xFF, 0xAD, 0xAF,
>> + 0xBA, 0xC3, 0xD8, 0xC5, 0x9F, 0xC6, 0x9E, 0xC1, 0xDC, 0xC0,
>> + 0x00, 0x61, 0x00, 0x5A, 0x00, 0x74,
>> +};
>> +
>>
>> ...
>>
>> +static unsigned char s6e8ax0_22_gamma_300[] = {
>> + 0xFA, 0x01, 0x60, 0x10, 0x60, 0xB5, 0xD3, 0xBD, 0xB1, 0xD2,
>> + 0xB0, 0xC0, 0xDC, 0xC0, 0x94, 0xBA, 0x91, 0xAC, 0xC5, 0xA9,
>> + 0x00, 0xC2, 0x00, 0xB7, 0x00, 0xED,
>> +};
>> +
>> +static unsigned char *s6e8ax0_22_gamma_table[] = {
>> + s6e8ax0_22_gamma_30,
>> + s6e8ax0_22_gamma_50,
>> + s6e8ax0_22_gamma_60,
>> + s6e8ax0_22_gamma_70,
>> + s6e8ax0_22_gamma_80,
>> + s6e8ax0_22_gamma_90,
>> + s6e8ax0_22_gamma_100,
>> + s6e8ax0_22_gamma_120,
>> + s6e8ax0_22_gamma_130,
>> + s6e8ax0_22_gamma_140,
>> + s6e8ax0_22_gamma_150,
>> + s6e8ax0_22_gamma_160,
>> + s6e8ax0_22_gamma_170,
>> + s6e8ax0_22_gamma_180,
>> + s6e8ax0_22_gamma_190,
>> + s6e8ax0_22_gamma_200,
>> + s6e8ax0_22_gamma_210,
>> + s6e8ax0_22_gamma_220,
>> + s6e8ax0_22_gamma_230,
>> + s6e8ax0_22_gamma_240,
>> + s6e8ax0_22_gamma_250,
>> + s6e8ax0_22_gamma_260,
>> + s6e8ax0_22_gamma_270,
>> + s6e8ax0_22_gamma_280,
>> + s6e8ax0_22_gamma_300,
>> +};
>
> I suggest making all the above arrays const. Otherwise the compiler
> might end up deciding to needlessly allocate space in writeable storage
> for them.
>
> If that means that ops->cmd_write() needs constification as well then
> let's just do that, for it is the right thing to do.
>
Ok, I will change it to const arrays with ops->cmd_write() function parameter.
>> +static void s6e8ax0_panel_cond(struct s6e8ax0 *lcd)
>> +{
>> + struct mipi_dsim_master_ops *ops = lcd_to_master_ops(lcd);
>> +
>> + unsigned char data_to_send[] = {
>> + 0xf8, 0x3d, 0x35, 0x00, 0x00, 0x00, 0x93, 0x00, 0x3c,
>> + 0x7d, 0x08, 0x27, 0x7d, 0x3f, 0x00, 0x00, 0x00, 0x20,
>> + 0x04, 0x08, 0x6e, 0x00, 0x00, 0x00, 0x02, 0x08, 0x08,
>> + 0x23, 0x23, 0xc0, 0xc8, 0x08, 0x48, 0xc1, 0x00, 0xc1,
>> + 0xff, 0xff, 0xc8
>> + };
>
> Arrays like this certainly should be const. As it stands, the compiler
> needs to generate room on the stack and generate a local copy of the
> array each time this function is called!
>
>> + ops->cmd_write(lcd_to_master(lcd), MIPI_DSI_DCS_LONG_WRITE,
>> + data_to_send, ARRAY_SIZE(data_to_send));
>> +}
>> +
>> +static void s6e8ax0_display_cond(struct s6e8ax0 *lcd)
>> +{
>> + struct mipi_dsim_master_ops *ops = lcd_to_master_ops(lcd);
>> + unsigned char data_to_send[] = {
>> + 0xf2, 0x80, 0x03, 0x0d
>> + };
>> +
>> + ops->cmd_write(lcd_to_master(lcd), MIPI_DSI_DCS_LONG_WRITE,
>> + data_to_send, ARRAY_SIZE(data_to_send));
>> +}
>> +
>> +/* Gamma 2.2 Setting (200cd, 7500K, 10MPCD) */
>> +static void s6e8ax0_gamma_cond(struct s6e8ax0 *lcd)
>> +{
>> + struct mipi_dsim_master_ops *ops = lcd_to_master_ops(lcd);
>> + unsigned int gamma = lcd->bd->props.brightness;
>> +
>> + ops->cmd_write(lcd_to_master(lcd), MIPI_DSI_DCS_LONG_WRITE,
>> + s6e8ax0_22_gamma_table[gamma],
>> + ARRAY_SIZE(s6e8ax0_22_gamma_table));
>
> This seems wrong. ARRAY_SIZE(s6e8ax0_22_gamma_table) does not
> represent the size of s6e8ax0_22_gamma_table[gamma]!
>
>> +}
>> +
>>
>> ...
>>
>> +static int s6e8ax0_update_gamma_ctrl(struct s6e8ax0 *lcd, int brightness)
>> +{
>> + struct mipi_dsim_master_ops *ops = lcd_to_master_ops(lcd);
>> +
>> + ops->cmd_write(lcd_to_master(lcd), MIPI_DSI_DCS_LONG_WRITE,
>> + s6e8ax0_22_gamma_table[brightness],
>> + ARRAY_SIZE(s6e8ax0_22_gamma_table));
>
> Ditto.
>
>> + /* update gamma table. */
>> + s6e8ax0_gamma_update(lcd);
>> + lcd->gamma = brightness;
>> +
>> + return 0;
>> +}
>> +
>>
>> ...
>>
>> +static void s6e8ax0_power_on(struct mipi_dsim_lcd_device *dsim_dev, int power)
>> +{
>> + struct s6e8ax0 *lcd = dev_get_drvdata(&dsim_dev->dev);
>> +
>> + msleep(lcd->ddi_pd->power_on_delay);
>> +
>> + /* lcd power on */
>> + if (power)
>> + s6e8ax0_regulator_enable(lcd);
>> + else
>> + s6e8ax0_regulator_disable(lcd);
>> +
>> + msleep(lcd->ddi_pd->reset_delay);
>> +
>> + /* lcd reset */
>> + if (lcd->ddi_pd->reset)
>> + lcd->ddi_pd->reset(lcd->ld);
>> + msleep(5);
>> +}
>
> Maybe we should hold lcd->lock across this function to prevent other
> code paths from getting in and fiddling with the hardware while it is
> powering on?
>
In s6e8ax0_regulator_enable()/disable() functions, lcd->lock was used to hold.
>>
>> ...
>>
>
> --
> 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
>
--
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
