[PATCH 0/3] drm/ssd130x: some small ssd132x fixes

[PATCH 0/3] drm/ssd130x: some small ssd132x fixes

[John Keeping]

Three small fixes for ssd132x family chips in the ssd130x driver.

Details are in the patches.

John Keeping (3):
  drm/ssd130x: Fix reset timing for ssd132x
  drm/ssd130x: fix ssd132x encoding
  drm/ssd130x: ensure ssd132x pitch is correct

 drivers/gpu/drm/solomon/ssd130x.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

-- 
2.47.1

[PATCH 1/3] drm/ssd130x: Fix reset timing for ssd132x

[John Keeping]

The ssd132x family of chips require the result pulse to be at least
100us in length.  Increase the reset time to meet this requirement.

Signed-off-by: John Keeping <jkeeping@inmusicbrands.com>
---
 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 b777690fd6607..2622172228361 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -363,7 +363,7 @@ static void ssd130x_reset(struct ssd130x_device *ssd130x)
 
 	/* Reset the screen */
 	gpiod_set_value_cansleep(ssd130x->reset, 1);
-	udelay(4);
+	usleep_range(100, 1000);
 	gpiod_set_value_cansleep(ssd130x->reset, 0);
 	udelay(4);
 }
-- 
2.47.1

[PATCH 2/3] drm/ssd130x: fix ssd132x encoding

[John Keeping]

The ssd132x buffer is encoded one pixel per nibble, with two pixels in
each byte.  When encoding and 8-bit greyscale input, take the top 4-bits
as the value and ensure the two pixels are distinct and do not overwrite
each other.

Signed-off-by: John Keeping <jkeeping@inmusicbrands.com>
---
 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 2622172228361..64f1123080996 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -880,7 +880,7 @@ static int ssd132x_update_rect(struct ssd130x_device *ssd130x,
 			u8 n1 = buf[i * width + j];
 			u8 n2 = buf[i * width + j + 1];
 
-			data_array[array_idx++] = (n2 << 4) | n1;
+			data_array[array_idx++] = (n2 & 0xf0) | (n1 >> 4);
 		}
 	}
 
-- 
2.47.1

[PATCH 3/3] drm/ssd130x: ensure ssd132x pitch is correct

[John Keeping]

The bounding rectangle is adjusted to ensure it aligns to
SSD132X_SEGMENT_WIDTH, which may adjust the pitch.  Calcuate the pitch
after alighting the left and right edge.

