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
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 <[email protected]>
Signed-off-by: Michal Vokáč <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Michal Vokáč <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Michal Vokáč <[email protected]>
> ---
> 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
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 <[email protected]>
Signed-off-by: Michal Vokáč <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Michal Vokáč <[email protected]>
Reviewed-by: Alexandre Belloni <[email protected]>
> ---
> 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
The reset signal of the SSD1306 OLED display is actually active-low.
Adapt the DT to reflect the real world.
Reviewed-by: Rob Herring <[email protected]>
Signed-off-by: Michal Vokáč <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Michal Vokáč <[email protected]>
Reviewed-by: Alexandre Belloni <[email protected]>
> ---
> 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
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 <[email protected]>
> Signed-off-by: Michal Vokáč <[email protected]>
Acked-by: Alexandre Belloni <[email protected]>
> ---
> 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