[PATCH fbdev-for-next 1/2] dt-bindings: display: ssd1307fb: Add reset-active-low property

[PATCH fbdev-for-next 1/2] dt-bindings: display: ssd1307fb: Add reset-active-low property

[Vokáč Michal]

This reverts commit 519b4dba586198eed8f72ba07bc71808af2e0e32.
It is true that the actual implementation has never been there. But
contrary to what the reverted commit message says it does make sense
to add it.

Current implementation of the reset signal is hard-coded to active low
with the assumption that reset-gpios is specified as GPIO_ACTIVE_HIGH.
That is technically wrong as the DTS authors should know that SSD130x
displays need active low reset and hence they are temped to use
GPIO_ACTIVE_LOW. But with that the reset is broken. So reset-acive-low
property can be used to invert the signal once again to fix this.

Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
---
 Documentation/devicetree/bindings/display/ssd1307fb.txt | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/ssd1307fb.txt b/Documentation/devicetree/bindings/display/ssd1307fb.txt
index 209d931..a5ead10 100644
--- a/Documentation/devicetree/bindings/display/ssd1307fb.txt
+++ b/Documentation/devicetree/bindings/display/ssd1307fb.txt
@@ -16,6 +16,8 @@ Required properties:
 Optional properties:
   - reset-gpios: The GPIO used to reset the OLED display, if available. See
                  Documentation/devicetree/bindings/gpio/gpio.txt for details.
+  - reset-active-low: Bool flag to indicate the GPIO specified in "reset-gpios"
+                      property is active low.
   - vbat-supply: The supply for VBAT
   - solomon,segment-no-remap: Display needs normal (non-inverted) data column
                               to segment mapping
@@ -35,7 +37,7 @@ ssd1307: oled@3c {
         compatible = "solomon,ssd1307fb-i2c";
         reg = <0x3c>;
         pwms = <&pwm 4 3000>;
-        reset-gpios = <&gpio2 7>;
+        reset-gpios = <&gpio2 7 GPIO_ACTIVE_LOW>;
         reset-active-low;
 };
 