Signed-off-by: John Keeping <jkeeping@inmusicbrands.com>
---
 drivers/gpu/drm/solomon/ssd130x.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index 64f1123080996..38f31c3624062 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -1037,7 +1037,7 @@ static int ssd132x_fb_blit_rect(struct drm_framebuffer *fb,
 				struct drm_format_conv_state *fmtcnv_state)
 {
 	struct ssd130x_device *ssd130x = drm_to_ssd130x(fb->dev);
-	unsigned int dst_pitch = drm_rect_width(rect);
+	unsigned int dst_pitch;
 	struct iosys_map dst;
 	int ret = 0;
 
@@ -1046,6 +1046,8 @@ static int ssd132x_fb_blit_rect(struct drm_framebuffer *fb,
 	rect->x2 = min_t(unsigned int, round_up(rect->x2, SSD132X_SEGMENT_WIDTH),
 			 ssd130x->width);
 
+	dst_pitch = drm_rect_width(rect);
+
 	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
 	if (ret)
 		return ret;
-- 
2.47.1

Re: [PATCH 1/3] drm/ssd130x: Fix reset timing for ssd132x

[Javier Martinez Canillas]

John Keeping <jkeeping@inmusicbrands.com> writes:

Hello John,

Thanks for your patches!

> The ssd132x family of chips require the result pulse to be at least
> 100us in length.  Increase the reset time to meet this requirement.
>

That's not what the datasheet says AFAIU. It says the following in the
"8.9 Power ON and OFF sequence" section.

Power ON sequence:

1. Power ON VDD.
2. After VDD become stable, set RES# pin LOW (logic LOW) for at least
   3us (t1) and then HIGH (logic HIGH).
3. After set RES# pin LOW (logic LOW), wait for at least 3us (t2).
   Then Power ON VCC.
4. After VCC become stable, send command AFh for display ON. SEG/COM
   will be ON after 100ms (tAF).

> Signed-off-by: John Keeping <jkeeping@inmusicbrands.com>
> ---
>  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 b777690fd6607..2622172228361 100644
> --- a/drivers/gpu/drm/solomon/ssd130x.c
> +++ b/drivers/gpu/drm/solomon/ssd130x.c
> @@ -363,7 +363,7 @@ static void ssd130x_reset(struct ssd130x_device *ssd130x)
>  
>  	/* Reset the screen */
>  	gpiod_set_value_cansleep(ssd130x->reset, 1);
> -	udelay(4);
> +	usleep_range(100, 1000);
>  	gpiod_set_value_cansleep(ssd130x->reset, 0);
>  	udelay(4);

That's why I think that the udelay(4) are correct here, since that will
make for the delay to be bigger than 3 usecs.

Now, is true that the mentioned 100ms (tAF) after sending an AFh command
might not happen. Since I see there's no delay after sending a display ON
command in ssd130x_encoder_atomic_enable():

  static void ssd130x_encoder_atomic_enable(struct drm_encoder *encoder,
                                            struct drm_atomic_state *state)
  {
..
           ssd130x_write_cmd(ssd130x, 1, SSD13XX_DISPLAY_ON);

Maybe the solution is to add the here?

Also, according to the timers-howto doc [0], the usleep_range() helper
should only be used for small msecs (in the 1~20ms range) and msleep()
used for larger msecs.

[0]: https://www.kernel.org/doc/Documentation/timers/timers-howto.rst

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 2/3] drm/ssd130x: fix ssd132x encoding

[Javier Martinez Canillas]

John Keeping <jkeeping@inmusicbrands.com> writes:

> The ssd132x buffer is encoded one pixel per nibble, with two pixels in
> each byte.  When encoding and 8-bit greyscale input, take the top 4-bits

I think the correct phrase is "encoding an 8-bit" ?

> as the value and ensure the two pixels are distinct and do not overwrite
> each other.
>

Fixes: fdd591e00a9c ("drm/ssd130x: Add support for the SSD132x OLED controller family")

> Signed-off-by: John Keeping <jkeeping@inmusicbrands.com>
> ---
>  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 2622172228361..64f1123080996 100644
> --- a/drivers/gpu/drm/solomon/ssd130x.c
> +++ b/drivers/gpu/drm/solomon/ssd130x.c
> @@ -880,7 +880,7 @@ static int ssd132x_update_rect(struct ssd130x_device *ssd130x,
>  			u8 n1 = buf[i * width + j];
>  			u8 n2 = buf[i * width + j + 1];
>  
> -			data_array[array_idx++] = (n2 << 4) | n1;
> +			data_array[array_idx++] = (n2 & 0xf0) | (n1 >> 4);
>  		}
>  	}

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 3/3] drm/ssd130x: ensure ssd132x pitch is correct

[Javier Martinez Canillas]

John Keeping <jkeeping@inmusicbrands.com> writes:

> The bounding rectangle is adjusted to ensure it aligns to
> SSD132X_SEGMENT_WIDTH, which may adjust the pitch.  Calcuate the pitch

Calculate

> after alighting the left and right edge.
>

aligning

> Signed-off-by: John Keeping <jkeeping@inmusicbrands.com>
> ---
>  drivers/gpu/drm/solomon/ssd130x.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
> index 64f1123080996..38f31c3624062 100644
> --- a/drivers/gpu/drm/solomon/ssd130x.c
> +++ b/drivers/gpu/drm/solomon/ssd130x.c
> @@ -1037,7 +1037,7 @@ static int ssd132x_fb_blit_rect(struct drm_framebuffer *fb,
>  				struct drm_format_conv_state *fmtcnv_state)
>  {
>  	struct ssd130x_device *ssd130x = drm_to_ssd130x(fb->dev);
> -	unsigned int dst_pitch = drm_rect_width(rect);
> +	unsigned int dst_pitch;
>  	struct iosys_map dst;
>  	int ret = 0;
>  
> @@ -1046,6 +1046,8 @@ static int ssd132x_fb_blit_rect(struct drm_framebuffer *fb,
>  	rect->x2 = min_t(unsigned int, round_up(rect->x2, SSD132X_SEGMENT_WIDTH),
>  			 ssd130x->width);
>  
> +	dst_pitch = drm_rect_width(rect);
> +
>  	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
>  	if (ret)
>  		return ret;
> -- 
> 2.47.1
>

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 3/3] drm/ssd130x: ensure ssd132x pitch is correct

[Javier Martinez Canillas]

Javier Martinez Canillas <javierm@redhat.com> writes:

> John Keeping <jkeeping@inmusicbrands.com> writes:
>
>> The bounding rectangle is adjusted to ensure it aligns to
>> SSD132X_SEGMENT_WIDTH, which may adjust the pitch.  Calcuate the pitch
>
> Calculate
>
>> after alighting the left and right edge.
>>
>
> aligning
>

I forgot that this patch also needs a

Fixes: fdd591e00a9c ("drm/ssd130x: Add support for the SSD132x OLED controller family")

>> Signed-off-by: John Keeping <jkeeping@inmusicbrands.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 1/3] drm/ssd130x: Fix reset timing for ssd132x

[John Keeping]

Hi Javier,

Thanks for the review.

On Tue, Jan 14, 2025 at 11:21:25PM +0100, Javier Martinez Canillas wrote:
> John Keeping <jkeeping@inmusicbrands.com> writes:
> Thanks for your patches!
> 
> > The ssd132x family of chips require the result pulse to be at least
> > 100us in length.  Increase the reset time to meet this requirement.
> >
> 
> That's not what the datasheet says AFAIU. It says the following in the
> "8.9 Power ON and OFF sequence" section.
> 
> Power ON sequence:
> 
> 1. Power ON VDD.
> 2. After VDD become stable, set RES# pin LOW (logic LOW) for at least
>    3us (t1) and then HIGH (logic HIGH).
> 3. After set RES# pin LOW (logic LOW), wait for at least 3us (t2).
>    Then Power ON VCC.
> 4. After VCC become stable, send command AFh for display ON. SEG/COM
>    will be ON after 100ms (tAF).

The version of the datasheet I have for SD1322 says:

Power ON sequence:

1. Power ON VCI, VDDIO.
2. After VCI, V DDIO become stable, set wait time at least 1ms (t 0) for
   internal V DD become stable. Then set RES# pin LOW (logic low) for at
   least 100us (t1) (4) and then HIGH (logic high).
3. After set RES# pin LOW (logic low), wait for at least 100us (t2).
   Then Power ON V CC.(1)
4. After VCC become stable, send command AFh for display ON. SEG/COM
   will be ON after 200ms (t AF).

And on the hardware I have 4us seems to be too short.

However, having tested it again today it seems to be fine with the 4us
delay so I suspect this was a misleading change in the midst of other
debugging.

I will drop this patch from v2.

> > Signed-off-by: John Keeping <jkeeping@inmusicbrands.com>
> > ---
> >  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 b777690fd6607..2622172228361 100644
> > --- a/drivers/gpu/drm/solomon/ssd130x.c
> > +++ b/drivers/gpu/drm/solomon/ssd130x.c
> > @@ -363,7 +363,7 @@ static void ssd130x_reset(struct ssd130x_device *ssd130x)
> >  
> >  	/* Reset the screen */
> >  	gpiod_set_value_cansleep(ssd130x->reset, 1);
> > -	udelay(4);
> > +	usleep_range(100, 1000);
> >  	gpiod_set_value_cansleep(ssd130x->reset, 0);
> >  	udelay(4);
> 
> That's why I think that the udelay(4) are correct here, since that will
> make for the delay to be bigger than 3 usecs.
> 
> Now, is true that the mentioned 100ms (tAF) after sending an AFh command
> might not happen. Since I see there's no delay after sending a display ON
> command in ssd130x_encoder_atomic_enable():

I don't think this matters.  It is a delay before the user sees the
image, but that is not relevant to the timing of any commands


Regards,
John

Re: [PATCH 1/3] drm/ssd130x: Fix reset timing for ssd132x

[Javier Martinez Canillas]

John Keeping <jkeeping@inmusicbrands.com> writes:

> Hi Javier,
>
> Thanks for the review.
>
> On Tue, Jan 14, 2025 at 11:21:25PM +0100, Javier Martinez Canillas wrote:
>> John Keeping <jkeeping@inmusicbrands.com> writes:
>> Thanks for your patches!
>> 
>> > The ssd132x family of chips require the result pulse to be at least
>> > 100us in length.  Increase the reset time to meet this requirement.
>> >
>> 
>> That's not what the datasheet says AFAIU. It says the following in the
>> "8.9 Power ON and OFF sequence" section.
>> 
>> Power ON sequence:
>> 
>> 1. Power ON VDD.
>> 2. After VDD become stable, set RES# pin LOW (logic LOW) for at least
>>    3us (t1) and then HIGH (logic HIGH).
>> 3. After set RES# pin LOW (logic LOW), wait for at least 3us (t2).
>>    Then Power ON VCC.
>> 4. After VCC become stable, send command AFh for display ON. SEG/COM
>>    will be ON after 100ms (tAF).

>
> The version of the datasheet I have for SD1322 says:
>
> Power ON sequence:
>
> 1. Power ON VCI, VDDIO.
> 2. After VCI, V DDIO become stable, set wait time at least 1ms (t 0) for
>    internal V DD become stable. Then set RES# pin LOW (logic low) for at
>    least 100us (t1) (4) and then HIGH (logic high).
> 3. After set RES# pin LOW (logic low), wait for at least 100us (t2).
>    Then Power ON V CC.(1)

Oh, that's interesting... I was looking at the datasheet for SSD1327 (the
only SSD132x OLED I have). Maybe we could parameterize the delay values
and be new members to the struct ssd130x_deviceinfo ?

> 4. After VCC become stable, send command AFh for display ON. SEG/COM
>    will be ON after 200ms (t AF).
>
> And on the hardware I have 4us seems to be too short.
>

Yeah, it says 100us in your datasheet while it says 3us in the one I've
for the SSD1327.

> However, having tested it again today it seems to be fine with the 4us
> delay so I suspect this was a misleading change in the midst of other
> debugging.
>

Got it.

> I will drop this patch from v2.
>

Ok.

>> > Signed-off-by: John Keeping <jkeeping@inmusicbrands.com>
>> > ---
>> >  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 b777690fd6607..2622172228361 100644
>> > --- a/drivers/gpu/drm/solomon/ssd130x.c
>> > +++ b/drivers/gpu/drm/solomon/ssd130x.c
>> > @@ -363,7 +363,7 @@ static void ssd130x_reset(struct ssd130x_device *ssd130x)
>> >  
>> >  	/* Reset the screen */
>> >  	gpiod_set_value_cansleep(ssd130x->reset, 1);
>> > -	udelay(4);
>> > +	usleep_range(100, 1000);
>> >  	gpiod_set_value_cansleep(ssd130x->reset, 0);
>> >  	udelay(4);
>> 
>> That's why I think that the udelay(4) are correct here, since that will
>> make for the delay to be bigger than 3 usecs.
>> 
>> Now, is true that the mentioned 100ms (tAF) after sending an AFh command
>> might not happen. Since I see there's no delay after sending a display ON
>> command in ssd130x_encoder_atomic_enable():
>
> I don't think this matters.  It is a delay before the user sees the
> image, but that is not relevant to the timing of any commands
>

Oh right, it's only about the time that the SEG/COM are ON indeed.

>
> Regards,
> John
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat