Re: [PATCH 1/6] media: mach-pxa: Register the camera sensor fixed-rate clock

Hi Petr,

Thanks a lot for reviewing and testing the series.

On Tue, 2021-01-05 at 17:41 +0100, Petr Cvek wrote:

Dne 04. 01. 21 v 17:57 Ezequiel Garcia napsal(a):

quoted

The pxa-camera capture driver currently registers a v4l2-clk
clock, named "mclk", to represent the mt9m111 sensor clock.

Register a proper fixed-rate clock using the generic clock framework,
which will allow to remove the v4l2-clk clock in the pxa-camera
driver in a follow-up commit.

BTW the mclk output to a sensor is actually a variable rate, divided from lcdclk (which can be changed too). PXA camera driver  is using variable
pcdev->mclk_divisor to generate the mclk from lcdclk. 

Hm, now that I look at this, I see the pxa-camera driver
is requiring a clock:

        pcdev->clk = devm_clk_get(&pdev->dev, NULL);                             
        if (IS_ERR(pcdev->clk))                                                  
                return PTR_ERR(pcdev->clk);   

Where is this clock registered in the non-devicetree case?

The rate change is done in pxa_camera_activate():

https://elixir.bootlin.com/linux/v5.11-rc2/source/drivers/media/platform/pxa_camera.c#L1136

        __raw_writel(pcdev->mclk_divisor | cicr4, pcdev->base + CICR4);

Would it be possible to register a correct clock type with possibility to change the divisor by the standard way?

Right, so you mean the pxa-camera driver is the one providing the clock for the sensors?

In that case, I guess the pxa-camera driver should be the one registering
a CCF clock. Other drivers are doing this, through clk_register for instance.

However, for the sake of this series, which is meant to get rid
of the v4l2-clk API, I would say it's fine to just register a fixed-rate.

This is similar to what v4l2_clk_register was doing, which was registering
a dummy clock.

Having said that, as I mentioned above, I'm wondering if the mach-pxa
boards are really working, given I'm not seeing the clock for pxa-camera.

Maybe the best way forward is to just accept that pxa-camera
is only supported for the device tree platforms, and therefore drop the
support from mach-pxa/ boards.

Thanks,
Ezequiel
 

Petr

quoted

Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Robert Jarzmik <robert.jarzmik@free.fr>
Signed-off-by: Ezequiel Garcia <redacted>
---
 arch/arm/mach-pxa/devices.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/mach-pxa/devices.c b/arch/arm/mach-pxa/devices.c
index 524d6093e0c7..09b8495f3fd9 100644
--- a/arch/arm/mach-pxa/devices.c
+++ b/arch/arm/mach-pxa/devices.c

@@ -4,6 +4,7 @@

 #include <linux/init.h>
 #include <linux/platform_device.h>
 #include <linux/clkdev.h>
+#include <linux/clk-provider.h>
 #include <linux/dma-mapping.h>
 #include <linux/dmaengine.h>
 #include <linux/spi/pxa2xx_spi.h>

@@ -634,6 +635,13 @@ static struct platform_device pxa27x_device_camera = {

 
 void __init pxa_set_camera_info(struct pxacamera_platform_data *info)
 {
+       struct clk *mclk;
+
+       /* Register a fixed-rate clock for camera sensors. */
+       mclk = clk_register_fixed_rate(NULL, "pxa_camera_clk", NULL, 0,
+                                            info->mclk_10khz * 10000);
+       if (!IS_ERR(mclk))
+               clkdev_create(mclk, "mclk", NULL);
        pxa_register_device(&pxa27x_device_camera, info);
 }