[RESEND PATCH v2 1/3] video: ssd1307fb: Use gpiod_set_value_cansleep() for reset

[RESEND PATCH v2 1/3] video: ssd1307fb: Use gpiod_set_value_cansleep() for reset

[Michal Vokáč]

The reset signal can be produced by GPIO expander that can sleep.
In that case the probe function fails. Allow using GPIO expanders for
the reset signal by using the non-atomic gpiod_set_value_cansleep()
function.

Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>
---
v2 changes: add R-by from Fabio

 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 ba66c02..e7ae135 100644
--- a/drivers/video/fbdev/ssd1307fb.c
+++ b/drivers/video/fbdev/ssd1307fb.c
@@ -728,9 +728,9 @@ static int ssd1307fb_probe(struct i2c_client *client,
 
 	if (par->reset) {
 		/* Reset the screen */
-		gpiod_set_value(par->reset, 0);
+		gpiod_set_value_cansleep(par->reset, 0);
 		udelay(4);
-		gpiod_set_value(par->reset, 1);
+		gpiod_set_value_cansleep(par->reset, 1);
 		udelay(4);
 	}
 
-- 
2.1.4

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

[Michal Vokáč]

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>
---
v2 changes: Split the DT changes into separate patch.

 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 e7ae135..7b5bc42 100644
--- a/drivers/video/fbdev/ssd1307fb.c
+++ b/drivers/video/fbdev/ssd1307fb.c
@@ -728,10 +728,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

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

[Michal Vokáč]

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>
---
v2 changes: New patch in the series

 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 e54f5ab..be3406e 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: [RESEND PATCH v2 1/3] video: ssd1307fb: Use gpiod_set_value_cansleep() for reset

[Bartlomiej Zolnierkiewicz]

On 09/27/2018 11:24 AM, Michal Vokáč wrote:
> The reset signal can be produced by GPIO expander that can sleep.
> In that case the probe function fails. Allow using GPIO expanders for
> the reset signal by using the non-atomic gpiod_set_value_cansleep()
> function.
> 
> Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
> Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>

Patch queued for 4.20, thanks.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

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

[Bartlomiej Zolnierkiewicz]

On 09/27/2018 11:24 AM, Michal Vokáč 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.
> 
> Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>

Patch queued for 4.20, thanks.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

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

[Vokáč Michal]

On 27.9.2018 11:24, Michal Vokáč wrote:
> 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>
> ---
> v2 changes: New patch in the series

Bartlomiej just queued the first two patches for v4.20.
Will somebody take this one? Otherwise this SoM will end up with
broken OLED display reset.

Thanks, Michal.

> 
>   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 e54f5ab..be3406e 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>;
> 

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

[Shawn Guo]

On Mon, Oct 08, 2018 at 11:45:36AM +0000, Vokáč Michal wrote:
> On 27.9.2018 11:24, Michal Vokáč wrote:
> > 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>
> > ---
> > v2 changes: New patch in the series
> 
> Bartlomiej just queued the first two patches for v4.20.
> Will somebody take this one? Otherwise this SoM will end up with
> broken OLED display reset.

Well, it means the change breaks the ABI between kernel and device tree,
e.g. the new kernel will not work with existing/installed DTBs.

Shawn

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

[Bartlomiej Zolnierkiewicz]

On 10/09/2018 02:20 AM, Shawn Guo wrote:
> On Mon, Oct 08, 2018 at 11:45:36AM +0000, Vokáč Michal wrote:
>> On 27.9.2018 11:24, Michal Vokáč wrote:
>>> 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>
>>> ---
>>> v2 changes: New patch in the series
>>
>> Bartlomiej just queued the first two patches for v4.20.
>> Will somebody take this one? Otherwise this SoM will end up with
>> broken OLED display reset.
> 
> Well, it means the change breaks the ABI between kernel and device tree,
> e.g. the new kernel will not work with existing/installed DTBs.

Should I revert this sdd10307fb patch then?

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

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

[Vokáč Michal]

On 9.10.2018 09:51, Bartlomiej Zolnierkiewicz wrote:
> 
> On 10/09/2018 02:20 AM, Shawn Guo wrote:
>> On Mon, Oct 08, 2018 at 11:45:36AM +0000, Vokáč Michal wrote:
>>> On 27.9.2018 11:24, Michal Vokáč wrote:
>>>> 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>
>>>> ---
>>>> v2 changes: New patch in the series
>>>
>>> Bartlomiej just queued the first two patches for v4.20.
>>> Will somebody take this one? Otherwise this SoM will end up with
>>> broken OLED display reset.
>>
>> Well, it means the change breaks the ABI between kernel and device tree,
>> e.g. the new kernel will not work with existing/installed DTBs.
> 
> Should I revert this sdd10307fb patch then?

Sorry for the inconvenience :( Lesson learned..

So in other words (no offense): broken drivers need to stay broken because
users may already get used to the broken behavior?

Personally I would not describe this change as a device tree ABI change.
It is not a change in the DT binding. Or are "stable device tree API" and
"device tree ABI" really two different things?

The first patch should be OK though.

Michal

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

[Fabio Estevam]

Hi Michal,

On Tue, Oct 9, 2018 at 5:30 AM Vokáč Michal <Michal.Vokac@ysoft.com> wrote:

> Sorry for the inconvenience :( Lesson learned..
>
> So in other words (no offense): broken drivers need to stay broken because
> users may already get used to the broken behavior?

In order to keep the old dtb's working you could introduce a new
property (like reset-gpio-active-low, for example).

Then the driver behavior can be made untouched for the old dtb's and
only new dtb's with this new property would have the correct GPIO
reset behavior.

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

[Vokáč Michal]

On 9.10.2018 14:36, Fabio Estevam wrote:
> Hi Michal,
> 
> On Tue, Oct 9, 2018 at 5:30 AM Vokáč Michal <Michal.Vokac@ysoft.com> wrote:
> 
>> Sorry for the inconvenience :( Lesson learned..
>>
>> So in other words (no offense): broken drivers need to stay broken because
>> users may already get used to the broken behavior?
> 
> In order to keep the old dtb's working you could introduce a new
> property (like reset-gpio-active-low, for example).
> 
> Then the driver behavior can be made untouched for the old dtb's and
> only new dtb's with this new property would have the correct GPIO
> reset behavior.

Thank you very much Fabio!
I saw these xxx-active-low/high properties in many device tree
sources wondering why the heck people use them when they could
use GPIO_ACTIVE_LOW/HIGH. And this is the explanation.

And I feel like an idiot once again: git grep -l "reset-active-low"
first hit is:

  Documentation/devicetree/bindings/display/ssd1307fb.txt

Oooops.
The weird thing is that usage of reset-active-low is documented
in the example but it is not implemented.

So the patch no.2 should be reverted and patch no.3 not applied at all.

I will prepare a new patch utilizing the reset-active-low property.

Again, sorry for the troubles and thank you for your comments.
Michal

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

[Bartlomiej Zolnierkiewicz]

On 10/09/2018 03:18 PM, Vokáč Michal wrote:
> On 9.10.2018 14:36, Fabio Estevam wrote:
>> Hi Michal,
>>
>> On Tue, Oct 9, 2018 at 5:30 AM Vokáč Michal <Michal.Vokac@ysoft.com> wrote:
>>
>>> Sorry for the inconvenience :( Lesson learned..
>>>
>>> So in other words (no offense): broken drivers need to stay broken because
>>> users may already get used to the broken behavior?
>>
>> In order to keep the old dtb's working you could introduce a new
>> property (like reset-gpio-active-low, for example).
>>
>> Then the driver behavior can be made untouched for the old dtb's and
>> only new dtb's with this new property would have the correct GPIO
>> reset behavior.
> 
> Thank you very much Fabio!
> I saw these xxx-active-low/high properties in many device tree
> sources wondering why the heck people use them when they could
> use GPIO_ACTIVE_LOW/HIGH. And this is the explanation.
> 
> And I feel like an idiot once again: git grep -l "reset-active-low"
> first hit is:
> 
>   Documentation/devicetree/bindings/display/ssd1307fb.txt
> 
> Oooops.
> The weird thing is that usage of reset-active-low is documented
> in the example but it is not implemented.
> 
> So the patch no.2 should be reverted and patch no.3 not applied at all.

OK, I've applied the patch below to fbdev-for-next tree.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics


From 64f83a816b27c7b5e026a74ecb5c61dbabfae997 Mon Sep 17 00:00:00 2001
From: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Date: Tue, 9 Oct 2018 15:18:42 +0200
Subject: [PATCH] Revert "video: ssd1307fb: Do not hard code active-low reset
 sequence"

This reverts commit 9827f26374fb85e1811f2adbcc25c8a3992dbe7f.

On 10/09/2018 02:20 AM, Shawn Guo wrote:

> Well, it means the change breaks the ABI between kernel and device tree,
> e.g. the new kernel will not work with existing/installed DTBs.

Revert the change until DTB compatibility issue is resolved.

Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.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 3b361bc..4061a20 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, 1);
-		udelay(4);
 		gpiod_set_value_cansleep(par->reset, 0);
 		udelay(4);
+		gpiod_set_value_cansleep(par->reset, 1);
+		udelay(4);
 	}
 
 	if (par->vbat_reg) {
-- 
1.9.1