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
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áč <[email protected]>
---
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
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áč <[email protected]>
---
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
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áč <[email protected]>
---
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
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áč <[email protected]>
---
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
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áč <[email protected]>
> ---
> 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 <[email protected]>
> + 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
>
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áč <[email protected]>
>> ---
>> 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 <[email protected]>
>
>> + 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
>>