[PATCH v2 1/7] drm: mxsfb: Simplify LCDIF clock handling

[Marek Vasut]

On 4/7/22 09:57, Lucas Stach wrote:
> Am Mittwoch, dem 06.04.2022 um 23:45 +0200 schrieb Marek Vasut:
>> On 4/6/22 21:32, Lucas Stach wrote:
>>> Hi Marek,
>>
>> Hi,
>>
>>> Am Freitag, dem 11.03.2022 um 18:05 +0100 schrieb Marek Vasut:
>>>> The current clock handling in the LCDIF driver is a convoluted mess.
>>>
>>> Here we agree...
>>>
>>>> Implement runtime PM ops which turn the clock ON and OFF and let the
>>>> pm_runtime_get_sync()/pm_runtime_put_sync() calls in .atomic_enable
>>>> and .atomic_disable callbacks turn the clock ON and OFF at the right
>>>> time.
>>>>
>>>> This requires slight reordering in mxsfb_crtc_atomic_enable() and
>>>> mxsfb_crtc_atomic_disable(), since all the register accesses must
>>>> happen only with clock enabled and clock frequency configuration
>>>> must happen with clock disabled.
>>>>
>>> ... on that one I don't. Please don't move the pixel clock into the RPM
>>> calls. We have a very well defined point between atomic enable/disable
>>> where the pixel clock is actually needed. All the other configuration
>>> accesses don't need the pixel clock to be active.
>>
>> On the other hand, "all the other" configuration happens during probe,
>> at which point all the clock are enabled anyway. And then during
>> runtime, the pixel clock of this IP are either enabled or this IP is
>> completely shut down.
>>
>> So, where exactly does this patch make the clock handling any worse than
>> it currently is ?
>>
> There is an even stronger argument: runtime PM does not guarantee that
> the runtime_suspend is actually called after you put your last
> reference. A simple "echo on > /sys/your-device/power/control" will
> prevent the device from ever entering runtime suspend. So if you have a
> clock like the pixel clock that _needs_ to be disabled for
> configuration purposes you _must_ not handle this clock via RPM, but
> via explicit clock handling in the driver.

OK, patch discarded.