Re: PATCH] backlight: add lms501kf03 LCD driver

2012-12-17 Thread Jingoo Han
On Tuesday, December 18, 2012 12:51 AM, Joe Perches wrote
> On Mon, 2012-12-17 at 17:22 +0900, Jingoo Han wrote:
> > Add the lms501kf03 LCD panel driver. The lms501kf03 LCD panel (800
> > x 480) driver uses 3-wired SPI inteface.
> 
> A trivial note:
> 
> > diff --git a/drivers/video/backlight/lms501kf03.c 
> > b/drivers/video/backlight/lms501kf03.c
> 
> []
> 
> > +static const unsigned short seq_rgb_gamma[] = {
> > +   0xc1, 0x01, 0x03, 0x07, 0x0f, 0x1a, 0x22, 0x2c, 0x33, 0x3c,
> > +   0x46, 0x4f, 0x58, 0x60, 0x69, 0x71, 0x79, 0x82, 0x89, 0x92,
> > +   0x9a, 0xa1, 0xa9, 0xb1, 0xb9, 0xc1, 0xc9, 0xcf, 0xd6, 0xde,
> > +   0xe5, 0xec, 0xf3, 0xf9, 0xff, 0xdd, 0x39, 0x07, 0x1c, 0xcb,
> > +   0xab, 0x5f, 0x49, 0x80, 0x03, 0x07, 0x0f, 0x19, 0x20, 0x2a,
> > +   0x31, 0x39, 0x42, 0x4b, 0x53, 0x5b, 0x63, 0x6b, 0x73, 0x7b,
> > +   0x83, 0x8a, 0x92, 0x9b, 0xa2, 0xaa, 0xb2, 0xba, 0xc2, 0xca,
> > +   0xd0, 0xd8, 0xe1, 0xe8, 0xf0, 0xf8, 0xff, 0xf7, 0xd8, 0xbe,
> > +   0xa7, 0x39, 0x40, 0x85, 0x8c, 0xc0, 0x04, 0x07, 0x0c, 0x17,
> > +   0x1c, 0x23, 0x2b, 0x34, 0x3b, 0x43, 0x4c, 0x54, 0x5b, 0x63,
> > +   0x6a, 0x73, 0x7a, 0x82, 0x8a, 0x91, 0x98, 0xa1, 0xa8, 0xb0,
> > +   0xb7, 0xc1, 0xc9, 0xcf, 0xd9, 0xe3, 0xea, 0xf4, 0xff, 0x00,
> > +   0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > +   ENDDEF
> > +};
> 
> All of these ushort arrays could be uchar.
> 
> > +static int lms501kf03_spi_write(struct lms501kf03 *lcd, unsigned char 
> > address,
> > +   unsigned char command)
> > +{
> > +   int ret;
> > +
> > +   ret = lms501kf03_spi_write_byte(lcd, address, command);
> > +
> > +   return ret;
> > +}
> > +
> > +static int lms501kf03_panel_send_sequence(struct lms501kf03 *lcd,
> > +   const unsigned short *wbuf)
> > +{
> > +   int ret = 0, i = 0;
> > +
> > +   while (wbuf[i] != ENDDEF) {
> 
> Using an unsigned short where the high order byte
> is an end-of-buffer indicator is a bit space wasteful.
> 
> Perhaps a sized struct or array instead.

OK, I will use unsigned char, instead of unsigned short.

Thanks.




--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: PATCH] backlight: add lms501kf03 LCD driver

2012-12-17 Thread Sachin Kamat
On 18 December 2012 06:59, Jingoo Han  wrote:
> On Monday, Monday, December 17, 2012 7:01 PM, Sachin Kamat wrote
>>
>> Hi Jingoo,
>>
>> I had already submitted a patch for adding support for this driver [1]
>> and you had also provided your review comments on them ([2] and [3]).
>> There were certain comments from Andrew Morton that needed to be addressed
>> which I could not due to some other priorities.
>
> CC'ed Ilho Lee
>
> You have abandoned this patch for 7 months!
>
> In addition, before you submitted a patch, it was already developed and
> managed by me with Ilho Lee. So, ownership should be given to me and Ilho Lee.
> Also, I am not sure that you can manage this lms501kf03 LCD driver.
>

I leave it to the maintainers to decide if it is an ethical practice
to claim ownership by incorporating review comments on the original
patch.

>>
>> IMO, it would be better to address comments on that driver rather than
>> posting a new (similar) driver altogether.


>>
>> [1] http://marc.info/?l=linux-fbdev=133455903202255=4
>> [2] http://marc.info/?l=linux-fbdev=133574414215045=4
>> [3] http://marc.info/?l=linux-fbdev=133576237619447=4
>>
>> On 17 December 2012 13:52, Jingoo Han  wrote:
>> > Add the lms501kf03 LCD panel driver. The lms501kf03 LCD panel (800
>> > x 480) driver uses 3-wired SPI inteface.
>> >
>> > Signed-off-by: Ilho Lee 
>> > Signed-off-by: Jingoo Han 
>> > ---
>> >  drivers/video/backlight/Kconfig  |8 +
>> >  drivers/video/backlight/Makefile |1 +
>> >  drivers/video/backlight/lms501kf03.c |  448 
>> > ++
>> >  3 files changed, 457 insertions(+), 0 deletions(-)
>> >  create mode 100644 drivers/video/backlight/lms501kf03.c
>> >
>> > diff --git a/drivers/video/backlight/Kconfig 
>> > b/drivers/video/backlight/Kconfig
>> > index 765a945..081d6cf 100644
>> > --- a/drivers/video/backlight/Kconfig
>> > +++ b/drivers/video/backlight/Kconfig
>> > @@ -126,6 +126,14 @@ config LCD_AMS369FG06
>> >   If you have an AMS369FG06 AMOLED Panel, say Y to enable its
>> >   LCD control driver.
>> >
>> > +config LCD_LMS501KF03
>> > +   tristate "LMS501KF03 LCD Driver"
>> > +   depends on SPI
>> > +   default n
>> > +   help
>> > + If you have an LMS501KF03 LCD Panel, say Y to enable its
>> > + LCD control driver.
>> > +
>> >  endif # LCD_CLASS_DEVICE
>> >
>> >  #
>> > diff --git a/drivers/video/backlight/Makefile 
>> > b/drivers/video/backlight/Makefile
>> > index e7ce729..d02a728 100644
>> > --- a/drivers/video/backlight/Makefile
>> > +++ b/drivers/video/backlight/Makefile
>> > @@ -14,6 +14,7 @@ obj-$(CONFIG_LCD_TOSA)   += tosa_lcd.o
>> >  obj-$(CONFIG_LCD_S6E63M0)  += s6e63m0.o
>> >  obj-$(CONFIG_LCD_LD9040)   += ld9040.o
>> >  obj-$(CONFIG_LCD_AMS369FG06)   += ams369fg06.o
>> > +obj-$(CONFIG_LCD_LMS501KF03)   += lms501kf03.o
>> >
>> >  obj-$(CONFIG_BACKLIGHT_CLASS_DEVICE) += backlight.o
>> >  obj-$(CONFIG_BACKLIGHT_ATMEL_PWM)+= atmel-pwm-bl.o
>> > diff --git a/drivers/video/backlight/lms501kf03.c 
>> > b/drivers/video/backlight/lms501kf03.c
>> > new file mode 100644
>> > index 000..c30ea53
>> > --- /dev/null
>> > +++ b/drivers/video/backlight/lms501kf03.c
>> > @@ -0,0 +1,448 @@
>> > +/*
>> > + * lms501kf03 TFT LCD panel driver.
>> > + *
>> > + * Copyright (c) 2012 Samsung Electronics Co., Ltd.
>> > + * Author: Jingoo Han  
>> > + *
>> > + * This program is free software; you can redistribute it and/or modify it
>> > + * under the terms of the GNU General Public License as published by the
>> > + * Free Software Foundation; either version 2 of the License, or (at your
>> > + * option) any later version.
>> > + */
>> > +
>> > +#include 
>> > +#include 
>> > +#include 
>> > +#include 
>> > +#include 
>> > +#include 
>> > +#include 
>> > +#include 
>> > +
>> > +#define ENDDEF 0xff00
>> > +#define COMMAND_ONLY   0x00
>> > +#define DATA_ONLY  0x01
>> > +
>> > +struct lms501kf03 {
>> > +   struct device   *dev;
>> > +   struct spi_device   *spi;
>> > +   unsigned intpower;
>> > +   struct lcd_device   *ld;
>> > +   struct lcd_platform_data*lcd_pd;
>> > +};
>> > +
>> > +static const unsigned short seq_password[] = {
>> > +   0xb9, 0xff, 0x83, 0x69,
>> > +   ENDDEF
>> > +};
>> > +
>> > +static const unsigned short seq_power[] = {
>> > +   0xb1, 0x01, 0x00, 0x34, 0x06, 0x00, 0x14, 0x14, 0x20, 0x28,
>> > +   0x12, 0x12, 0x17, 0x0a, 0x01, 0xe6, 0xe6, 0xe6, 0xe6, 0xe6,
>> > +   ENDDEF
>> > +};
>> > +
>> > +static const unsigned short seq_display[] = {
>> > +   0xb2, 0x00, 0x2b, 0x03, 0x03, 0x70, 0x00, 0xff, 0x00, 0x00,
>> > +   0x00, 0x00, 0x03, 0x03, 0x00, 0x01,
>> > +   ENDDEF
>> > +};
>> > +
>> > +static const unsigned short seq_rgb_if[] = {
>> > +   0xb3, 0x09,
>> > +   ENDDEF
>> > +};
>> > +
>> > +static const unsigned short 

Re: PATCH] backlight: add lms501kf03 LCD driver

2012-12-17 Thread Jingoo Han
On Tuesday, December 18, 2012 2:00 PM, Sachin Kamat wrote
> On 18 December 2012 06:59, Jingoo Han  wrote:
> > On Monday, Monday, December 17, 2012 7:01 PM, Sachin Kamat wrote
> >>
> >> Hi Jingoo,
> >>
> >> I had already submitted a patch for adding support for this driver [1]
> >> and you had also provided your review comments on them ([2] and [3]).
> >> There were certain comments from Andrew Morton that needed to be addressed
> >> which I could not due to some other priorities.
> >
> > CC'ed Ilho Lee
> >
> > You have abandoned this patch for 7 months!
> >
> > In addition, before you submitted a patch, it was already developed and
> > managed by me with Ilho Lee. So, ownership should be given to me and Ilho 
> > Lee.
> > Also, I am not sure that you can manage this lms501kf03 LCD driver.
> >
> 
> I leave it to the maintainers to decide if it is an ethical practice
> to claim ownership by incorporating review comments on the original
> patch.


