[PATCH 0/3] Few ignored framebuffer fixes/additions

[PATCH 0/3] Few ignored framebuffer fixes/additions

[Maxime Ripard]

Hi Andrew, 

Sorry to bother you again with the framebuffer stuff but the new
maintainer doesn't appear to be responsive either.k

This is a collection of patches that have been silently ignored,
despite being pinged numerous times:
  - Patch 1 has been sent for the first time on June 7 [1], resent on
    June 18 [2], plus two pings on July, 3rd [3] and July, 12th [4]. With
    no comments whatsoever.
  - Patch 2 has been sent on June, 21st [5]. Jean Christophe commented
    on it on June, 24th [6], but every attempts at getting clear
    explanations on what was expected have been ignored ever since
    (June 25th [7], July 12th [8])
  - Patch 3 was sent on June 21st along with Patch 2 [9]. There was
    comments on it and a second version was sent on June 24th
    [10]. Without any comments so far.

Could you take a look at those patches? We already missed 3.11, and
I'd really like not to miss another merge window because of the fbdev
curse.

Thanks,
Maxime

[1]: https://lkml.org/lkml/2013/6/7/116
[2]: https://lkml.org/lkml/2013/6/18/130
[3]: https://lkml.org/lkml/2013/7/3/105
[4]: https://lkml.org/lkml/2013/7/12/177
[5]: https://lkml.org/lkml/2013/6/21/405
[6]: https://lkml.org/lkml/2013/6/24/313
[7]: https://lkml.org/lkml/2013/6/25/274
[8]: https://lkml.org/lkml/2013/7/12/103
[9]: https://lkml.org/lkml/2013/6/21/407
[10]: https://lkml.org/lkml/2013/6/24/302

Alexandre Belloni (1):
  fb: backlight: HX8357: Add HX8369 support

Hector Palacios (1):
  video: mxsfb: fix color settings for 18bit data bus and 32bpp

Maxime Ripard (1):
  video: hx8357: Make IM pins optional

 drivers/video/backlight/hx8357.c | 255 +++++++++++++++++++++++++++++++++------
 drivers/video/mxsfb.c            |  26 ----
 2 files changed, 218 insertions(+), 63 deletions(-)

-- 
1.8.3.2

[PATCH 1/3] video: mxsfb: fix color settings for 18bit data bus and 32bpp

[Maxime Ripard]

From: Hector Palacios <hector.palacios@digi.com>

For a combination of 18bit LCD data bus width and a color
mode of 32bpp, the driver was setting the color mapping to
rgb666, which is wrong, as the color in memory realy has an
rgb888 layout.

This patch also removes the setting of flag CTRL_DF24 that
makes the driver dimiss the upper 2 bits when handling 32/24bpp
colors in a diplay with 18bit data bus width. This flag made
true color images display wrong in such configurations.

Finally, the color mapping rgb666 has also been removed as nobody
is using it and high level applications like Qt5 cannot work
with it either.

Reference: https://lkml.org/lkml/2013/5/23/220
Signed-off-by: Hector Palacios <hector.palacios@digi.com>
Acked-by: Juergen Beisert <jbe@pengutronix.de>
Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/video/mxsfb.c | 26 --------------------------
 1 file changed, 26 deletions(-)

diff --git a/drivers/video/mxsfb.c b/drivers/video/mxsfb.c
index 3ba3771..dc09ebe 100644
--- a/drivers/video/mxsfb.c
+++ b/drivers/video/mxsfb.c
@@ -239,24 +239,6 @@ static const struct fb_bitfield def_rgb565[] = {
 	}
 };
 
