Re: [PATCH] drm/sun4i: tcon: Unconditionally reset the TCON

[Maxime Ripard]

On Fri, Sep 08, 2017 at 02:49:52PM +0800, Chen-Yu Tsai wrote:
> On Fri, Sep 8, 2017 at 2:42 PM, Maxime Ripard
>  wrote:
> > Hi,
> >
> > On Fri, Sep 08, 2017 at 12:15:45PM +0800, Chen-Yu Tsai wrote:
> >> When binding the TCON, we were checking the reset control status and
> >> asserting reset if it wasn't in reset. The check failed to account for
> >> the reset control API returning error codes if the status callback was
> >> not implemented.
> >>
> >> Since we want the TCON to be reset in all cases, and re-asserting the
> >> reset control does no harm, just assert the reset unconditionally.
> >>
> >> Fixes: 9026e0d122ac ("drm: Add Allwinner A10 Display Engine support")
> >> Signed-off-by: Chen-Yu Tsai 
> >> ---
> >>  drivers/gpu/drm/sun4i/sun4i_tcon.c | 3 +--
> >>  1 file changed, 1 insertion(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c 
> >> b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> >> index d9791292553e..eb32676d5b01 100644
> >> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> >> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> >> @@ -530,8 +530,7 @@ static int sun4i_tcon_bind(struct device *dev, struct 
> >> device *master,
> >>   }
> >>
> >>   /* Make sure our TCON is reset */
> >> - if (!reset_control_status(tcon->lcd_rst))
> >> - reset_control_assert(tcon->lcd_rst);
> >> + reset_control_assert(tcon->lcd_rst);
> >
> > That doesn't really reset it if it was already de-asserted. In that
> > case, the TCON will not be reset, unlike what your commit log says.
> 
> Why not? It is now asserting the reset control, regardless of the
> status.

Oh, right, sorry.

> > Maybe you wanted to use reset_control_reset?
> 
> If you prefer it, sure. When I did this patch we didn't support the
> .reset callback, so this is an explicit assert followed by the existing
> deassert.

Still, if we rework that part, I guess that just using reset
unconditonally would make it more obvious.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/sun4i: tcon: Unconditionally reset the TCON

[Chen-Yu Tsai]

On Fri, Sep 8, 2017 at 2:42 PM, Maxime Ripard
 wrote:
> Hi,
>
> On Fri, Sep 08, 2017 at 12:15:45PM +0800, Chen-Yu Tsai wrote:
>> When binding the TCON, we were checking the reset control status and
>> asserting reset if it wasn't in reset. The check failed to account for
>> the reset control API returning error codes if the status callback was
>> not implemented.
>>
>> Since we want the TCON to be reset in all cases, and re-asserting the
>> reset control does no harm, just assert the reset unconditionally.
>>
>> Fixes: 9026e0d122ac ("drm: Add Allwinner A10 Display Engine support")
>> Signed-off-by: Chen-Yu Tsai 
>> ---
>>  drivers/gpu/drm/sun4i/sun4i_tcon.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c 
>> b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> index d9791292553e..eb32676d5b01 100644
>> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> @@ -530,8 +530,7 @@ static int sun4i_tcon_bind(struct device *dev, struct 
>> device *master,
>>   }
>>
>>   /* Make sure our TCON is reset */
>> - if (!reset_control_status(tcon->lcd_rst))
>> - reset_control_assert(tcon->lcd_rst);
>> + reset_control_assert(tcon->lcd_rst);
>
> That doesn't really reset it if it was already de-asserted. In that
> case, the TCON will not be reset, unlike what your commit log says.

Why not? It is now asserting the reset control, regardless of the status.

> Maybe you wanted to use reset_control_reset?

If you prefer it, sure. When I did this patch we didn't support the
.reset callback, so this is an explicit assert followed by the existing
deassert.

ChenYu
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/sun4i: tcon: Unconditionally reset the TCON

[Maxime Ripard]

Hi,

On Fri, Sep 08, 2017 at 12:15:45PM +0800, Chen-Yu Tsai wrote:
> When binding the TCON, we were checking the reset control status and
> asserting reset if it wasn't in reset. The check failed to account for
> the reset control API returning error codes if the status callback was
> not implemented.
> 
> Since we want the TCON to be reset in all cases, and re-asserting the
> reset control does no harm, just assert the reset unconditionally.
> 
> Fixes: 9026e0d122ac ("drm: Add Allwinner A10 Display Engine support")
> Signed-off-by: Chen-Yu Tsai 
> ---
>  drivers/gpu/drm/sun4i/sun4i_tcon.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c 
> b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> index d9791292553e..eb32676d5b01 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> @@ -530,8 +530,7 @@ static int sun4i_tcon_bind(struct device *dev, struct 
> device *master,
>   }
>  
>   /* Make sure our TCON is reset */
> - if (!reset_control_status(tcon->lcd_rst))
> - reset_control_assert(tcon->lcd_rst);
> + reset_control_assert(tcon->lcd_rst);

That doesn't really reset it if it was already de-asserted. In that
case, the TCON will not be reset, unlike what your commit log says.

Maybe you wanted to use reset_control_reset?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/sun4i: tcon: Unconditionally reset the TCON

[Chen-Yu Tsai]

When binding the TCON, we were checking the reset control status and
asserting reset if it wasn't in reset. The check failed to account for
the reset control API returning error codes if the status callback was
not implemented.

Since we want the TCON to be reset in all cases, and re-asserting the
reset control does no harm, just assert the reset unconditionally.

Fixes: 9026e0d122ac ("drm: Add Allwinner A10 Display Engine support")
Signed-off-by: Chen-Yu Tsai 
---
 drivers/gpu/drm/sun4i/sun4i_tcon.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c 
b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index d9791292553e..eb32676d5b01 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -530,8 +530,7 @@ static int sun4i_tcon_bind(struct device *dev, struct 
device *master,
}
 
/* Make sure our TCON is reset */
-   if (!reset_control_status(tcon->lcd_rst))
-   reset_control_assert(tcon->lcd_rst);
+   reset_control_assert(tcon->lcd_rst);
 
ret = reset_control_deassert(tcon->lcd_rst);
if (ret) {
-- 
2.14.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel