[PATCH] added S6E63M0 AMOLED LCD Panel driver.

Hello, all.

this is S6E63M0 AMOLED LCD Panel(480x800) driver using 3-wired SPI interface
also almost features for lcd panel driver has been implemented in here.
and I added new structure common for all the lcd panel drivers to
include/linux/lcd.h file.

the contents are as following.
- LCD Panel driver needs interfaces for controlling device power such as
power on/off and reset. these interfaces are device specific so it should be
implemented to machine code at this time, we should create new structure
for registering these functions as callbacks and also a header file for
that structure and finally registered callback functions would be called
by lcd panel driver.
such header file(including new structure for lcd panel) would be added
for all the lcd panel drivers.

If anyone provides common structure for registering such callback functions
then we could reduce unnecessary header files for lcd panel.
I thought that suitable anyone could be include/linux/lcd.h

I added new structure to lcd.h like following,
struct lcd_platform_data {
	/* reset lcd panel device. */
	int (*reset)(struct lcd_device *ld);
	/* on or off to lcd panel. if 'enable' is 0 then
	   lcd power off and 1, lcd power on. */
	int (*power_on)(struct lcd_device *ld, int enable);

	/* it indicates whether lcd panel was enabled
	   from bootloader or not. */
	int lcd_enabled;
	/* it means delay for stable time when it becomes low to high
	   or high to low that is dependent on whether reset gpio is
	   low active or high active. */
	unsigned int reset_delay;
	/* stable time needing to become lcd power on. */
	unsigned int power_on_delay;
	/* stable time needing to become lcd power off. */
	unsigned int power_off_delay;

	/* it could be used for any purpose. */
	void *pdata;
};

Please review this patch and lcd panel driver.

Signed-off-by: InKi Dae <inki.dae@samsung.com>
Reviewed-by: KyungMin Park <kyungmin.park.samsung.com>

Best Regards,
InKi Dae.

On Fri, 26 Mar 2010 12:24:24 +0900
InKi Dae [off-list ref] wrote:

+static int s6e63m0_ldi_init(struct s6e63m0 *lcd)
+{
+	int ret;
+
+	ret = s6e63m0_panel_send_sequence(lcd, SEQ_PANEL_CONDITION_SET);
+	ret |= s6e63m0_panel_send_sequence(lcd, SEQ_DISPLAY_CONDITION_SET);
+	ret |= s6e63m0_panel_send_sequence(lcd, SEQ_GAMMA_SETTING);
+	ret |= s6e63m0_panel_send_sequence(lcd, SEQ_ETC_CONDITION_SET);
+	ret |= s6e63m0_panel_send_sequence(lcd, SEQ_ACL_ON);
+	ret |= s6e63m0_panel_send_sequence(lcd, SEQ_ELVSS_ON);
+
+	return ret;
+}

Well.  If one call to s6e63m0_panel_send_sequence() returns -ENOMEM and
another call returns -EIO (for example), this function will return some
other, incorrect errno.

Which is a rather minor problem, unless some caller is explicitly
looking for some particular error code, which doesn't happen often.

On Tuesday, March 30, 2010 4:03 PM, Andrew Morton wrote:

On Fri, 26 Mar 2010 12:24:24 +0900
InKi Dae [off-list ref] wrote:

quoted

+static int s6e63m0_ldi_init(struct s6e63m0 *lcd)
+{
+	int ret;
+
+	ret = s6e63m0_panel_send_sequence(lcd, SEQ_PANEL_CONDITION_SET);
+	ret |= s6e63m0_panel_send_sequence(lcd, SEQ_DISPLAY_CONDITION_SET);
+	ret |= s6e63m0_panel_send_sequence(lcd, SEQ_GAMMA_SETTING);
+	ret |= s6e63m0_panel_send_sequence(lcd, SEQ_ETC_CONDITION_SET);
+	ret |= s6e63m0_panel_send_sequence(lcd, SEQ_ACL_ON);
+	ret |= s6e63m0_panel_send_sequence(lcd, SEQ_ELVSS_ON);
+
+	return ret;
+}

Well.  If one call to s6e63m0_panel_send_sequence() returns -ENOMEM and
another call returns -EIO (for example), this function will return some
other, incorrect errno.

Which is a rather minor problem, unless some caller is explicitly
looking for some particular error code, which doesn't happen often.

Why not handle the calls with a loop?

+static int s6e63m0_ldi_init(struct s6e63m0 *lcd)
+{
+	const unsigned short *init_seq[] = {
+		SEQ_PANEL_CONDITION_SET,
+		SEQ_DISPLAY_CONDITION_SET,
+		SEQ_GAMMA_SETTING,
+		SEQ_ETC_CONDITION_SET,
+		SEQ_ACL_ON,
+		SEQ_ELVSS_ON,
+	};
+	int i, ret;
+
+	for (i = 0; i < ARRAY_SIZE(init_seq); i++) {
+		ret = s6e63m0_panel_send_sequence(lcd, init_seq[i]);
+		if (ret)
+			break;
+	}
+	return ret;
+}

Note that _s6e63m0_gamma_ctl has the same issue.  Actually, the whole
driver has issues with returning errors properly.

Regards,
Hartley

Hi Andrew,

all the calls to s6e63m0_panel_send_sequence() would return -EINVAL.
by api_async() of driver/spi/spi.c
so I think that those return values aren't changed to other.

and final step is to check only whether the return value is 0 or not.
if you still think that this code has minor problem or you want it to
be corrected
then I will patch this code to be corrected anytime.

Thank you.

2010/3/31 Andrew Morton [off-list ref]:

On Fri, 26 Mar 2010 12:24:24 +0900
InKi Dae [off-list ref] wrote:

quoted

+static int s6e63m0_ldi_init(struct s6e63m0 *lcd)
+{
+     int ret;
+
+     ret = s6e63m0_panel_send_sequence(lcd, SEQ_PANEL_CONDITION_SET);
+     ret |= s6e63m0_panel_send_sequence(lcd, SEQ_DISPLAY_CONDITION_SET);
+     ret |= s6e63m0_panel_send_sequence(lcd, SEQ_GAMMA_SETTING);
+     ret |= s6e63m0_panel_send_sequence(lcd, SEQ_ETC_CONDITION_SET);
+     ret |= s6e63m0_panel_send_sequence(lcd, SEQ_ACL_ON);
+     ret |= s6e63m0_panel_send_sequence(lcd, SEQ_ELVSS_ON);
+
+     return ret;
+}

Well.  If one call to s6e63m0_panel_send_sequence() returns -ENOMEM and
another call returns -EIO (for example), this function will return some
other, incorrect errno.

Which is a rather minor problem, unless some caller is explicitly
looking for some particular error code, which doesn't happen often.

Hi Hertley,

it's a good way.
this way is more clear and also Andrew's concern could be solved.

I'd like to apply it to local repository and then I will send the patch
in the near future.

and Andrew, If you think that Hertley's way is clear then I would make
the patch. maybe it would become second patch.

Thank you.

2010/3/31 H Hartley Sweeten [off-list ref]:

On Tuesday, March 30, 2010 4:03 PM, Andrew Morton wrote:

quoted

On Fri, 26 Mar 2010 12:24:24 +0900
InKi Dae [off-list ref] wrote:

quoted

+static int s6e63m0_ldi_init(struct s6e63m0 *lcd)
+{
+    int ret;
+
+    ret = s6e63m0_panel_send_sequence(lcd, SEQ_PANEL_CONDITION_SET);
+    ret |= s6e63m0_panel_send_sequence(lcd, SEQ_DISPLAY_CONDITION_SET);
+    ret |= s6e63m0_panel_send_sequence(lcd, SEQ_GAMMA_SETTING);
+    ret |= s6e63m0_panel_send_sequence(lcd, SEQ_ETC_CONDITION_SET);
+    ret |= s6e63m0_panel_send_sequence(lcd, SEQ_ACL_ON);
+    ret |= s6e63m0_panel_send_sequence(lcd, SEQ_ELVSS_ON);
+
+    return ret;
+}

Well.  If one call to s6e63m0_panel_send_sequence() returns -ENOMEM and
another call returns -EIO (for example), this function will return some
other, incorrect errno.

Which is a rather minor problem, unless some caller is explicitly
looking for some particular error code, which doesn't happen often.

