[PATCH 1/2] video: ssd1307fb: Use gpiod_set_value_cansleep() for reset

[PATCH 1/2] 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>
---
 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

[PATCH 2/2] 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.

The only single in-tree user of the display is converted and builds
fine.

Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
---
I am not really sure wheater this should be in one commit or the DT
changes should be done in separate commit. Just tell me and I will
split/merge the changes as you want. Thanks.

 arch/arm/boot/dts/imx28-cfa10036.dts | 3 ++-
 drivers/video/fbdev/ssd1307fb.c      | 4 ++--
 2 files changed, 4 insertions(+), 3 deletions(-)

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>;
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

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

[Fabio Estevam]

Hi Michal,

On Wed, Sep 19, 2018 at 6:18 AM, Michal Vokáč <michal.vokac@ysoft.com> 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.
>
> The only single in-tree user of the display is converted and builds
> fine.
>
> Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
> ---
> I am not really sure wheater this should be in one commit or the DT
> changes should be done in separate commit. Just tell me and I will
> split/merge the changes as you want. Thanks.

Please separate in two patches: one for the dts and another for the driver.

Thanks

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

[Michal Vokáč]

On 19.9.2018 14:12, Fabio Estevam wrote:
> Hi Michal,
> 
> On Wed, Sep 19, 2018 at 6:18 AM, Michal Vokáč <michal.vokac@ysoft.com> 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.
>>
>> The only single in-tree user of the display is converted and builds
>> fine.
>>
>> Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
>> ---
>> I am not really sure wheater this should be in one commit or the DT
>> changes should be done in separate commit. Just tell me and I will
>> split/merge the changes as you want. Thanks.
> 
> Please separate in two patches: one for the dts and another for the driver.

OK, thank you. I will send v2 shortly.

Michal

Re: [PATCH 1/2] video: ssd1307fb: Use gpiod_set_value_cansleep() for reset

[Fabio Estevam]

Hi Michal,

On Wed, Sep 19, 2018 at 6:18 AM, Michal Vokáč <michal.vokac@ysoft.com> 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>