[PATCH v2 0/4] Fix ssd1307fb OLED driver reset

[PATCH v2 0/4] Fix ssd1307fb OLED driver reset

[Vokáč Michal]

Hi all,

this is my third attempt to fix the ssd1307fb OLED driver reset. In the
second attempt [1] Rob suggested to take different aproach. That is to
apply what was originaly part of the first round and eventually merged
but reverted later on [2][3].

Next step is to apply a fixup for the single user (in-tree) that uses
this OLED. As Rob suggested, the fixup may be applied only after
someone complains their display broke.

I am not really sure what is the propper way to handle this so the
series contains the original patches + a minor fix in the docs and
the fixup as the last patch.

Adding Alexandre and Maxime from Bootlin to the Cc list as you seem
to be the last ones who touched the Crystalfontz platform. Your coment
regarding the status of the platform and whether the fixup should be
applied straight away or not at all will be appreciated.

Thank you,
Michal

[1] https://patchwork.kernel.org/patch/10665597/#22327227
[2] https://patchwork.kernel.org/patch/10617729/
[3] https://patchwork.kernel.org/patch/10617731/#22257175

Michal Vokáč (4):
  dt-bindings: display: ssd1307fb: Remove reset-active-low from examples
  video: ssd1307fb: Do not hard code active-low reset sequence
  ARM: dts: imx28-cfa10036: Fix the reset gpio signal polarity
  ARM: mxs: cfa10036: Fixup OLED display reset polarity

 .../devicetree/bindings/display/ssd1307fb.txt      |  2 -
 arch/arm/boot/dts/imx28-cfa10036.dts               |  3 +-
 arch/arm/mach-mxs/mach-mxs.c                       | 44 ++++++++++++++++++++++
 drivers/video/fbdev/ssd1307fb.c                    |  4 +-
 4 files changed, 48 insertions(+), 5 deletions(-)

-- 
2.1.4

[PATCH v2 1/4] dt-bindings: display: ssd1307fb: Remove reset-active-low from examples

[Vokáč Michal]

The reset-active-low property has been removed brom the binding a while
ago. So remove it from the examples as well.

Fixes: 519b4db ("fbdev: ssd1307fb: Remove reset-active-low from the DT binding document")
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
---
Changes from v1:
 - Add R-by from Rob

 Documentation/devicetree/bindings/display/ssd1307fb.txt | 2 --
 1 file changed, 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/ssd1307fb.txt b/Documentation/devicetree/bindings/display/ssd1307fb.txt
