Re: [PATCH v9 4/4] media: i2c: imx334: update pixel and link frequency

[Sakari Ailus]

Hi Jacopo, Shravan,

On Fri, Jan 13, 2023 at 10:31:33AM +0100, Jacopo Mondi wrote:
> Hi Shravan
> 
> On Fri, Jan 13, 2023 at 06:31:35AM +0530, shravan kumar wrote:
> > From: Shravan Chippa <shravan.chippa@xxxxxxxxxxxxx>
> >
> > Update pixel_rate and link frequency for 1920x1080@30
> > while changing mode.
> >
> > Add dummy ctrl cases for pixel_rate and link frequency
> > to avoid error while changing the modes dynamically
> >
> > Suggested-by: Sakari Ailus <sakari.ailus@xxxxxx>
> > Signed-off-by: Shravan Chippa <shravan.chippa@xxxxxxxxxxxxx>
> > ---
> >  drivers/media/i2c/imx334.c | 17 +++++++++++++----
> >  1 file changed, 13 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> > index 4ab1b9eb9a64..373022fbd6b2 100644
> > --- a/drivers/media/i2c/imx334.c
> > +++ b/drivers/media/i2c/imx334.c
> > @@ -49,7 +49,8 @@
> >  #define IMX334_INCLK_RATE	24000000
> >
> >  /* CSI2 HW configuration */
> > -#define IMX334_LINK_FREQ	891000000
> > +#define IMX334_LINK_FREQ_891M	891000000
> > +#define IMX334_LINK_FREQ_445M	445500000
> 
> Good!
> 
> >  #define IMX334_NUM_DATA_LANES	4
> >
> >  #define IMX334_REG_MIN		0x00
> > @@ -144,7 +145,8 @@ struct imx334 {
> >  };
> >
> >  static const s64 link_freq[] = {
> > -	IMX334_LINK_FREQ,
> > +	IMX334_LINK_FREQ_891M,
> > +	IMX334_LINK_FREQ_445M,
> >  };
> >
> >  /* Sensor mode registers */
> > @@ -468,7 +470,7 @@ static const struct imx334_mode supported_modes[] = {
> >  		.vblank_min = 45,
> >  		.vblank_max = 132840,
> >  		.pclk = 297000000,
> > -		.link_freq_idx = 0,
> > +		.link_freq_idx = 1,
> >  		.reg_list = {
> >  			.num_of_regs = ARRAY_SIZE(mode_1920x1080_regs),
> >  			.regs = mode_1920x1080_regs,
> > @@ -598,6 +600,11 @@ static int imx334_update_controls(struct imx334 *imx334,
> >  	if (ret)
> >  		return ret;
> >
> > +	ret = __v4l2_ctrl_modify_range(imx334->pclk_ctrl, mode->pclk,
> > +				       mode->pclk, 1, mode->pclk);
> > +	if (ret)
> > +		return ret;
> > +
> >  	ret = __v4l2_ctrl_modify_range(imx334->hblank_ctrl, mode->hblank,
> >  				       mode->hblank, 1, mode->hblank);
> >  	if (ret)
> > @@ -698,6 +705,8 @@ static int imx334_set_ctrl(struct v4l2_ctrl *ctrl)
> >  		pm_runtime_put(imx334->dev);
> >
> >  		break;
> > +	case V4L2_CID_PIXEL_RATE:
> > +	case V4L2_CID_LINK_FREQ:
> >  	case V4L2_CID_HBLANK:
> >  		ret = 0;
> >  		break;
> > @@ -1102,7 +1111,7 @@ static int imx334_parse_hw_config(struct imx334 *imx334)
> >  	}
> >
> >  	for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++)
> > -		if (bus_cfg.link_frequencies[i] == IMX334_LINK_FREQ)
> > +		if (bus_cfg.link_frequencies[i] == IMX334_LINK_FREQ_891M)
> 
> Is it legit to specify 445MHz in device tree ? I think so, as it's one
> of the frequencies the sensor can operate at. If that's the case the
> code here will fail, as it only recognize 891MHz ?
> 
> The DTS property serve to specify a sub-set of all the link-frequencies the
> driver can to operate at. If a dtb specifies 445MHz only, it means
> your driver cannot operate at 891MHz full resolution mode. Sakari, is
> my understanding correct here ?
> 
> In theory you could require dtbs to support both frequencies, but
> old dtbs will only have 891MHz specified, should they continue to work but
> with only the full resolution mode available ?

The driver should enable only those frequencies present in DT here, and one
matching frequency is enough for the driver to work.

Also the CCS driver does this but it does quite a bit more as well. The
code below seems good to me, too (see comments).

> 
> A possible way out is to:
> 
> 1) Collect the allowed frequencies at dtbs probe time
> 
>         static const s64 link_freq[] = {
>                 IMX334_LINK_FREQ_891M,
>                 IMX334_LINK_FREQ_445M,

This should go to the control framework menu.

>         };
>         static s64 enabled_link_freq[ARRAY_SIZE(link_freq)] = {};
> 
> 
>         ...
> 
>   	for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) {
>                 for (j = 0; j < ARRAY_SIZE(link_freq); j++) {
>                         if (bus_cfg.link_frequencies[i] == link_freq[j])
>                                 enabled_link_frequencies[j] = link_freq[j];

I'd use a bit mask you can pass to the control framework directly.

>                 }
>         }
> 
>         for (i = 0; i < ARRAY_SIZE(link_freq); i++) {
>                 if (enabled_link_freq[i] != 0)
>                         break;

With a bitmask, you can remove this entirely loop.

>         }
>         if (i == ARRAY_SIZE(link_freq)) {
> 		dev_err(imx334->dev, "no valid link frequencies in DTS");
> 		ret = -EINVAL;
> 		goto done_endpoint_free;
>         }
> 
>         ...
> 
> 2) When enumerating or setting mode, make sure the mode's
>    enabled_link_freq[mode->link_freq_idx] != 0
> 
>    but this might quickly get complex and error prone.
> 
> Sakari, what is the best way to handle situations like this one ?
> The driver is upstream working with a single link_frequency of 891MHz.
> A new link frequency is added, and it is now allowed to have DTS
> specify both frequencies, or just one of them. Should the driver rule
> out all modes that require a link_frequency not specified in DTS ?
> 
> There are several examples of drivers handling multiple link_freqs in
> media/i2c/ but I haven't find out that filters the modes according to
> the specified frequencies (I admit I only quickly looked).

-- 
Kind regards,

Sakari Ailus