I am not claiming ownership by incorporating review comments
on the original patch!!!


It is the history about LMS501KF03 LCD driver.

  2011.9~2011.12: Original code was developed by Ilho Lee, Jingoo Han
  2012.1~2012.10: Original code was managed by Jingoo Han

  2012.4.16: 1st patch was submitted by Sachin Kamat using original code.
  2012.4.30: 3rd patch was submitted by Sachin Kamat using original code.

  2012.5.01: Andrew gave the comment on 3rd patch.

  2012.5.01~2012.12.16: There is no response from Sachin Kamat.

  2012.12.17: 4th patch was submitted by Jingoo Han using original code.


Original patch was developed by Ilho Lee and Me during 2011.9~2011.12.
You just copied the original patch and send it on 2012.4.16.
Moreover, you have not concerned for 7 months(2012.5.01~2012.12.16).

So, ownership should be given to Ilho Lee and Me.
I don't want to make a noise.


> 
> >>
> >> IMO, it would be better to address comments on that driver rather than
> >> posting a new (similar) driver altogether.
> 
> 
> >>
> >> [1] http://marc.info/?l=linux-fbdev=133455903202255=4
> >> [2] http://marc.info/?l=linux-fbdev=133574414215045=4
> >> [3] http://marc.info/?l=linux-fbdev=133576237619447=4
> >>
> >> On 17 December 2012 13:52, Jingoo Han  wrote:
> >> > Add the lms501kf03 LCD panel driver. The lms501kf03 LCD panel (800
> >> > x 480) driver uses 3-wired SPI inteface.
> >> >
> >> > Signed-off-by: Ilho Lee 
> >> > Signed-off-by: Jingoo Han 
> >> > ---
> >> >  drivers/video/backlight/Kconfig  |8 +
> >> >  drivers/video/backlight/Makefile |1 +
> >> >  drivers/video/backlight/lms501kf03.c |  448 
> >> > ++
> >> >  3 files changed, 457 insertions(+), 0 deletions(-)
> >> >  create mode 100644 drivers/video/backlight/lms501kf03.c
> >> >
> >> > diff --git a/drivers/video/backlight/Kconfig 
> >> > b/drivers/video/backlight/Kconfig
> >> > index 765a945..081d6cf 100644
> >> > --- a/drivers/video/backlight/Kconfig
> >> > +++ b/drivers/video/backlight/Kconfig
> >> > @@ -126,6 +126,14 @@ config LCD_AMS369FG06
> >> >   If you have an AMS369FG06 AMOLED Panel, say Y to enable its
> >> >   LCD control driver.
> >> >
> >> > +config LCD_LMS501KF03
> >> > +   tristate "LMS501KF03 LCD Driver"
> >> > +   depends on SPI
> >> > +   default n
> >> > +   help
> >> > + If you have an LMS501KF03 LCD Panel, say Y to enable its
> >> > + LCD control driver.
> >> > +
> >> >  endif # LCD_CLASS_DEVICE
> >> >
> >> >  #
> >> > diff --git a/drivers/video/backlight/Makefile 
> >> > b/drivers/video/backlight/Makefile
> >> > index e7ce729..d02a728 100644
> >> > --- a/drivers/video/backlight/Makefile
> >> > +++ b/drivers/video/backlight/Makefile
> >> > @@ -14,6 +14,7 @@ obj-$(CONFIG_LCD_TOSA)   += tosa_lcd.o
> >> >  obj-$(CONFIG_LCD_S6E63M0)  += s6e63m0.o
> >> >  obj-$(CONFIG_LCD_LD9040)   += ld9040.o
> >> >  obj-$(CONFIG_LCD_AMS369FG06)   += ams369fg06.o
> >> > +obj-$(CONFIG_LCD_LMS501KF03)   += lms501kf03.o
> >> >
> >> >  obj-$(CONFIG_BACKLIGHT_CLASS_DEVICE) += backlight.o
> >> >  obj-$(CONFIG_BACKLIGHT_ATMEL_PWM)+= atmel-pwm-bl.o
> >> > diff --git a/drivers/video/backlight/lms501kf03.c 
> >> > b/drivers/video/backlight/lms501kf03.c
> >> > new file mode 100644
> >> > index 000..c30ea53
> >> > --- /dev/null
> >> > +++ b/drivers/video/backlight/lms501kf03.c
> >> > @@ -0,0 +1,448 @@
> >> > +/*
> >> > + * lms501kf03 TFT LCD panel driver.
> >> > + *
> >> > + * Copyright (c) 2012 Samsung Electronics Co., Ltd.
> >> > + * Author: Jingoo Han  
> >> > + *
> >> > + * This program is free software; you can redistribute it and/or modify 
> >> > it
> >> > + * under the terms of the GNU General Public License as published by the
> >> > + * Free Software Foundation; either version 2 of the License, or (at 
> >> > your
> >> > + * option) any later version.
> >> > + */
> >> > +
> >> > +#include 
> >> > +#include 
> >> > +#include 
> >> > +#include 
> >> > +#include 
> >> > +#include 
> >> > 

Re: PATCH] backlight: add lms501kf03 LCD driver

2012-12-17 Thread Jingoo Han
On Tuesday, December 18, 2012 1:51 AM, devendra.aaru wrote
> Hello,
> 
> > +static int lms501kf03_spi_write(struct lms501kf03 *lcd, unsigned char 
> > address,
> > +   unsigned char command)
> > +{
> > +   int ret;
> > +
> > +   ret = lms501kf03_spi_write_byte(lcd, address, command);
> > +
> > +   return ret;
> 
> there is redundancy here,
> you can do just removing the ret and do return.

OK. I will fix it.

> 
> > +}
> > +
> > +static int lms501kf03_panel_send_sequence(struct lms501kf03 *lcd,
> > +   const unsigned short *wbuf)
> > +{
> > +   int ret = 0, i = 0;
> > +
> > +   while (wbuf[i] != ENDDEF) {
> > +   if (i == 0)
> > +   ret = lms501kf03_spi_write(lcd, COMMAND_ONLY, 
> > wbuf[i]);
> > +   else
> > +   ret = lms501kf03_spi_write(lcd, DATA_ONLY, wbuf[i]);
> > +   if (ret)
> > +   break;
> > +   i += 1;
> > +   }
> > +
> > +   return ret;
> > +}
> > +
> > +static int lms501kf03_ldi_init(struct lms501kf03 *lcd)
> > +{
> > +   int ret, i;
> > +   static const unsigned short *init_seq[] = {
> > +   seq_password,
> > +   seq_power,
> > +   seq_display,
> > +   seq_rgb_if,
> > +   seq_display_inv,
> > +   seq_vcom,
> > +   seq_gate,
> > +   seq_panel,
> > +   seq_col_mod,
> > +   seq_w_gamma,
> > +   seq_rgb_gamma,
> > +   seq_sleep_out,
> > +   };
> > +
> > +   for (i = 0; i < ARRAY_SIZE(init_seq); i++) {
> > +   ret = lms501kf03_panel_send_sequence(lcd, init_seq[i]);
> > +   if (ret)
> > +   break;
> > +   }
> > +   /* according to the datasheet, 120ms delay time is required. */
> why the 120ms delay required would be good to specify as comment. or
> you can put the link to datasheet if available.

OK, I will add more explanation.
However, link to datasheet is not available.

> 
> > +   msleep(120);
> > +
> > +   return ret;
> > +}
> > +
> > +static int lms501kf03_ldi_enable(struct lms501kf03 *lcd)
> > +{
> > +   return lms501kf03_panel_send_sequence(lcd, seq_display_on);
> > +}
> > +
> > +static int lms501kf03_ldi_disable(struct lms501kf03 *lcd)
> > +{
> > +   return lms501kf03_panel_send_sequence(lcd, seq_display_off);
> > +}
> > +
> > +static int lms501kf03_power_is_on(int power)
> > +{
> > +   return (power) <= FB_BLANK_NORMAL;
> > +}
> > +
> > +static int lms501kf03_power_on(struct lms501kf03 *lcd)
> > +{
> > +   int ret = 0;
> > +   struct lcd_platform_data *pd;
> > +
> > +   pd = lcd->lcd_pd;
> > +
> > +   if (!pd->power_on) {
> > +   dev_err(lcd->dev, "power_on is NULL.\n");
> > +   return -EFAULT;
> we may need to do -EINVAL instead of EFAULT as EFAULT tends to be for
> the invalid memory addresses.

OK. I will fix it.

> 
> > +   } else {
> > +   pd->power_on(lcd->ld, 1);
> > +   msleep(pd->power_on_delay);
> > +   }
> > +
> > +   if (!pd->reset) {
> > +   dev_err(lcd->dev, "reset is NULL.\n");
> > +   return -EFAULT;
> 
> may be here too..

OK. I will fix it.

> 
> > +   } else {
> > +   pd->reset(lcd->ld);
> > +   msleep(pd->reset_delay);
> > +   }
> > +
> > +   ret = lms501kf03_ldi_init(lcd);
> > +   if (ret) {
> > +   dev_err(lcd->dev, "failed to initialize ldi.\n");
> > +   return ret;
> > +   }
> > +
> > +   ret = lms501kf03_ldi_enable(lcd);
> > +   if (ret) {
> > +   dev_err(lcd->dev, "failed to enable ldi.\n");
> > +   return ret;
> > +   }
> > +
> > +   return 0;
> > +}
> > +
> > +static int lms501kf03_power_off(struct lms501kf03 *lcd)
> > +{
> > +   int ret = 0;
> > +   struct lcd_platform_data *pd;
> > +
> > +   pd = lcd->lcd_pd;
> > +
> > +   ret = lms501kf03_ldi_disable(lcd);
> > +   if (ret) {
> > +   dev_err(lcd->dev, "lcd setting failed.\n");
> > +   return -EIO;
> > +   }
> > +
> > +   msleep(pd->power_off_delay);
> > +
> > +   pd->power_on(lcd->ld, 0);
> > +
> 
> seems that you are calling the core lcd framework api, i am curious to
> know why, :p , and obviously i dunno why its calling that way.
> 
> > +   return 0;
> > +}
> > +
> > +static int lms501kf03_power(struct lms501kf03 *lcd, int power)
> > +{
> > +   int ret = 0;
> > +
> > +   if (lms501kf03_power_is_on(power) &&
> > +   !lms501kf03_power_is_on(lcd->power))
> > +   ret = lms501kf03_power_on(lcd);
> > +   else if (!lms501kf03_power_is_on(power) &&
> > +   lms501kf03_power_is_on(lcd->power))
> > +   ret = lms501kf03_power_off(lcd);
> > +
> 
> seems that ret is used 

Re: PATCH] backlight: add lms501kf03 LCD driver

2012-12-17 Thread Jingoo Han
On Monday, Monday, December 17, 2012 7:01 PM, Sachin Kamat wrote
> 
> Hi Jingoo,
> 
> I had already submitted a patch for adding support for this driver [1]
> and you had also provided your review comments on them ([2] and [3]).
> There were certain comments from Andrew Morton that needed to be addressed
> which I could not due to some other priorities.

CC'ed Ilho Lee

You have abandoned this patch for 7 months!

In addition, before you submitted a patch, it was already developed and
managed by me with Ilho Lee. So, ownership should be given to me and Ilho Lee.
Also, I am not sure that you can manage this lms501kf03 LCD driver.

> 
> IMO, it would be better to address comments on that driver rather than
> posting a new (similar) driver altogether.
> 
> [1] http://marc.info/?l=linux-fbdev=133455903202255=4
> [2] http://marc.info/?l=linux-fbdev=133574414215045=4
> [3] http://marc.info/?l=linux-fbdev=133576237619447=4
> 
> On 17 December 2012 13:52, Jingoo Han  wrote:
> > Add the lms501kf03 LCD panel driver. The lms501kf03 LCD panel (800
> > x 480) driver uses 3-wired SPI inteface.
> >
> > Signed-off-by: Ilho Lee 
> > Signed-off-by: Jingoo Han 
> > ---
> >  drivers/video/backlight/Kconfig  |8 +
> >  drivers/video/backlight/Makefile |1 +
> >  drivers/video/backlight/lms501kf03.c |  448 
> > ++
> >  3 files changed, 457 insertions(+), 0 deletions(-)
> >  create mode 100644 drivers/video/backlight/lms501kf03.c
> >
> > diff --git a/drivers/video/backlight/Kconfig 
> > b/drivers/video/backlight/Kconfig
> > index 765a945..081d6cf 100644
> > --- a/drivers/video/backlight/Kconfig
> > +++ b/drivers/video/backlight/Kconfig
> > @@ -126,6 +126,14 @@ config LCD_AMS369FG06
> >   If you have an AMS369FG06 AMOLED Panel, say Y to enable its
> >   LCD control driver.
> >
> > +config LCD_LMS501KF03
> > +   tristate "LMS501KF03 LCD Driver"
> > +   depends on SPI
> > +   default n
> > +   help
> > + If you have an LMS501KF03 LCD Panel, say Y to enable its
> > + LCD control driver.
> > +
> >  endif # LCD_CLASS_DEVICE
> >
> >  #
> > diff --git a/drivers/video/backlight/Makefile 
> > b/drivers/video/backlight/Makefile
> > index e7ce729..d02a728 100644
> > --- a/drivers/video/backlight/Makefile
> > +++ b/drivers/video/backlight/Makefile
> > @@ -14,6 +14,7 @@ obj-$(CONFIG_LCD_TOSA)   += tosa_lcd.o
> >  obj-$(CONFIG_LCD_S6E63M0)  += s6e63m0.o
> >  obj-$(CONFIG_LCD_LD9040)   += ld9040.o
> >  obj-$(CONFIG_LCD_AMS369FG06)   += ams369fg06.o
> > +obj-$(CONFIG_LCD_LMS501KF03)   += lms501kf03.o
> >
> >  obj-$(CONFIG_BACKLIGHT_CLASS_DEVICE) += backlight.o
> >  obj-$(CONFIG_BACKLIGHT_ATMEL_PWM)+= atmel-pwm-bl.o
> > diff --git a/drivers/video/backlight/lms501kf03.c 
> > b/drivers/video/backlight/lms501kf03.c
> > new file mode 100644
> > index 000..c30ea53
> > --- /dev/null
> > +++ b/drivers/video/backlight/lms501kf03.c
> > @@ -0,0 +1,448 @@
> > +/*
> > + * lms501kf03 TFT LCD panel driver.
> > + *
> > + * Copyright (c) 2012 Samsung Electronics Co., Ltd.
> > + * Author: Jingoo Han  
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of the GNU General Public License as published by the
> > + * Free Software Foundation; either version 2 of the License, or (at your
> > + * option) any later version.
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#define ENDDEF 0xff00
> > +#define COMMAND_ONLY   0x00
> > +#define DATA_ONLY  0x01
> > +
> > +struct lms501kf03 {
> > +   struct device   *dev;
> > +   struct spi_device   *spi;
> > +   unsigned intpower;
> > +   struct lcd_device   *ld;
> > +   struct lcd_platform_data*lcd_pd;
> > +};
> > +
> > +static const unsigned short seq_password[] = {
> > +   0xb9, 0xff, 0x83, 0x69,
> > +   ENDDEF
> > +};
> > +
> > +static const unsigned short seq_power[] = {
> > +   0xb1, 0x01, 0x00, 0x34, 0x06, 0x00, 0x14, 0x14, 0x20, 0x28,
> > +   0x12, 0x12, 0x17, 0x0a, 0x01, 0xe6, 0xe6, 0xe6, 0xe6, 0xe6,
> > +   ENDDEF
> > +};
> > +
> > +static const unsigned short seq_display[] = {
> > +   0xb2, 0x00, 0x2b, 0x03, 0x03, 0x70, 0x00, 0xff, 0x00, 0x00,
> > +   0x00, 0x00, 0x03, 0x03, 0x00, 0x01,
> > +   ENDDEF
> > +};
> > +
> > +static const unsigned short seq_rgb_if[] = {
> > +   0xb3, 0x09,
> > +   ENDDEF
> > +};
> > +
> > +static const unsigned short seq_display_inv[] = {
> > +   0xb4, 0x01, 0x08, 0x77, 0x0e, 0x06,
> > +   ENDDEF
> > +};
> > +
> > +static const unsigned short seq_vcom[] = {
> > +   0xb6, 0x4c, 0x2e,
> > +   ENDDEF
> > +};
> > +
> > +static const unsigned short seq_gate[] = {
> > +   0xd5, 0x00, 0x05, 0x03, 0x29, 0x01, 0x07, 0x17, 0x68, 0x13,
> > 

Re: PATCH] backlight: add lms501kf03 LCD driver

2012-12-17 Thread devendra.aaru
Hello,

> +static int lms501kf03_spi_write(struct lms501kf03 *lcd, unsigned char 
> address,
> +   unsigned char command)
> +{
> +   int ret;
> +
> +   ret = lms501kf03_spi_write_byte(lcd, address, command);
> +
> +   return ret;

there is redundancy here,
you can do just removing the ret and do return.

> +}
> +
> +static int lms501kf03_panel_send_sequence(struct lms501kf03 *lcd,
> +   const unsigned short *wbuf)
> +{
> +   int ret = 0, i = 0;
> +
> +   while (wbuf[i] != ENDDEF) {
> +   if (i == 0)
> +   ret = lms501kf03_spi_write(lcd, COMMAND_ONLY, 
> wbuf[i]);
> +   else
> +   ret = lms501kf03_spi_write(lcd, DATA_ONLY, wbuf[i]);
> +   if (ret)
> +   break;
> +   i += 1;
> +   }
> +
> +   return ret;
> +}
> +
> +static int lms501kf03_ldi_init(struct lms501kf03 *lcd)
> +{
> +   int ret, i;
> +   static const unsigned short *init_seq[] = {
> +   seq_password,
> +   seq_power,
> +   seq_display,
> +   seq_rgb_if,
> +   seq_display_inv,
> +   seq_vcom,
> +   seq_gate,
> +   seq_panel,
> +   seq_col_mod,
> +   seq_w_gamma,
> +   seq_rgb_gamma,
> +   seq_sleep_out,
> +   };
> +
> +   for (i = 0; i < ARRAY_SIZE(init_seq); i++) {
> +   ret = lms501kf03_panel_send_sequence(lcd, init_seq[i]);
> +   if (ret)
> +   break;
> +   }
> +   /* according to the datasheet, 120ms delay time is required. */
why the 120ms delay required would be good to specify as comment. or
you can put the link to datasheet if available.

> +   msleep(120);
> +
> +   return ret;
> +}
> +
> +static int lms501kf03_ldi_enable(struct lms501kf03 *lcd)
> +{
> +   return lms501kf03_panel_send_sequence(lcd, seq_display_on);
> +}
> +
> +static int lms501kf03_ldi_disable(struct lms501kf03 *lcd)
> +{
> +   return lms501kf03_panel_send_sequence(lcd, seq_display_off);
> +}
> +
> +static int lms501kf03_power_is_on(int power)
> +{
> +   return (power) <= FB_BLANK_NORMAL;
> +}
> +
> +static int lms501kf03_power_on(struct lms501kf03 *lcd)
> +{
> +   int ret = 0;
> +   struct lcd_platform_data *pd;
> +
> +   pd = lcd->lcd_pd;
> +
> +   if (!pd->power_on) {
> +   dev_err(lcd->dev, "power_on is NULL.\n");
> +   return -EFAULT;
we may need to do -EINVAL instead of EFAULT as EFAULT tends to be for
the invalid memory addresses.

> +   } else {
> +   pd->power_on(lcd->ld, 1);
> +   msleep(pd->power_on_delay);
> +   }
> +
> +   if (!pd->reset) {
> +   dev_err(lcd->dev, "reset is NULL.\n");
> +   return -EFAULT;

may be here too..

> +   } else {
> +   pd->reset(lcd->ld);
> +   msleep(pd->reset_delay);
> +   }
> +
> +   ret = lms501kf03_ldi_init(lcd);
> +   if (ret) {
> +   dev_err(lcd->dev, "failed to initialize ldi.\n");
> +   return ret;
> +   }
> +
> +   ret = lms501kf03_ldi_enable(lcd);
> +   if (ret) {
> +   dev_err(lcd->dev, "failed to enable ldi.\n");
> +   return ret;
> +   }
> +
> +   return 0;
> +}
> +
> +static int lms501kf03_power_off(struct lms501kf03 *lcd)
> +{
> +   int ret = 0;
> +   struct lcd_platform_data *pd;
> +
> +   pd = lcd->lcd_pd;
> +
> +   ret = lms501kf03_ldi_disable(lcd);
> +   if (ret) {
> +   dev_err(lcd->dev, "lcd setting failed.\n");
> +   return -EIO;
> +   }
> +
> +   msleep(pd->power_off_delay);
> +
> +   pd->power_on(lcd->ld, 0);
> +

seems that you are calling the core lcd framework api, i am curious to
know why, :p , and obviously i dunno why its calling that way.

> +   return 0;
> +}
> +
> +static int lms501kf03_power(struct lms501kf03 *lcd, int power)
> +{
> +   int ret = 0;
> +
> +   if (lms501kf03_power_is_on(power) &&
> +   !lms501kf03_power_is_on(lcd->power))
> +   ret = lms501kf03_power_on(lcd);
> +   else if (!lms501kf03_power_is_on(power) &&
> +   lms501kf03_power_is_on(lcd->power))
> +   ret = lms501kf03_power_off(lcd);
> +

seems that ret is used unitialised in this function bug is covered by
ret = 0 assignment, but why that else case is missed, or i am just
misreading?

> +   if (!ret)
> +   lcd->power = power;
> +
> +   return ret;
> +}
> +