@@ -43,7 +45,7 @@ ssd1306: oled@3c {
         compatible = "solomon,ssd1306fb-i2c";
         reg = <0x3c>;
         pwms = <&pwm 4 3000>;
-        reset-gpios = <&gpio2 7>;
+        reset-gpios = <&gpio2 7 GPIO_ACTIVE_LOW>;
         reset-active-low;
         solomon,com-lrremap;
         solomon,com-invdir;
-- 
2.1.4

[PATCH fbdev-for-next 2/2] video: ssd1307fb: Add support for the reset-active-low property

[Vokáč Michal]

The SSD130x OLED display reset signal is active low. Now the reset
sequence is implemented in such a way that DTS authors are forced to
define the reset-gpios property with GPIO_ACTIVE_HIGH to make the reset
work.

Add the reset-active-low property so the signal is inverted once again
and the GPIO_ACTIVE_LOW work as expected.

Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
---
 drivers/video/fbdev/ssd1307fb.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/video/fbdev/ssd1307fb.c b/drivers/video/fbdev/ssd1307fb.c
index e7ae135..790f1c4 100644
--- a/drivers/video/fbdev/ssd1307fb.c
+++ b/drivers/video/fbdev/ssd1307fb.c
@@ -608,6 +608,7 @@ static int ssd1307fb_probe(struct i2c_client *client,
 	struct fb_deferred_io *ssd1307fb_defio;
 	u32 vmem_size;
 	struct ssd1307fb_par *par;
+	bool reset_active_low;
 	u8 *vmem;
 	int ret;
 
@@ -671,6 +672,7 @@ static int ssd1307fb_probe(struct i2c_client *client,
 	par->com_seq = of_property_read_bool(node, "solomon,com-seq");
 	par->com_lrremap = of_property_read_bool(node, "solomon,com-lrremap");
 	par->com_invdir = of_property_read_bool(node, "solomon,com-invdir");
+	reset_active_low = of_property_read_bool(node, "reset-active-low");
 
 	par->contrast = 127;
 	par->vcomh = par->device_info->default_vcomh;
@@ -728,9 +730,9 @@ static int ssd1307fb_probe(struct i2c_client *client,
 
 	if (par->reset) {
 		/* Reset the screen */
-		gpiod_set_value_cansleep(par->reset, 0);
+		gpiod_set_value_cansleep(par->reset, reset_active_low);
 		udelay(4);
-		gpiod_set_value_cansleep(par->reset, 1);
+		gpiod_set_value_cansleep(par->reset, !reset_active_low);
 		udelay(4);
 	}
 
-- 
2.1.4

Re: [PATCH fbdev-for-next 2/2] video: ssd1307fb: Add support for the reset-active-low property

[Vokáč Michal]

On 12.11.2018 17:55, Rob Herring wrote:
> On Fri, Nov 02, 2018 at 02:56:35PM +0000, Vokáč Michal wrote:
>> The SSD130x OLED display reset signal is active low. Now the reset
>> sequence is implemented in such a way that DTS authors are forced to
>> define the reset-gpios property with GPIO_ACTIVE_HIGH to make the reset
>> work.
>>
>> Add the reset-active-low property so the signal is inverted once again
>> and the GPIO_ACTIVE_LOW work as expected.
>>
>> Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
>> ---
>>   drivers/video/fbdev/ssd1307fb.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/video/fbdev/ssd1307fb.c b/drivers/video/fbdev/ssd1307fb.c
>> index e7ae135..790f1c4 100644
>> --- a/drivers/video/fbdev/ssd1307fb.c
>> +++ b/drivers/video/fbdev/ssd1307fb.c
>> @@ -608,6 +608,7 @@ static int ssd1307fb_probe(struct i2c_client *client,
>>   	struct fb_deferred_io *ssd1307fb_defio;
>>   	u32 vmem_size;
>>   	struct ssd1307fb_par *par;
>> +	bool reset_active_low;
>>   	u8 *vmem;
>>   	int ret;
>>   
>> @@ -671,6 +672,7 @@ static int ssd1307fb_probe(struct i2c_client *client,
>>   	par->com_seq = of_property_read_bool(node, "solomon,com-seq");
>>   	par->com_lrremap = of_property_read_bool(node, "solomon,com-lrremap");
>>   	par->com_invdir = of_property_read_bool(node, "solomon,com-invdir");
>> +	reset_active_low = of_property_read_bool(node, "reset-active-low");
>>   
>>   	par->contrast = 127;
>>   	par->vcomh = par->device_info->default_vcomh;
>> @@ -728,9 +730,9 @@ static int ssd1307fb_probe(struct i2c_client *client,
>>   
>>   	if (par->reset) {
>>   		/* Reset the screen */
>> -		gpiod_set_value_cansleep(par->reset, 0);
>> +		gpiod_set_value_cansleep(par->reset, reset_active_low);
>>   		udelay(4);
>> -		gpiod_set_value_cansleep(par->reset, 1);
>> +		gpiod_set_value_cansleep(par->reset, !reset_active_low);
> 
> I think you and whomever wrote the original code are misinterpretting
> how the gpiod API works. 1 means make the signal active and this case
> active is low.

I totally agree and I think I understand that correctly.

> It is strange, but does mean a reset sequence should always be set to a
> 1 and then a 0 and it will work with either polarity in the DT.

I agree the reset should be done as a 0 -> 1 -> 0 sequence and that should
just work. The problem is it is implemented vice versa and so it works only
if you have GPIO_ACTIVE_HIGH in DT for a signal that is actually active low.

And what it actually does is that it holds the controller in reset since
the GPIO is successfully acquired (because of GPIOD_OUT_LOW here [1]) and
later on it only releases the reset.

As a DT author I would like to somehow clearly state that the OLED display
uses active low reset in my DT.

My first attempt to fix this was to change the reset sequence [2].
It was applied and then reverted as it is not backward compatible with
deployed DTB files [3].

[1] https://elixir.bootlin.com/linux/v4.20-rc3/source/drivers/video/fbdev/ssd1307fb.c#L570
[2] https://patchwork.kernel.org/patch/10617729/
[3] https://patchwork.kernel.org/patch/10617731/

Michal

Re: [PATCH fbdev-for-next 2/2] video: ssd1307fb: Add support for the reset-active-low property

[Rob Herring]

On Mon, Nov 19, 2018 at 9:12 AM Vokáč Michal <Michal.Vokac@ysoft.com> wrote:
>
> On 12.11.2018 17:55, Rob Herring wrote:
> > On Fri, Nov 02, 2018 at 02:56:35PM +0000, Vokáč Michal wrote:
> >> The SSD130x OLED display reset signal is active low. Now the reset
> >> sequence is implemented in such a way that DTS authors are forced to
> >> define the reset-gpios property with GPIO_ACTIVE_HIGH to make the reset
> >> work.
> >>
> >> Add the reset-active-low property so the signal is inverted once again
> >> and the GPIO_ACTIVE_LOW work as expected.
> >>
> >> Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
> >> ---
> >>   drivers/video/fbdev/ssd1307fb.c | 6 ++++--
> >>   1 file changed, 4 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/video/fbdev/ssd1307fb.c b/drivers/video/fbdev/ssd1307fb.c
> >> index e7ae135..790f1c4 100644
> >> --- a/drivers/video/fbdev/ssd1307fb.c
> >> +++ b/drivers/video/fbdev/ssd1307fb.c
> >> @@ -608,6 +608,7 @@ static int ssd1307fb_probe(struct i2c_client *client,
> >>      struct fb_deferred_io *ssd1307fb_defio;
> >>      u32 vmem_size;
> >>      struct ssd1307fb_par *par;
> >> +    bool reset_active_low;
> >>      u8 *vmem;
> >>      int ret;
> >>
> >> @@ -671,6 +672,7 @@ static int ssd1307fb_probe(struct i2c_client *client,
> >>      par->com_seq = of_property_read_bool(node, "solomon,com-seq");
> >>      par->com_lrremap = of_property_read_bool(node, "solomon,com-lrremap");
> >>      par->com_invdir = of_property_read_bool(node, "solomon,com-invdir");
> >> +    reset_active_low = of_property_read_bool(node, "reset-active-low");
> >>
> >>      par->contrast = 127;
> >>      par->vcomh = par->device_info->default_vcomh;
> >> @@ -728,9 +730,9 @@ static int ssd1307fb_probe(struct i2c_client *client,
> >>
> >>      if (par->reset) {
> >>              /* Reset the screen */
> >> -            gpiod_set_value_cansleep(par->reset, 0);
> >> +            gpiod_set_value_cansleep(par->reset, reset_active_low);
> >>              udelay(4);
> >> -            gpiod_set_value_cansleep(par->reset, 1);
> >> +            gpiod_set_value_cansleep(par->reset, !reset_active_low);
> >
> > I think you and whomever wrote the original code are misinterpretting
> > how the gpiod API works. 1 means make the signal active and this case
> > active is low.
>
> I totally agree and I think I understand that correctly.
>
> > It is strange, but does mean a reset sequence should always be set to a
> > 1 and then a 0 and it will work with either polarity in the DT.
>
> I agree the reset should be done as a 0 -> 1 -> 0 sequence and that should
> just work. The problem is it is implemented vice versa and so it works only
> if you have GPIO_ACTIVE_HIGH in DT for a signal that is actually active low.
>
> And what it actually does is that it holds the controller in reset since
> the GPIO is successfully acquired (because of GPIOD_OUT_LOW here [1]) and
> later on it only releases the reset.
>
> As a DT author I would like to somehow clearly state that the OLED display
> uses active low reset in my DT.
>
> My first attempt to fix this was to change the reset sequence [2].
> It was applied and then reverted as it is not backward compatible with
> deployed DTB files [3].
>
> [1] https://elixir.bootlin.com/linux/v4.20-rc3/source/drivers/video/fbdev/ssd1307fb.c#L570
> [2] https://patchwork.kernel.org/patch/10617729/
> [3] https://patchwork.kernel.org/patch/10617731/

Okay, now I understand the background. We've hit this somewhere else too.

Rather than have a binding demonstrating what not to do, I'd like to
fix this in another way. I also don't want to live with this forever
when there's only 1 board affected (in tree at least) and there's only
an ABI if someone notices (I'm happy though that the maintainers
caught this). There's 2 other options. The 1st is add a fixup to the
DT for this platform to ensure that the GPIO is configured active low
(the Versatile platform code has an example fixup). With that, apply
what was originally applied (or revert the revert). The fixup could be
applied only after someone complains their display broke. The 2nd
option is just add an of_machine_is_compatible check within this
driver. In that case, you wouldn't fix dts file for the platform
(unless you also want to manually check reset-gpios).

Rob

Re: [PATCH fbdev-for-next 2/2] video: ssd1307fb: Add support for the reset-active-low property

[Vokáč Michal]

On 19.11.2018 23:32, Rob Herring wrote:
> On Mon, Nov 19, 2018 at 9:12 AM Vokáč Michal <Michal.Vokac@ysoft.com> wrote:
>> On 12.11.2018 17:55, Rob Herring wrote:
>>> On Fri, Nov 02, 2018 at 02:56:35PM +0000, Vokáč Michal wrote:
>>>> The SSD130x OLED display reset signal is active low. Now the reset
>>>> sequence is implemented in such a way that DTS authors are forced to
>>>> define the reset-gpios property with GPIO_ACTIVE_HIGH to make the reset
>>>> work.
>>>>
>>>> Add the reset-active-low property so the signal is inverted once again
>>>> and the GPIO_ACTIVE_LOW work as expected.
>>>>
>>>> Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
>>>> ---
>>>>    drivers/video/fbdev/ssd1307fb.c | 6 ++++--
>>>>    1 file changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/video/fbdev/ssd1307fb.c b/drivers/video/fbdev/ssd1307fb.c
>>>> index e7ae135..790f1c4 100644
>>>> --- a/drivers/video/fbdev/ssd1307fb.c
>>>> +++ b/drivers/video/fbdev/ssd1307fb.c
>>>> @@ -608,6 +608,7 @@ static int ssd1307fb_probe(struct i2c_client *client,
>>>>       struct fb_deferred_io *ssd1307fb_defio;
>>>>       u32 vmem_size;
>>>>       struct ssd1307fb_par *par;
>>>> +    bool reset_active_low;
>>>>       u8 *vmem;
>>>>       int ret;
>>>>
>>>> @@ -671,6 +672,7 @@ static int ssd1307fb_probe(struct i2c_client *client,
>>>>       par->com_seq = of_property_read_bool(node, "solomon,com-seq");
>>>>       par->com_lrremap = of_property_read_bool(node, "solomon,com-lrremap");
>>>>       par->com_invdir = of_property_read_bool(node, "solomon,com-invdir");
>>>> +    reset_active_low = of_property_read_bool(node, "reset-active-low");
>>>>
>>>>       par->contrast = 127;
>>>>       par->vcomh = par->device_info->default_vcomh;
>>>> @@ -728,9 +730,9 @@ static int ssd1307fb_probe(struct i2c_client *client,
>>>>
>>>>       if (par->reset) {
>>>>               /* Reset the screen */
>>>> -            gpiod_set_value_cansleep(par->reset, 0);
>>>> +            gpiod_set_value_cansleep(par->reset, reset_active_low);
>>>>               udelay(4);
>>>> -            gpiod_set_value_cansleep(par->reset, 1);
>>>> +            gpiod_set_value_cansleep(par->reset, !reset_active_low);
>>>
>>> I think you and whomever wrote the original code are misinterpretting
>>> how the gpiod API works. 1 means make the signal active and this case
>>> active is low.
>>
>> I totally agree and I think I understand that correctly.
>>
>>> It is strange, but does mean a reset sequence should always be set to a
>>> 1 and then a 0 and it will work with either polarity in the DT.
>>
>> I agree the reset should be done as a 0 -> 1 -> 0 sequence and that should
>> just work. The problem is it is implemented vice versa and so it works only
>> if you have GPIO_ACTIVE_HIGH in DT for a signal that is actually active low.
>>
>> And what it actually does is that it holds the controller in reset since
>> the GPIO is successfully acquired (because of GPIOD_OUT_LOW here [1]) and
>> later on it only releases the reset.
>>
>> As a DT author I would like to somehow clearly state that the OLED display
>> uses active low reset in my DT.
>>
>> My first attempt to fix this was to change the reset sequence [2].
>> It was applied and then reverted as it is not backward compatible with
>> deployed DTB files [3].
>>
>> [1] https://elixir.bootlin.com/linux/v4.20-rc3/source/drivers/video/fbdev/ssd1307fb.c#L570
>> [2] https://patchwork.kernel.org/patch/10617729/
>> [3] https://patchwork.kernel.org/patch/10617731/
> 
> Okay, now I understand the background. We've hit this somewhere else too.
> 
> Rather than have a binding demonstrating what not to do, I'd like to
> fix this in another way. I also don't want to live with this forever
> when there's only 1 board affected (in tree at least) and there's only
> an ABI if someone notices (I'm happy though that the maintainers
> caught this). There's 2 other options. The 1st is add a fixup to the
> DT for this platform to ensure that the GPIO is configured active low
> (the Versatile platform code has an example fixup). With that, apply
> what was originally applied (or revert the revert). The fixup could be
> applied only after someone complains their display broke. The 2nd
> option is just add an of_machine_is_compatible check within this
> driver. In that case, you wouldn't fix dts file for the platform
> (unless you also want to manually check reset-gpios).

Hi Rob,

I am still trying to figure out what exactly you meant by the 1st and
2nd option. Both concepts are new to me.

Regarding the 1st option, what you meant by "this platform" here:
> Add a fixup to the DT for this platform
The only board in tree that uses the OLED (imx28-cfa10036) and its
dts file?

I am also not sure where to look for the example. When you say
Versatile platform code I tend to look into plat-versatile or
mach-versatile. I could not find anything I could use as an example
in there. I think that is not what you meant.

Regarding the 2nd option, you suggest to use of_machine_is_compatible
to decide what reset sequence to use? In case of imx28-cfa10036 use
the old 0 -> 1, in all other cases use a new correct sequence 1 -> 0?
That also does not seem right.

I will appreciate more details how to proceed. Thank you,
Michal

Re: [PATCH fbdev-for-next 2/2] video: ssd1307fb: Add support for the reset-active-low property

[Rob Herring]

On Mon, Nov 26, 2018 at 6:25 AM Vokáč Michal <Michal.Vokac@ysoft.com> wrote:
>
> On 19.11.2018 23:32, Rob Herring wrote:
> > On Mon, Nov 19, 2018 at 9:12 AM Vokáč Michal <Michal.Vokac@ysoft.com> wrote:
> >> On 12.11.2018 17:55, Rob Herring wrote:
> >>> On Fri, Nov 02, 2018 at 02:56:35PM +0000, Vokáč Michal wrote:
> >>>> The SSD130x OLED display reset signal is active low. Now the reset
> >>>> sequence is implemented in such a way that DTS authors are forced to
> >>>> define the reset-gpios property with GPIO_ACTIVE_HIGH to make the reset
> >>>> work.
> >>>>
> >>>> Add the reset-active-low property so the signal is inverted once again
> >>>> and the GPIO_ACTIVE_LOW work as expected.
> >>>>
> >>>> Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
> >>>> ---
> >>>>    drivers/video/fbdev/ssd1307fb.c | 6 ++++--
> >>>>    1 file changed, 4 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/video/fbdev/ssd1307fb.c b/drivers/video/fbdev/ssd1307fb.c
> >>>> index e7ae135..790f1c4 100644
> >>>> --- a/drivers/video/fbdev/ssd1307fb.c
> >>>> +++ b/drivers/video/fbdev/ssd1307fb.c
> >>>> @@ -608,6 +608,7 @@ static int ssd1307fb_probe(struct i2c_client *client,
> >>>>       struct fb_deferred_io *ssd1307fb_defio;
> >>>>       u32 vmem_size;
> >>>>       struct ssd1307fb_par *par;
> >>>> +    bool reset_active_low;
> >>>>       u8 *vmem;
> >>>>       int ret;
> >>>>
> >>>> @@ -671,6 +672,7 @@ static int ssd1307fb_probe(struct i2c_client *client,
> >>>>       par->com_seq = of_property_read_bool(node, "solomon,com-seq");
> >>>>       par->com_lrremap = of_property_read_bool(node, "solomon,com-lrremap");
> >>>>       par->com_invdir = of_property_read_bool(node, "solomon,com-invdir");
> >>>> +    reset_active_low = of_property_read_bool(node, "reset-active-low");
> >>>>
> >>>>       par->contrast = 127;
> >>>>       par->vcomh = par->device_info->default_vcomh;
> >>>> @@ -728,9 +730,9 @@ static int ssd1307fb_probe(struct i2c_client *client,
> >>>>
> >>>>       if (par->reset) {
> >>>>               /* Reset the screen */
> >>>> -            gpiod_set_value_cansleep(par->reset, 0);
> >>>> +            gpiod_set_value_cansleep(par->reset, reset_active_low);
> >>>>               udelay(4);
> >>>> -            gpiod_set_value_cansleep(par->reset, 1);
> >>>> +            gpiod_set_value_cansleep(par->reset, !reset_active_low);
> >>>
> >>> I think you and whomever wrote the original code are misinterpretting
> >>> how the gpiod API works. 1 means make the signal active and this case
> >>> active is low.
> >>
> >> I totally agree and I think I understand that correctly.
> >>
> >>> It is strange, but does mean a reset sequence should always be set to a
> >>> 1 and then a 0 and it will work with either polarity in the DT.
> >>
> >> I agree the reset should be done as a 0 -> 1 -> 0 sequence and that should
> >> just work. The problem is it is implemented vice versa and so it works only
> >> if you have GPIO_ACTIVE_HIGH in DT for a signal that is actually active low.
> >>
> >> And what it actually does is that it holds the controller in reset since
> >> the GPIO is successfully acquired (because of GPIOD_OUT_LOW here [1]) and
> >> later on it only releases the reset.
> >>
> >> As a DT author I would like to somehow clearly state that the OLED display
> >> uses active low reset in my DT.
> >>
> >> My first attempt to fix this was to change the reset sequence [2].
> >> It was applied and then reverted as it is not backward compatible with
> >> deployed DTB files [3].
> >>
> >> [1] https://elixir.bootlin.com/linux/v4.20-rc3/source/drivers/video/fbdev/ssd1307fb.c#L570
> >> [2] https://patchwork.kernel.org/patch/10617729/
> >> [3] https://patchwork.kernel.org/patch/10617731/
> >
> > Okay, now I understand the background. We've hit this somewhere else too.
> >
> > Rather than have a binding demonstrating what not to do, I'd like to
> > fix this in another way. I also don't want to live with this forever
> > when there's only 1 board affected (in tree at least) and there's only
> > an ABI if someone notices (I'm happy though that the maintainers
> > caught this). There's 2 other options. The 1st is add a fixup to the
> > DT for this platform to ensure that the GPIO is configured active low
> > (the Versatile platform code has an example fixup). With that, apply
> > what was originally applied (or revert the revert). The fixup could be
> > applied only after someone complains their display broke. The 2nd
> > option is just add an of_machine_is_compatible check within this
> > driver. In that case, you wouldn't fix dts file for the platform
> > (unless you also want to manually check reset-gpios).
>
> Hi Rob,
>
> I am still trying to figure out what exactly you meant by the 1st and
> 2nd option. Both concepts are new to me.
>
> Regarding the 1st option, what you meant by "this platform" here:
> > Add a fixup to the DT for this platform
> The only board in tree that uses the OLED (imx28-cfa10036) and its
> dts file?

Yes, that one.

>
> I am also not sure where to look for the example. When you say
> Versatile platform code I tend to look into plat-versatile or
> mach-versatile. I could not find anything I could use as an example
> in there. I think that is not what you meant.

See versatile_dt_pci_init(). Or look for other callers of of_update_property().

> Regarding the 2nd option, you suggest to use of_machine_is_compatible
> to decide what reset sequence to use? In case of imx28-cfa10036 use
> the old 0 -> 1, in all other cases use a new correct sequence 1 -> 0?
> That also does not seem right.

Correct. Though if you fix imx28-cfa10036 dts, then you have to handle
that case too.

Why is it not right? Ugly yes, but it's not wrong.

Rob

Re: [PATCH fbdev-for-next 2/2] video: ssd1307fb: Add support for the reset-active-low property

[Vokáč Michal]

On 26.11.2018 14:49, Rob Herring wrote:
> On Mon, Nov 26, 2018 at 6:25 AM Vokáč Michal <Michal.Vokac@ysoft.com> wrote:
>>
>> On 19.11.2018 23:32, Rob Herring wrote:
>>> On Mon, Nov 19, 2018 at 9:12 AM Vokáč Michal <Michal.Vokac@ysoft.com> wrote:
>>>> On 12.11.2018 17:55, Rob Herring wrote:
>>>>> On Fri, Nov 02, 2018 at 02:56:35PM +0000, Vokáč Michal wrote:
>>>>>> The SSD130x OLED display reset signal is active low. Now the reset
>>>>>> sequence is implemented in such a way that DTS authors are forced to
>>>>>> define the reset-gpios property with GPIO_ACTIVE_HIGH to make the reset
>>>>>> work.
>>>>>>
>>>>>> Add the reset-active-low property so the signal is inverted once again
>>>>>> and the GPIO_ACTIVE_LOW work as expected.
>>>>>>
>>>>>> Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
>>>>>> ---
>>>>>>     drivers/video/fbdev/ssd1307fb.c | 6 ++++--
>>>>>>     1 file changed, 4 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/video/fbdev/ssd1307fb.c b/drivers/video/fbdev/ssd1307fb.c
>>>>>> index e7ae135..790f1c4 100644
>>>>>> --- a/drivers/video/fbdev/ssd1307fb.c
>>>>>> +++ b/drivers/video/fbdev/ssd1307fb.c
>>>>>> @@ -608,6 +608,7 @@ static int ssd1307fb_probe(struct i2c_client *client,
>>>>>>        struct fb_deferred_io *ssd1307fb_defio;
>>>>>>        u32 vmem_size;
>>>>>>        struct ssd1307fb_par *par;
>>>>>> +    bool reset_active_low;
>>>>>>        u8 *vmem;
>>>>>>        int ret;
>>>>>>
>>>>>> @@ -671,6 +672,7 @@ static int ssd1307fb_probe(struct i2c_client *client,
>>>>>>        par->com_seq = of_property_read_bool(node, "solomon,com-seq");
>>>>>>        par->com_lrremap = of_property_read_bool(node, "solomon,com-lrremap");
>>>>>>        par->com_invdir = of_property_read_bool(node, "solomon,com-invdir");
>>>>>> +    reset_active_low = of_property_read_bool(node, "reset-active-low");
>>>>>>
>>>>>>        par->contrast = 127;
>>>>>>        par->vcomh = par->device_info->default_vcomh;
>>>>>> @@ -728,9 +730,9 @@ static int ssd1307fb_probe(struct i2c_client *client,
>>>>>>
>>>>>>        if (par->reset) {
>>>>>>                /* Reset the screen */
>>>>>> -            gpiod_set_value_cansleep(par->reset, 0);
>>>>>> +            gpiod_set_value_cansleep(par->reset, reset_active_low);
>>>>>>                udelay(4);
>>>>>> -            gpiod_set_value_cansleep(par->reset, 1);
>>>>>> +            gpiod_set_value_cansleep(par->reset, !reset_active_low);
>>>>>
>>>>> I think you and whomever wrote the original code are misinterpretting
>>>>> how the gpiod API works. 1 means make the signal active and this case
>>>>> active is low.
>>>>
>>>> I totally agree and I think I understand that correctly.
>>>>
>>>>> It is strange, but does mean a reset sequence should always be set to a
>>>>> 1 and then a 0 and it will work with either polarity in the DT.
>>>>
>>>> I agree the reset should be done as a 0 -> 1 -> 0 sequence and that should
>>>> just work. The problem is it is implemented vice versa and so it works only
>>>> if you have GPIO_ACTIVE_HIGH in DT for a signal that is actually active low.
>>>>
>>>> And what it actually does is that it holds the controller in reset since
>>>> the GPIO is successfully acquired (because of GPIOD_OUT_LOW here [1]) and
>>>> later on it only releases the reset.
>>>>
>>>> As a DT author I would like to somehow clearly state that the OLED display
>>>> uses active low reset in my DT.
>>>>
>>>> My first attempt to fix this was to change the reset sequence [2].
>>>> It was applied and then reverted as it is not backward compatible with
>>>> deployed DTB files [3].
>>>>
>>>> [1] https://elixir.bootlin.com/linux/v4.20-rc3/source/drivers/video/fbdev/ssd1307fb.c#L570
>>>> [2] https://patchwork.kernel.org/patch/10617729/
>>>> [3] https://patchwork.kernel.org/patch/10617731/
>>>
>>> Okay, now I understand the background. We've hit this somewhere else too.
>>>
>>> Rather than have a binding demonstrating what not to do, I'd like to
>>> fix this in another way. I also don't want to live with this forever
>>> when there's only 1 board affected (in tree at least) and there's only
>>> an ABI if someone notices (I'm happy though that the maintainers
>>> caught this). There's 2 other options. The 1st is add a fixup to the
>>> DT for this platform to ensure that the GPIO is configured active low
>>> (the Versatile platform code has an example fixup). With that, apply
>>> what was originally applied (or revert the revert). The fixup could be
>>> applied only after someone complains their display broke. The 2nd
>>> option is just add an of_machine_is_compatible check within this
>>> driver. In that case, you wouldn't fix dts file for the platform
>>> (unless you also want to manually check reset-gpios).
>>
>> Hi Rob,
>>
>> I am still trying to figure out what exactly you meant by the 1st and
>> 2nd option. Both concepts are new to me.
>>
>> Regarding the 1st option, what you meant by "this platform" here:
>>> Add a fixup to the DT for this platform
>> The only board in tree that uses the OLED (imx28-cfa10036) and its
>> dts file?
> 
> Yes, that one.
> 
>> I am also not sure where to look for the example. When you say
>> Versatile platform code I tend to look into plat-versatile or
>> mach-versatile. I could not find anything I could use as an example
>> in there. I think that is not what you meant.
> 
> See versatile_dt_pci_init(). Or look for other callers of of_update_property().

Excellent, I will look at that.
  
>> Regarding the 2nd option, you suggest to use of_machine_is_compatible
>> to decide what reset sequence to use? In case of imx28-cfa10036 use
>> the old 0 -> 1, in all other cases use a new correct sequence 1 -> 0?
>> That also does not seem right.
> 
> Correct. Though if you fix imx28-cfa10036 dts, then you have to handle
> that case too.
> 
> Why is it not right? Ugly yes, but it's not wrong.

Ugly is what I probably meant. It seems like other users (among drivers)
of of_machine_is_compatible are mostly drivers for IP blocks that need
slightly different handling on different SoC variants.

Thank you very much,
Michal