2018-09-19 09:21:02

by Michal Vokáč

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

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áč <[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 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



2018-09-19 09:21:14

by Michal Vokáč

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

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


2018-09-19 12:12:04

by Fabio Estevam

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

Hi Michal,

On Wed, Sep 19, 2018 at 6:18 AM, Michal Vokáč <[email protected]> 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áč <[email protected]>

Reviewed-by: Fabio Estevam <[email protected]>

2018-09-19 12:13:55

by Fabio Estevam

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

Hi Michal,

On Wed, Sep 19, 2018 at 6:18 AM, Michal Vokáč <[email protected]> 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áč <[email protected]>
> ---
> 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

2018-09-19 12:58:08

by Michal Vokáč

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

On 19.9.2018 14:12, Fabio Estevam wrote:
> Hi Michal,
>
> On Wed, Sep 19, 2018 at 6:18 AM, Michal Vokáč <[email protected]> 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áč <[email protected]>
>> ---
>> 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