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@xxxxxxxx>
>>> Cc: Robert Jarzmik <robert.jarzmik@xxxxxxx>
>>> Signed-off-by: Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx>
>>> ---
>>> 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);
>>> }
>>>
>>>
>
>
