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

[Petr Cvek <petrcvekcz@gmail.com>]

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:
>>
>> Dne 04. 01. 21 v 17:57 Ezequiel Garcia napsal(a):
>>> 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


>> 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
>  
> 
>> Petr
>>
>>
>>> Cc: Arnd Bergmann <arnd@arndb.de>
>>> Cc: Robert Jarzmik <robert.jarzmik@free.fr>
>>> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
>>> ---
>>>  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);
>>>  }
>>>  
>>>
> 
>