[snip]
> +
> +static int lms501kf03_probe(struct spi_device *spi)
> +{
> +   struct lms501kf03 *lcd = NULL;
> +   struct lcd_device *ld = NULL;
> +   int ret = 0;
> +
> +   lcd = devm_kzalloc(>dev, sizeof(struct lms501kf03), GFP_KERNEL);
> +   if (!lcd)
> +   return -ENOMEM;
> +

Re: PATCH] backlight: add lms501kf03 LCD driver

2012-12-17 Thread Joe Perches
On Mon, 2012-12-17 at 17:22 +0900, Jingoo Han wrote:
> Add the lms501kf03 LCD panel driver. The lms501kf03 LCD panel (800
> x 480) driver uses 3-wired SPI inteface.

A trivial note:

> diff --git a/drivers/video/backlight/lms501kf03.c 
> b/drivers/video/backlight/lms501kf03.c

[]

> +static const unsigned short seq_rgb_gamma[] = {
> + 0xc1, 0x01, 0x03, 0x07, 0x0f, 0x1a, 0x22, 0x2c, 0x33, 0x3c,
> + 0x46, 0x4f, 0x58, 0x60, 0x69, 0x71, 0x79, 0x82, 0x89, 0x92,
> + 0x9a, 0xa1, 0xa9, 0xb1, 0xb9, 0xc1, 0xc9, 0xcf, 0xd6, 0xde,
> + 0xe5, 0xec, 0xf3, 0xf9, 0xff, 0xdd, 0x39, 0x07, 0x1c, 0xcb,
> + 0xab, 0x5f, 0x49, 0x80, 0x03, 0x07, 0x0f, 0x19, 0x20, 0x2a,
> + 0x31, 0x39, 0x42, 0x4b, 0x53, 0x5b, 0x63, 0x6b, 0x73, 0x7b,
> + 0x83, 0x8a, 0x92, 0x9b, 0xa2, 0xaa, 0xb2, 0xba, 0xc2, 0xca,
> + 0xd0, 0xd8, 0xe1, 0xe8, 0xf0, 0xf8, 0xff, 0xf7, 0xd8, 0xbe,
> + 0xa7, 0x39, 0x40, 0x85, 0x8c, 0xc0, 0x04, 0x07, 0x0c, 0x17,
> + 0x1c, 0x23, 0x2b, 0x34, 0x3b, 0x43, 0x4c, 0x54, 0x5b, 0x63,
> + 0x6a, 0x73, 0x7a, 0x82, 0x8a, 0x91, 0x98, 0xa1, 0xa8, 0xb0,
> + 0xb7, 0xc1, 0xc9, 0xcf, 0xd9, 0xe3, 0xea, 0xf4, 0xff, 0x00,
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> + ENDDEF
> +};

All of these ushort arrays could be uchar.

> +static int lms501kf03_spi_write(struct lms501kf03 *lcd, unsigned char 
> address,
> + unsigned char command)
> +{
> + int ret;
> +
> + ret = lms501kf03_spi_write_byte(lcd, address, command);
> +
> + return ret;
> +}
> +
> +static int lms501kf03_panel_send_sequence(struct lms501kf03 *lcd,
> + const unsigned short *wbuf)
> +{
> + int ret = 0, i = 0;
> +
> + while (wbuf[i] != ENDDEF) {

Using an unsigned short where the high order byte
is an end-of-buffer indicator is a bit space wasteful.

Perhaps a sized struct or array instead.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: PATCH] backlight: add lms501kf03 LCD driver

2012-12-17 Thread Sachin Kamat
Hi Jingoo,

I had already submitted a patch for adding support for this driver [1]
and you had also provided your review comments on them ([2] and [3]).
There were certain comments from Andrew Morton that needed to be addressed
which I could not due to some other priorities.

IMO, it would be better to address comments on that driver rather than
posting a new (similar) driver altogether.

[1] http://marc.info/?l=linux-fbdev=133455903202255=4
[2] http://marc.info/?l=linux-fbdev=133574414215045=4
[3] http://marc.info/?l=linux-fbdev=133576237619447=4

On 17 December 2012 13:52, Jingoo Han  wrote:
> Add the lms501kf03 LCD panel driver. The lms501kf03 LCD panel (800
> x 480) driver uses 3-wired SPI inteface.
>
> Signed-off-by: Ilho Lee 
> Signed-off-by: Jingoo Han 
> ---
>  drivers/video/backlight/Kconfig  |8 +
>  drivers/video/backlight/Makefile |1 +
>  drivers/video/backlight/lms501kf03.c |  448 
> ++
>  3 files changed, 457 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/video/backlight/lms501kf03.c
>
> diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> index 765a945..081d6cf 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
> @@ -126,6 +126,14 @@ config LCD_AMS369FG06
>   If you have an AMS369FG06 AMOLED Panel, say Y to enable its
>   LCD control driver.
>
> +config LCD_LMS501KF03
> +   tristate "LMS501KF03 LCD Driver"
> +   depends on SPI
> +   default n
> +   help
> + If you have an LMS501KF03 LCD Panel, say Y to enable its
> + LCD control driver.
> +
>  endif # LCD_CLASS_DEVICE
>
>  #
> diff --git a/drivers/video/backlight/Makefile 
> b/drivers/video/backlight/Makefile
> index e7ce729..d02a728 100644
> --- a/drivers/video/backlight/Makefile
> +++ b/drivers/video/backlight/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_LCD_TOSA)   += tosa_lcd.o
>  obj-$(CONFIG_LCD_S6E63M0)  += s6e63m0.o
>  obj-$(CONFIG_LCD_LD9040)   += ld9040.o
>  obj-$(CONFIG_LCD_AMS369FG06)   += ams369fg06.o
> +obj-$(CONFIG_LCD_LMS501KF03)   += lms501kf03.o
>
>  obj-$(CONFIG_BACKLIGHT_CLASS_DEVICE) += backlight.o
>  obj-$(CONFIG_BACKLIGHT_ATMEL_PWM)+= atmel-pwm-bl.o
> diff --git a/drivers/video/backlight/lms501kf03.c 
> b/drivers/video/backlight/lms501kf03.c
> new file mode 100644
> index 000..c30ea53
> --- /dev/null
> +++ b/drivers/video/backlight/lms501kf03.c
> @@ -0,0 +1,448 @@
> +/*
> + * lms501kf03 TFT LCD panel driver.
> + *
> + * Copyright (c) 2012 Samsung Electronics Co., Ltd.
> + * Author: Jingoo Han  
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or (at your
> + * option) any later version.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define ENDDEF 0xff00
> +#define COMMAND_ONLY   0x00
> +#define DATA_ONLY  0x01
> +
> +struct lms501kf03 {
> +   struct device   *dev;
> +   struct spi_device   *spi;
> +   unsigned intpower;
> +   struct lcd_device   *ld;
> +   struct lcd_platform_data*lcd_pd;
> +};
> +
> +static const unsigned short seq_password[] = {
> +   0xb9, 0xff, 0x83, 0x69,
> +   ENDDEF
> +};
> +
> +static const unsigned short seq_power[] = {
> +   0xb1, 0x01, 0x00, 0x34, 0x06, 0x00, 0x14, 0x14, 0x20, 0x28,
> +   0x12, 0x12, 0x17, 0x0a, 0x01, 0xe6, 0xe6, 0xe6, 0xe6, 0xe6,
> +   ENDDEF
> +};
> +
> +static const unsigned short seq_display[] = {
> +   0xb2, 0x00, 0x2b, 0x03, 0x03, 0x70, 0x00, 0xff, 0x00, 0x00,
> +   0x00, 0x00, 0x03, 0x03, 0x00, 0x01,
> +   ENDDEF
> +};
> +
> +static const unsigned short seq_rgb_if[] = {
> +   0xb3, 0x09,
> +   ENDDEF
> +};
> +
> +static const unsigned short seq_display_inv[] = {
> +   0xb4, 0x01, 0x08, 0x77, 0x0e, 0x06,
> +   ENDDEF
> +};
> +
> +static const unsigned short seq_vcom[] = {
> +   0xb6, 0x4c, 0x2e,
> +   ENDDEF
> +};
> +
> +static const unsigned short seq_gate[] = {
> +   0xd5, 0x00, 0x05, 0x03, 0x29, 0x01, 0x07, 0x17, 0x68, 0x13,
> +   0x37, 0x20, 0x31, 0x8a, 0x46, 0x9b, 0x57, 0x13, 0x02, 0x75,
> +   0xb9, 0x64, 0xa8, 0x07, 0x0f, 0x04, 0x07,
> +   ENDDEF
> +};
> +
> +static const unsigned short seq_panel[] = {
> +   0xcc, 0x02,
> +   ENDDEF
> +};
> +
> +static const unsigned short seq_col_mod[] = {
> +   0x3a, 0x77,
> +   ENDDEF
> +};
> +
> +static const unsigned short seq_w_gamma[] = {
> +   0xe0, 0x00, 0x04, 0x09, 0x0f, 0x1f, 0x3f, 0x1f, 0x2f, 0x0a,
> +   0x0f, 0x10, 0x16, 0x18, 0x16, 0x17, 0x0d, 0x15, 0x00, 0x04,
> +   0x09, 0x0f, 0x38, 0x3f, 0x20, 0x39, 0x0a, 0x0f, 0x10, 0x16,
> +   0x18, 0x16, 

Re: PATCH] backlight: add lms501kf03 LCD driver

2012-12-17 Thread devendra.aaru
Hello,

 +static int lms501kf03_spi_write(struct lms501kf03 *lcd, unsigned char 
 address,
 +   unsigned char command)
 +{
 +   int ret;
 +
 +   ret = lms501kf03_spi_write_byte(lcd, address, command);
 +
 +   return ret;

there is redundancy here,
you can do just removing the ret and do return.

 +}
 +
 +static int lms501kf03_panel_send_sequence(struct lms501kf03 *lcd,
 +   const unsigned short *wbuf)
 +{
 +   int ret = 0, i = 0;
 +
 +   while (wbuf[i] != ENDDEF) {
 +   if (i == 0)
 +   ret = lms501kf03_spi_write(lcd, COMMAND_ONLY, 
 wbuf[i]);
 +   else
 +   ret = lms501kf03_spi_write(lcd, DATA_ONLY, wbuf[i]);
 +   if (ret)
 +   break;
 +   i += 1;
 +   }
 +
 +   return ret;
 +}
 +
 +static int lms501kf03_ldi_init(struct lms501kf03 *lcd)
 +{
 +   int ret, i;
 +   static const unsigned short *init_seq[] = {
 +   seq_password,
 +   seq_power,
 +   seq_display,
 +   seq_rgb_if,
 +   seq_display_inv,
 +   seq_vcom,
 +   seq_gate,
 +   seq_panel,
 +   seq_col_mod,
 +   seq_w_gamma,
 +   seq_rgb_gamma,
 +   seq_sleep_out,
 +   };
 +
 +   for (i = 0; i  ARRAY_SIZE(init_seq); i++) {
 +   ret = lms501kf03_panel_send_sequence(lcd, init_seq[i]);
 +   if (ret)
 +   break;
 +   }
 +   /* according to the datasheet, 120ms delay time is required. */
why the 120ms delay required would be good to specify as comment. or
you can put the link to datasheet if available.

 +   msleep(120);
 +
 +   return ret;
 +}
 +
 +static int lms501kf03_ldi_enable(struct lms501kf03 *lcd)
 +{
 +   return lms501kf03_panel_send_sequence(lcd, seq_display_on);
 +}
 +
 +static int lms501kf03_ldi_disable(struct lms501kf03 *lcd)
 +{
 +   return lms501kf03_panel_send_sequence(lcd, seq_display_off);
 +}
 +
 +static int lms501kf03_power_is_on(int power)
 +{
 +   return (power) = FB_BLANK_NORMAL;
 +}
 +
 +static int lms501kf03_power_on(struct lms501kf03 *lcd)
 +{
 +   int ret = 0;
 +   struct lcd_platform_data *pd;
 +
 +   pd = lcd-lcd_pd;
 +
 +   if (!pd-power_on) {
 +   dev_err(lcd-dev, power_on is NULL.\n);
 +   return -EFAULT;
we may need to do -EINVAL instead of EFAULT as EFAULT tends to be for
the invalid memory addresses.

 +   } else {
 +   pd-power_on(lcd-ld, 1);
 +   msleep(pd-power_on_delay);
 +   }
 +
 +   if (!pd-reset) {
 +   dev_err(lcd-dev, reset is NULL.\n);
 +   return -EFAULT;

may be here too..

 +   } else {
 +   pd-reset(lcd-ld);
 +   msleep(pd-reset_delay);
 +   }
 +
 +   ret = lms501kf03_ldi_init(lcd);
 +   if (ret) {
 +   dev_err(lcd-dev, failed to initialize ldi.\n);
 +   return ret;
 +   }
 +
 +   ret = lms501kf03_ldi_enable(lcd);
 +   if (ret) {
 +   dev_err(lcd-dev, failed to enable ldi.\n);
 +   return ret;
 +   }
 +
 +   return 0;
 +}
 +
 +static int lms501kf03_power_off(struct lms501kf03 *lcd)
 +{
 +   int ret = 0;
 +   struct lcd_platform_data *pd;
 +
 +   pd = lcd-lcd_pd;
 +
 +   ret = lms501kf03_ldi_disable(lcd);
 +   if (ret) {
 +   dev_err(lcd-dev, lcd setting failed.\n);
 +   return -EIO;
 +   }
 +
 +   msleep(pd-power_off_delay);
 +
 +   pd-power_on(lcd-ld, 0);
 +

seems that you are calling the core lcd framework api, i am curious to
know why, :p , and obviously i dunno why its calling that way.

 +   return 0;
 +}
 +
 +static int lms501kf03_power(struct lms501kf03 *lcd, int power)
 +{
 +   int ret = 0;
 +
 +   if (lms501kf03_power_is_on(power) 
 +   !lms501kf03_power_is_on(lcd-power))
 +   ret = lms501kf03_power_on(lcd);
 +   else if (!lms501kf03_power_is_on(power) 
 +   lms501kf03_power_is_on(lcd-power))
 +   ret = lms501kf03_power_off(lcd);
 +

seems that ret is used unitialised in this function bug is covered by
ret = 0 assignment, but why that else case is missed, or i am just
misreading?

 +   if (!ret)
 +   lcd-power = power;
 +
 +   return ret;
 +}
 +

