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.