| Message ID | 1541170556-75032-2-git-send-email-michal.vokac@ysoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [fbdev-for-next,1/2] dt-bindings: display: ssd1307fb: Add reset-active-low property | expand |
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
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
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
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
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
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); }
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(-)