[snip]
 +
 +static int lms501kf03_probe(struct spi_device *spi)
 +{
 +   struct lms501kf03 *lcd = NULL;
 +   struct lcd_device *ld = NULL;
 +   int ret = 0;
 +
 +   lcd = devm_kzalloc(spi-dev, sizeof(struct lms501kf03), GFP_KERNEL);
 +   if (!lcd)
 +   return -ENOMEM;
 +

glad you used devm api.

 +   /* lms501kf03 lcd panel uses 3-wire 9-bit SPI Mode. */
 +   spi-bits_per_word = 9;
 +
 +   ret = spi_setup(spi);
 +   if (ret  0) {
 + 

Re: PATCH] backlight: add lms501kf03 LCD driver

2012-12-17 Thread Jingoo Han
On Monday, Monday, December 17, 2012 7:01 PM, Sachin Kamat wrote
 
 Hi Jingoo,
 
 I had already submitted a patch for adding support for this driver [1]
 and you had also provided your review comments on them ([2] and [3]).
 There were certain comments from Andrew Morton that needed to be addressed
 which I could not due to some other priorities.

CC'ed Ilho Lee

You have abandoned this patch for 7 months!

In addition, before you submitted a patch, it was already developed and
managed by me with Ilho Lee. So, ownership should be given to me and Ilho Lee.
Also, I am not sure that you can manage this lms501kf03 LCD driver.

 
 IMO, it would be better to address comments on that driver rather than
 posting a new (similar) driver altogether.
 
 [1] http://marc.info/?l=linux-fbdevm=133455903202255w=4
 [2] http://marc.info/?l=linux-fbdevm=133574414215045w=4
 [3] http://marc.info/?l=linux-fbdevm=133576237619447w=4
 
 On 17 December 2012 13:52, Jingoo Han jg1@samsung.com wrote:
  Add the lms501kf03 LCD panel driver. The lms501kf03 LCD panel (800
  x 480) driver uses 3-wired SPI inteface.
 
  Signed-off-by: Ilho Lee ilho215@samsung.com
  Signed-off-by: Jingoo Han jg1@samsung.com
  ---
   drivers/video/backlight/Kconfig  |8 +
   drivers/video/backlight/Makefile |1 +
   drivers/video/backlight/lms501kf03.c |  448 
  ++
   3 files changed, 457 insertions(+), 0 deletions(-)
   create mode 100644 drivers/video/backlight/lms501kf03.c
 
  diff --git a/drivers/video/backlight/Kconfig 
  b/drivers/video/backlight/Kconfig
  index 765a945..081d6cf 100644
  --- a/drivers/video/backlight/Kconfig
  +++ b/drivers/video/backlight/Kconfig
  @@ -126,6 +126,14 @@ config LCD_AMS369FG06
If you have an AMS369FG06 AMOLED Panel, say Y to enable its
LCD control driver.
 
  +config LCD_LMS501KF03
  +   tristate LMS501KF03 LCD Driver
  +   depends on SPI
  +   default n
  +   help
  + If you have an LMS501KF03 LCD Panel, say Y to enable its
  + LCD control driver.
  +
   endif # LCD_CLASS_DEVICE
 
   #
  diff --git a/drivers/video/backlight/Makefile 
  b/drivers/video/backlight/Makefile
  index e7ce729..d02a728 100644
  --- a/drivers/video/backlight/Makefile
  +++ b/drivers/video/backlight/Makefile
  @@ -14,6 +14,7 @@ obj-$(CONFIG_LCD_TOSA)   += tosa_lcd.o
   obj-$(CONFIG_LCD_S6E63M0)  += s6e63m0.o
   obj-$(CONFIG_LCD_LD9040)   += ld9040.o
   obj-$(CONFIG_LCD_AMS369FG06)   += ams369fg06.o
  +obj-$(CONFIG_LCD_LMS501KF03)   += lms501kf03.o
 
   obj-$(CONFIG_BACKLIGHT_CLASS_DEVICE) += backlight.o
   obj-$(CONFIG_BACKLIGHT_ATMEL_PWM)+= atmel-pwm-bl.o
  diff --git a/drivers/video/backlight/lms501kf03.c 
  b/drivers/video/backlight/lms501kf03.c
  new file mode 100644
  index 000..c30ea53
  --- /dev/null
  +++ b/drivers/video/backlight/lms501kf03.c
  @@ -0,0 +1,448 @@
  +/*
  + * lms501kf03 TFT LCD panel driver.
  + *
  + * Copyright (c) 2012 Samsung Electronics Co., Ltd.
  + * Author: Jingoo Han  jg1@samsung.com
  + *
  + * This program is free software; you can redistribute it and/or modify it
  + * under the terms of the GNU General Public License as published by the
  + * Free Software Foundation; either version 2 of the License, or (at your
  + * option) any later version.
  + */
  +
  +#include linux/backlight.h
  +#include linux/delay.h
  +#include linux/fb.h
  +#include linux/gpio.h
  +#include linux/lcd.h
  +#include linux/module.h
  +#include linux/spi/spi.h
  +#include linux/wait.h
  +
  +#define ENDDEF 0xff00
  +#define COMMAND_ONLY   0x00
  +#define DATA_ONLY  0x01
  +
  +struct lms501kf03 {
  +   struct device   *dev;
  +   struct spi_device   *spi;
  +   unsigned intpower;
  +   struct lcd_device   *ld;
  +   struct lcd_platform_data*lcd_pd;
  +};
  +
  +static const unsigned short seq_password[] = {
  +   0xb9, 0xff, 0x83, 0x69,
  +   ENDDEF
  +};
  +
  +static const unsigned short seq_power[] = {
  +   0xb1, 0x01, 0x00, 0x34, 0x06, 0x00, 0x14, 0x14, 0x20, 0x28,
  +   0x12, 0x12, 0x17, 0x0a, 0x01, 0xe6, 0xe6, 0xe6, 0xe6, 0xe6,
  +   ENDDEF
  +};
  +
  +static const unsigned short seq_display[] = {
  +   0xb2, 0x00, 0x2b, 0x03, 0x03, 0x70, 0x00, 0xff, 0x00, 0x00,
  +   0x00, 0x00, 0x03, 0x03, 0x00, 0x01,
  +   ENDDEF
  +};
  +
  +static const unsigned short seq_rgb_if[] = {
  +   0xb3, 0x09,
  +   ENDDEF
  +};
  +
  +static const unsigned short seq_display_inv[] = {
  +   0xb4, 0x01, 0x08, 0x77, 0x0e, 0x06,
  +   ENDDEF
  +};
  +
  +static const unsigned short seq_vcom[] = {
  +   0xb6, 0x4c, 0x2e,
  +   ENDDEF
  +};
  +
  +static const unsigned short seq_gate[] = {
  +   0xd5, 0x00, 0x05, 0x03, 0x29, 0x01, 0x07, 0x17, 0x68, 0x13,
  +   0x37, 0x20, 0x31, 0x8a, 0x46, 0x9b, 0x57, 0x13, 0x02, 0x75,
  

Re: PATCH] backlight: add lms501kf03 LCD driver

2012-12-17 Thread Jingoo Han
On Tuesday, December 18, 2012 1:51 AM, devendra.aaru wrote
 Hello,
 
  +static int lms501kf03_spi_write(struct lms501kf03 *lcd, unsigned char 
  address,
  +   unsigned char command)
  +{
  +   int ret;
  +
  +   ret = lms501kf03_spi_write_byte(lcd, address, command);
  +
  +   return ret;
 
 there is redundancy here,
 you can do just removing the ret and do return.

OK. I will fix it.

 
  +}
  +
  +static int lms501kf03_panel_send_sequence(struct lms501kf03 *lcd,
  +   const unsigned short *wbuf)
  +{
  +   int ret = 0, i = 0;
  +
  +   while (wbuf[i] != ENDDEF) {
  +   if (i == 0)
  +   ret = lms501kf03_spi_write(lcd, COMMAND_ONLY, 
  wbuf[i]);
  +   else
  +   ret = lms501kf03_spi_write(lcd, DATA_ONLY, wbuf[i]);
  +   if (ret)
  +   break;
  +   i += 1;
  +   }
  +
  +   return ret;
  +}
  +
  +static int lms501kf03_ldi_init(struct lms501kf03 *lcd)
  +{
  +   int ret, i;
  +   static const unsigned short *init_seq[] = {
  +   seq_password,
  +   seq_power,
  +   seq_display,
  +   seq_rgb_if,
  +   seq_display_inv,
  +   seq_vcom,
  +   seq_gate,
  +   seq_panel,
  +   seq_col_mod,
  +   seq_w_gamma,
  +   seq_rgb_gamma,
  +   seq_sleep_out,
  +   };
  +
  +   for (i = 0; i  ARRAY_SIZE(init_seq); i++) {
  +   ret = lms501kf03_panel_send_sequence(lcd, init_seq[i]);
  +   if (ret)
  +   break;
  +   }
  +   /* according to the datasheet, 120ms delay time is required. */
 why the 120ms delay required would be good to specify as comment. or
 you can put the link to datasheet if available.

OK, I will add more explanation.
However, link to datasheet is not available.

 
  +   msleep(120);
  +
  +   return ret;
  +}
  +
  +static int lms501kf03_ldi_enable(struct lms501kf03 *lcd)
  +{
  +   return lms501kf03_panel_send_sequence(lcd, seq_display_on);
  +}
  +
  +static int lms501kf03_ldi_disable(struct lms501kf03 *lcd)
  +{
  +   return lms501kf03_panel_send_sequence(lcd, seq_display_off);
  +}
  +
  +static int lms501kf03_power_is_on(int power)
  +{
  +   return (power) = FB_BLANK_NORMAL;
  +}
  +
  +static int lms501kf03_power_on(struct lms501kf03 *lcd)
  +{
  +   int ret = 0;
  +   struct lcd_platform_data *pd;
  +
  +   pd = lcd-lcd_pd;
  +
  +   if (!pd-power_on) {
  +   dev_err(lcd-dev, power_on is NULL.\n);
  +   return -EFAULT;
 we may need to do -EINVAL instead of EFAULT as EFAULT tends to be for
 the invalid memory addresses.

OK. I will fix it.

 
  +   } else {
  +   pd-power_on(lcd-ld, 1);
  +   msleep(pd-power_on_delay);
  +   }
  +
  +   if (!pd-reset) {
  +   dev_err(lcd-dev, reset is NULL.\n);
  +   return -EFAULT;
 
 may be here too..

OK. I will fix it.

 
  +   } else {
  +   pd-reset(lcd-ld);
  +   msleep(pd-reset_delay);
  +   }
  +
  +   ret = lms501kf03_ldi_init(lcd);
  +   if (ret) {
  +   dev_err(lcd-dev, failed to initialize ldi.\n);
  +   return ret;
  +   }
  +
  +   ret = lms501kf03_ldi_enable(lcd);
  +   if (ret) {
  +   dev_err(lcd-dev, failed to enable ldi.\n);
  +   return ret;
  +   }
  +
  +   return 0;
  +}
  +
  +static int lms501kf03_power_off(struct lms501kf03 *lcd)
  +{
  +   int ret = 0;
  +   struct lcd_platform_data *pd;
  +
  +   pd = lcd-lcd_pd;
  +
  +   ret = lms501kf03_ldi_disable(lcd);
  +   if (ret) {
  +   dev_err(lcd-dev, lcd setting failed.\n);
  +   return -EIO;
  +   }
  +
  +   msleep(pd-power_off_delay);
  +
  +   pd-power_on(lcd-ld, 0);
  +
 
 seems that you are calling the core lcd framework api, i am curious to
 know why, :p , and obviously i dunno why its calling that way.
 
  +   return 0;
  +}
  +
  +static int lms501kf03_power(struct lms501kf03 *lcd, int power)
  +{
  +   int ret = 0;
  +
  +   if (lms501kf03_power_is_on(power) 
  +   !lms501kf03_power_is_on(lcd-power))
  +   ret = lms501kf03_power_on(lcd);
  +   else if (!lms501kf03_power_is_on(power) 
  +   lms501kf03_power_is_on(lcd-power))
  +   ret = lms501kf03_power_off(lcd);
  +
 
 seems that ret is used unitialised in this function bug is covered by
 ret = 0 assignment, but why that else case is missed, or i am just
 misreading?

else case is as follows:

+ else
+   return = 0;

So, else case is not necessary.

 
  +   if (!ret)
  +   lcd-power = power;
  +
  +   return ret;
  +}
  +
 
 [snip]
  +
  +static int 

Re: PATCH] backlight: add lms501kf03 LCD driver

2012-12-17 Thread Jingoo Han
On Tuesday, December 18, 2012 2:00 PM, Sachin Kamat wrote
 On 18 December 2012 06:59, Jingoo Han jg1@samsung.com wrote:
  On Monday, Monday, December 17, 2012 7:01 PM, Sachin Kamat wrote
 
  Hi Jingoo,
 
  I had already submitted a patch for adding support for this driver [1]
  and you had also provided your review comments on them ([2] and [3]).
  There were certain comments from Andrew Morton that needed to be addressed
  which I could not due to some other priorities.
 
  CC'ed Ilho Lee
 
  You have abandoned this patch for 7 months!
 
  In addition, before you submitted a patch, it was already developed and
  managed by me with Ilho Lee. So, ownership should be given to me and Ilho 
  Lee.
  Also, I am not sure that you can manage this lms501kf03 LCD driver.
 
 
 I leave it to the maintainers to decide if it is an ethical practice
 to claim ownership by incorporating review comments on the original
 patch.


I am not claiming ownership by incorporating review comments
on the original patch!!!


It is the history about LMS501KF03 LCD driver.

  2011.9~2011.12: Original code was developed by Ilho Lee, Jingoo Han
  2012.1~2012.10: Original code was managed by Jingoo Han

  2012.4.16: 1st patch was submitted by Sachin Kamat using original code.
  2012.4.30: 3rd patch was submitted by Sachin Kamat using original code.

  2012.5.01: Andrew gave the comment on 3rd patch.

  2012.5.01~2012.12.16: There is no response from Sachin Kamat.

  2012.12.17: 4th patch was submitted by Jingoo Han using original code.


Original patch was developed by Ilho Lee and Me during 2011.9~2011.12.
You just copied the original patch and send it on 2012.4.16.
Moreover, you have not concerned for 7 months(2012.5.01~2012.12.16).

So, ownership should be given to Ilho Lee and Me.
I don't want to make a noise.


 
 
  IMO, it would be better to address comments on that driver rather than
  posting a new (similar) driver altogether.
 
 
 
  [1] http://marc.info/?l=linux-fbdevm=133455903202255w=4
  [2] http://marc.info/?l=linux-fbdevm=133574414215045w=4
  [3] http://marc.info/?l=linux-fbdevm=133576237619447w=4
 
  On 17 December 2012 13:52, Jingoo Han jg1@samsung.com wrote:
   Add the lms501kf03 LCD panel driver. The lms501kf03 LCD panel (800
   x 480) driver uses 3-wired SPI inteface.
  
   Signed-off-by: Ilho Lee ilho215@samsung.com
   Signed-off-by: Jingoo Han jg1@samsung.com
   ---
drivers/video/backlight/Kconfig  |8 +
drivers/video/backlight/Makefile |1 +
drivers/video/backlight/lms501kf03.c |  448 
   ++
3 files changed, 457 insertions(+), 0 deletions(-)
create mode 100644 drivers/video/backlight/lms501kf03.c
  
   diff --git a/drivers/video/backlight/Kconfig 
   b/drivers/video/backlight/Kconfig
   index 765a945..081d6cf 100644
   --- a/drivers/video/backlight/Kconfig
   +++ b/drivers/video/backlight/Kconfig
   @@ -126,6 +126,14 @@ config LCD_AMS369FG06
 If you have an AMS369FG06 AMOLED Panel, say Y to enable its
 LCD control driver.
  
   +config LCD_LMS501KF03
   +   tristate LMS501KF03 LCD Driver
   +   depends on SPI
   +   default n
   +   help
   + If you have an LMS501KF03 LCD Panel, say Y to enable its
   + LCD control driver.
   +