index 209d931..b67f8ca 100644
--- a/Documentation/devicetree/bindings/display/ssd1307fb.txt
+++ b/Documentation/devicetree/bindings/display/ssd1307fb.txt
@@ -36,7 +36,6 @@ ssd1307: oled@3c {
         reg = <0x3c>;
         pwms = <&pwm 4 3000>;
         reset-gpios = <&gpio2 7>;
-        reset-active-low;
 };
 
 ssd1306: oled@3c {
@@ -44,7 +43,6 @@ ssd1306: oled@3c {
         reg = <0x3c>;
         pwms = <&pwm 4 3000>;
         reset-gpios = <&gpio2 7>;
-        reset-active-low;
         solomon,com-lrremap;
         solomon,com-invdir;
         solomon,com-offset = <32>;
-- 
2.1.4

Re: [PATCH v2 1/4] dt-bindings: display: ssd1307fb: Remove reset-active-low from examples

[Alexandre Belloni]

On 20/12/2018 12:13:54+0000, Vokáč Michal wrote:
> The reset-active-low property has been removed brom the binding a while
> ago. So remove it from the examples as well.
> 
> Fixes: 519b4db ("fbdev: ssd1307fb: Remove reset-active-low from the DT binding document")
> Reviewed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

> ---
> Changes from v1:
>  - Add R-by from Rob
> 
>  Documentation/devicetree/bindings/display/ssd1307fb.txt | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/ssd1307fb.txt b/Documentation/devicetree/bindings/display/ssd1307fb.txt
> index 209d931..b67f8ca 100644
> --- a/Documentation/devicetree/bindings/display/ssd1307fb.txt
> +++ b/Documentation/devicetree/bindings/display/ssd1307fb.txt
> @@ -36,7 +36,6 @@ ssd1307: oled@3c {
>          reg = <0x3c>;
>          pwms = <&pwm 4 3000>;
>          reset-gpios = <&gpio2 7>;
> -        reset-active-low;
>  };
>  
>  ssd1306: oled@3c {
> @@ -44,7 +43,6 @@ ssd1306: oled@3c {
>          reg = <0x3c>;
>          pwms = <&pwm 4 3000>;
>          reset-gpios = <&gpio2 7>;
> -        reset-active-low;
>          solomon,com-lrremap;
>          solomon,com-invdir;
>          solomon,com-offset = <32>;
> -- 
> 2.1.4
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH v2 2/4] video: ssd1307fb: Do not hard code active-low reset sequence

[Vokáč Michal]

The SSD130x OLED display reset signal is active low. Now the reset
sequence is implemented in such a way that users are forced to
define reset-gpios as GPIO_ACTIVE_HIGH in DT to make the reset work.

Do not hard code the active-low sequence into the driver but instead
allow the user to specify the gpio as GPIO_ACTIVE_LOW to reflect
the real world.

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
---
Changes from v1:
 - Add R-by from Rob

 drivers/video/fbdev/ssd1307fb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/video/fbdev/ssd1307fb.c b/drivers/video/fbdev/ssd1307fb.c
index 4061a20..3b361bc 100644
--- a/drivers/video/fbdev/ssd1307fb.c
+++ b/drivers/video/fbdev/ssd1307fb.c
@@ -667,10 +667,10 @@ static int ssd1307fb_probe(struct i2c_client *client,
 
 	if (par->reset) {
 		/* Reset the screen */
-		gpiod_set_value_cansleep(par->reset, 0);
-		udelay(4);
 		gpiod_set_value_cansleep(par->reset, 1);
 		udelay(4);
+		gpiod_set_value_cansleep(par->reset, 0);
+		udelay(4);
 	}
 
 	if (par->vbat_reg) {
-- 
2.1.4

Re: [PATCH v2 2/4] video: ssd1307fb: Do not hard code active-low reset sequence

[Alexandre Belloni]

On 20/12/2018 12:13:55+0000, Vokáč Michal wrote:
> The SSD130x OLED display reset signal is active low. Now the reset
> sequence is implemented in such a way that users are forced to
> define reset-gpios as GPIO_ACTIVE_HIGH in DT to make the reset work.
> 
> Do not hard code the active-low sequence into the driver but instead
> allow the user to specify the gpio as GPIO_ACTIVE_LOW to reflect
> the real world.
> 
> Reviewed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

> ---
> Changes from v1:
>  - Add R-by from Rob
> 
>  drivers/video/fbdev/ssd1307fb.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/video/fbdev/ssd1307fb.c b/drivers/video/fbdev/ssd1307fb.c
> index 4061a20..3b361bc 100644
> --- a/drivers/video/fbdev/ssd1307fb.c
> +++ b/drivers/video/fbdev/ssd1307fb.c
> @@ -667,10 +667,10 @@ static int ssd1307fb_probe(struct i2c_client *client,
>  
>  	if (par->reset) {
>  		/* Reset the screen */
> -		gpiod_set_value_cansleep(par->reset, 0);
> -		udelay(4);
>  		gpiod_set_value_cansleep(par->reset, 1);
>  		udelay(4);
> +		gpiod_set_value_cansleep(par->reset, 0);
> +		udelay(4);
>  	}
>  
>  	if (par->vbat_reg) {
> -- 
> 2.1.4
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH v2 3/4] ARM: dts: imx28-cfa10036: Fix the reset gpio signal polarity

[Vokáč Michal]

The reset signal of the SSD1306 OLED display is actually active-low.
Adapt the DT to reflect the real world.

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
---
Changes from v1:
 - Add R-by from Rob

 arch/arm/boot/dts/imx28-cfa10036.dts | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/imx28-cfa10036.dts b/arch/arm/boot/dts/imx28-cfa10036.dts
index 8337ca2..d6eca31 100644
--- a/arch/arm/boot/dts/imx28-cfa10036.dts
+++ b/arch/arm/boot/dts/imx28-cfa10036.dts
@@ -11,6 +11,7 @@
 
 /dts-v1/;
 #include "imx28.dtsi"
+#include <dt-bindings/gpio/gpio.h>
 
 / {
 	model = "Crystalfontz CFA-10036 Board";
@@ -95,7 +96,7 @@
 					pinctrl-names = "default";
 					pinctrl-0 = <&ssd1306_cfa10036>;
 					reg = <0x3c>;
-					reset-gpios = <&gpio2 7 0>;
+					reset-gpios = <&gpio2 7 GPIO_ACTIVE_LOW>;
 					solomon,height = <32>;
 					solomon,width = <128>;
 					solomon,page-offset = <0>;
-- 
2.1.4

Re: [PATCH v2 3/4] ARM: dts: imx28-cfa10036: Fix the reset gpio signal polarity

[Alexandre Belloni]

On 20/12/2018 12:13:56+0000, Vokáč Michal wrote:
> The reset signal of the SSD1306 OLED display is actually active-low.
> Adapt the DT to reflect the real world.
> 
> Reviewed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

> ---
> Changes from v1:
>  - Add R-by from Rob
> 
>  arch/arm/boot/dts/imx28-cfa10036.dts | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/imx28-cfa10036.dts b/arch/arm/boot/dts/imx28-cfa10036.dts
> index 8337ca2..d6eca31 100644
> --- a/arch/arm/boot/dts/imx28-cfa10036.dts
> +++ b/arch/arm/boot/dts/imx28-cfa10036.dts
> @@ -11,6 +11,7 @@
>  
>  /dts-v1/;
>  #include "imx28.dtsi"
> +#include <dt-bindings/gpio/gpio.h>
>  
>  / {
>  	model = "Crystalfontz CFA-10036 Board";
> @@ -95,7 +96,7 @@
>  					pinctrl-names = "default";
>  					pinctrl-0 = <&ssd1306_cfa10036>;
>  					reg = <0x3c>;
> -					reset-gpios = <&gpio2 7 0>;
> +					reset-gpios = <&gpio2 7 GPIO_ACTIVE_LOW>;
>  					solomon,height = <32>;
>  					solomon,width = <128>;
>  					solomon,page-offset = <0>;
> -- 
> 2.1.4
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH v2 4/4] ARM: mxs: cfa10036: Fixup OLED display reset polarity

[Vokáč Michal]

There was a bug in reset signal generation in ssd1307fb OLED driver.
The display needs an active-low reset signal but the driver produced
the correct sequence only if the GPIO used for reset was specified as
GPIO_ACTIVE_HIGH.

Now as the OLED driver is fixed it is also necessarry to implement
a fixup for all current users of the old DT ABI. There is only one
in-tree user and that is the Crystalfontz CFA-10036 board. In case
this board is booting and GPIO_ACTIVE_HIGH is used for reset we
override it to GPIO_ACTIVE_LOW.

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
---
Changes from v1:
 - Add R-by from Rob
 - Use of_property_read_variable_u32_array to read the GPIO specifier
   array instead of reading it manualy in for cycle. (Rob)

 arch/arm/mach-mxs/mach-mxs.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/arch/arm/mach-mxs/mach-mxs.c b/arch/arm/mach-mxs/mach-mxs.c
index 1c6062d..50038d6 100644
--- a/arch/arm/mach-mxs/mach-mxs.c
+++ b/arch/arm/mach-mxs/mach-mxs.c
@@ -21,6 +21,7 @@
 #include <linux/reboot.h>
 #include <linux/micrel_phy.h>
 #include <linux/of_address.h>
+#include <linux/of_gpio.h>
 #include <linux/of_platform.h>
 #include <linux/phy.h>
 #include <linux/pinctrl/consumer.h>
@@ -268,9 +269,52 @@ static void __init apx4devkit_init(void)
 					   apx4devkit_phy_fixup);
 }
 
+#define OLED_RESET_GPIO_LEN	3
+#define OLED_RESET_GPIO_SIZE	(OLED_RESET_GPIO_LEN * sizeof(u32))
+
+static void __init crystalfontz_oled_reset_fixup(void)
+{
+	struct property *newgpio;
+	struct device_node *np;
+	u32 *gpiospec;
+	int ret;
+
+	np = of_find_compatible_node(NULL, NULL, "solomon,ssd1306fb-i2c");
+	if (!np)
+		return;
+
+	newgpio = kzalloc(sizeof(*newgpio) + OLED_RESET_GPIO_SIZE, GFP_KERNEL);
+	if (!newgpio)
+		return;
+
+	newgpio->value = newgpio + 1;
+	newgpio->length = OLED_RESET_GPIO_SIZE;
+	newgpio->name = kstrdup("reset-gpios", GFP_KERNEL);
+	if (!newgpio->name) {
+		kfree(newgpio);
+		return;
+	}
+
+	gpiospec = newgpio->value;
+	ret = of_property_read_variable_u32_array(np, "reset-gpios", gpiospec,
+						  OLED_RESET_GPIO_LEN, 0);
+
+	if (ret < 0) {
+		kfree(newgpio);
+		return;
+	}
+
+	if (!(gpiospec[2] & OF_GPIO_ACTIVE_LOW)) {
+		gpiospec[2] |= OF_GPIO_ACTIVE_LOW;
+		cpu_to_be32_array(gpiospec, gpiospec, OLED_RESET_GPIO_LEN);
+		of_update_property(np, newgpio);
+	}
+}
+
 static void __init crystalfontz_init(void)
 {
 	update_fec_mac_prop(OUI_CRYSTALFONTZ);
+	crystalfontz_oled_reset_fixup();
 }
 
 static void __init duckbill_init(void)
-- 
2.1.4

Re: [PATCH v2 4/4] ARM: mxs: cfa10036: Fixup OLED display reset polarity

[Alexandre Belloni]

On 20/12/2018 12:13:57+0000, Vokáč Michal wrote:
> There was a bug in reset signal generation in ssd1307fb OLED driver.
> The display needs an active-low reset signal but the driver produced
> the correct sequence only if the GPIO used for reset was specified as
> GPIO_ACTIVE_HIGH.
> 
> Now as the OLED driver is fixed it is also necessarry to implement
> a fixup for all current users of the old DT ABI. There is only one
> in-tree user and that is the Crystalfontz CFA-10036 board. In case
> this board is booting and GPIO_ACTIVE_HIGH is used for reset we
> override it to GPIO_ACTIVE_LOW.
> 

Honestly, the platform has been discontinued and I don't really think
it is worth having that fixup in the tree forever.

> Reviewed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
> ---
> Changes from v1:
>  - Add R-by from Rob
>  - Use of_property_read_variable_u32_array to read the GPIO specifier
>    array instead of reading it manualy in for cycle. (Rob)
> 
>  arch/arm/mach-mxs/mach-mxs.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/arch/arm/mach-mxs/mach-mxs.c b/arch/arm/mach-mxs/mach-mxs.c
> index 1c6062d..50038d6 100644
> --- a/arch/arm/mach-mxs/mach-mxs.c
> +++ b/arch/arm/mach-mxs/mach-mxs.c
> @@ -21,6 +21,7 @@
>  #include <linux/reboot.h>
>  #include <linux/micrel_phy.h>
>  #include <linux/of_address.h>
> +#include <linux/of_gpio.h>
>  #include <linux/of_platform.h>
>  #include <linux/phy.h>
>  #include <linux/pinctrl/consumer.h>
> @@ -268,9 +269,52 @@ static void __init apx4devkit_init(void)
>  					   apx4devkit_phy_fixup);
>  }
>  
> +#define OLED_RESET_GPIO_LEN	3
> +#define OLED_RESET_GPIO_SIZE	(OLED_RESET_GPIO_LEN * sizeof(u32))
> +
> +static void __init crystalfontz_oled_reset_fixup(void)
> +{
> +	struct property *newgpio;
> +	struct device_node *np;
> +	u32 *gpiospec;
> +	int ret;
> +
> +	np = of_find_compatible_node(NULL, NULL, "solomon,ssd1306fb-i2c");
> +	if (!np)
> +		return;
> +
> +	newgpio = kzalloc(sizeof(*newgpio) + OLED_RESET_GPIO_SIZE, GFP_KERNEL);
> +	if (!newgpio)
> +		return;
> +
> +	newgpio->value = newgpio + 1;
> +	newgpio->length = OLED_RESET_GPIO_SIZE;
> +	newgpio->name = kstrdup("reset-gpios", GFP_KERNEL);
> +	if (!newgpio->name) {
> +		kfree(newgpio);
> +		return;
> +	}
> +
> +	gpiospec = newgpio->value;
> +	ret = of_property_read_variable_u32_array(np, "reset-gpios", gpiospec,
> +						  OLED_RESET_GPIO_LEN, 0);
> +
> +	if (ret < 0) {
> +		kfree(newgpio);
> +		return;
> +	}
> +
> +	if (!(gpiospec[2] & OF_GPIO_ACTIVE_LOW)) {
> +		gpiospec[2] |= OF_GPIO_ACTIVE_LOW;
> +		cpu_to_be32_array(gpiospec, gpiospec, OLED_RESET_GPIO_LEN);
> +		of_update_property(np, newgpio);
> +	}
> +}
> +
>  static void __init crystalfontz_init(void)
>  {
>  	update_fec_mac_prop(OUI_CRYSTALFONTZ);
> +	crystalfontz_oled_reset_fixup();
>  }
>  
>  static void __init duckbill_init(void)
> -- 
> 2.1.4
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com