[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 1/2] dt-bindings: display: ssd1307fb: Add reset-active-low property

[Vokáč Michal]

VGhpcyByZXZlcnRzIGNvbW1pdCA1MTliNGRiYTU4NjE5OGVlZDhmNzJiYTA3YmM3MTgwOGFmMmUw
ZTMyLg0KSXQgaXMgdHJ1ZSB0aGF0IHRoZSBhY3R1YWwgaW1wbGVtZW50YXRpb24gaGFzIG5ldmVy
IGJlZW4gdGhlcmUuIEJ1dA0KY29udHJhcnkgdG8gd2hhdCB0aGUgcmV2ZXJ0ZWQgY29tbWl0IG1l
c3NhZ2Ugc2F5cyBpdCBkb2VzIG1ha2Ugc2Vuc2UNCnRvIGFkZCBpdC4NCg0KQ3VycmVudCBpbXBs
ZW1lbnRhdGlvbiBvZiB0aGUgcmVzZXQgc2lnbmFsIGlzIGhhcmQtY29kZWQgdG8gYWN0aXZlIGxv
dw0Kd2l0aCB0aGUgYXNzdW1wdGlvbiB0aGF0IHJlc2V0LWdwaW9zIGlzIHNwZWNpZmllZCBhcyBH
UElPX0FDVElWRV9ISUdILg0KVGhhdCBpcyB0ZWNobmljYWxseSB3cm9uZyBhcyB0aGUgRFRTIGF1
dGhvcnMgc2hvdWxkIGtub3cgdGhhdCBTU0QxMzB4DQpkaXNwbGF5cyBuZWVkIGFjdGl2ZSBsb3cg
cmVzZXQgYW5kIGhlbmNlIHRoZXkgYXJlIHRlbXBlZCB0byB1c2UNCkdQSU9fQUNUSVZFX0xPVy4g
QnV0IHdpdGggdGhhdCB0aGUgcmVzZXQgaXMgYnJva2VuLiBTbyByZXNldC1hY2l2ZS1sb3cNCnBy
b3BlcnR5IGNhbiBiZSB1c2VkIHRvIGludmVydCB0aGUgc2lnbmFsIG9uY2UgYWdhaW4gdG8gZml4
IHRoaXMuDQoNClNpZ25lZC1vZmYtYnk6IE1pY2hhbCBWb2vDocSNIDxtaWNoYWwudm9rYWNAeXNv
ZnQuY29tPg0KLS0tDQogRG9jdW1lbnRhdGlvbi9kZXZpY2V0cmVlL2JpbmRpbmdzL2Rpc3BsYXkv
c3NkMTMwN2ZiLnR4dCB8IDYgKysrKy0tDQogMSBmaWxlIGNoYW5nZWQsIDQgaW5zZXJ0aW9ucygr
KSwgMiBkZWxldGlvbnMoLSkNCg0KZGlmZiAtLWdpdCBhL0RvY3VtZW50YXRpb24vZGV2aWNldHJl
ZS9iaW5kaW5ncy9kaXNwbGF5L3NzZDEzMDdmYi50eHQgYi9Eb2N1bWVudGF0aW9uL2RldmljZXRy
ZWUvYmluZGluZ3MvZGlzcGxheS9zc2QxMzA3ZmIudHh0DQppbmRleCAyMDlkOTMxLi5hNWVhZDEw
IDEwMDY0NA0KLS0tIGEvRG9jdW1lbnRhdGlvbi9kZXZpY2V0cmVlL2JpbmRpbmdzL2Rpc3BsYXkv
c3NkMTMwN2ZiLnR4dA0KKysrIGIvRG9jdW1lbnRhdGlvbi9kZXZpY2V0cmVlL2JpbmRpbmdzL2Rp
c3BsYXkvc3NkMTMwN2ZiLnR4dA0KQEAgLTE2LDYgKzE2LDggQEAgUmVxdWlyZWQgcHJvcGVydGll
czoNCiBPcHRpb25hbCBwcm9wZXJ0aWVzOg0KICAgLSByZXNldC1ncGlvczogVGhlIEdQSU8gdXNl
ZCB0byByZXNldCB0aGUgT0xFRCBkaXNwbGF5LCBpZiBhdmFpbGFibGUuIFNlZQ0KICAgICAgICAg
ICAgICAgICAgRG9jdW1lbnRhdGlvbi9kZXZpY2V0cmVlL2JpbmRpbmdzL2dwaW8vZ3Bpby50eHQg
Zm9yIGRldGFpbHMuDQorICAtIHJlc2V0LWFjdGl2ZS1sb3c6IEJvb2wgZmxhZyB0byBpbmRpY2F0
ZSB0aGUgR1BJTyBzcGVjaWZpZWQgaW4gInJlc2V0LWdwaW9zIg0KKyAgICAgICAgICAgICAgICAg
ICAgICBwcm9wZXJ0eSBpcyBhY3RpdmUgbG93Lg0KICAgLSB2YmF0LXN1cHBseTogVGhlIHN1cHBs
eSBmb3IgVkJBVA0KICAgLSBzb2xvbW9uLHNlZ21lbnQtbm8tcmVtYXA6IERpc3BsYXkgbmVlZHMg
bm9ybWFsIChub24taW52ZXJ0ZWQpIGRhdGEgY29sdW1uDQogICAgICAgICAgICAgICAgICAgICAg
ICAgICAgICAgdG8gc2VnbWVudCBtYXBwaW5nDQpAQCAtMzUsNyArMzcsNyBAQCBzc2QxMzA3OiBv
bGVkQDNjIHsNCiAgICAgICAgIGNvbXBhdGlibGUgPSAic29sb21vbixzc2QxMzA3ZmItaTJjIjsN
CiAgICAgICAgIHJlZyA9IDwweDNjPjsNCiAgICAgICAgIHB3bXMgPSA8JnB3bSA0IDMwMDA+Ow0K
LSAgICAgICAgcmVzZXQtZ3Bpb3MgPSA8JmdwaW8yIDc+Ow0KKyAgICAgICAgcmVzZXQtZ3Bpb3Mg
PSA8JmdwaW8yIDcgR1BJT19BQ1RJVkVfTE9XPjsNCiAgICAgICAgIHJlc2V0LWFjdGl2ZS1sb3c7
DQogfTsNCiANCkBAIC00Myw3ICs0NSw3IEBAIHNzZDEzMDY6IG9sZWRAM2Mgew0KICAgICAgICAg
Y29tcGF0aWJsZSA9ICJzb2xvbW9uLHNzZDEzMDZmYi1pMmMiOw0KICAgICAgICAgcmVnID0gPDB4
M2M+Ow0KICAgICAgICAgcHdtcyA9IDwmcHdtIDQgMzAwMD47DQotICAgICAgICByZXNldC1ncGlv
cyA9IDwmZ3BpbzIgNz47DQorICAgICAgICByZXNldC1ncGlvcyA9IDwmZ3BpbzIgNyBHUElPX0FD
VElWRV9MT1c+Ow0KICAgICAgICAgcmVzZXQtYWN0aXZlLWxvdzsNCiAgICAgICAgIHNvbG9tb24s
Y29tLWxycmVtYXA7DQogICAgICAgICBzb2xvbW9uLGNvbS1pbnZkaXI7DQotLSANCjIuMS40DQoN
Cg=

