diff mbox series

drm/ssd130x: Init display before the SSD130X_DISPLAY_ON command

Message ID 20230125184230.3343206-1-javierm@redhat.com (mailing list archive)
State Accepted
Headers show
Series drm/ssd130x: Init display before the SSD130X_DISPLAY_ON command | expand

Commit Message

Javier Martinez Canillas Jan. 25, 2023, 6:42 p.m. UTC
Commit 622113b9f11f ("drm/ssd130x: Replace simple display helpers with the
atomic helpers") changed the driver to just use the atomic helpers instead
of the simple KMS abstraction layer.

But the commit also made a subtle change on the display power sequence and
initialization order, by moving the ssd130x_power_on() call to the encoder
.atomic_enable handler and the ssd130x_init() call to CRTC .reset handler.

Before this change, both ssd130x_power_on() and ssd130x_init() were called
in the simple display pipeline .enable handler, so the display was already
initialized by the time the SSD130X_DISPLAY_ON command was sent.

For some reasons, it only made the ssd130x SPI driver to fail but the I2C
was still working. That is the reason why the bug was not noticed before.

To revert to the old driver behavior, move the ssd130x_init() call to the
encoder .atomic_enable as well. Besides fixing the panel not being turned
on when using SPI, it also gets rid of the custom CRTC .reset callback.

Fixes: 622113b9f11f ("drm/ssd130x: Replace simple display helpers with the atomic helpers")
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

 drivers/gpu/drm/solomon/ssd130x.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

Comments

Thomas Zimmermann Jan. 25, 2023, 7:56 p.m. UTC | #1
Am 25.01.23 um 19:42 schrieb Javier Martinez Canillas:
> Commit 622113b9f11f ("drm/ssd130x: Replace simple display helpers with the
> atomic helpers") changed the driver to just use the atomic helpers instead
> of the simple KMS abstraction layer.
> 
> But the commit also made a subtle change on the display power sequence and
> initialization order, by moving the ssd130x_power_on() call to the encoder
> .atomic_enable handler and the ssd130x_init() call to CRTC .reset handler.
> 
> Before this change, both ssd130x_power_on() and ssd130x_init() were called
> in the simple display pipeline .enable handler, so the display was already
> initialized by the time the SSD130X_DISPLAY_ON command was sent.
> 
> For some reasons, it only made the ssd130x SPI driver to fail but the I2C
> was still working. That is the reason why the bug was not noticed before.
> 
> To revert to the old driver behavior, move the ssd130x_init() call to the
> encoder .atomic_enable as well. Besides fixing the panel not being turned
> on when using SPI, it also gets rid of the custom CRTC .reset callback.
> 
> Fixes: 622113b9f11f ("drm/ssd130x: Replace simple display helpers with the atomic helpers")
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
> 
>   drivers/gpu/drm/solomon/ssd130x.c | 18 +++++++-----------
>   1 file changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
> index b16330a8b624..8cbf5aa66e19 100644
> --- a/drivers/gpu/drm/solomon/ssd130x.c
> +++ b/drivers/gpu/drm/solomon/ssd130x.c
> @@ -663,18 +663,8 @@ static const struct drm_crtc_helper_funcs ssd130x_crtc_helper_funcs = {
>   	.atomic_check = drm_crtc_helper_atomic_check,
>   };
>   
> -static void ssd130x_crtc_reset(struct drm_crtc *crtc)
> -{
> -	struct drm_device *drm = crtc->dev;
> -	struct ssd130x_device *ssd130x = drm_to_ssd130x(drm);
> -
> -	ssd130x_init(ssd130x);
> -
> -	drm_atomic_helper_crtc_reset(crtc);
> -}
> -
>   static const struct drm_crtc_funcs ssd130x_crtc_funcs = {
> -	.reset = ssd130x_crtc_reset,
> +	.reset = drm_atomic_helper_crtc_reset,
>   	.destroy = drm_crtc_cleanup,
>   	.set_config = drm_atomic_helper_set_config,
>   	.page_flip = drm_atomic_helper_page_flip,
> @@ -693,6 +683,12 @@ static void ssd130x_encoder_helper_atomic_enable(struct drm_encoder *encoder,
>   	if (ret)
>   		return;
>   
> +	ret = ssd130x_init(ssd130x);
> +	if (ret) {
> +		ssd130x_power_off(ssd130x);
> +		return;
> +	}
> +
>   	ssd130x_write_cmd(ssd130x, 1, SSD130X_DISPLAY_ON);
>   
>   	backlight_enable(ssd130x->bl_dev);
Javier Martinez Canillas Jan. 26, 2023, 7:18 p.m. UTC | #2
On 1/25/23 20:56, Thomas Zimmermann wrote:
> 
> 
> Am 25.01.23 um 19:42 schrieb Javier Martinez Canillas:
>> Commit 622113b9f11f ("drm/ssd130x: Replace simple display helpers with the
>> atomic helpers") changed the driver to just use the atomic helpers instead
>> of the simple KMS abstraction layer.
>>
>> But the commit also made a subtle change on the display power sequence and
>> initialization order, by moving the ssd130x_power_on() call to the encoder
>> .atomic_enable handler and the ssd130x_init() call to CRTC .reset handler.
>>
>> Before this change, both ssd130x_power_on() and ssd130x_init() were called
>> in the simple display pipeline .enable handler, so the display was already
>> initialized by the time the SSD130X_DISPLAY_ON command was sent.
>>
>> For some reasons, it only made the ssd130x SPI driver to fail but the I2C
>> was still working. That is the reason why the bug was not noticed before.
>>
>> To revert to the old driver behavior, move the ssd130x_init() call to the
>> encoder .atomic_enable as well. Besides fixing the panel not being turned
>> on when using SPI, it also gets rid of the custom CRTC .reset callback.
>>
>> Fixes: 622113b9f11f ("drm/ssd130x: Replace simple display helpers with the atomic helpers")
>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> 
> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
> 

Applied this to drm-misc (drm-misc-next). Thanks!
diff mbox series

Patch

diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index b16330a8b624..8cbf5aa66e19 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -663,18 +663,8 @@  static const struct drm_crtc_helper_funcs ssd130x_crtc_helper_funcs = {
 	.atomic_check = drm_crtc_helper_atomic_check,
 };
 
-static void ssd130x_crtc_reset(struct drm_crtc *crtc)
-{
-	struct drm_device *drm = crtc->dev;
-	struct ssd130x_device *ssd130x = drm_to_ssd130x(drm);
-
-	ssd130x_init(ssd130x);
-
-	drm_atomic_helper_crtc_reset(crtc);
-}
-
 static const struct drm_crtc_funcs ssd130x_crtc_funcs = {
-	.reset = ssd130x_crtc_reset,
+	.reset = drm_atomic_helper_crtc_reset,
 	.destroy = drm_crtc_cleanup,
 	.set_config = drm_atomic_helper_set_config,
 	.page_flip = drm_atomic_helper_page_flip,
@@ -693,6 +683,12 @@  static void ssd130x_encoder_helper_atomic_enable(struct drm_encoder *encoder,
 	if (ret)
 		return;
 
+	ret = ssd130x_init(ssd130x);
+	if (ret) {
+		ssd130x_power_off(ssd130x);
+		return;
+	}
+
 	ssd130x_write_cmd(ssd130x, 1, SSD130X_DISPLAY_ON);
 
 	backlight_enable(ssd130x->bl_dev);