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 ?
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,
};
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];
}
}
for (i = 0; i < ARRAY_SIZE(link_freq); i++) {
if (enabled_link_freq[i] != 0)
break;
}
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).
Thanks
j
}
> goto done_endpoint_free;
>
> ret = -EINVAL;
> --
> 2.34.1
>