[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

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

[Vokáč Michal]

VGhlIFNTRDEzMHggT0xFRMKgZGlzcGxheSByZXNldCBzaWduYWwgaXMgYWN0aXZlIGxvdy4gTm93
IHRoZSByZXNldA0Kc2VxdWVuY2UgaXMgaW1wbGVtZW50ZWQgaW4gc3VjaCBhIHdheSB0aGF0IERU
UyBhdXRob3JzIGFyZSBmb3JjZWQgdG8NCmRlZmluZSB0aGUgcmVzZXQtZ3Bpb3MgcHJvcGVydHkg
d2l0aCBHUElPX0FDVElWRV9ISUdIIHRvIG1ha2UgdGhlIHJlc2V0DQp3b3JrLg0KDQpBZGQgdGhl
IHJlc2V0LWFjdGl2ZS1sb3cgcHJvcGVydHkgc28gdGhlIHNpZ25hbCBpcyBpbnZlcnRlZCBvbmNl
IGFnYWluDQphbmQgdGhlIEdQSU9fQUNUSVZFX0xPVyB3b3JrIGFzIGV4cGVjdGVkLg0KDQpTaWdu
ZWQtb2ZmLWJ5OiBNaWNoYWwgVm9rw6HEjSA8bWljaGFsLnZva2FjQHlzb2Z0LmNvbT4NCi0tLQ0K
IGRyaXZlcnMvdmlkZW8vZmJkZXYvc3NkMTMwN2ZiLmMgfCA2ICsrKystLQ0KIDEgZmlsZSBjaGFu
Z2VkLCA0IGluc2VydGlvbnMoKyksIDIgZGVsZXRpb25zKC0pDQoNCmRpZmYgLS1naXQgYS9kcml2
ZXJzL3ZpZGVvL2ZiZGV2L3NzZDEzMDdmYi5jIGIvZHJpdmVycy92aWRlby9mYmRldi9zc2QxMzA3
ZmIuYw0KaW5kZXggZTdhZTEzNS4uNzkwZjFjNCAxMDA2NDQNCi0tLSBhL2RyaXZlcnMvdmlkZW8v
ZmJkZXYvc3NkMTMwN2ZiLmMNCisrKyBiL2RyaXZlcnMvdmlkZW8vZmJkZXYvc3NkMTMwN2ZiLmMN
CkBAIC02MDgsNiArNjA4LDcgQEAgc3RhdGljIGludCBzc2QxMzA3ZmJfcHJvYmUoc3RydWN0IGky
Y19jbGllbnQgKmNsaWVudCwNCiAJc3RydWN0IGZiX2RlZmVycmVkX2lvICpzc2QxMzA3ZmJfZGVm
aW87DQogCXUzMiB2bWVtX3NpemU7DQogCXN0cnVjdCBzc2QxMzA3ZmJfcGFyICpwYXI7DQorCWJv
b2wgcmVzZXRfYWN0aXZlX2xvdzsNCiAJdTggKnZtZW07DQogCWludCByZXQ7DQogDQpAQCAtNjcx
LDYgKzY3Miw3IEBAIHN0YXRpYyBpbnQgc3NkMTMwN2ZiX3Byb2JlKHN0cnVjdCBpMmNfY2xpZW50
ICpjbGllbnQsDQogCXBhci0+Y29tX3NlcSA9IG9mX3Byb3BlcnR5X3JlYWRfYm9vbChub2RlLCAi
c29sb21vbixjb20tc2VxIik7DQogCXBhci0+Y29tX2xycmVtYXAgPSBvZl9wcm9wZXJ0eV9yZWFk
X2Jvb2wobm9kZSwgInNvbG9tb24sY29tLWxycmVtYXAiKTsNCiAJcGFyLT5jb21faW52ZGlyID0g
b2ZfcHJvcGVydHlfcmVhZF9ib29sKG5vZGUsICJzb2xvbW9uLGNvbS1pbnZkaXIiKTsNCisJcmVz
ZXRfYWN0aXZlX2xvdyA9IG9mX3Byb3BlcnR5X3JlYWRfYm9vbChub2RlLCAicmVzZXQtYWN0aXZl
LWxvdyIpOw0KIA0KIAlwYXItPmNvbnRyYXN0ID0gMTI3Ow0KIAlwYXItPnZjb21oID0gcGFyLT5k
ZXZpY2VfaW5mby0+ZGVmYXVsdF92Y29taDsNCkBAIC03MjgsOSArNzMwLDkgQEAgc3RhdGljIGlu
dCBzc2QxMzA3ZmJfcHJvYmUoc3RydWN0IGkyY19jbGllbnQgKmNsaWVudCwNCiANCiAJaWYgKHBh
ci0+cmVzZXQpIHsNCiAJCS8qIFJlc2V0IHRoZSBzY3JlZW4gKi8NCi0JCWdwaW9kX3NldF92YWx1
ZV9jYW5zbGVlcChwYXItPnJlc2V0LCAwKTsNCisJCWdwaW9kX3NldF92YWx1ZV9jYW5zbGVlcChw
YXItPnJlc2V0LCByZXNldF9hY3RpdmVfbG93KTsNCiAJCXVkZWxheSg0KTsNCi0JCWdwaW9kX3Nl
dF92YWx1ZV9jYW5zbGVlcChwYXItPnJlc2V0LCAxKTsNCisJCWdwaW9kX3NldF92YWx1ZV9jYW5z
bGVlcChwYXItPnJlc2V0LCAhcmVzZXRfYWN0aXZlX2xvdyk7DQogCQl1ZGVsYXkoNCk7DQogCX0N
CiANCi0tIA0KMi4xLjQNCg0K

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

[Rob Herring]

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.

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.

Rob

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

[Vokáč Michal]

T24gMTIuMTEuMjAxOCAxNzo1NSwgUm9iIEhlcnJpbmcgd3JvdGU6DQo+IE9uIEZyaSwgTm92IDAy
LCAyMDE4IGF0IDAyOjU2OjM1UE0gKzAwMDAsIFZva8OhxI0gTWljaGFsIHdyb3RlOg0KPj4gVGhl
IFNTRDEzMHggT0xFRCBkaXNwbGF5IHJlc2V0IHNpZ25hbCBpcyBhY3RpdmUgbG93LiBOb3cgdGhl
IHJlc2V0DQo+PiBzZXF1ZW5jZSBpcyBpbXBsZW1lbnRlZCBpbiBzdWNoIGEgd2F5IHRoYXQgRFRT
IGF1dGhvcnMgYXJlIGZvcmNlZCB0bw0KPj4gZGVmaW5lIHRoZSByZXNldC1ncGlvcyBwcm9wZXJ0
eSB3aXRoIEdQSU9fQUNUSVZFX0hJR0ggdG8gbWFrZSB0aGUgcmVzZXQNCj4+IHdvcmsuDQo+Pg0K
Pj4gQWRkIHRoZSByZXNldC1hY3RpdmUtbG93IHByb3BlcnR5IHNvIHRoZSBzaWduYWwgaXMgaW52
ZXJ0ZWQgb25jZSBhZ2Fpbg0KPj4gYW5kIHRoZSBHUElPX0FDVElWRV9MT1cgd29yayBhcyBleHBl
Y3RlZC4NCj4+DQo+PiBTaWduZWQtb2ZmLWJ5OiBNaWNoYWwgVm9rw6HEjSA8bWljaGFsLnZva2Fj
QHlzb2Z0LmNvbT4NCj4+IC0tLQ0KPj4gICBkcml2ZXJzL3ZpZGVvL2ZiZGV2L3NzZDEzMDdmYi5j
IHwgNiArKysrLS0NCj4+ICAgMSBmaWxlIGNoYW5nZWQsIDQgaW5zZXJ0aW9ucygrKSwgMiBkZWxl
dGlvbnMoLSkNCj4+DQo+PiBkaWZmIC0tZ2l0IGEvZHJpdmVycy92aWRlby9mYmRldi9zc2QxMzA3
ZmIuYyBiL2RyaXZlcnMvdmlkZW8vZmJkZXYvc3NkMTMwN2ZiLmMNCj4+IGluZGV4IGU3YWUxMzUu
Ljc5MGYxYzQgMTAwNjQ0DQo+PiAtLS0gYS9kcml2ZXJzL3ZpZGVvL2ZiZGV2L3NzZDEzMDdmYi5j
DQo+PiArKysgYi9kcml2ZXJzL3ZpZGVvL2ZiZGV2L3NzZDEzMDdmYi5jDQo+PiBAQCAtNjA4LDYg
KzYwOCw3IEBAIHN0YXRpYyBpbnQgc3NkMTMwN2ZiX3Byb2JlKHN0cnVjdCBpMmNfY2xpZW50ICpj
bGllbnQsDQo+PiAgIAlzdHJ1Y3QgZmJfZGVmZXJyZWRfaW8gKnNzZDEzMDdmYl9kZWZpbzsNCj4+
ICAgCXUzMiB2bWVtX3NpemU7DQo+PiAgIAlzdHJ1Y3Qgc3NkMTMwN2ZiX3BhciAqcGFyOw0KPj4g
Kwlib29sIHJlc2V0X2FjdGl2ZV9sb3c7DQo+PiAgIAl1OCAqdm1lbTsNCj4+ICAgCWludCByZXQ7
DQo+PiAgIA0KPj4gQEAgLTY3MSw2ICs2NzIsNyBAQCBzdGF0aWMgaW50IHNzZDEzMDdmYl9wcm9i
ZShzdHJ1Y3QgaTJjX2NsaWVudCAqY2xpZW50LA0KPj4gICAJcGFyLT5jb21fc2VxID0gb2ZfcHJv
cGVydHlfcmVhZF9ib29sKG5vZGUsICJzb2xvbW9uLGNvbS1zZXEiKTsNCj4+ICAgCXBhci0+Y29t
X2xycmVtYXAgPSBvZl9wcm9wZXJ0eV9yZWFkX2Jvb2wobm9kZSwgInNvbG9tb24sY29tLWxycmVt
YXAiKTsNCj4+ICAgCXBhci0+Y29tX2ludmRpciA9IG9mX3Byb3BlcnR5X3JlYWRfYm9vbChub2Rl
LCAic29sb21vbixjb20taW52ZGlyIik7DQo+PiArCXJlc2V0X2FjdGl2ZV9sb3cgPSBvZl9wcm9w
ZXJ0eV9yZWFkX2Jvb2wobm9kZSwgInJlc2V0LWFjdGl2ZS1sb3ciKTsNCj4+ICAgDQo+PiAgIAlw
YXItPmNvbnRyYXN0ID0gMTI3Ow0KPj4gICAJcGFyLT52Y29taCA9IHBhci0+ZGV2aWNlX2luZm8t
PmRlZmF1bHRfdmNvbWg7DQo+PiBAQCAtNzI4LDkgKzczMCw5IEBAIHN0YXRpYyBpbnQgc3NkMTMw
N2ZiX3Byb2JlKHN0cnVjdCBpMmNfY2xpZW50ICpjbGllbnQsDQo+PiAgIA0KPj4gICAJaWYgKHBh
ci0+cmVzZXQpIHsNCj4+ICAgCQkvKiBSZXNldCB0aGUgc2NyZWVuICovDQo+PiAtCQlncGlvZF9z
ZXRfdmFsdWVfY2Fuc2xlZXAocGFyLT5yZXNldCwgMCk7DQo+PiArCQlncGlvZF9zZXRfdmFsdWVf
Y2Fuc2xlZXAocGFyLT5yZXNldCwgcmVzZXRfYWN0aXZlX2xvdyk7DQo+PiAgIAkJdWRlbGF5KDQp
Ow0KPj4gLQkJZ3Bpb2Rfc2V0X3ZhbHVlX2NhbnNsZWVwKHBhci0+cmVzZXQsIDEpOw0KPj4gKwkJ
Z3Bpb2Rfc2V0X3ZhbHVlX2NhbnNsZWVwKHBhci0+cmVzZXQsICFyZXNldF9hY3RpdmVfbG93KTsN
Cj4gDQo+IEkgdGhpbmsgeW91IGFuZCB3aG9tZXZlciB3cm90ZSB0aGUgb3JpZ2luYWwgY29kZSBh
cmUgbWlzaW50ZXJwcmV0dGluZw0KPiBob3cgdGhlIGdwaW9kIEFQSSB3b3Jrcy4gMSBtZWFucyBt
YWtlIHRoZSBzaWduYWwgYWN0aXZlIGFuZCB0aGlzIGNhc2UNCj4gYWN0aXZlIGlzIGxvdy4NCg0K
SSB0b3RhbGx5IGFncmVlIGFuZCBJIHRoaW5rIEkgdW5kZXJzdGFuZCB0aGF0IGNvcnJlY3RseS4N
Cg0KPiBJdCBpcyBzdHJhbmdlLCBidXQgZG9lcyBtZWFuIGEgcmVzZXQgc2VxdWVuY2Ugc2hvdWxk
IGFsd2F5cyBiZSBzZXQgdG8gYQ0KPiAxIGFuZCB0aGVuIGEgMCBhbmQgaXQgd2lsbCB3b3JrIHdp
dGggZWl0aGVyIHBvbGFyaXR5IGluIHRoZSBEVC4NCg0KSSBhZ3JlZSB0aGUgcmVzZXQgc2hvdWxk
IGJlIGRvbmUgYXMgYSAwIC0+IDEgLT4gMCBzZXF1ZW5jZSBhbmQgdGhhdCBzaG91bGQNCmp1c3Qg
d29yay4gVGhlIHByb2JsZW0gaXMgaXQgaXMgaW1wbGVtZW50ZWQgdmljZSB2ZXJzYSBhbmQgc28g
aXQgd29ya3Mgb25seQ0KaWYgeW91IGhhdmUgR1BJT19BQ1RJVkVfSElHSCBpbiBEVCBmb3IgYSBz
aWduYWwgdGhhdCBpcyBhY3R1YWxseSBhY3RpdmUgbG93Lg0KDQpBbmQgd2hhdCBpdCBhY3R1YWxs
eSBkb2VzIGlzIHRoYXQgaXQgaG9sZHMgdGhlIGNvbnRyb2xsZXIgaW4gcmVzZXQgc2luY2UNCnRo
ZSBHUElPIGlzIHN1Y2Nlc3NmdWxseSBhY3F1aXJlZCAoYmVjYXVzZSBvZiBHUElPRF9PVVRfTE9X
IGhlcmUgWzFdKSBhbmQNCmxhdGVyIG9uIGl0IG9ubHkgcmVsZWFzZXMgdGhlIHJlc2V0Lg0KDQpB
cyBhIERUIGF1dGhvciBJIHdvdWxkIGxpa2UgdG8gc29tZWhvdyBjbGVhcmx5IHN0YXRlIHRoYXQg
dGhlIE9MRUQgZGlzcGxheQ0KdXNlcyBhY3RpdmUgbG93IHJlc2V0IGluIG15IERULg0KDQpNeSBm
aXJzdCBhdHRlbXB0IHRvIGZpeCB0aGlzIHdhcyB0byBjaGFuZ2UgdGhlIHJlc2V0IHNlcXVlbmNl
IFsyXS4NCkl0IHdhcyBhcHBsaWVkIGFuZCB0aGVuIHJldmVydGVkIGFzIGl0IGlzIG5vdCBiYWNr
d2FyZCBjb21wYXRpYmxlIHdpdGgNCmRlcGxveWVkIERUQiBmaWxlcyBbM10uDQoNClsxXSBodHRw
czovL2VsaXhpci5ib290bGluLmNvbS9saW51eC92NC4yMC1yYzMvc291cmNlL2RyaXZlcnMvdmlk
ZW8vZmJkZXYvc3NkMTMwN2ZiLmMjTDU3MA0KWzJdIGh0dHBzOi8vcGF0Y2h3b3JrLmtlcm5lbC5v
cmcvcGF0Y2gvMTA2MTc3MjkvDQpbM10gaHR0cHM6Ly9wYXRjaHdvcmsua2VybmVsLm9yZy9wYXRj
aC8xMDYxNzczMS8NCg0KTWljaGFsDQo

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

[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

[Vokáč Michal]

T24gMTkuMTEuMjAxOCAyMzozMiwgUm9iIEhlcnJpbmcgd3JvdGU6DQo+IE9uIE1vbiwgTm92IDE5
LCAyMDE4IGF0IDk6MTIgQU0gVm9rw6HEjSBNaWNoYWwgPE1pY2hhbC5Wb2thY0B5c29mdC5jb20+
IHdyb3RlOg0KPj4gT24gMTIuMTEuMjAxOCAxNzo1NSwgUm9iIEhlcnJpbmcgd3JvdGU6DQo+Pj4g
T24gRnJpLCBOb3YgMDIsIDIwMTggYXQgMDI6NTY6MzVQTSArMDAwMCwgVm9rw6HEjSBNaWNoYWwg
d3JvdGU6DQo+Pj4+IFRoZSBTU0QxMzB4IE9MRUQgZGlzcGxheSByZXNldCBzaWduYWwgaXMgYWN0
aXZlIGxvdy4gTm93IHRoZSByZXNldA0KPj4+PiBzZXF1ZW5jZSBpcyBpbXBsZW1lbnRlZCBpbiBz
dWNoIGEgd2F5IHRoYXQgRFRTIGF1dGhvcnMgYXJlIGZvcmNlZCB0bw0KPj4+PiBkZWZpbmUgdGhl
IHJlc2V0LWdwaW9zIHByb3BlcnR5IHdpdGggR1BJT19BQ1RJVkVfSElHSCB0byBtYWtlIHRoZSBy
ZXNldA0KPj4+PiB3b3JrLg0KPj4+Pg0KPj4+PiBBZGQgdGhlIHJlc2V0LWFjdGl2ZS1sb3cgcHJv
cGVydHkgc28gdGhlIHNpZ25hbCBpcyBpbnZlcnRlZCBvbmNlIGFnYWluDQo+Pj4+IGFuZCB0aGUg
R1BJT19BQ1RJVkVfTE9XIHdvcmsgYXMgZXhwZWN0ZWQuDQo+Pj4+DQo+Pj4+IFNpZ25lZC1vZmYt
Ynk6IE1pY2hhbCBWb2vDocSNIDxtaWNoYWwudm9rYWNAeXNvZnQuY29tPg0KPj4+PiAtLS0NCj4+
Pj4gICAgZHJpdmVycy92aWRlby9mYmRldi9zc2QxMzA3ZmIuYyB8IDYgKysrKy0tDQo+Pj4+ICAg
IDEgZmlsZSBjaGFuZ2VkLCA0IGluc2VydGlvbnMoKyksIDIgZGVsZXRpb25zKC0pDQo+Pj4+DQo+
Pj4+IGRpZmYgLS1naXQgYS9kcml2ZXJzL3ZpZGVvL2ZiZGV2L3NzZDEzMDdmYi5jIGIvZHJpdmVy
cy92aWRlby9mYmRldi9zc2QxMzA3ZmIuYw0KPj4+PiBpbmRleCBlN2FlMTM1Li43OTBmMWM0IDEw
MDY0NA0KPj4+PiAtLS0gYS9kcml2ZXJzL3ZpZGVvL2ZiZGV2L3NzZDEzMDdmYi5jDQo+Pj4+ICsr
KyBiL2RyaXZlcnMvdmlkZW8vZmJkZXYvc3NkMTMwN2ZiLmMNCj4+Pj4gQEAgLTYwOCw2ICs2MDgs
NyBAQCBzdGF0aWMgaW50IHNzZDEzMDdmYl9wcm9iZShzdHJ1Y3QgaTJjX2NsaWVudCAqY2xpZW50
LA0KPj4+PiAgICAgICBzdHJ1Y3QgZmJfZGVmZXJyZWRfaW8gKnNzZDEzMDdmYl9kZWZpbzsNCj4+
Pj4gICAgICAgdTMyIHZtZW1fc2l6ZTsNCj4+Pj4gICAgICAgc3RydWN0IHNzZDEzMDdmYl9wYXIg
KnBhcjsNCj4+Pj4gKyAgICBib29sIHJlc2V0X2FjdGl2ZV9sb3c7DQo+Pj4+ICAgICAgIHU4ICp2
bWVtOw0KPj4+PiAgICAgICBpbnQgcmV0Ow0KPj4+Pg0KPj4+PiBAQCAtNjcxLDYgKzY3Miw3IEBA
IHN0YXRpYyBpbnQgc3NkMTMwN2ZiX3Byb2JlKHN0cnVjdCBpMmNfY2xpZW50ICpjbGllbnQsDQo+
Pj4+ICAgICAgIHBhci0+Y29tX3NlcSA9IG9mX3Byb3BlcnR5X3JlYWRfYm9vbChub2RlLCAic29s
b21vbixjb20tc2VxIik7DQo+Pj4+ICAgICAgIHBhci0+Y29tX2xycmVtYXAgPSBvZl9wcm9wZXJ0
eV9yZWFkX2Jvb2wobm9kZSwgInNvbG9tb24sY29tLWxycmVtYXAiKTsNCj4+Pj4gICAgICAgcGFy
LT5jb21faW52ZGlyID0gb2ZfcHJvcGVydHlfcmVhZF9ib29sKG5vZGUsICJzb2xvbW9uLGNvbS1p
bnZkaXIiKTsNCj4+Pj4gKyAgICByZXNldF9hY3RpdmVfbG93ID0gb2ZfcHJvcGVydHlfcmVhZF9i
b29sKG5vZGUsICJyZXNldC1hY3RpdmUtbG93Iik7DQo+Pj4+DQo+Pj4+ICAgICAgIHBhci0+Y29u
dHJhc3QgPSAxMjc7DQo+Pj4+ICAgICAgIHBhci0+dmNvbWggPSBwYXItPmRldmljZV9pbmZvLT5k
ZWZhdWx0X3Zjb21oOw0KPj4+PiBAQCAtNzI4LDkgKzczMCw5IEBAIHN0YXRpYyBpbnQgc3NkMTMw
N2ZiX3Byb2JlKHN0cnVjdCBpMmNfY2xpZW50ICpjbGllbnQsDQo+Pj4+DQo+Pj4+ICAgICAgIGlm
IChwYXItPnJlc2V0KSB7DQo+Pj4+ICAgICAgICAgICAgICAgLyogUmVzZXQgdGhlIHNjcmVlbiAq
Lw0KPj4+PiAtICAgICAgICAgICAgZ3Bpb2Rfc2V0X3ZhbHVlX2NhbnNsZWVwKHBhci0+cmVzZXQs
IDApOw0KPj4+PiArICAgICAgICAgICAgZ3Bpb2Rfc2V0X3ZhbHVlX2NhbnNsZWVwKHBhci0+cmVz
ZXQsIHJlc2V0X2FjdGl2ZV9sb3cpOw0KPj4+PiAgICAgICAgICAgICAgIHVkZWxheSg0KTsNCj4+
Pj4gLSAgICAgICAgICAgIGdwaW9kX3NldF92YWx1ZV9jYW5zbGVlcChwYXItPnJlc2V0LCAxKTsN
Cj4+Pj4gKyAgICAgICAgICAgIGdwaW9kX3NldF92YWx1ZV9jYW5zbGVlcChwYXItPnJlc2V0LCAh
cmVzZXRfYWN0aXZlX2xvdyk7DQo+Pj4NCj4+PiBJIHRoaW5rIHlvdSBhbmQgd2hvbWV2ZXIgd3Jv
dGUgdGhlIG9yaWdpbmFsIGNvZGUgYXJlIG1pc2ludGVycHJldHRpbmcNCj4+PiBob3cgdGhlIGdw
aW9kIEFQSSB3b3Jrcy4gMSBtZWFucyBtYWtlIHRoZSBzaWduYWwgYWN0aXZlIGFuZCB0aGlzIGNh
c2UNCj4+PiBhY3RpdmUgaXMgbG93Lg0KPj4NCj4+IEkgdG90YWxseSBhZ3JlZSBhbmQgSSB0aGlu
ayBJIHVuZGVyc3RhbmQgdGhhdCBjb3JyZWN0bHkuDQo+Pg0KPj4+IEl0IGlzIHN0cmFuZ2UsIGJ1
dCBkb2VzIG1lYW4gYSByZXNldCBzZXF1ZW5jZSBzaG91bGQgYWx3YXlzIGJlIHNldCB0byBhDQo+
Pj4gMSBhbmQgdGhlbiBhIDAgYW5kIGl0IHdpbGwgd29yayB3aXRoIGVpdGhlciBwb2xhcml0eSBp
biB0aGUgRFQuDQo+Pg0KPj4gSSBhZ3JlZSB0aGUgcmVzZXQgc2hvdWxkIGJlIGRvbmUgYXMgYSAw
IC0+IDEgLT4gMCBzZXF1ZW5jZSBhbmQgdGhhdCBzaG91bGQNCj4+IGp1c3Qgd29yay4gVGhlIHBy
b2JsZW0gaXMgaXQgaXMgaW1wbGVtZW50ZWQgdmljZSB2ZXJzYSBhbmQgc28gaXQgd29ya3Mgb25s
eQ0KPj4gaWYgeW91IGhhdmUgR1BJT19BQ1RJVkVfSElHSCBpbiBEVCBmb3IgYSBzaWduYWwgdGhh
dCBpcyBhY3R1YWxseSBhY3RpdmUgbG93Lg0KPj4NCj4+IEFuZCB3aGF0IGl0IGFjdHVhbGx5IGRv
ZXMgaXMgdGhhdCBpdCBob2xkcyB0aGUgY29udHJvbGxlciBpbiByZXNldCBzaW5jZQ0KPj4gdGhl
IEdQSU8gaXMgc3VjY2Vzc2Z1bGx5IGFjcXVpcmVkIChiZWNhdXNlIG9mIEdQSU9EX09VVF9MT1cg
aGVyZSBbMV0pIGFuZA0KPj4gbGF0ZXIgb24gaXQgb25seSByZWxlYXNlcyB0aGUgcmVzZXQuDQo+
Pg0KPj4gQXMgYSBEVCBhdXRob3IgSSB3b3VsZCBsaWtlIHRvIHNvbWVob3cgY2xlYXJseSBzdGF0
ZSB0aGF0IHRoZSBPTEVEIGRpc3BsYXkNCj4+IHVzZXMgYWN0aXZlIGxvdyByZXNldCBpbiBteSBE
VC4NCj4+DQo+PiBNeSBmaXJzdCBhdHRlbXB0IHRvIGZpeCB0aGlzIHdhcyB0byBjaGFuZ2UgdGhl
IHJlc2V0IHNlcXVlbmNlIFsyXS4NCj4+IEl0IHdhcyBhcHBsaWVkIGFuZCB0aGVuIHJldmVydGVk
IGFzIGl0IGlzIG5vdCBiYWNrd2FyZCBjb21wYXRpYmxlIHdpdGgNCj4+IGRlcGxveWVkIERUQiBm
aWxlcyBbM10uDQo+Pg0KPj4gWzFdIGh0dHBzOi8vZWxpeGlyLmJvb3RsaW4uY29tL2xpbnV4L3Y0
LjIwLXJjMy9zb3VyY2UvZHJpdmVycy92aWRlby9mYmRldi9zc2QxMzA3ZmIuYyNMNTcwDQo+PiBb
Ml0gaHR0cHM6Ly9wYXRjaHdvcmsua2VybmVsLm9yZy9wYXRjaC8xMDYxNzcyOS8NCj4+IFszXSBo
dHRwczovL3BhdGNod29yay5rZXJuZWwub3JnL3BhdGNoLzEwNjE3NzMxLw0KPiANCj4gT2theSwg
bm93IEkgdW5kZXJzdGFuZCB0aGUgYmFja2dyb3VuZC4gV2UndmUgaGl0IHRoaXMgc29tZXdoZXJl
IGVsc2UgdG9vLg0KPiANCj4gUmF0aGVyIHRoYW4gaGF2ZSBhIGJpbmRpbmcgZGVtb25zdHJhdGlu
ZyB3aGF0IG5vdCB0byBkbywgSSdkIGxpa2UgdG8NCj4gZml4IHRoaXMgaW4gYW5vdGhlciB3YXku
IEkgYWxzbyBkb24ndCB3YW50IHRvIGxpdmUgd2l0aCB0aGlzIGZvcmV2ZXINCj4gd2hlbiB0aGVy
ZSdzIG9ubHkgMSBib2FyZCBhZmZlY3RlZCAoaW4gdHJlZSBhdCBsZWFzdCkgYW5kIHRoZXJlJ3Mg
b25seQ0KPiBhbiBBQkkgaWYgc29tZW9uZSBub3RpY2VzIChJJ20gaGFwcHkgdGhvdWdoIHRoYXQg
dGhlIG1haW50YWluZXJzDQo+IGNhdWdodCB0aGlzKS4gVGhlcmUncyAyIG90aGVyIG9wdGlvbnMu
IFRoZSAxc3QgaXMgYWRkIGEgZml4dXAgdG8gdGhlDQo+IERUIGZvciB0aGlzIHBsYXRmb3JtIHRv
IGVuc3VyZSB0aGF0IHRoZSBHUElPIGlzIGNvbmZpZ3VyZWQgYWN0aXZlIGxvdw0KPiAodGhlIFZl
cnNhdGlsZSBwbGF0Zm9ybSBjb2RlIGhhcyBhbiBleGFtcGxlIGZpeHVwKS4gV2l0aCB0aGF0LCBh
cHBseQ0KPiB3aGF0IHdhcyBvcmlnaW5hbGx5IGFwcGxpZWQgKG9yIHJldmVydCB0aGUgcmV2ZXJ0
KS4gVGhlIGZpeHVwIGNvdWxkIGJlDQo+IGFwcGxpZWQgb25seSBhZnRlciBzb21lb25lIGNvbXBs
YWlucyB0aGVpciBkaXNwbGF5IGJyb2tlLiBUaGUgMm5kDQo+IG9wdGlvbiBpcyBqdXN0IGFkZCBh
biBvZl9tYWNoaW5lX2lzX2NvbXBhdGlibGUgY2hlY2sgd2l0aGluIHRoaXMNCj4gZHJpdmVyLiBJ
biB0aGF0IGNhc2UsIHlvdSB3b3VsZG4ndCBmaXggZHRzIGZpbGUgZm9yIHRoZSBwbGF0Zm9ybQ0K
PiAodW5sZXNzIHlvdSBhbHNvIHdhbnQgdG8gbWFudWFsbHkgY2hlY2sgcmVzZXQtZ3Bpb3MpLg0K
DQpIaSBSb2IsDQoNCkkgYW0gc3RpbGwgdHJ5aW5nIHRvIGZpZ3VyZSBvdXQgd2hhdCBleGFjdGx5
IHlvdSBtZWFudCBieSB0aGUgMXN0IGFuZA0KMm5kIG9wdGlvbi4gQm90aCBjb25jZXB0cyBhcmUg
bmV3IHRvIG1lLg0KDQpSZWdhcmRpbmcgdGhlIDFzdCBvcHRpb24sIHdoYXQgeW91IG1lYW50IGJ5
ICJ0aGlzIHBsYXRmb3JtIiBoZXJlOg0KPiBBZGQgYSBmaXh1cCB0byB0aGUgRFQgZm9yIHRoaXMg
cGxhdGZvcm0NClRoZSBvbmx5IGJvYXJkIGluIHRyZWUgdGhhdCB1c2VzIHRoZSBPTEVEIChpbXgy
OC1jZmExMDAzNikgYW5kIGl0cw0KZHRzIGZpbGU/DQoNCkkgYW0gYWxzbyBub3Qgc3VyZSB3aGVy
ZSB0byBsb29rIGZvciB0aGUgZXhhbXBsZS4gV2hlbiB5b3Ugc2F5DQpWZXJzYXRpbGUgcGxhdGZv
cm0gY29kZSBJIHRlbmQgdG8gbG9vayBpbnRvIHBsYXQtdmVyc2F0aWxlIG9yDQptYWNoLXZlcnNh
dGlsZS4gSSBjb3VsZCBub3QgZmluZCBhbnl0aGluZyBJIGNvdWxkIHVzZSBhcyBhbiBleGFtcGxl
DQppbiB0aGVyZS4gSSB0aGluayB0aGF0IGlzIG5vdCB3aGF0IHlvdSBtZWFudC4NCg0KUmVnYXJk
aW5nIHRoZSAybmQgb3B0aW9uLCB5b3Ugc3VnZ2VzdCB0byB1c2Ugb2ZfbWFjaGluZV9pc19jb21w
YXRpYmxlDQp0byBkZWNpZGUgd2hhdCByZXNldCBzZXF1ZW5jZSB0byB1c2U/IEluIGNhc2Ugb2Yg
aW14MjgtY2ZhMTAwMzYgdXNlDQp0aGUgb2xkIDAgLT4gMSwgaW4gYWxsIG90aGVyIGNhc2VzIHVz
ZSBhIG5ldyBjb3JyZWN0IHNlcXVlbmNlIDEgLT4gMD8NClRoYXQgYWxzbyBkb2VzIG5vdCBzZWVt
IHJpZ2h0Lg0KDQpJIHdpbGwgYXBwcmVjaWF0ZSBtb3JlIGRldGFpbHMgaG93IHRvIHByb2NlZWQu
IFRoYW5rIHlvdSwNCk1pY2hhbA0K

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

[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

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

[Vokáč Michal]

T24gMjYuMTEuMjAxOCAxNDo0OSwgUm9iIEhlcnJpbmcgd3JvdGU6DQo+IE9uIE1vbiwgTm92IDI2
LCAyMDE4IGF0IDY6MjUgQU0gVm9rw6HEjSBNaWNoYWwgPE1pY2hhbC5Wb2thY0B5c29mdC5jb20+
IHdyb3RlOg0KPj4NCj4+IE9uIDE5LjExLjIwMTggMjM6MzIsIFJvYiBIZXJyaW5nIHdyb3RlOg0K
Pj4+IE9uIE1vbiwgTm92IDE5LCAyMDE4IGF0IDk6MTIgQU0gVm9rw6HEjSBNaWNoYWwgPE1pY2hh
bC5Wb2thY0B5c29mdC5jb20+IHdyb3RlOg0KPj4+PiBPbiAxMi4xMS4yMDE4IDE3OjU1LCBSb2Ig
SGVycmluZyB3cm90ZToNCj4+Pj4+IE9uIEZyaSwgTm92IDAyLCAyMDE4IGF0IDAyOjU2OjM1UE0g
KzAwMDAsIFZva8OhxI0gTWljaGFsIHdyb3RlOg0KPj4+Pj4+IFRoZSBTU0QxMzB4IE9MRUQgZGlz
cGxheSByZXNldCBzaWduYWwgaXMgYWN0aXZlIGxvdy4gTm93IHRoZSByZXNldA0KPj4+Pj4+IHNl
cXVlbmNlIGlzIGltcGxlbWVudGVkIGluIHN1Y2ggYSB3YXkgdGhhdCBEVFMgYXV0aG9ycyBhcmUg
Zm9yY2VkIHRvDQo+Pj4+Pj4gZGVmaW5lIHRoZSByZXNldC1ncGlvcyBwcm9wZXJ0eSB3aXRoIEdQ
SU9fQUNUSVZFX0hJR0ggdG8gbWFrZSB0aGUgcmVzZXQNCj4+Pj4+PiB3b3JrLg0KPj4+Pj4+DQo+
Pj4+Pj4gQWRkIHRoZSByZXNldC1hY3RpdmUtbG93IHByb3BlcnR5IHNvIHRoZSBzaWduYWwgaXMg
aW52ZXJ0ZWQgb25jZSBhZ2Fpbg0KPj4+Pj4+IGFuZCB0aGUgR1BJT19BQ1RJVkVfTE9XIHdvcmsg
YXMgZXhwZWN0ZWQuDQo+Pj4+Pj4NCj4+Pj4+PiBTaWduZWQtb2ZmLWJ5OiBNaWNoYWwgVm9rw6HE
jSA8bWljaGFsLnZva2FjQHlzb2Z0LmNvbT4NCj4+Pj4+PiAtLS0NCj4+Pj4+PiAgICAgZHJpdmVy
cy92aWRlby9mYmRldi9zc2QxMzA3ZmIuYyB8IDYgKysrKy0tDQo+Pj4+Pj4gICAgIDEgZmlsZSBj
aGFuZ2VkLCA0IGluc2VydGlvbnMoKyksIDIgZGVsZXRpb25zKC0pDQo+Pj4+Pj4NCj4+Pj4+PiBk
aWZmIC0tZ2l0IGEvZHJpdmVycy92aWRlby9mYmRldi9zc2QxMzA3ZmIuYyBiL2RyaXZlcnMvdmlk
ZW8vZmJkZXYvc3NkMTMwN2ZiLmMNCj4+Pj4+PiBpbmRleCBlN2FlMTM1Li43OTBmMWM0IDEwMDY0
NA0KPj4+Pj4+IC0tLSBhL2RyaXZlcnMvdmlkZW8vZmJkZXYvc3NkMTMwN2ZiLmMNCj4+Pj4+PiAr
KysgYi9kcml2ZXJzL3ZpZGVvL2ZiZGV2L3NzZDEzMDdmYi5jDQo+Pj4+Pj4gQEAgLTYwOCw2ICs2
MDgsNyBAQCBzdGF0aWMgaW50IHNzZDEzMDdmYl9wcm9iZShzdHJ1Y3QgaTJjX2NsaWVudCAqY2xp
ZW50LA0KPj4+Pj4+ICAgICAgICBzdHJ1Y3QgZmJfZGVmZXJyZWRfaW8gKnNzZDEzMDdmYl9kZWZp
bzsNCj4+Pj4+PiAgICAgICAgdTMyIHZtZW1fc2l6ZTsNCj4+Pj4+PiAgICAgICAgc3RydWN0IHNz
ZDEzMDdmYl9wYXIgKnBhcjsNCj4+Pj4+PiArICAgIGJvb2wgcmVzZXRfYWN0aXZlX2xvdzsNCj4+
Pj4+PiAgICAgICAgdTggKnZtZW07DQo+Pj4+Pj4gICAgICAgIGludCByZXQ7DQo+Pj4+Pj4NCj4+
Pj4+PiBAQCAtNjcxLDYgKzY3Miw3IEBAIHN0YXRpYyBpbnQgc3NkMTMwN2ZiX3Byb2JlKHN0cnVj
dCBpMmNfY2xpZW50ICpjbGllbnQsDQo+Pj4+Pj4gICAgICAgIHBhci0+Y29tX3NlcSA9IG9mX3By
b3BlcnR5X3JlYWRfYm9vbChub2RlLCAic29sb21vbixjb20tc2VxIik7DQo+Pj4+Pj4gICAgICAg
IHBhci0+Y29tX2xycmVtYXAgPSBvZl9wcm9wZXJ0eV9yZWFkX2Jvb2wobm9kZSwgInNvbG9tb24s
Y29tLWxycmVtYXAiKTsNCj4+Pj4+PiAgICAgICAgcGFyLT5jb21faW52ZGlyID0gb2ZfcHJvcGVy
dHlfcmVhZF9ib29sKG5vZGUsICJzb2xvbW9uLGNvbS1pbnZkaXIiKTsNCj4+Pj4+PiArICAgIHJl
c2V0X2FjdGl2ZV9sb3cgPSBvZl9wcm9wZXJ0eV9yZWFkX2Jvb2wobm9kZSwgInJlc2V0LWFjdGl2
ZS1sb3ciKTsNCj4+Pj4+Pg0KPj4+Pj4+ICAgICAgICBwYXItPmNvbnRyYXN0ID0gMTI3Ow0KPj4+
Pj4+ICAgICAgICBwYXItPnZjb21oID0gcGFyLT5kZXZpY2VfaW5mby0+ZGVmYXVsdF92Y29taDsN
Cj4+Pj4+PiBAQCAtNzI4LDkgKzczMCw5IEBAIHN0YXRpYyBpbnQgc3NkMTMwN2ZiX3Byb2JlKHN0
cnVjdCBpMmNfY2xpZW50ICpjbGllbnQsDQo+Pj4+Pj4NCj4+Pj4+PiAgICAgICAgaWYgKHBhci0+
cmVzZXQpIHsNCj4+Pj4+PiAgICAgICAgICAgICAgICAvKiBSZXNldCB0aGUgc2NyZWVuICovDQo+
Pj4+Pj4gLSAgICAgICAgICAgIGdwaW9kX3NldF92YWx1ZV9jYW5zbGVlcChwYXItPnJlc2V0LCAw
KTsNCj4+Pj4+PiArICAgICAgICAgICAgZ3Bpb2Rfc2V0X3ZhbHVlX2NhbnNsZWVwKHBhci0+cmVz
ZXQsIHJlc2V0X2FjdGl2ZV9sb3cpOw0KPj4+Pj4+ICAgICAgICAgICAgICAgIHVkZWxheSg0KTsN
Cj4+Pj4+PiAtICAgICAgICAgICAgZ3Bpb2Rfc2V0X3ZhbHVlX2NhbnNsZWVwKHBhci0+cmVzZXQs
IDEpOw0KPj4+Pj4+ICsgICAgICAgICAgICBncGlvZF9zZXRfdmFsdWVfY2Fuc2xlZXAocGFyLT5y
ZXNldCwgIXJlc2V0X2FjdGl2ZV9sb3cpOw0KPj4+Pj4NCj4+Pj4+IEkgdGhpbmsgeW91IGFuZCB3
aG9tZXZlciB3cm90ZSB0aGUgb3JpZ2luYWwgY29kZSBhcmUgbWlzaW50ZXJwcmV0dGluZw0KPj4+
Pj4gaG93IHRoZSBncGlvZCBBUEkgd29ya3MuIDEgbWVhbnMgbWFrZSB0aGUgc2lnbmFsIGFjdGl2
ZSBhbmQgdGhpcyBjYXNlDQo+Pj4+PiBhY3RpdmUgaXMgbG93Lg0KPj4+Pg0KPj4+PiBJIHRvdGFs
bHkgYWdyZWUgYW5kIEkgdGhpbmsgSSB1bmRlcnN0YW5kIHRoYXQgY29ycmVjdGx5Lg0KPj4+Pg0K
Pj4+Pj4gSXQgaXMgc3RyYW5nZSwgYnV0IGRvZXMgbWVhbiBhIHJlc2V0IHNlcXVlbmNlIHNob3Vs
ZCBhbHdheXMgYmUgc2V0IHRvIGENCj4+Pj4+IDEgYW5kIHRoZW4gYSAwIGFuZCBpdCB3aWxsIHdv
cmsgd2l0aCBlaXRoZXIgcG9sYXJpdHkgaW4gdGhlIERULg0KPj4+Pg0KPj4+PiBJIGFncmVlIHRo
ZSByZXNldCBzaG91bGQgYmUgZG9uZSBhcyBhIDAgLT4gMSAtPiAwIHNlcXVlbmNlIGFuZCB0aGF0
IHNob3VsZA0KPj4+PiBqdXN0IHdvcmsuIFRoZSBwcm9ibGVtIGlzIGl0IGlzIGltcGxlbWVudGVk
IHZpY2UgdmVyc2EgYW5kIHNvIGl0IHdvcmtzIG9ubHkNCj4+Pj4gaWYgeW91IGhhdmUgR1BJT19B
Q1RJVkVfSElHSCBpbiBEVCBmb3IgYSBzaWduYWwgdGhhdCBpcyBhY3R1YWxseSBhY3RpdmUgbG93
Lg0KPj4+Pg0KPj4+PiBBbmQgd2hhdCBpdCBhY3R1YWxseSBkb2VzIGlzIHRoYXQgaXQgaG9sZHMg
dGhlIGNvbnRyb2xsZXIgaW4gcmVzZXQgc2luY2UNCj4+Pj4gdGhlIEdQSU8gaXMgc3VjY2Vzc2Z1
bGx5IGFjcXVpcmVkIChiZWNhdXNlIG9mIEdQSU9EX09VVF9MT1cgaGVyZSBbMV0pIGFuZA0KPj4+
PiBsYXRlciBvbiBpdCBvbmx5IHJlbGVhc2VzIHRoZSByZXNldC4NCj4+Pj4NCj4+Pj4gQXMgYSBE
VCBhdXRob3IgSSB3b3VsZCBsaWtlIHRvIHNvbWVob3cgY2xlYXJseSBzdGF0ZSB0aGF0IHRoZSBP
TEVEIGRpc3BsYXkNCj4+Pj4gdXNlcyBhY3RpdmUgbG93IHJlc2V0IGluIG15IERULg0KPj4+Pg0K
Pj4+PiBNeSBmaXJzdCBhdHRlbXB0IHRvIGZpeCB0aGlzIHdhcyB0byBjaGFuZ2UgdGhlIHJlc2V0
IHNlcXVlbmNlIFsyXS4NCj4+Pj4gSXQgd2FzIGFwcGxpZWQgYW5kIHRoZW4gcmV2ZXJ0ZWQgYXMg
aXQgaXMgbm90IGJhY2t3YXJkIGNvbXBhdGlibGUgd2l0aA0KPj4+PiBkZXBsb3llZCBEVEIgZmls
ZXMgWzNdLg0KPj4+Pg0KPj4+PiBbMV0gaHR0cHM6Ly9lbGl4aXIuYm9vdGxpbi5jb20vbGludXgv
djQuMjAtcmMzL3NvdXJjZS9kcml2ZXJzL3ZpZGVvL2ZiZGV2L3NzZDEzMDdmYi5jI0w1NzANCj4+
Pj4gWzJdIGh0dHBzOi8vcGF0Y2h3b3JrLmtlcm5lbC5vcmcvcGF0Y2gvMTA2MTc3MjkvDQo+Pj4+
IFszXSBodHRwczovL3BhdGNod29yay5rZXJuZWwub3JnL3BhdGNoLzEwNjE3NzMxLw0KPj4+DQo+
Pj4gT2theSwgbm93IEkgdW5kZXJzdGFuZCB0aGUgYmFja2dyb3VuZC4gV2UndmUgaGl0IHRoaXMg
c29tZXdoZXJlIGVsc2UgdG9vLg0KPj4+DQo+Pj4gUmF0aGVyIHRoYW4gaGF2ZSBhIGJpbmRpbmcg
ZGVtb25zdHJhdGluZyB3aGF0IG5vdCB0byBkbywgSSdkIGxpa2UgdG8NCj4+PiBmaXggdGhpcyBp
biBhbm90aGVyIHdheS4gSSBhbHNvIGRvbid0IHdhbnQgdG8gbGl2ZSB3aXRoIHRoaXMgZm9yZXZl
cg0KPj4+IHdoZW4gdGhlcmUncyBvbmx5IDEgYm9hcmQgYWZmZWN0ZWQgKGluIHRyZWUgYXQgbGVh
c3QpIGFuZCB0aGVyZSdzIG9ubHkNCj4+PiBhbiBBQkkgaWYgc29tZW9uZSBub3RpY2VzIChJJ20g
aGFwcHkgdGhvdWdoIHRoYXQgdGhlIG1haW50YWluZXJzDQo+Pj4gY2F1Z2h0IHRoaXMpLiBUaGVy
ZSdzIDIgb3RoZXIgb3B0aW9ucy4gVGhlIDFzdCBpcyBhZGQgYSBmaXh1cCB0byB0aGUNCj4+PiBE
VCBmb3IgdGhpcyBwbGF0Zm9ybSB0byBlbnN1cmUgdGhhdCB0aGUgR1BJTyBpcyBjb25maWd1cmVk
IGFjdGl2ZSBsb3cNCj4+PiAodGhlIFZlcnNhdGlsZSBwbGF0Zm9ybSBjb2RlIGhhcyBhbiBleGFt
cGxlIGZpeHVwKS4gV2l0aCB0aGF0LCBhcHBseQ0KPj4+IHdoYXQgd2FzIG9yaWdpbmFsbHkgYXBw
bGllZCAob3IgcmV2ZXJ0IHRoZSByZXZlcnQpLiBUaGUgZml4dXAgY291bGQgYmUNCj4+PiBhcHBs
aWVkIG9ubHkgYWZ0ZXIgc29tZW9uZSBjb21wbGFpbnMgdGhlaXIgZGlzcGxheSBicm9rZS4gVGhl
IDJuZA0KPj4+IG9wdGlvbiBpcyBqdXN0IGFkZCBhbiBvZl9tYWNoaW5lX2lzX2NvbXBhdGlibGUg
Y2hlY2sgd2l0aGluIHRoaXMNCj4+PiBkcml2ZXIuIEluIHRoYXQgY2FzZSwgeW91IHdvdWxkbid0
IGZpeCBkdHMgZmlsZSBmb3IgdGhlIHBsYXRmb3JtDQo+Pj4gKHVubGVzcyB5b3UgYWxzbyB3YW50
IHRvIG1hbnVhbGx5IGNoZWNrIHJlc2V0LWdwaW9zKS4NCj4+DQo+PiBIaSBSb2IsDQo+Pg0KPj4g
SSBhbSBzdGlsbCB0cnlpbmcgdG8gZmlndXJlIG91dCB3aGF0IGV4YWN0bHkgeW91IG1lYW50IGJ5
IHRoZSAxc3QgYW5kDQo+PiAybmQgb3B0aW9uLiBCb3RoIGNvbmNlcHRzIGFyZSBuZXcgdG8gbWUu
DQo+Pg0KPj4gUmVnYXJkaW5nIHRoZSAxc3Qgb3B0aW9uLCB3aGF0IHlvdSBtZWFudCBieSAidGhp
cyBwbGF0Zm9ybSIgaGVyZToNCj4+PiBBZGQgYSBmaXh1cCB0byB0aGUgRFQgZm9yIHRoaXMgcGxh
dGZvcm0NCj4+IFRoZSBvbmx5IGJvYXJkIGluIHRyZWUgdGhhdCB1c2VzIHRoZSBPTEVEIChpbXgy
OC1jZmExMDAzNikgYW5kIGl0cw0KPj4gZHRzIGZpbGU/DQo+IA0KPiBZZXMsIHRoYXQgb25lLg0K
PiANCj4+IEkgYW0gYWxzbyBub3Qgc3VyZSB3aGVyZSB0byBsb29rIGZvciB0aGUgZXhhbXBsZS4g
V2hlbiB5b3Ugc2F5DQo+PiBWZXJzYXRpbGUgcGxhdGZvcm0gY29kZSBJIHRlbmQgdG8gbG9vayBp
bnRvIHBsYXQtdmVyc2F0aWxlIG9yDQo+PiBtYWNoLXZlcnNhdGlsZS4gSSBjb3VsZCBub3QgZmlu
ZCBhbnl0aGluZyBJIGNvdWxkIHVzZSBhcyBhbiBleGFtcGxlDQo+PiBpbiB0aGVyZS4gSSB0aGlu
ayB0aGF0IGlzIG5vdCB3aGF0IHlvdSBtZWFudC4NCj4gDQo+IFNlZSB2ZXJzYXRpbGVfZHRfcGNp
X2luaXQoKS4gT3IgbG9vayBmb3Igb3RoZXIgY2FsbGVycyBvZiBvZl91cGRhdGVfcHJvcGVydHko
KS4NCg0KRXhjZWxsZW50LCBJIHdpbGwgbG9vayBhdCB0aGF0Lg0KICANCj4+IFJlZ2FyZGluZyB0
aGUgMm5kIG9wdGlvbiwgeW91IHN1Z2dlc3QgdG8gdXNlIG9mX21hY2hpbmVfaXNfY29tcGF0aWJs
ZQ0KPj4gdG8gZGVjaWRlIHdoYXQgcmVzZXQgc2VxdWVuY2UgdG8gdXNlPyBJbiBjYXNlIG9mIGlt
eDI4LWNmYTEwMDM2IHVzZQ0KPj4gdGhlIG9sZCAwIC0+IDEsIGluIGFsbCBvdGhlciBjYXNlcyB1
c2UgYSBuZXcgY29ycmVjdCBzZXF1ZW5jZSAxIC0+IDA/DQo+PiBUaGF0IGFsc28gZG9lcyBub3Qg
c2VlbSByaWdodC4NCj4gDQo+IENvcnJlY3QuIFRob3VnaCBpZiB5b3UgZml4IGlteDI4LWNmYTEw
MDM2IGR0cywgdGhlbiB5b3UgaGF2ZSB0byBoYW5kbGUNCj4gdGhhdCBjYXNlIHRvby4NCj4gDQo+
IFdoeSBpcyBpdCBub3QgcmlnaHQ/IFVnbHkgeWVzLCBidXQgaXQncyBub3Qgd3JvbmcuDQoNClVn
bHkgaXMgd2hhdCBJIHByb2JhYmx5IG1lYW50LiBJdCBzZWVtcyBsaWtlIG90aGVyIHVzZXJzIChh
bW9uZyBkcml2ZXJzKQ0Kb2Ygb2ZfbWFjaGluZV9pc19jb21wYXRpYmxlIGFyZSBtb3N0bHkgZHJp
dmVycyBmb3IgSVAgYmxvY2tzIHRoYXQgbmVlZA0Kc2xpZ2h0bHkgZGlmZmVyZW50IGhhbmRsaW5n
IG9uIGRpZmZlcmVudCBTb0MgdmFyaWFudHMuDQoNClRoYW5rIHlvdSB2ZXJ5IG11Y2gsDQpNaWNo
YWwNCg=