From: Chen-Yu Tsai <[email protected]>
The SSD130x's command to toggle COM scan direction uses bit 3 and only
bit 3 to set the direction of the scanout. The driver has an incorrect
GENMASK(3, 2), causing the setting to be set on bit 2, rendering it
ineffective.
Fix the mask to only bit 3, so that the requested setting is applied
correctly.
Fixes: a61732e80867 ("drm: Add driver for Solomon SSD130x OLED displays")
Signed-off-by: Chen-Yu Tsai <[email protected]>
---
drivers/gpu/drm/solomon/ssd130x.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index ce4dc20412e0..ccd378135589 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -61,7 +61,7 @@
#define SSD130X_SET_COM_PINS_CONFIG 0xda
#define SSD130X_SET_VCOMH 0xdb
-#define SSD130X_SET_COM_SCAN_DIR_MASK GENMASK(3, 2)
+#define SSD130X_SET_COM_SCAN_DIR_MASK GENMASK(3, 3)
#define SSD130X_SET_COM_SCAN_DIR_SET(val) FIELD_PREP(SSD130X_SET_COM_SCAN_DIR_MASK, (val))
#define SSD130X_SET_CLOCK_DIV_MASK GENMASK(3, 0)
#define SSD130X_SET_CLOCK_DIV_SET(val) FIELD_PREP(SSD130X_SET_CLOCK_DIV_MASK, (val))
--
2.34.1
Hello Chen-Yu,
Thanks a lot for your patch.
On 3/8/22 17:07, Chen-Yu Tsai wrote:
> From: Chen-Yu Tsai <[email protected]>
>
> The SSD130x's command to toggle COM scan direction uses bit 3 and only
> bit 3 to set the direction of the scanout. The driver has an incorrect
> GENMASK(3, 2), causing the setting to be set on bit 2, rendering it
> ineffective.
>
> Fix the mask to only bit 3, so that the requested setting is applied
> correctly.
>
Sigh, you are correct. I thought that triple checked the datasheet
when writing this but I got it wrong anyways...
> Fixes: a61732e80867 ("drm: Add driver for Solomon SSD130x OLED displays")
> Signed-off-by: Chen-Yu Tsai <[email protected]>
> ---
Acked-by: Javier Martinez Canillas <[email protected]>
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
From: Chen-Yu Tsai <[email protected]>
Currently the ssd130x driver only sets the segment remap setting when
the device tree requests it; it however does not clear the setting if
it is not requested. This leads to the setting incorrectly persisting
if the hardware is always on and has no reset GPIO wired. This might
happen when a developer is trying to find the correct settings for an
unknown module, and cause the developer to get confused because the
settings from the device tree are not consistently applied.
Make the driver apply the segment remap setting consistently, setting
the value correctly based on the device tree setting. This also makes
this setting's behavior consistent with the other settings, which are
always applied.
Fixes: a61732e80867 ("drm: Add driver for Solomon SSD130x OLED displays")
Signed-off-by: Chen-Yu Tsai <[email protected]>
---
drivers/gpu/drm/solomon/ssd130x.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index ccd378135589..d08d86ef07bc 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -48,7 +48,7 @@
#define SSD130X_CONTRAST 0x81
#define SSD130X_SET_LOOKUP_TABLE 0x91
#define SSD130X_CHARGE_PUMP 0x8d
-#define SSD130X_SEG_REMAP_ON 0xa1
+#define SSD130X_SET_SEG_REMAP 0xa0
#define SSD130X_DISPLAY_OFF 0xae
#define SSD130X_SET_MULTIPLEX_RATIO 0xa8
#define SSD130X_DISPLAY_ON 0xaf
@@ -61,6 +61,8 @@
#define SSD130X_SET_COM_PINS_CONFIG 0xda
#define SSD130X_SET_VCOMH 0xdb
+#define SSD130X_SET_SEG_REMAP_MASK GENMASK(0, 0)
+#define SSD130X_SET_SEG_REMAP_SET(val) FIELD_PREP(SSD130X_SET_SEG_REMAP_MASK, (val))
#define SSD130X_SET_COM_SCAN_DIR_MASK GENMASK(3, 3)
#define SSD130X_SET_COM_SCAN_DIR_SET(val) FIELD_PREP(SSD130X_SET_COM_SCAN_DIR_MASK, (val))
#define SSD130X_SET_CLOCK_DIV_MASK GENMASK(3, 0)
@@ -235,7 +237,7 @@ static void ssd130x_power_off(struct ssd130x_device *ssd130x)
static int ssd130x_init(struct ssd130x_device *ssd130x)
{
- u32 precharge, dclk, com_invdir, compins, chargepump;
+ u32 precharge, dclk, com_invdir, compins, chargepump, seg_remap;
int ret;
/* Set initial contrast */
@@ -244,11 +246,11 @@ static int ssd130x_init(struct ssd130x_device *ssd130x)
return ret;
/* Set segment re-map */
- if (ssd130x->seg_remap) {
- ret = ssd130x_write_cmd(ssd130x, 1, SSD130X_SEG_REMAP_ON);
- if (ret < 0)
- return ret;
- }
+ seg_remap = (SSD130X_SET_SEG_REMAP |
+ SSD130X_SET_SEG_REMAP_SET(ssd130x->seg_remap));
+ ret = ssd130x_write_cmd(ssd130x, 1, seg_remap);
+ if (ret < 0)
+ return ret;
/* Set COM direction */
com_invdir = (SSD130X_SET_COM_SCAN_DIR |
--
2.34.1
On 3/8/22 17:07, Chen-Yu Tsai wrote:
> From: Chen-Yu Tsai <[email protected]>
>
> Currently the ssd130x driver only sets the segment remap setting when
> the device tree requests it; it however does not clear the setting if
> it is not requested. This leads to the setting incorrectly persisting
> if the hardware is always on and has no reset GPIO wired. This might
> happen when a developer is trying to find the correct settings for an
> unknown module, and cause the developer to get confused because the
> settings from the device tree are not consistently applied.
>
> Make the driver apply the segment remap setting consistently, setting
> the value correctly based on the device tree setting. This also makes
> this setting's behavior consistent with the other settings, which are
> always applied.
>
Nice catch. This is certainly much better. Thanks!
> Fixes: a61732e80867 ("drm: Add driver for Solomon SSD130x OLED displays")
> Signed-off-by: Chen-Yu Tsai <[email protected]>
> ---
Acked-by: Javier Martinez Canillas <[email protected]>
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
On Wed, Mar 9, 2022 at 2:57 AM Chen-Yu Tsai <[email protected]> wrote:
> From: Chen-Yu Tsai <[email protected]>
>
> The SSD130x's command to toggle COM scan direction uses bit 3 and only
> bit 3 to set the direction of the scanout. The driver has an incorrect
> GENMASK(3, 2), causing the setting to be set on bit 2, rendering it
> ineffective.
>
> Fix the mask to only bit 3, so that the requested setting is applied
> correctly.
>
> Fixes: a61732e80867 ("drm: Add driver for Solomon SSD130x OLED displays")
> Signed-off-by: Chen-Yu Tsai <[email protected]>
Thanks, this fixes the vertically-mirrored display on my Adafruit
FeatherWing 128x32 OLED.
Tested-by: Geert Uytterhoeven <[email protected]>
Reviewed-by: Geert Uytterhoeven <[email protected]>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On 3/9/22 13:56, Geert Uytterhoeven wrote:
> On Wed, Mar 9, 2022 at 2:57 AM Chen-Yu Tsai <[email protected]> wrote:
>> From: Chen-Yu Tsai <[email protected]>
>>
>> The SSD130x's command to toggle COM scan direction uses bit 3 and only
>> bit 3 to set the direction of the scanout. The driver has an incorrect
>> GENMASK(3, 2), causing the setting to be set on bit 2, rendering it
>> ineffective.
>>
>> Fix the mask to only bit 3, so that the requested setting is applied
>> correctly.
>>
>> Fixes: a61732e80867 ("drm: Add driver for Solomon SSD130x OLED displays")
>> Signed-off-by: Chen-Yu Tsai <[email protected]>
>
> Thanks, this fixes the vertically-mirrored display on my Adafruit
> FeatherWing 128x32 OLED.
> Tested-by: Geert Uytterhoeven <[email protected]>
> Reviewed-by: Geert Uytterhoeven <[email protected]>
>
Thanks for the testing and review. I've pushed both patches to drm-misc-next.
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat