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

Dne 06. 01. 21 v 16:53 Ezequiel Garcia napsal(a):

Hi Petr,

Thanks a lot for reviewing and testing the series.

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

quoted

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?

I think this is where the clock is defined 

	PXA27X_CKEN_1RATE("pxa27x-camera.0", NULL, CAMERA, pxa27x_lcd_bus_parents, 0),

https://elixir.bootlin.com/linux/v5.10.2/source/drivers/clk/pxa/clk-pxa27x.c#L180

quoted

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.

Yeah that would make the sense, because the camera controller controls the divider and enable signals.

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.

I guess. Just the 1:1 replacement. 

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.

PXA camera worked without devicetree without problems (I'm not sure if I ever used devicetree). The definition should be in that file above (but I'm not that familiar with the clock framework).

best regards,
Petr

Thanks,
Ezequiel
 

quoted

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);
 }