On Tue, 20 Dec 2011 17:00:22 +0900
Donghwa Lee [off-list ref] 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.
It can be modified depending on each panel properites.

This patch is based on Samsung Soc MIPI DSI Driver.
Please refer to the "[PATCH v3] video: support MIPI-DSI controller driver".

http://marc.info/?l=linux-fbdev&m=132435297125837&w=2

It's difficult when interdependent patches are spread around different
mailing lists, different patch series and even different maintainers.

I suggest that you send *all* the patches as a single patch series,
cc'ing all relevant individuals and mailing lists on all patches.  That
way, a single person (perhaps me) can merge all the patches in a single
unit.

+config LCD_S6E8AX0
+	tristate "S6E8AX0 MIPI AMOLED LCD Driver"
+	depends on S5P_MIPI_DSI && BACKLIGHT_CLASS_DEVICE

See, I don't have S5P_MIPI_DSI so I can't even compile-test this.

These two can (and hence should) be implemented as nice type-checked C
functions.

+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
+	};
+
+	ops->cmd_write(lcd_to_master(lcd), MIPI_DSI_DCS_LONG_WRITE,
+		data_to_send, ARRAY_SIZE(data_to_send));
+}

It should be possible to make data_to_send[] static const.  That will
save quite a lot of runtime overhead and space.  It will require that
mipi_dsim_master_ops.cmd_write() take a const pointer.

+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
+	};

Ditto.

+	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], GAMMA_TABLE_COUNT);
+}
+
+static void s6e8ax0_gamma_update(struct s6e8ax0 *lcd)
+{
+	struct mipi_dsim_master_ops *ops = lcd_to_master_ops(lcd);
+	unsigned char data_to_send[] = {
+		0xf7, 0x03
+	};

And many more.

...

+static void s6e8ax0_power_on(struct mipi_dsim_lcd_device *dsim_dev, int power)
+{
+	struct s6e8ax0 *lcd = dev_get_drvdata(&dsim_dev->dev);
+
+	mdelay(lcd->ddi_pd->power_on_delay);
+
+	/* lcd power on */
+	if (power)
+		s6e8ax0_regulator_enable(lcd);
+	else
+		s6e8ax0_regulator_disable(lcd);
+
+	mdelay(lcd->ddi_pd->reset_delay);
+
+	/* lcd reset */
+	if (lcd->ddi_pd->reset)
+		lcd->ddi_pd->reset(lcd->ld);
+	mdelay(5);
+}

This driver uses mdelay() a lot.  It would be far better to use
msleep() where possible.  Please check all the mdelay() sites, see
which ones can be converted?

No, please don't define data in a header file.

See, if this header gets included by two or more .c files then the
kernel contains two or more copies of the data.  And if this header
*isn't* included in two or more header files then there was no point in
putting the data in a header file!

So either a) move this data into the .c file which uses it or b) put it
into a .c file, remove the "static" and add declarations to a header
file.

...