Re: [PATCH 06/10] media: ar0521: Configure pixel rate using LINK_FREQ

[Jacopo Mondi <jacopo@jmondi.org>]

Hi Sakari

On Thu, Oct 06, 2022 at 06:42:31PM +0300, Sakari Ailus wrote:
> Hi Jacopo,
>
> On Wed, Oct 05, 2022 at 09:06:09PM +0200, Jacopo Mondi wrote:
> > Now that the V4L2_LINK_FREQUENCY control is available, use it to
> > configure the sensor pixel rate.
> >
> > Adjust the default pixel rate to match the first listed link_frequency.
> >
> > Validated with:
> >
> > $ v4l2-ctl -l -d /dev/v4l-subdev3
> >  link_frequency 0x009f0901 (intmenu): min=0 max=1 default=0 value=0
> >  pixel_rate 0x009f0902 (int64)  : min=168000000 max=414000000 step=1 default=414000000 value=207000000 flags=read-only
> >
> > 26.493166 (30.78 fps) cam0-stream0 seq: 000008 bytesused: 1843200
> > 26.525629 (30.80 fps) cam0-stream0 seq: 000009 bytesused: 1843200
> >
> > $ yavta -w "0x009f0901 1" /dev/v4l-subdev3
> > $ v4l2-ctl -l -d /dev/v4l-subdev3
> >  link_frequency 0x009f0901 (intmenu): min=0 max=1 default=0 value=1
> >  pixel_rate 0x009f0902 (int64)  : min=168000000 max=414000000 step=1 default=414000000 value=414000000 flags=read-only
> >
> > 54.700859 (61.37 fps) cam0-stream0 seq: 000039 bytesused: 1843200
> > 54.717192 (61.23 fps) cam0-stream0 seq: 000040 bytesused: 1843200
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  drivers/media/i2c/ar0521.c | 21 +++++++++++++++++----
> >  1 file changed, 17 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c
> > index c5410b091654..b1580c99f5e3 100644
> > --- a/drivers/media/i2c/ar0521.c
> > +++ b/drivers/media/i2c/ar0521.c
> > @@ -24,7 +24,7 @@
> >  #define AR0521_PLL_MAX		(1280 * 1000 * 1000)
> >
> >  /* Effective pixel sample rate on the pixel array. */
> > -#define AR0521_PIXEL_CLOCK_RATE	 (184 * 1000 * 1000)
> > +#define AR0521_PIXEL_CLOCK_RATE	 (207 * 1000 * 1000)
> >  #define AR0521_PIXEL_CLOCK_MIN	 (168 * 1000 * 1000)
> >  #define AR0521_PIXEL_CLOCK_MAX	 (414 * 1000 * 1000)
> >
> > @@ -91,7 +91,10 @@ static const char * const ar0521_supply_names[] = {
> >  };
> >
> >  static const s64 ar0521_link_frequencies[] = {
> > -	184000000,
> > +	/* 30 FPS at full resolution */
> > +	207000000,
> > +	/* 60 FPS at full resolution */
> > +	414000000,
> >  };
> >
> >  struct ar0521_ctrls {
> > @@ -330,10 +333,21 @@ static u32 calc_pll(struct ar0521_dev *sensor, u32 freq, u16 *pre_ptr, u16 *mult
> >  static void ar0521_calc_mode(struct ar0521_dev *sensor)
> >  {
> >  	unsigned int pixel_clock;
> > +	unsigned int link_freq;
> > +	s64 frequency;
> > +	u32 pixel_rate;
> >  	u16 pre, mult;
> >  	u32 vco;
> >  	int bpp;
> >
> > +	/* Update the PIXEL_RATE value using the desired link_frequency. */
> > +	bpp = ar0521_code_to_bpp(sensor);
> > +	link_freq = sensor->ctrls.link_freq->val;
> > +	frequency = ar0521_link_frequencies[link_freq];
> > +	pixel_rate = frequency * sensor->lane_count * 2 / bpp;
> > +
> > +	__v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixrate, pixel_rate);
> > +
> >  	/*
> >  	 * PLL1 and PLL2 are computed equally even if the application note
> >  	 * suggests a slower PLL1 clock. Maintain pll1 and pll2 divider and
> > @@ -380,8 +394,7 @@ static void ar0521_calc_mode(struct ar0521_dev *sensor)
> >  	 * VCO not to exceed the limits specified by the datasheet and
> >  	 * consequentially reduce the obtained pixel clock.
> >  	 */
> > -	pixel_clock = AR0521_PIXEL_CLOCK_RATE * 2 / sensor->lane_count;
> > -	bpp = ar0521_code_to_bpp(sensor);
> > +	pixel_clock = pixel_rate * 2 / sensor->lane_count;
> >  	sensor->pll.vt_pix = bpp / 2;
> >  	vco = pixel_clock * sensor->pll.vt_pix;
> >
>
> I checked how the driver validates the link frequencies and it seems this
> one doesn't. It's also missing from DT bindings.

As Dave noted, I don't have an handler for LINK_FREQ in s_ctrl.
I will add one

>
> Would you be kind enough to add it?

And add the property to bindings..

>
> Thanks.
>
> --
> Kind regards,
>
> Sakari Ailus