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

[Tomi Valkeinen]

On 22/08/2022 17:05, Laurent Pinchart wrote:
> On Mon, Aug 22, 2022 at 04:49:02PM +0300, Tomi Valkeinen wrote:
>> 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.
> 
> But the two fields whose default value isn't 0 are set in the startup()
> function (albeit incorrectly as discussed below), so it should be fine.

That is true. But I'd rather write 0 to VCLKEN in the startup, before 
writing the configuration.

>> 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?
> 
> Because it will ensure that we don't get surprises when we later restart
> the device, such as mentioned below :-)

Well, but that's a bug in the startup code. I don't think the shutdown 
code should do things to make startup work better if the startup does 
something wrong. Nevertheless, while I slightly disagree, I'm fine with 
writing zero there in shutdown.

  Tomi