1. Patchew
  2. linux
  3. [PATCH 0/3] drm/ssd130x: some small ssd132x fixes
  4. View patch

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

John Keeping posted 3 patches 11 months, 1 week ago
There is a newer version of this series
[PATCH 1/3] drm/ssd130x: Fix reset timing for ssd132x
Posted by John Keeping 11 months, 1 week ago 
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
Re: [PATCH 1/3] drm/ssd130x: Fix reset timing for ssd132x
Posted by Javier Martinez Canillas 11 months, 1 week ago 
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 1/3] drm/ssd130x: Fix reset timing for ssd132x
Posted by John Keeping 11 months ago 
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
Posted by Javier Martinez Canillas 11 months ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.