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

[Ezequiel Garcia <ezequiel@collabora.com>]

On Fri, 2021-01-08 at 12:02 +0100, Petr Cvek wrote:
> 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
> 

Ah, nice. That's the part I was missing.
> 
> > > 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. 
> 

Exactly.

> > 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).
> 

That's fine.

I'm quite sure this series should be fine,
since we are just replacing the dummy v4l2-clk with
dummy proper CCF clock.

Thanks!
Ezequiel