[PATCH v2 2/4] drm: rcar-du: dsi: Improve DSI shutdown

[Tomi Valkeinen]

On 22/08/2022 16:20, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Mon, Aug 22, 2022 at 04:05:10PM +0300, Tomi Valkeinen wrote:
>> From: Tomi Valkeinen <tomi.valkeinen+renesas at ideasonboard.com>
>>
>> Improve the DSI shutdown procedure by clearing various bits that were
>> set while enabling the DSI output. There has been no clear issues caused
>> by these, but it's safer to ensure that the features are disabled at the
>> start of the next DSI enable.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas at ideasonboard.com>
>> ---
>>   drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c
>> index 7f2be490fcf8..6a10a35f1122 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c
>> +++ b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c
>> @@ -441,9 +441,21 @@ static int rcar_mipi_dsi_startup(struct rcar_mipi_dsi *dsi,
>>   
>>   static void rcar_mipi_dsi_shutdown(struct rcar_mipi_dsi *dsi)
>>   {
>> +	/* Disable VCLKEN */
>> +	rcar_mipi_dsi_clr(dsi, VCLKEN, VCLKEN_CKEN);
>> +
>> +	/* Disable DOT clock */
>> +	rcar_mipi_dsi_clr(dsi, VCLKSET, VCLKSET_CKEN);
> 
> I think you can write 0 to those two registers, this will also be safer.
> With this,

VCLKEN has only the single VCLKEN_CKEN bit and the rest of the bits are 
reserved with default value of 0, however VCLKSET has other fields and 
the default value of those fields is not 0.

Why do you think it's safer to set the whole register to 0? Isn't it 
better to just do what we want to do, which makes the purpose clear and, 
I think, is safer as we don't touch bits we don't know about?

> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> I think there's a bug in rcar_mipi_dsi_startup() related to this by the
> way, the function only uses rcar_mipi_dsi_set() to set bits, so if the
> DSI format is modified between two starts, bad things will happen.

Oh, that's bad. rcar_mipi_dsi_set() is not a very good function as it's 
easy to misuse it like that. I'll make a fix for that.

  Tomi