-static const struct fb_bitfield def_rgb666[] = {
-	[RED] = {
-		.offset = 16,
-		.length = 6,
-	},
-	[GREEN] = {
-		.offset = 8,
-		.length = 6,
-	},
-	[BLUE] = {
-		.offset = 0,
-		.length = 6,
-	},
-	[TRANSP] = {	/* no support for transparency */
-		.length = 0,
-	}
-};
-
 static const struct fb_bitfield def_rgb888[] = {
 	[RED] = {
 		.offset = 16,
@@ -309,9 +291,6 @@ static int mxsfb_check_var(struct fb_var_screeninfo *var,
 			break;
 		case STMLCDIF_16BIT:
 		case STMLCDIF_18BIT:
-			/* 24 bit to 18 bit mapping */
-			rgb = def_rgb666;
-			break;
 		case STMLCDIF_24BIT:
 			/* real 24 bit */
 			rgb = def_rgb888;
@@ -453,11 +432,6 @@ static int mxsfb_set_par(struct fb_info *fb_info)
 			return -EINVAL;
 		case STMLCDIF_16BIT:
 		case STMLCDIF_18BIT:
-			/* 24 bit to 18 bit mapping */
-			ctrl |= CTRL_DF24; /* ignore the upper 2 bits in
-					    *  each colour component
-					    */
-			break;
 		case STMLCDIF_24BIT:
 			/* real 24 bit */
 			break;
-- 
1.8.3.2

[PATCH 2/3] video: hx8357: Make IM pins optional

[Maxime Ripard]

The IM pins of the HX8357 controller are used to define the interface
used to feed pixel stream to the LCD panel.

Most of the time, these pins are directly routed to either the ground or
the VCC to set their values.

Remove the need to assign GPIOs to these pins when we are in such a case.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/video/backlight/hx8357.c | 52 +++++++++++++++++++++++-----------------
 1 file changed, 30 insertions(+), 22 deletions(-)

diff --git a/drivers/video/backlight/hx8357.c b/drivers/video/backlight/hx8357.c
index a0482b5..ed94796 100644
--- a/drivers/video/backlight/hx8357.c
+++ b/drivers/video/backlight/hx8357.c
@@ -76,6 +76,7 @@ struct hx8357_data {
 	unsigned		reset;
 	struct spi_device	*spi;
 	int			state;
+	bool			use_im_pins;
 };
 
 static u8 hx8357_seq_power[] = {
@@ -250,9 +251,11 @@ static int hx8357_lcd_init(struct lcd_device *lcdev)
 	 * Set the interface selection pins to SPI mode, with three
 	 * wires
 	 */
-	gpio_set_value_cansleep(lcd->im_pins[0], 1);
-	gpio_set_value_cansleep(lcd->im_pins[1], 0);
-	gpio_set_value_cansleep(lcd->im_pins[2], 1);
+	if (lcd->use_im_pins) {
+		gpio_set_value_cansleep(lcd->im_pins[0], 1);
+		gpio_set_value_cansleep(lcd->im_pins[1], 0);
+		gpio_set_value_cansleep(lcd->im_pins[2], 1);
+	}
 
 	/* Reset the screen */
 	gpio_set_value(lcd->reset, 1);
@@ -424,26 +427,31 @@ static int hx8357_probe(struct spi_device *spi)
 		return -EINVAL;
 	}
 
-	for (i = 0; i < HX8357_NUM_IM_PINS; i++) {
-		lcd->im_pins[i] = of_get_named_gpio(spi->dev.of_node,
-						"im-gpios", i);
-		if (lcd->im_pins[i] = -EPROBE_DEFER) {
-			dev_info(&spi->dev, "GPIO requested is not here yet, deferring the probe\n");
-			return -EPROBE_DEFER;
-		}
-		if (!gpio_is_valid(lcd->im_pins[i])) {
-			dev_err(&spi->dev, "Missing dt property: im-gpios\n");
-			return -EINVAL;
+	if (of_find_property(spi->dev.of_node, "im-gpios", NULL)) {
+		lcd->use_im_pins = 1;
+
+		for (i = 0; i < HX8357_NUM_IM_PINS; i++) {
+			lcd->im_pins[i] = of_get_named_gpio(spi->dev.of_node,
+							    "im-gpios", i);
+			if (lcd->im_pins[i] = -EPROBE_DEFER) {
+				dev_info(&spi->dev, "GPIO requested is not here yet, deferring the probe\n");
+				return -EPROBE_DEFER;
+			}
+			if (!gpio_is_valid(lcd->im_pins[i])) {
+				dev_err(&spi->dev, "Missing dt property: im-gpios\n");
+				return -EINVAL;
+			}
+
+			ret = devm_gpio_request_one(&spi->dev, lcd->im_pins[i],
+						    GPIOF_OUT_INIT_LOW, "im_pins");
+			if (ret) {
+				dev_err(&spi->dev, "failed to request gpio %d: %d\n",
+					lcd->im_pins[i], ret);
+				return -EINVAL;
+			}
 		}
-
-		ret = devm_gpio_request_one(&spi->dev, lcd->im_pins[i],
-					GPIOF_OUT_INIT_LOW, "im_pins");
-		if (ret) {
-			dev_err(&spi->dev, "failed to request gpio %d: %d\n",
-				lcd->im_pins[i], ret);
-			return -EINVAL;
-		}
-	}
+	} else
+		lcd->use_im_pins = 0;
 
 	lcdev = lcd_device_register("mxsfb", &spi->dev, lcd, &hx8357_ops);
 	if (IS_ERR(lcdev)) {
-- 
1.8.3.2

Re: [PATCH 2/3] video: hx8357: Make IM pins optional

[Jingoo Han]

On Tuesday, July 16, 2013 12:27 AM, Maxime Ripard wrote:
> 
> The IM pins of the HX8357 controller are used to define the interface
> used to feed pixel stream to the LCD panel.
> 
> Most of the time, these pins are directly routed to either the ground or
> the VCC to set their values.
> 
> Remove the need to assign GPIOs to these pins when we are in such a case.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  drivers/video/backlight/hx8357.c | 52 +++++++++++++++++++++++-----------------
>  1 file changed, 30 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/video/backlight/hx8357.c b/drivers/video/backlight/hx8357.c
> index a0482b5..ed94796 100644
> --- a/drivers/video/backlight/hx8357.c
> +++ b/drivers/video/backlight/hx8357.c
> @@ -76,6 +76,7 @@ struct hx8357_data {
>  	unsigned		reset;
>  	struct spi_device	*spi;
>  	int			state;
> +	bool			use_im_pins;
>  };
> 
>  static u8 hx8357_seq_power[] = {
> @@ -250,9 +251,11 @@ static int hx8357_lcd_init(struct lcd_device *lcdev)
>  	 * Set the interface selection pins to SPI mode, with three
>  	 * wires
>  	 */
> -	gpio_set_value_cansleep(lcd->im_pins[0], 1);
> -	gpio_set_value_cansleep(lcd->im_pins[1], 0);
> -	gpio_set_value_cansleep(lcd->im_pins[2], 1);
> +	if (lcd->use_im_pins) {
> +		gpio_set_value_cansleep(lcd->im_pins[0], 1);
> +		gpio_set_value_cansleep(lcd->im_pins[1], 0);
> +		gpio_set_value_cansleep(lcd->im_pins[2], 1);
> +	}
> 
>  	/* Reset the screen */
>  	gpio_set_value(lcd->reset, 1);
> @@ -424,26 +427,31 @@ static int hx8357_probe(struct spi_device *spi)
>  		return -EINVAL;
>  	}
> 
> -	for (i = 0; i < HX8357_NUM_IM_PINS; i++) {
> -		lcd->im_pins[i] = of_get_named_gpio(spi->dev.of_node,
> -						"im-gpios", i);
> -		if (lcd->im_pins[i] = -EPROBE_DEFER) {
> -			dev_info(&spi->dev, "GPIO requested is not here yet, deferring the probe\n");
> -			return -EPROBE_DEFER;
> -		}
> -		if (!gpio_is_valid(lcd->im_pins[i])) {
> -			dev_err(&spi->dev, "Missing dt property: im-gpios\n");
> -			return -EINVAL;
> +	if (of_find_property(spi->dev.of_node, "im-gpios", NULL)) {
> +		lcd->use_im_pins = 1;
> +
> +		for (i = 0; i < HX8357_NUM_IM_PINS; i++) {
> +			lcd->im_pins[i] = of_get_named_gpio(spi->dev.of_node,
> +							    "im-gpios", i);
> +			if (lcd->im_pins[i] = -EPROBE_DEFER) {
> +				dev_info(&spi->dev, "GPIO requested is not here yet, deferring the
> probe\n");
> +				return -EPROBE_DEFER;
> +			}
> +			if (!gpio_is_valid(lcd->im_pins[i])) {
> +				dev_err(&spi->dev, "Missing dt property: im-gpios\n");
> +				return -EINVAL;
> +			}
> +
> +			ret = devm_gpio_request_one(&spi->dev, lcd->im_pins[i],
> +						    GPIOF_OUT_INIT_LOW, "im_pins");

This makes a checkpatch warning such as 'WARNING: line over 80 characters'.
How about the following?

			ret = devm_gpio_request_one(&spi->dev, lcd->im_pins[i],
						GPIOF_OUT_INIT_LOW, "im_pins");

> +			if (ret) {
> +				dev_err(&spi->dev, "failed to request gpio %d: %d\n",
> +					lcd->im_pins[i], ret);
> +				return -EINVAL;
> +			}
>  		}
> -
> -		ret = devm_gpio_request_one(&spi->dev, lcd->im_pins[i],
> -					GPIOF_OUT_INIT_LOW, "im_pins");
> -		if (ret) {
> -			dev_err(&spi->dev, "failed to request gpio %d: %d\n",
> -				lcd->im_pins[i], ret);
> -			return -EINVAL;
> -		}
> -	}
> +	} else
> +		lcd->use_im_pins = 0;

According to the 'Documentation/CodingStyle', braces are necessary as below.

	} else {
		lcd->use_im_pins = 0;
	}


Others look good.
Acked-by: Jingoo Han <jg1.han@samsung.com>

Best regards,
Jingoo Han

> 
>  	lcdev = lcd_device_register("mxsfb", &spi->dev, lcd, &hx8357_ops);
>  	if (IS_ERR(lcdev)) {
> --
> 1.8.3.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] video: hx8357: Make IM pins optional

[Mike Galbraith]

On Tue, 2013-07-16 at 09:49 +0900, Jingoo Han wrote: 
> On Tuesday, July 16, 2013 12:27 AM, Maxime Ripard wrote:

> > +
> > +			ret = devm_gpio_request_one(&spi->dev, lcd->im_pins[i],
> > +						    GPIOF_OUT_INIT_LOW, "im_pins");
> 
> This makes a checkpatch warning such as 'WARNING: line over 80 characters'.
> How about the following?
> 
> 			ret = devm_gpio_request_one(&spi->dev, lcd->im_pins[i],
> 						GPIOF_OUT_INIT_LOW, "im_pins");

IIRC, some maintainers gripe (davem?) when they see such alignment,
preferring the original arg below arg alignment vs strict 80 column.

-Mike

Re: [PATCH 2/3] video: hx8357: Make IM pins optional

['Maxime Ripard']

[-- Attachment #1: Type: text/plain, Size: 1301 bytes --]

Hi Jingoo, Mike,

On Tue, Jul 16, 2013 at 05:29:40AM +0200, Mike Galbraith wrote:
> On Tue, 2013-07-16 at 09:49 +0900, Jingoo Han wrote: 
> > On Tuesday, July 16, 2013 12:27 AM, Maxime Ripard wrote:
> 
> > > +
> > > +			ret = devm_gpio_request_one(&spi->dev, lcd->im_pins[i],
> > > +						    GPIOF_OUT_INIT_LOW, "im_pins");
> > 
> > This makes a checkpatch warning such as 'WARNING: line over 80 characters'.
> > How about the following?
> > 
> > 			ret = devm_gpio_request_one(&spi->dev, lcd->im_pins[i],
> > 						GPIOF_OUT_INIT_LOW, "im_pins");
> 
> IIRC, some maintainers gripe (davem?) when they see such alignment,
> preferring the original arg below arg alignment vs strict 80 column.

As far as I know, the coding guide styles are quite fuzzy about this:
  - The new line is not required to be aligned with the braces above
  - Yet, the emacs config given does indent like this.
  - 80 characters is said not to be a hard limit

I don't really know if there's a better solution here, except maybe:
ret = devm_gpio_request_one(&spi-dev, lcd->im_pins[i],
			    GPIOF_OUT_INIT_LOW,
			    "im_pins");

But it's not really a big deal, is it?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/3] video: hx8357: Make IM pins optional

[Jingoo Han]

On Friday, July 19, 2013 5:36 PM, Maxime Ripard wrote:
> On Tue, Jul 16, 2013 at 05:29:40AM +0200, Mike Galbraith wrote:
> > On Tue, 2013-07-16 at 09:49 +0900, Jingoo Han wrote:
> > > On Tuesday, July 16, 2013 12:27 AM, Maxime Ripard wrote:
> >
> > > > +
> > > > +			ret = devm_gpio_request_one(&spi->dev, lcd->im_pins[i],
> > > > +						    GPIOF_OUT_INIT_LOW, "im_pins");
> > >
> > > This makes a checkpatch warning such as 'WARNING: line over 80 characters'.
> > > How about the following?
> > >
> > > 			ret = devm_gpio_request_one(&spi->dev, lcd->im_pins[i],
> > > 						GPIOF_OUT_INIT_LOW, "im_pins");
> >
> > IIRC, some maintainers gripe (davem?) when they see such alignment,
> > preferring the original arg below arg alignment vs strict 80 column.
> 
> As far as I know, the coding guide styles are quite fuzzy about this:
>   - The new line is not required to be aligned with the braces above
>   - Yet, the emacs config given does indent like this.
>   - 80 characters is said not to be a hard limit

Even though 80 characters is not a hard limit, 80 characters is preferred
if possible.

> 
> I don't really know if there's a better solution here, except maybe:
> ret = devm_gpio_request_one(&spi-dev, lcd->im_pins[i],
> 			    GPIOF_OUT_INIT_LOW,
> 			    "im_pins");

Yes, I think that this can be used. :)


Best regards,
Jingoo Han

> 
> But it's not really a big deal, is it?
> 
> Maxime
> 
> --
> Maxime Ripard, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com

[PATCH 3/3] fb: backlight: HX8357: Add HX8369 support

[Maxime Ripard]

From: Alexandre Belloni <alexandre.belloni@free-electrons.com>

Add support for the Himax HX8369 controller as it is quite similar to the
hx8357.

Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/video/backlight/hx8357.c | 203 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 188 insertions(+), 15 deletions(-)

diff --git a/drivers/video/backlight/hx8357.c b/drivers/video/backlight/hx8357.c
index ed94796..b035fab 100644
--- a/drivers/video/backlight/hx8357.c
+++ b/drivers/video/backlight/hx8357.c
@@ -71,6 +71,18 @@
 #define HX8357_SET_POWER_NORMAL		0xd2
 #define HX8357_SET_PANEL_RELATED	0xe9
 
+#define HX8369_SET_DISPLAY_BRIGHTNESS		0x51
+#define HX8369_WRITE_CABC_DISPLAY_VALUE		0x53
+#define HX8369_WRITE_CABC_BRIGHT_CTRL		0x55
+#define HX8369_WRITE_CABC_MIN_BRIGHTNESS	0x5e
+#define HX8369_SET_POWER			0xb1
+#define HX8369_SET_DISPLAY_MODE			0xb2
+#define HX8369_SET_DISPLAY_WAVEFORM_CYC		0xb4
+#define HX8369_SET_VCOM				0xb6
+#define HX8369_SET_EXTENSION_COMMAND		0xb9
+#define HX8369_SET_GIP				0xd5
+#define HX8369_SET_GAMMA_CURVE_RELATED		0xe0
+
 struct hx8357_data {
 	unsigned		im_pins[HX8357_NUM_IM_PINS];
 	unsigned		reset;
@@ -144,6 +156,61 @@ static u8 hx8357_seq_display_mode[] = {
 	HX8357_SET_DISPLAY_MODE_RGB_INTERFACE,
 };
 
+static u8 hx8369_seq_write_CABC_min_brightness[] = {
+	HX8369_WRITE_CABC_MIN_BRIGHTNESS, 0x00,
+};
+
+static u8 hx8369_seq_write_CABC_control[] = {
+	HX8369_WRITE_CABC_DISPLAY_VALUE, 0x24,
+};
+
+static u8 hx8369_seq_set_display_brightness[] = {
+	HX8369_SET_DISPLAY_BRIGHTNESS, 0xFF,
+};
+
+static u8 hx8369_seq_write_CABC_control_setting[] = {
+	HX8369_WRITE_CABC_BRIGHT_CTRL, 0x02,
+};
+
+static u8 hx8369_seq_extension_command[] = {
+	HX8369_SET_EXTENSION_COMMAND, 0xff, 0x83, 0x69,
+};
+
+static u8 hx8369_seq_display_related[] = {
+	HX8369_SET_DISPLAY_MODE, 0x00, 0x2b, 0x03, 0x03, 0x70, 0x00,
+	0xff, 0x00, 0x00, 0x00, 0x00, 0x03, 0x03, 0x00,	0x01,
+};
+
+static u8 hx8369_seq_panel_waveform_cycle[] = {
+	HX8369_SET_DISPLAY_WAVEFORM_CYC, 0x0a, 0x1d, 0x80, 0x06, 0x02,
+};
+
+static u8 hx8369_seq_set_address_mode[] = {
+	HX8357_SET_ADDRESS_MODE, 0x00,
+};
+
+static u8 hx8369_seq_vcom[] = {
+	HX8369_SET_VCOM, 0x3e, 0x3e,
+};
+
+static u8 hx8369_seq_gip[] = {
+	HX8369_SET_GIP, 0x00, 0x01, 0x03, 0x25, 0x01, 0x02, 0x28, 0x70,
+	0x11, 0x13, 0x00, 0x00, 0x40, 0x26, 0x51, 0x37, 0x00, 0x00, 0x71,
+	0x35, 0x60, 0x24, 0x07, 0x0f, 0x04, 0x04,
+};
+
+static u8 hx8369_seq_power[] = {
+	HX8369_SET_POWER, 0x01, 0x00, 0x34, 0x03, 0x00, 0x11, 0x11, 0x32,
+	0x2f, 0x3f, 0x3f, 0x01, 0x3a, 0x01, 0xe6, 0xe6, 0xe6, 0xe6, 0xe6,
+};
+
+static u8 hx8369_seq_gamma_curve_related[] = {
+	HX8369_SET_GAMMA_CURVE_RELATED, 0x00, 0x0d, 0x19, 0x2f, 0x3b, 0x3d,
+	0x2e, 0x4a, 0x08, 0x0e, 0x0f, 0x14, 0x16, 0x14, 0x14, 0x14, 0x1e,
+	0x00, 0x0d, 0x19, 0x2f, 0x3b, 0x3d, 0x2e, 0x4a, 0x08, 0x0e, 0x0f,
+	0x14, 0x16, 0x14, 0x14, 0x14, 0x1e,
+};
+
 static int hx8357_spi_write_then_read(struct lcd_device *lcdev,
 				u8 *txbuf, u16 txlen,
 				u8 *rxbuf, u16 rxlen)
@@ -242,6 +309,18 @@ static int hx8357_exit_standby(struct lcd_device *lcdev)
 	return 0;
 }
 
+static void hx8357_lcd_reset(struct lcd_device *lcdev)
+{
+	struct hx8357_data *lcd = lcd_get_data(lcdev);
+
+	gpio_set_value(lcd->reset, 1);
+	usleep_range(10000, 12000);
+	gpio_set_value(lcd->reset, 0);
+	usleep_range(10000, 12000);
+	gpio_set_value(lcd->reset, 1);
+	msleep(120);
+}
+
 static int hx8357_lcd_init(struct lcd_device *lcdev)
 {
 	struct hx8357_data *lcd = lcd_get_data(lcdev);
@@ -257,14 +336,6 @@ static int hx8357_lcd_init(struct lcd_device *lcdev)
 		gpio_set_value_cansleep(lcd->im_pins[2], 1);
 	}
 
-	/* Reset the screen */
-	gpio_set_value(lcd->reset, 1);
-	usleep_range(10000, 12000);
-	gpio_set_value(lcd->reset, 0);
-	usleep_range(10000, 12000);
-	gpio_set_value(lcd->reset, 1);
-	msleep(120);
-
 	ret = hx8357_spi_write_array(lcdev, hx8357_seq_power,
 				ARRAY_SIZE(hx8357_seq_power));
 	if (ret < 0)
@@ -359,6 +430,94 @@ static int hx8357_lcd_init(struct lcd_device *lcdev)
 	return 0;
 }
 
+static int hx8369_lcd_init(struct lcd_device *lcdev)
+{
+	int ret;
+
+	ret = hx8357_spi_write_array(lcdev, hx8369_seq_extension_command,
+				ARRAY_SIZE(hx8369_seq_extension_command));
+	if (ret < 0)
+		return ret;
+	usleep_range(10000, 12000);
+
+	ret = hx8357_spi_write_array(lcdev, hx8369_seq_display_related,
+				ARRAY_SIZE(hx8369_seq_display_related));
+	if (ret < 0)
+		return ret;
+
+	ret = hx8357_spi_write_array(lcdev, hx8369_seq_panel_waveform_cycle,
+				ARRAY_SIZE(hx8369_seq_panel_waveform_cycle));
+	if (ret < 0)
+		return ret;
+
+	ret = hx8357_spi_write_array(lcdev, hx8369_seq_set_address_mode,
+				ARRAY_SIZE(hx8369_seq_set_address_mode));
+	if (ret < 0)
+		return ret;
+
+	ret = hx8357_spi_write_array(lcdev, hx8369_seq_vcom,
+				ARRAY_SIZE(hx8369_seq_vcom));
+	if (ret < 0)
+		return ret;
+
+	ret = hx8357_spi_write_array(lcdev, hx8369_seq_gip,
+				ARRAY_SIZE(hx8369_seq_gip));
+	if (ret < 0)
+		return ret;
+
+	ret = hx8357_spi_write_array(lcdev, hx8369_seq_power,
+				ARRAY_SIZE(hx8369_seq_power));
+	if (ret < 0)
+		return ret;
+
+	ret = hx8357_spi_write_byte(lcdev, HX8357_EXIT_SLEEP_MODE);
+	if (ret < 0)
+		return ret;
+
+	msleep(120);
+
+	ret = hx8357_spi_write_array(lcdev, hx8369_seq_gamma_curve_related,
+				ARRAY_SIZE(hx8369_seq_gamma_curve_related));
+	if (ret < 0)
+		return ret;
+
+	ret = hx8357_spi_write_byte(lcdev, HX8357_EXIT_SLEEP_MODE);
+	if (ret < 0)
+		return ret;
+	usleep_range(1000, 1200);
+
+	ret = hx8357_spi_write_array(lcdev, hx8369_seq_write_CABC_control,
+				ARRAY_SIZE(hx8369_seq_write_CABC_control));
+	if (ret < 0)
+		return ret;
+	usleep_range(10000, 12000);
+
+	ret = hx8357_spi_write_array(lcdev,
+			hx8369_seq_write_CABC_control_setting,
+			ARRAY_SIZE(hx8369_seq_write_CABC_control_setting));
+	if (ret < 0)
+		return ret;
+
+	ret = hx8357_spi_write_array(lcdev,
+			hx8369_seq_write_CABC_min_brightness,
+			ARRAY_SIZE(hx8369_seq_write_CABC_min_brightness));
+	if (ret < 0)
+		return ret;
+	usleep_range(10000, 12000);
+
+	ret = hx8357_spi_write_array(lcdev, hx8369_seq_set_display_brightness,
+				ARRAY_SIZE(hx8369_seq_set_display_brightness));
+	if (ret < 0)
+		return ret;
+
+	ret = hx8357_spi_write_byte(lcdev, HX8357_SET_DISPLAY_ON);
+	if (ret < 0)
+		return ret;
+	msleep(100);
+
+	return 0;
+}
+
 #define POWER_IS_ON(pwr)	((pwr) <= FB_BLANK_NORMAL)
 
 static int hx8357_set_power(struct lcd_device *lcdev, int power)
@@ -391,10 +550,24 @@ static struct lcd_ops hx8357_ops = {
 	.get_power	= hx8357_get_power,
 };
 
+static const struct of_device_id hx8357_dt_ids[] = {
+	{
+		.compatible = "himax,hx8357",
+		.data = hx8357_lcd_init,
+	},
+	{
+		.compatible = "himax,hx8369",
+		.data = hx8369_lcd_init,
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(of, hx8357_dt_ids);
+
 static int hx8357_probe(struct spi_device *spi)
 {
 	struct lcd_device *lcdev;
 	struct hx8357_data *lcd;
+	const struct of_device_id *match;
 	int i, ret;
 
 	lcd = devm_kzalloc(&spi->dev, sizeof(*lcd), GFP_KERNEL);
@@ -411,6 +584,10 @@ static int hx8357_probe(struct spi_device *spi)
 
 	lcd->spi = spi;
 
+	match = of_match_device(hx8357_dt_ids, &spi->dev);
+	if (!match || !match->data)
+		return -EINVAL;
+
 	lcd->reset = of_get_named_gpio(spi->dev.of_node, "gpios-reset", 0);
 	if (!gpio_is_valid(lcd->reset)) {
 		dev_err(&spi->dev, "Missing dt property: gpios-reset\n");
@@ -460,7 +637,9 @@ static int hx8357_probe(struct spi_device *spi)
 	}
 	spi_set_drvdata(spi, lcdev);
 
-	ret = hx8357_lcd_init(lcdev);
+	hx8357_lcd_reset(lcdev);
+
+	ret = ((int (*)(struct lcd_device *))match->data)(lcdev);
 	if (ret) {
 		dev_err(&spi->dev, "Couldn't initialize panel\n");
 		goto init_error;
@@ -483,12 +662,6 @@ static int hx8357_remove(struct spi_device *spi)
 	return 0;
 }
 
-static const struct of_device_id hx8357_dt_ids[] = {
-	{ .compatible = "himax,hx8357" },
-	{},
-};
-MODULE_DEVICE_TABLE(of, hx8357_dt_ids);
-
 static struct spi_driver hx8357_driver = {
 	.probe  = hx8357_probe,
 	.remove = hx8357_remove,
-- 
1.8.3.2

Re: [PATCH 3/3] fb: backlight: HX8357: Add HX8369 support

[Jingoo Han]

On Tuesday, July 16, 2013 12:27 AM, Maxime Ripard wrote:
> 
> From: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> 
> Add support for the Himax HX8369 controller as it is quite similar to the
> hx8357.
> 
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Hi Maxime Ripard,

I reviewed this patch with Himax HX8369 datasheet.
I cannot find any problems. It looks good.

However, if possible, please add comment on huge delays such as
msleep(120), msleep(100), etc.


Acked-by: Jingoo Han <jg1.han@samsung.com>

Best regards,
Jingoo Han

> ---
>  drivers/video/backlight/hx8357.c | 203 ++++++++++++++++++++++++++++++++++++---
>  1 file changed, 188 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/video/backlight/hx8357.c b/drivers/video/backlight/hx8357.c
> index ed94796..b035fab 100644
> --- a/drivers/video/backlight/hx8357.c
> +++ b/drivers/video/backlight/hx8357.c
> @@ -71,6 +71,18 @@
>  #define HX8357_SET_POWER_NORMAL		0xd2
>  #define HX8357_SET_PANEL_RELATED	0xe9
> 
> +#define HX8369_SET_DISPLAY_BRIGHTNESS		0x51
> +#define HX8369_WRITE_CABC_DISPLAY_VALUE		0x53
> +#define HX8369_WRITE_CABC_BRIGHT_CTRL		0x55
> +#define HX8369_WRITE_CABC_MIN_BRIGHTNESS	0x5e
> +#define HX8369_SET_POWER			0xb1
> +#define HX8369_SET_DISPLAY_MODE			0xb2
> +#define HX8369_SET_DISPLAY_WAVEFORM_CYC		0xb4
> +#define HX8369_SET_VCOM				0xb6
> +#define HX8369_SET_EXTENSION_COMMAND		0xb9
> +#define HX8369_SET_GIP				0xd5
> +#define HX8369_SET_GAMMA_CURVE_RELATED		0xe0
> +
>  struct hx8357_data {
>  	unsigned		im_pins[HX8357_NUM_IM_PINS];
>  	unsigned		reset;
> @@ -144,6 +156,61 @@ static u8 hx8357_seq_display_mode[] = {
>  	HX8357_SET_DISPLAY_MODE_RGB_INTERFACE,
>  };
> 
> +static u8 hx8369_seq_write_CABC_min_brightness[] = {
> +	HX8369_WRITE_CABC_MIN_BRIGHTNESS, 0x00,
> +};
> +
> +static u8 hx8369_seq_write_CABC_control[] = {
> +	HX8369_WRITE_CABC_DISPLAY_VALUE, 0x24,
> +};
> +
> +static u8 hx8369_seq_set_display_brightness[] = {
> +	HX8369_SET_DISPLAY_BRIGHTNESS, 0xFF,
> +};
> +
> +static u8 hx8369_seq_write_CABC_control_setting[] = {
> +	HX8369_WRITE_CABC_BRIGHT_CTRL, 0x02,
> +};
> +
> +static u8 hx8369_seq_extension_command[] = {
> +	HX8369_SET_EXTENSION_COMMAND, 0xff, 0x83, 0x69,
> +};
> +
> +static u8 hx8369_seq_display_related[] = {
> +	HX8369_SET_DISPLAY_MODE, 0x00, 0x2b, 0x03, 0x03, 0x70, 0x00,
> +	0xff, 0x00, 0x00, 0x00, 0x00, 0x03, 0x03, 0x00,	0x01,
> +};
> +
> +static u8 hx8369_seq_panel_waveform_cycle[] = {
> +	HX8369_SET_DISPLAY_WAVEFORM_CYC, 0x0a, 0x1d, 0x80, 0x06, 0x02,
> +};
> +
> +static u8 hx8369_seq_set_address_mode[] = {
> +	HX8357_SET_ADDRESS_MODE, 0x00,
> +};
> +
> +static u8 hx8369_seq_vcom[] = {
> +	HX8369_SET_VCOM, 0x3e, 0x3e,
> +};
> +
> +static u8 hx8369_seq_gip[] = {
> +	HX8369_SET_GIP, 0x00, 0x01, 0x03, 0x25, 0x01, 0x02, 0x28, 0x70,
> +	0x11, 0x13, 0x00, 0x00, 0x40, 0x26, 0x51, 0x37, 0x00, 0x00, 0x71,
> +	0x35, 0x60, 0x24, 0x07, 0x0f, 0x04, 0x04,
> +};
> +
> +static u8 hx8369_seq_power[] = {
> +	HX8369_SET_POWER, 0x01, 0x00, 0x34, 0x03, 0x00, 0x11, 0x11, 0x32,
> +	0x2f, 0x3f, 0x3f, 0x01, 0x3a, 0x01, 0xe6, 0xe6, 0xe6, 0xe6, 0xe6,
> +};
> +
> +static u8 hx8369_seq_gamma_curve_related[] = {
> +	HX8369_SET_GAMMA_CURVE_RELATED, 0x00, 0x0d, 0x19, 0x2f, 0x3b, 0x3d,
> +	0x2e, 0x4a, 0x08, 0x0e, 0x0f, 0x14, 0x16, 0x14, 0x14, 0x14, 0x1e,
> +	0x00, 0x0d, 0x19, 0x2f, 0x3b, 0x3d, 0x2e, 0x4a, 0x08, 0x0e, 0x0f,
> +	0x14, 0x16, 0x14, 0x14, 0x14, 0x1e,
> +};
> +
>  static int hx8357_spi_write_then_read(struct lcd_device *lcdev,
>  				u8 *txbuf, u16 txlen,
>  				u8 *rxbuf, u16 rxlen)
> @@ -242,6 +309,18 @@ static int hx8357_exit_standby(struct lcd_device *lcdev)
>  	return 0;
>  }
> 
> +static void hx8357_lcd_reset(struct lcd_device *lcdev)
> +{
> +	struct hx8357_data *lcd = lcd_get_data(lcdev);
> +
> +	gpio_set_value(lcd->reset, 1);
> +	usleep_range(10000, 12000);
> +	gpio_set_value(lcd->reset, 0);
> +	usleep_range(10000, 12000);
> +	gpio_set_value(lcd->reset, 1);
> +	msleep(120);
> +}
> +
>  static int hx8357_lcd_init(struct lcd_device *lcdev)
>  {
>  	struct hx8357_data *lcd = lcd_get_data(lcdev);
> @@ -257,14 +336,6 @@ static int hx8357_lcd_init(struct lcd_device *lcdev)
>  		gpio_set_value_cansleep(lcd->im_pins[2], 1);
>  	}
> 
> -	/* Reset the screen */
> -	gpio_set_value(lcd->reset, 1);
> -	usleep_range(10000, 12000);
> -	gpio_set_value(lcd->reset, 0);
> -	usleep_range(10000, 12000);
> -	gpio_set_value(lcd->reset, 1);
> -	msleep(120);
> -
>  	ret = hx8357_spi_write_array(lcdev, hx8357_seq_power,
>  				ARRAY_SIZE(hx8357_seq_power));
>  	if (ret < 0)
> @@ -359,6 +430,94 @@ static int hx8357_lcd_init(struct lcd_device *lcdev)
>  	return 0;
>  }
> 
> +static int hx8369_lcd_init(struct lcd_device *lcdev)
> +{
> +	int ret;
> +
> +	ret = hx8357_spi_write_array(lcdev, hx8369_seq_extension_command,
> +				ARRAY_SIZE(hx8369_seq_extension_command));
> +	if (ret < 0)
> +		return ret;
> +	usleep_range(10000, 12000);
> +
> +	ret = hx8357_spi_write_array(lcdev, hx8369_seq_display_related,
> +				ARRAY_SIZE(hx8369_seq_display_related));
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = hx8357_spi_write_array(lcdev, hx8369_seq_panel_waveform_cycle,
> +				ARRAY_SIZE(hx8369_seq_panel_waveform_cycle));
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = hx8357_spi_write_array(lcdev, hx8369_seq_set_address_mode,
> +				ARRAY_SIZE(hx8369_seq_set_address_mode));
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = hx8357_spi_write_array(lcdev, hx8369_seq_vcom,
> +				ARRAY_SIZE(hx8369_seq_vcom));
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = hx8357_spi_write_array(lcdev, hx8369_seq_gip,
> +				ARRAY_SIZE(hx8369_seq_gip));
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = hx8357_spi_write_array(lcdev, hx8369_seq_power,
> +				ARRAY_SIZE(hx8369_seq_power));
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = hx8357_spi_write_byte(lcdev, HX8357_EXIT_SLEEP_MODE);
> +	if (ret < 0)
> +		return ret;
> +
> +	msleep(120);
> +
> +	ret = hx8357_spi_write_array(lcdev, hx8369_seq_gamma_curve_related,
> +				ARRAY_SIZE(hx8369_seq_gamma_curve_related));
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = hx8357_spi_write_byte(lcdev, HX8357_EXIT_SLEEP_MODE);
> +	if (ret < 0)
> +		return ret;
> +	usleep_range(1000, 1200);
> +
> +	ret = hx8357_spi_write_array(lcdev, hx8369_seq_write_CABC_control,
> +				ARRAY_SIZE(hx8369_seq_write_CABC_control));
> +	if (ret < 0)
> +		return ret;
> +	usleep_range(10000, 12000);
> +
> +	ret = hx8357_spi_write_array(lcdev,
> +			hx8369_seq_write_CABC_control_setting,
> +			ARRAY_SIZE(hx8369_seq_write_CABC_control_setting));
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = hx8357_spi_write_array(lcdev,
> +			hx8369_seq_write_CABC_min_brightness,
> +			ARRAY_SIZE(hx8369_seq_write_CABC_min_brightness));
> +	if (ret < 0)
> +		return ret;
> +	usleep_range(10000, 12000);
> +
> +	ret = hx8357_spi_write_array(lcdev, hx8369_seq_set_display_brightness,
> +				ARRAY_SIZE(hx8369_seq_set_display_brightness));
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = hx8357_spi_write_byte(lcdev, HX8357_SET_DISPLAY_ON);
> +	if (ret < 0)
> +		return ret;
> +	msleep(100);
> +
> +	return 0;
> +}
> +
>  #define POWER_IS_ON(pwr)	((pwr) <= FB_BLANK_NORMAL)
> 
>  static int hx8357_set_power(struct lcd_device *lcdev, int power)
> @@ -391,10 +550,24 @@ static struct lcd_ops hx8357_ops = {
>  	.get_power	= hx8357_get_power,
>  };
> 
> +static const struct of_device_id hx8357_dt_ids[] = {
> +	{
> +		.compatible = "himax,hx8357",
> +		.data = hx8357_lcd_init,
> +	},
> +	{
> +		.compatible = "himax,hx8369",
> +		.data = hx8369_lcd_init,
> +	},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, hx8357_dt_ids);
> +
>  static int hx8357_probe(struct spi_device *spi)
>  {
>  	struct lcd_device *lcdev;
>  	struct hx8357_data *lcd;
> +	const struct of_device_id *match;
>  	int i, ret;
> 
>  	lcd = devm_kzalloc(&spi->dev, sizeof(*lcd), GFP_KERNEL);
> @@ -411,6 +584,10 @@ static int hx8357_probe(struct spi_device *spi)
> 
>  	lcd->spi = spi;
> 
> +	match = of_match_device(hx8357_dt_ids, &spi->dev);
> +	if (!match || !match->data)
> +		return -EINVAL;
> +
>  	lcd->reset = of_get_named_gpio(spi->dev.of_node, "gpios-reset", 0);
>  	if (!gpio_is_valid(lcd->reset)) {
>  		dev_err(&spi->dev, "Missing dt property: gpios-reset\n");
> @@ -460,7 +637,9 @@ static int hx8357_probe(struct spi_device *spi)
>  	}
>  	spi_set_drvdata(spi, lcdev);
> 
> -	ret = hx8357_lcd_init(lcdev);
> +	hx8357_lcd_reset(lcdev);
> +
> +	ret = ((int (*)(struct lcd_device *))match->data)(lcdev);
>  	if (ret) {
>  		dev_err(&spi->dev, "Couldn't initialize panel\n");
>  		goto init_error;
> @@ -483,12 +662,6 @@ static int hx8357_remove(struct spi_device *spi)
>  	return 0;
>  }
> 
> -static const struct of_device_id hx8357_dt_ids[] = {
> -	{ .compatible = "himax,hx8357" },
> -	{},
> -};
> -MODULE_DEVICE_TABLE(of, hx8357_dt_ids);
> -
>  static struct spi_driver hx8357_driver = {
>  	.probe  = hx8357_probe,
>  	.remove = hx8357_remove,
> --
> 1.8.3.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] fb: backlight: HX8357: Add HX8369 support

['Maxime Ripard']

[-- Attachment #1: Type: text/plain, Size: 1039 bytes --]

Hi Jingoo,

On Tue, Jul 16, 2013 at 11:04:09AM +0900, Jingoo Han wrote:
> On Tuesday, July 16, 2013 12:27 AM, Maxime Ripard wrote:
> > 
> > From: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> > 
> > Add support for the Himax HX8369 controller as it is quite similar to the
> > hx8357.
> > 
> > Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> 
> Hi Maxime Ripard,
> 
> I reviewed this patch with Himax HX8369 datasheet.
> I cannot find any problems. It looks good.
> 
> However, if possible, please add comment on huge delays such as
> msleep(120), msleep(100), etc.

Right. I'll send a follow-up patch if this is merged right away, or
merge the comments in the next iteration of the patches.

> Acked-by: Jingoo Han <jg1.han@samsung.com>

Thanks for taking the time to review this!

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 0/3] Few ignored framebuffer fixes/additions

[Andrew Morton]

On Mon, 15 Jul 2013 17:26:59 +0200 Maxime Ripard <maxime.ripard@free-electrons.com> wrote:

> Sorry to bother you again with the framebuffer stuff but the new
> maintainer doesn't appear to be responsive either.

Jean-Christophe is doing things - on 11 July he sent out a call for
late patches to linux-fbdev@vger.kernel.org.

So hopefully the resend of this patch series will be handled
appropriately?

Re: [PATCH 0/3] Few ignored framebuffer fixes/additions

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 708 bytes --]

Hi Andrew,

On Wed, Jul 17, 2013 at 02:37:09PM -0700, Andrew Morton wrote:
> On Mon, 15 Jul 2013 17:26:59 +0200 Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> 
> > Sorry to bother you again with the framebuffer stuff but the new
> > maintainer doesn't appear to be responsive either.
> 
> Jean-Christophe is doing things - on 11 July he sent out a call for
> late patches to linux-fbdev@vger.kernel.org.
> 
> So hopefully the resend of this patch series will be handled
> appropriately?

Hopefully. We'll see how it turns out then, and let you know.

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]