Why not handle the calls with a loop?

+static int s6e63m0_ldi_init(struct s6e63m0 *lcd)
+{
+       const unsigned short *init_seq[] = {
+               SEQ_PANEL_CONDITION_SET,
+               SEQ_DISPLAY_CONDITION_SET,
+               SEQ_GAMMA_SETTING,
+               SEQ_ETC_CONDITION_SET,
+               SEQ_ACL_ON,
+               SEQ_ELVSS_ON,
+       };
+       int i, ret;
+
+       for (i = 0; i < ARRAY_SIZE(init_seq); i++) {
+               ret = s6e63m0_panel_send_sequence(lcd, init_seq[i]);
+               if (ret)
+                       break;
+       }
+       return ret;
+}

Note that _s6e63m0_gamma_ctl has the same issue.  Actually, the whole
driver has issues with returning errors properly.

Regards,
Hartley

On Wed, 31 Mar 2010 11:41:54 +0900 InKi Dae [off-list ref] wrote:

Hi Andrew,

all the calls to s6e63m0_panel_send_sequence() would return -EINVAL.
by api_async() of driver/spi/spi.c

No, spi_async() does

	master->transfer(spi, message);

which can return at least EIO, EINPROGRESS, EINVAL or ETIMEDOUT.

so I think that those return values aren't changed to other.

and final step is to check only whether the return value is 0 or not.
if you still think that this code has minor problem or you want it to
be corrected
then I will patch this code to be corrected anytime.

It's a bug.

Also s6e63m0_power_on() is sloppy.  It again or's together disparate
errnos.  Then if _anything_ failed it returns hardwired -EIO, but it
should instead propagate the callee's errno back up to the caller.

And s6e63m0_power_on() can return -EFAULT in several places, which is
nonsensical.

None of this is very critical, just ... sloppy.

On Tue, 30 Mar 2010 19:55:01 -0400
Andrew Morton [off-list ref] wrote:

On Wed, 31 Mar 2010 11:41:54 +0900 InKi Dae [off-list ref] wrote:

quoted

Hi Andrew,

all the calls to s6e63m0_panel_send_sequence() would return -EINVAL.
by api_async() of driver/spi/spi.c

No, spi_async() does

	master->transfer(spi, message);

which can return at least EIO, EINPROGRESS, EINVAL or ETIMEDOUT.

quoted

so I think that those return values aren't changed to other.

and final step is to check only whether the return value is 0 or not.
if you still think that this code has minor problem or you want it to
be corrected
then I will patch this code to be corrected anytime.

It's a bug.

Also s6e63m0_power_on() is sloppy.  It again or's together disparate
errnos.  Then if _anything_ failed it returns hardwired -EIO, but it
should instead propagate the callee's errno back up to the caller.

And s6e63m0_power_on() can return -EFAULT in several places, which is
nonsensical.

None of this is very critical, just ... sloppy.

ping?

I'm sorry for being late.

this is second patch that your concern is solved.

Please review this patch.

Thank you.

Best Regards,
InKi Dae.

2010/4/28 Andrew Morton [off-list ref]:

On Tue, 30 Mar 2010 19:55:01 -0400
Andrew Morton [off-list ref] wrote:

quoted

On Wed, 31 Mar 2010 11:41:54 +0900 InKi Dae [off-list ref] wrote:

quoted

Hi Andrew,

all the calls to s6e63m0_panel_send_sequence() would return -EINVAL.
by api_async() of driver/spi/spi.c

No, spi_async() does

      master->transfer(spi, message);

which can return at least EIO, EINPROGRESS, EINVAL or ETIMEDOUT.

quoted

so I think that those return values aren't changed to other.

and final step is to check only whether the return value is 0 or not.
if you still think that this code has minor problem or you want it to
be corrected
then I will patch this code to be corrected anytime.

It's a bug.

Also s6e63m0_power_on() is sloppy.  It again or's together disparate
errnos.  Then if _anything_ failed it returns hardwired -EIO, but it
should instead propagate the callee's errno back up to the caller.

And s6e63m0_power_on() can return -EFAULT in several places, which is
nonsensical.

None of this is very critical, just ... sloppy.

ping?