[PATCH 0/4] Fix ssd1307fb OLED driver reset

[PATCH 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                       | 45 ++++++++++++++++++++++
 drivers/video/fbdev/ssd1307fb.c                    |  4 +-
 4 files changed, 49 insertions(+), 5 deletions(-)

-- 
2.1.4

[PATCH 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")
Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
---
 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

[PATCH 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.

Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
---
 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

[PATCH 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.

Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
---
 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

[PATCH 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.

Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
---
 arch/arm/mach-mxs/mach-mxs.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/arch/arm/mach-mxs/mach-mxs.c b/arch/arm/mach-mxs/mach-mxs.c
index 1c6062d..23c260c 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,53 @@ 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 i, 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;
+	for (i = 0; i < OLED_RESET_GPIO_LEN; i++) {
+		ret = of_property_read_u32_index(np, "reset-gpios", i,
+						 &gpiospec[i]);
+		if (ret) {
+			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 4/4] ARM: mxs: cfa10036: Fixup OLED display reset polarity

[Rob Herring]

On Tue, Dec 04, 2018 at 03:03:40PM +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.
> 
> Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
> ---
>  arch/arm/mach-mxs/mach-mxs.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/arch/arm/mach-mxs/mach-mxs.c b/arch/arm/mach-mxs/mach-mxs.c
> index 1c6062d..23c260c 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,53 @@ 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 i, 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;
> +	for (i = 0; i < OLED_RESET_GPIO_LEN; i++) {
> +		ret = of_property_read_u32_index(np, "reset-gpios", i,
> +						 &gpiospec[i]);

Don't we have a helper to read the whole array?

Otherwise, for the series:

Reviewed-by: Rob Herring <robh@kernel.org>

> +		if (ret) {
> +			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 4/4] ARM: mxs: cfa10036: Fixup OLED display reset polarity

[Vokáč Michal]

On 19.12.2018 18:29, Rob Herring wrote:
> On Tue, Dec 04, 2018 at 03:03:40PM +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.
>>
>> Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
>> ---
>>   arch/arm/mach-mxs/mach-mxs.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 45 insertions(+)
>>
>> diff --git a/arch/arm/mach-mxs/mach-mxs.c b/arch/arm/mach-mxs/mach-mxs.c
>> index 1c6062d..23c260c 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,53 @@ 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 i, 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;
>> +	for (i = 0; i < OLED_RESET_GPIO_LEN; i++) {
>> +		ret = of_property_read_u32_index(np, "reset-gpios", i,
>> +						 &gpiospec[i]);
> 
> Don't we have a helper to read the whole array?

Hi Rob,
yep, I will fix this.
Thank you for your review and for pointing me to the right solution.

Michal

> 
> Otherwise, for the series:
> 
> Reviewed-by: Rob Herring <robh@kernel.org>
> 
>> +		if (ret) {
>> +			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
>>