endif # LCD_CLASS_DEVICE
  
#
   diff --git a/drivers/video/backlight/Makefile 
   b/drivers/video/backlight/Makefile
   index e7ce729..d02a728 100644
   --- a/drivers/video/backlight/Makefile
   +++ b/drivers/video/backlight/Makefile
   @@ -14,6 +14,7 @@ obj-$(CONFIG_LCD_TOSA)   += tosa_lcd.o
obj-$(CONFIG_LCD_S6E63M0)  += s6e63m0.o
obj-$(CONFIG_LCD_LD9040)   += ld9040.o
obj-$(CONFIG_LCD_AMS369FG06)   += ams369fg06.o
   +obj-$(CONFIG_LCD_LMS501KF03)   += lms501kf03.o
  
obj-$(CONFIG_BACKLIGHT_CLASS_DEVICE) += backlight.o
obj-$(CONFIG_BACKLIGHT_ATMEL_PWM)+= atmel-pwm-bl.o
   diff --git a/drivers/video/backlight/lms501kf03.c 
   b/drivers/video/backlight/lms501kf03.c
   new file mode 100644
   index 000..c30ea53
   --- /dev/null
   +++ b/drivers/video/backlight/lms501kf03.c
   @@ -0,0 +1,448 @@
   +/*
   + * lms501kf03 TFT LCD panel driver.
   + *
   + * Copyright (c) 2012 Samsung Electronics Co., Ltd.
   + * Author: Jingoo Han  jg1@samsung.com
   + *
   + * This program is free software; you can redistribute it and/or modify 
   it
   + * under the terms of the GNU General Public License as published by the
   + * Free Software Foundation; either version 2 of the License, or (at 
   your
   + * option) any later version.
   + */
   +
   +#include linux/backlight.h
   +#include linux/delay.h
   +#include linux/fb.h
   +#include linux/gpio.h
   +#include linux/lcd.h
   +#include linux/module.h
   +#include linux/spi/spi.h
   +#include linux/wait.h
   +
   +#define ENDDEF 0xff00
   +#define COMMAND_ONLY   0x00
   +#define DATA_ONLY  0x01
   +
   +struct 

Re: PATCH] backlight: add lms501kf03 LCD driver

2012-12-17 Thread Sachin Kamat
On 18 December 2012 06:59, Jingoo Han jg1@samsung.com wrote:
 On Monday, Monday, December 17, 2012 7:01 PM, Sachin Kamat wrote

 Hi Jingoo,

 I had already submitted a patch for adding support for this driver [1]
 and you had also provided your review comments on them ([2] and [3]).
 There were certain comments from Andrew Morton that needed to be addressed
 which I could not due to some other priorities.

 CC'ed Ilho Lee

 You have abandoned this patch for 7 months!

 In addition, before you submitted a patch, it was already developed and
 managed by me with Ilho Lee. So, ownership should be given to me and Ilho Lee.
 Also, I am not sure that you can manage this lms501kf03 LCD driver.


I leave it to the maintainers to decide if it is an ethical practice
to claim ownership by incorporating review comments on the original
patch.


 IMO, it would be better to address comments on that driver rather than
 posting a new (similar) driver altogether.



 [1] http://marc.info/?l=linux-fbdevm=133455903202255w=4
 [2] http://marc.info/?l=linux-fbdevm=133574414215045w=4
 [3] http://marc.info/?l=linux-fbdevm=133576237619447w=4

 On 17 December 2012 13:52, Jingoo Han jg1@samsung.com wrote:
  Add the lms501kf03 LCD panel driver. The lms501kf03 LCD panel (800
  x 480) driver uses 3-wired SPI inteface.
 
  Signed-off-by: Ilho Lee ilho215@samsung.com
  Signed-off-by: Jingoo Han jg1@samsung.com
  ---
   drivers/video/backlight/Kconfig  |8 +
   drivers/video/backlight/Makefile |1 +
   drivers/video/backlight/lms501kf03.c |  448 
  ++
   3 files changed, 457 insertions(+), 0 deletions(-)
   create mode 100644 drivers/video/backlight/lms501kf03.c
 
  diff --git a/drivers/video/backlight/Kconfig 
  b/drivers/video/backlight/Kconfig
  index 765a945..081d6cf 100644
  --- a/drivers/video/backlight/Kconfig
  +++ b/drivers/video/backlight/Kconfig
  @@ -126,6 +126,14 @@ config LCD_AMS369FG06
If you have an AMS369FG06 AMOLED Panel, say Y to enable its
LCD control driver.
 
  +config LCD_LMS501KF03
  +   tristate LMS501KF03 LCD Driver
  +   depends on SPI
  +   default n
  +   help
  + If you have an LMS501KF03 LCD Panel, say Y to enable its
  + LCD control driver.
  +
   endif # LCD_CLASS_DEVICE
 
   #
  diff --git a/drivers/video/backlight/Makefile 
  b/drivers/video/backlight/Makefile
  index e7ce729..d02a728 100644
  --- a/drivers/video/backlight/Makefile
  +++ b/drivers/video/backlight/Makefile
  @@ -14,6 +14,7 @@ obj-$(CONFIG_LCD_TOSA)   += tosa_lcd.o
   obj-$(CONFIG_LCD_S6E63M0)  += s6e63m0.o
   obj-$(CONFIG_LCD_LD9040)   += ld9040.o
   obj-$(CONFIG_LCD_AMS369FG06)   += ams369fg06.o
  +obj-$(CONFIG_LCD_LMS501KF03)   += lms501kf03.o
 
   obj-$(CONFIG_BACKLIGHT_CLASS_DEVICE) += backlight.o
   obj-$(CONFIG_BACKLIGHT_ATMEL_PWM)+= atmel-pwm-bl.o
  diff --git a/drivers/video/backlight/lms501kf03.c 
  b/drivers/video/backlight/lms501kf03.c
  new file mode 100644
  index 000..c30ea53
  --- /dev/null
  +++ b/drivers/video/backlight/lms501kf03.c
  @@ -0,0 +1,448 @@
  +/*
  + * lms501kf03 TFT LCD panel driver.
  + *
  + * Copyright (c) 2012 Samsung Electronics Co., Ltd.
  + * Author: Jingoo Han  jg1@samsung.com
  + *
  + * This program is free software; you can redistribute it and/or modify it
  + * under the terms of the GNU General Public License as published by the
  + * Free Software Foundation; either version 2 of the License, or (at your
  + * option) any later version.
  + */
  +
  +#include linux/backlight.h
  +#include linux/delay.h
  +#include linux/fb.h
  +#include linux/gpio.h
  +#include linux/lcd.h
  +#include linux/module.h
  +#include linux/spi/spi.h
  +#include linux/wait.h
  +
  +#define ENDDEF 0xff00
  +#define COMMAND_ONLY   0x00
  +#define DATA_ONLY  0x01
  +
  +struct lms501kf03 {
  +   struct device   *dev;
  +   struct spi_device   *spi;
  +   unsigned intpower;
  +   struct lcd_device   *ld;
  +   struct lcd_platform_data*lcd_pd;
  +};
  +
  +static const unsigned short seq_password[] = {
  +   0xb9, 0xff, 0x83, 0x69,
  +   ENDDEF
  +};
  +
  +static const unsigned short seq_power[] = {
  +   0xb1, 0x01, 0x00, 0x34, 0x06, 0x00, 0x14, 0x14, 0x20, 0x28,
  +   0x12, 0x12, 0x17, 0x0a, 0x01, 0xe6, 0xe6, 0xe6, 0xe6, 0xe6,
  +   ENDDEF
  +};
  +
  +static const unsigned short seq_display[] = {
  +   0xb2, 0x00, 0x2b, 0x03, 0x03, 0x70, 0x00, 0xff, 0x00, 0x00,
  +   0x00, 0x00, 0x03, 0x03, 0x00, 0x01,
  +   ENDDEF
  +};
  +
  +static const unsigned short seq_rgb_if[] = {
  +   0xb3, 0x09,
  +   ENDDEF
  +};
  +
  +static const unsigned short seq_display_inv[] = {
  +   0xb4, 0x01, 0x08, 0x77, 0x0e, 0x06,
  +   ENDDEF
  +};
  +
  +static const unsigned short seq_vcom[] = {
  +   0xb6, 0x4c, 0x2e,
  

Re: PATCH] backlight: add lms501kf03 LCD driver

2012-12-17 Thread Jingoo Han
On Tuesday, December 18, 2012 12:51 AM, Joe Perches wrote
 On Mon, 2012-12-17 at 17:22 +0900, Jingoo Han wrote:
  Add the lms501kf03 LCD panel driver. The lms501kf03 LCD panel (800
  x 480) driver uses 3-wired SPI inteface.
 
 A trivial note:
 
  diff --git a/drivers/video/backlight/lms501kf03.c 
  b/drivers/video/backlight/lms501kf03.c
 
 []
 
  +static const unsigned short seq_rgb_gamma[] = {
  +   0xc1, 0x01, 0x03, 0x07, 0x0f, 0x1a, 0x22, 0x2c, 0x33, 0x3c,
  +   0x46, 0x4f, 0x58, 0x60, 0x69, 0x71, 0x79, 0x82, 0x89, 0x92,
  +   0x9a, 0xa1, 0xa9, 0xb1, 0xb9, 0xc1, 0xc9, 0xcf, 0xd6, 0xde,
  +   0xe5, 0xec, 0xf3, 0xf9, 0xff, 0xdd, 0x39, 0x07, 0x1c, 0xcb,
  +   0xab, 0x5f, 0x49, 0x80, 0x03, 0x07, 0x0f, 0x19, 0x20, 0x2a,
  +   0x31, 0x39, 0x42, 0x4b, 0x53, 0x5b, 0x63, 0x6b, 0x73, 0x7b,
  +   0x83, 0x8a, 0x92, 0x9b, 0xa2, 0xaa, 0xb2, 0xba, 0xc2, 0xca,
  +   0xd0, 0xd8, 0xe1, 0xe8, 0xf0, 0xf8, 0xff, 0xf7, 0xd8, 0xbe,
  +   0xa7, 0x39, 0x40, 0x85, 0x8c, 0xc0, 0x04, 0x07, 0x0c, 0x17,
  +   0x1c, 0x23, 0x2b, 0x34, 0x3b, 0x43, 0x4c, 0x54, 0x5b, 0x63,
  +   0x6a, 0x73, 0x7a, 0x82, 0x8a, 0x91, 0x98, 0xa1, 0xa8, 0xb0,
  +   0xb7, 0xc1, 0xc9, 0xcf, 0xd9, 0xe3, 0xea, 0xf4, 0xff, 0x00,
  +   0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
  +   ENDDEF
  +};
 
 All of these ushort arrays could be uchar.
 
  +static int lms501kf03_spi_write(struct lms501kf03 *lcd, unsigned char 
  address,
  +   unsigned char command)
  +{
  +   int ret;
  +
  +   ret = lms501kf03_spi_write_byte(lcd, address, command);
  +
  +   return ret;
  +}
  +
  +static int lms501kf03_panel_send_sequence(struct lms501kf03 *lcd,
  +   const unsigned short *wbuf)
  +{
  +   int ret = 0, i = 0;
  +
  +   while (wbuf[i] != ENDDEF) {
 
 Using an unsigned short where the high order byte
 is an end-of-buffer indicator is a bit space wasteful.
 
 Perhaps a sized struct or array instead.

OK, I will use unsigned char, instead of unsigned short.

Thanks.




--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: PATCH] backlight: add lms501kf03 LCD driver

2012-12-17 Thread Sachin Kamat
Hi Jingoo,

I had already submitted a patch for adding support for this driver [1]
and you had also provided your review comments on them ([2] and [3]).
There were certain comments from Andrew Morton that needed to be addressed
which I could not due to some other priorities.

IMO, it would be better to address comments on that driver rather than
posting a new (similar) driver altogether.

[1] http://marc.info/?l=linux-fbdevm=133455903202255w=4
[2] http://marc.info/?l=linux-fbdevm=133574414215045w=4
[3] http://marc.info/?l=linux-fbdevm=133576237619447w=4

On 17 December 2012 13:52, Jingoo Han jg1@samsung.com wrote:
 Add the lms501kf03 LCD panel driver. The lms501kf03 LCD panel (800
 x 480) driver uses 3-wired SPI inteface.

 Signed-off-by: Ilho Lee ilho215@samsung.com
 Signed-off-by: Jingoo Han jg1@samsung.com
 ---
  drivers/video/backlight/Kconfig  |8 +
  drivers/video/backlight/Makefile |1 +
  drivers/video/backlight/lms501kf03.c |  448 
 ++
  3 files changed, 457 insertions(+), 0 deletions(-)
  create mode 100644 drivers/video/backlight/lms501kf03.c

 diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
 index 765a945..081d6cf 100644
 --- a/drivers/video/backlight/Kconfig
 +++ b/drivers/video/backlight/Kconfig
 @@ -126,6 +126,14 @@ config LCD_AMS369FG06
   If you have an AMS369FG06 AMOLED Panel, say Y to enable its
   LCD control driver.

 +config LCD_LMS501KF03
 +   tristate LMS501KF03 LCD Driver
 +   depends on SPI
 +   default n
 +   help
 + If you have an LMS501KF03 LCD Panel, say Y to enable its
 + LCD control driver.
 +
  endif # LCD_CLASS_DEVICE

  #
 diff --git a/drivers/video/backlight/Makefile 
 b/drivers/video/backlight/Makefile
 index e7ce729..d02a728 100644
 --- a/drivers/video/backlight/Makefile
 +++ b/drivers/video/backlight/Makefile
 @@ -14,6 +14,7 @@ obj-$(CONFIG_LCD_TOSA)   += tosa_lcd.o
  obj-$(CONFIG_LCD_S6E63M0)  += s6e63m0.o
  obj-$(CONFIG_LCD_LD9040)   += ld9040.o
  obj-$(CONFIG_LCD_AMS369FG06)   += ams369fg06.o
 +obj-$(CONFIG_LCD_LMS501KF03)   += lms501kf03.o

  obj-$(CONFIG_BACKLIGHT_CLASS_DEVICE) += backlight.o
  obj-$(CONFIG_BACKLIGHT_ATMEL_PWM)+= atmel-pwm-bl.o
 diff --git a/drivers/video/backlight/lms501kf03.c 
 b/drivers/video/backlight/lms501kf03.c
 new file mode 100644
 index 000..c30ea53
 --- /dev/null
 +++ b/drivers/video/backlight/lms501kf03.c
 @@ -0,0 +1,448 @@
 +/*
 + * lms501kf03 TFT LCD panel driver.
 + *
 + * Copyright (c) 2012 Samsung Electronics Co., Ltd.
 + * Author: Jingoo Han  jg1@samsung.com
 + *
 + * This program is free software; you can redistribute it and/or modify it
 + * under the terms of the GNU General Public License as published by the
 + * Free Software Foundation; either version 2 of the License, or (at your
 + * option) any later version.
 + */
 +
 +#include linux/backlight.h
 +#include linux/delay.h
 +#include linux/fb.h
 +#include linux/gpio.h
 +#include linux/lcd.h
 +#include linux/module.h
 +#include linux/spi/spi.h
 +#include linux/wait.h
 +
 +#define ENDDEF 0xff00
 +#define COMMAND_ONLY   0x00
 +#define DATA_ONLY  0x01
 +
 +struct lms501kf03 {
 +   struct device   *dev;
 +   struct spi_device   *spi;
 +   unsigned intpower;
 +   struct lcd_device   *ld;
 +   struct lcd_platform_data*lcd_pd;
 +};
 +
 +static const unsigned short seq_password[] = {
 +   0xb9, 0xff, 0x83, 0x69,
 +   ENDDEF
 +};
 +
 +static const unsigned short seq_power[] = {
 +   0xb1, 0x01, 0x00, 0x34, 0x06, 0x00, 0x14, 0x14, 0x20, 0x28,
 +   0x12, 0x12, 0x17, 0x0a, 0x01, 0xe6, 0xe6, 0xe6, 0xe6, 0xe6,
 +   ENDDEF
 +};
 +
 +static const unsigned short seq_display[] = {
 +   0xb2, 0x00, 0x2b, 0x03, 0x03, 0x70, 0x00, 0xff, 0x00, 0x00,
 +   0x00, 0x00, 0x03, 0x03, 0x00, 0x01,
 +   ENDDEF
 +};
 +
 +static const unsigned short seq_rgb_if[] = {
 +   0xb3, 0x09,
 +   ENDDEF
 +};
 +
 +static const unsigned short seq_display_inv[] = {
 +   0xb4, 0x01, 0x08, 0x77, 0x0e, 0x06,
 +   ENDDEF
 +};
 +
 +static const unsigned short seq_vcom[] = {
 +   0xb6, 0x4c, 0x2e,
 +   ENDDEF
 +};
 +
 +static const unsigned short seq_gate[] = {
 +   0xd5, 0x00, 0x05, 0x03, 0x29, 0x01, 0x07, 0x17, 0x68, 0x13,
 +   0x37, 0x20, 0x31, 0x8a, 0x46, 0x9b, 0x57, 0x13, 0x02, 0x75,
 +   0xb9, 0x64, 0xa8, 0x07, 0x0f, 0x04, 0x07,
 +   ENDDEF
 +};
 +
 +static const unsigned short seq_panel[] = {
 +   0xcc, 0x02,
 +   ENDDEF
 +};
 +
 +static const unsigned short seq_col_mod[] = {
 +   0x3a, 0x77,
 +   ENDDEF
 +};
 +
 +static const unsigned short seq_w_gamma[] = {
 +   0xe0, 0x00, 0x04, 0x09, 0x0f, 0x1f, 0x3f, 0x1f, 0x2f, 0x0a,
 +   0x0f, 0x10, 0x16, 0x18, 0x16, 0x17, 0x0d, 0x15, 0x00, 0x04,
 +   0x09, 0x0f, 0x38, 0x3f, 0x20, 0x39, 

Re: PATCH] backlight: add lms501kf03 LCD driver

2012-12-17 Thread Joe Perches
On Mon, 2012-12-17 at 17:22 +0900, Jingoo Han wrote:
 Add the lms501kf03 LCD panel driver. The lms501kf03 LCD panel (800
 x 480) driver uses 3-wired SPI inteface.

A trivial note:

 diff --git a/drivers/video/backlight/lms501kf03.c 
 b/drivers/video/backlight/lms501kf03.c

[]

 +static const unsigned short seq_rgb_gamma[] = {
 + 0xc1, 0x01, 0x03, 0x07, 0x0f, 0x1a, 0x22, 0x2c, 0x33, 0x3c,
 + 0x46, 0x4f, 0x58, 0x60, 0x69, 0x71, 0x79, 0x82, 0x89, 0x92,
 + 0x9a, 0xa1, 0xa9, 0xb1, 0xb9, 0xc1, 0xc9, 0xcf, 0xd6, 0xde,
 + 0xe5, 0xec, 0xf3, 0xf9, 0xff, 0xdd, 0x39, 0x07, 0x1c, 0xcb,
 + 0xab, 0x5f, 0x49, 0x80, 0x03, 0x07, 0x0f, 0x19, 0x20, 0x2a,
 + 0x31, 0x39, 0x42, 0x4b, 0x53, 0x5b, 0x63, 0x6b, 0x73, 0x7b,
 + 0x83, 0x8a, 0x92, 0x9b, 0xa2, 0xaa, 0xb2, 0xba, 0xc2, 0xca,
 + 0xd0, 0xd8, 0xe1, 0xe8, 0xf0, 0xf8, 0xff, 0xf7, 0xd8, 0xbe,
 + 0xa7, 0x39, 0x40, 0x85, 0x8c, 0xc0, 0x04, 0x07, 0x0c, 0x17,
 + 0x1c, 0x23, 0x2b, 0x34, 0x3b, 0x43, 0x4c, 0x54, 0x5b, 0x63,
 + 0x6a, 0x73, 0x7a, 0x82, 0x8a, 0x91, 0x98, 0xa1, 0xa8, 0xb0,
 + 0xb7, 0xc1, 0xc9, 0xcf, 0xd9, 0xe3, 0xea, 0xf4, 0xff, 0x00,
 + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
 + ENDDEF
 +};

All of these ushort arrays could be uchar.

 +static int lms501kf03_spi_write(struct lms501kf03 *lcd, unsigned char 
 address,
 + unsigned char command)
 +{
 + int ret;
 +
 + ret = lms501kf03_spi_write_byte(lcd, address, command);
 +
 + return ret;
 +}
 +
 +static int lms501kf03_panel_send_sequence(struct lms501kf03 *lcd,
 + const unsigned short *wbuf)
 +{
 + int ret = 0, i = 0;
 +
 + while (wbuf[i] != ENDDEF) {

Using an unsigned short where the high order byte
is an end-of-buffer indicator is a bit space wasteful.

Perhaps a sized struct or array instead.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/