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.
