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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <db9537dc-2ba5-b8fc-ae3c-7d85841b0797@ysoft.com>
Date:   Mon, 19 Nov 2018 15:12:46 +0000
From:   Vokáč Michal <Michal.Vokac@...ft.com>
To:     Rob Herring <robh@...nel.org>
CC:     Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
        "linux-fbdev@...r.kernel.org" <linux-fbdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        Jyri Sarha <jsarha@...com>
Subject: Re: [PATCH fbdev-for-next 2/2] video: ssd1307fb: Add support for the
 reset-active-low property

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@...ft.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