| Message ID | 20230121033713.3535351-6-shravan.chippa@microchip.com |
|---|---|
| State | New |
| Headers | show |
| Series | media: i2c: imx334: support lower bandwidth mode | expand |
Hi Sakari, > -----Original Message----- > From: Sakari Ailus <sakari.ailus@iki.fi> > Sent: 24 January 2023 04:33 AM > To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com> > Cc: paul.j.murphy@intel.com; daniele.alessandrelli@intel.com; > mchehab@kernel.org; krzysztof.kozlowski+dt@linaro.org; > shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com; > kernel@pengutronix.de; linux-imx@nxp.com; linux-media@vger.kernel.org; > linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-arm- > kernel@lists.infradead.org > Subject: Re: [PATCH RESEND v10 5/5] media: i2c: imx334: update pixel and link > frequency > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > Hi Shravan, > > On Sat, Jan 21, 2023 at 09:07:13AM +0530, shravan kumar wrote: > > From: Shravan Chippa <shravan.chippa@microchip.com> > > > > 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. > > > > Add support to handle multiple link frequencies. > > > > Suggested-by: Sakari Ailus <sakari.ailus@iki.fi> > > Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com> > > --- > > drivers/media/i2c/imx334.c | 41 > > ++++++++++++++++++++++++++++---------- > > 1 file changed, 30 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c > > index 309c706114d2..62b104eaa437 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 > > #define IMX334_NUM_DATA_LANES 4 > > > > #define IMX334_REG_MIN 0x00 > > @@ -139,12 +140,14 @@ struct imx334 { > > u32 vblank; > > const struct imx334_mode *cur_mode; > > struct mutex mutex; > > + unsigned long menu_skip_mask; > > u32 cur_code; > > bool streaming; > > }; > > > > static const s64 link_freq[] = { > > - IMX334_LINK_FREQ, > > + IMX334_LINK_FREQ_891M, > > + IMX334_LINK_FREQ_445M, > > }; > > > > /* Sensor mode registers for 1920x1080@30fps */ @@ -468,7 +471,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 +601,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 +706,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; > > @@ -1047,7 +1057,7 @@ static int imx334_parse_hw_config(struct imx334 > *imx334) > > struct fwnode_handle *ep; > > unsigned long rate; > > int ret; > > - int i; > > + int i, j; > > unsigned int would be nicer. I will change. > > > > > if (!fwnode) > > return -ENXIO; > > @@ -1097,11 +1107,20 @@ static int imx334_parse_hw_config(struct imx334 > *imx334) > > goto done_endpoint_free; > > } > > > > - for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) > > - if (bus_cfg.link_frequencies[i] == IMX334_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]) { > > + set_bit(j, &imx334->menu_skip_mask); > > Is there a guarantee that you'll only be using the modes with the listed > frequencies? I don't see one but I might have missed it. If I understand it correctly, the question here is, the listed freqeunceis and modes are one to one mapped? Then yes. Thanks. shravan > > > + break; > > + } > > + } > > + > > + if (j == ARRAY_SIZE(link_freq)) { > > + ret = dev_err_probe(imx334->dev, -EINVAL, > > + "no supported link freq > > + found\n"); > > goto done_endpoint_free; > > - > > - ret = -EINVAL; > > + } > > + } > > > > done_endpoint_free: > > v4l2_fwnode_endpoint_free(&bus_cfg); > > @@ -1232,10 +1251,10 @@ static int imx334_init_controls(struct imx334 > *imx334) > > imx334->link_freq_ctrl = v4l2_ctrl_new_int_menu(ctrl_hdlr, > > &imx334_ctrl_ops, > > V4L2_CID_LINK_FREQ, > > - ARRAY_SIZE(link_freq) - > > - 1, > > - mode->link_freq_idx, > > + __fls(imx334->menu_skip_mask), > > + > > + __ffs(imx334->menu_skip_mask), > > link_freq); > > + > > if (imx334->link_freq_ctrl) > > imx334->link_freq_ctrl->flags |= > > V4L2_CTRL_FLAG_READ_ONLY; > > > > -- > Kind regards, > > Sakari Ailus
Hi Shravan, On Tue, Jan 24, 2023 at 05:34:02AM +0000, Shravan.Chippa@microchip.com wrote: > Hi Sakari, > > > -----Original Message----- > > From: Sakari Ailus <sakari.ailus@iki.fi> > > Sent: 24 January 2023 04:33 AM > > To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com> > > Cc: paul.j.murphy@intel.com; daniele.alessandrelli@intel.com; > > mchehab@kernel.org; krzysztof.kozlowski+dt@linaro.org; > > shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com; > > kernel@pengutronix.de; linux-imx@nxp.com; linux-media@vger.kernel.org; > > linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-arm- > > kernel@lists.infradead.org > > Subject: Re: [PATCH RESEND v10 5/5] media: i2c: imx334: update pixel and link > > frequency > > > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > > content is safe > > > > Hi Shravan, > > > > On Sat, Jan 21, 2023 at 09:07:13AM +0530, shravan kumar wrote: > > > From: Shravan Chippa <shravan.chippa@microchip.com> > > > > > > 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. > > > > > > Add support to handle multiple link frequencies. > > > > > > Suggested-by: Sakari Ailus <sakari.ailus@iki.fi> > > > Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com> > > > --- > > > drivers/media/i2c/imx334.c | 41 > > > ++++++++++++++++++++++++++++---------- > > > 1 file changed, 30 insertions(+), 11 deletions(-) > > > > > > diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c > > > index 309c706114d2..62b104eaa437 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 > > > #define IMX334_NUM_DATA_LANES 4 > > > > > > #define IMX334_REG_MIN 0x00 > > > @@ -139,12 +140,14 @@ struct imx334 { > > > u32 vblank; > > > const struct imx334_mode *cur_mode; > > > struct mutex mutex; > > > + unsigned long menu_skip_mask; > > > u32 cur_code; > > > bool streaming; > > > }; > > > > > > static const s64 link_freq[] = { > > > - IMX334_LINK_FREQ, > > > + IMX334_LINK_FREQ_891M, > > > + IMX334_LINK_FREQ_445M, > > > }; > > > > > > /* Sensor mode registers for 1920x1080@30fps */ @@ -468,7 +471,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 +601,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 +706,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; > > > @@ -1047,7 +1057,7 @@ static int imx334_parse_hw_config(struct imx334 > > *imx334) > > > struct fwnode_handle *ep; > > > unsigned long rate; > > > int ret; > > > - int i; > > > + int i, j; > > > > unsigned int would be nicer. > I will change. > > > > > > > > if (!fwnode) > > > return -ENXIO; > > > @@ -1097,11 +1107,20 @@ static int imx334_parse_hw_config(struct imx334 > > *imx334) > > > goto done_endpoint_free; > > > } > > > > > > - for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) > > > - if (bus_cfg.link_frequencies[i] == IMX334_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]) { > > > + set_bit(j, &imx334->menu_skip_mask); > > > > Is there a guarantee that you'll only be using the modes with the listed > > frequencies? I don't see one but I might have missed it. > > If I understand it correctly, the question here is, the listed > freqeunceis and modes are one to one mapped? Then yes. I don't see this being checked in imx334_set_pad_format(), for instance. If a frequency isn't in DT, the driver isn't supposed to be using it either. > > Thanks. > shravan > > > > > + break; > > > + } > > > + } > > > + > > > + if (j == ARRAY_SIZE(link_freq)) { > > > + ret = dev_err_probe(imx334->dev, -EINVAL, > > > + "no supported link freq > > > + found\n"); > > > goto done_endpoint_free; > > > - > > > - ret = -EINVAL; > > > + } > > > + } > > > > > > done_endpoint_free: > > > v4l2_fwnode_endpoint_free(&bus_cfg); > > > @@ -1232,10 +1251,10 @@ static int imx334_init_controls(struct imx334 > > *imx334) > > > imx334->link_freq_ctrl = v4l2_ctrl_new_int_menu(ctrl_hdlr, > > > &imx334_ctrl_ops, > > > V4L2_CID_LINK_FREQ, > > > - ARRAY_SIZE(link_freq) - > > > - 1, > > > - mode->link_freq_idx, > > > + __fls(imx334->menu_skip_mask), > > > + > > > + __ffs(imx334->menu_skip_mask), > > > link_freq); > > > + > > > if (imx334->link_freq_ctrl) > > > imx334->link_freq_ctrl->flags |= > > > V4L2_CTRL_FLAG_READ_ONLY;
Hi Sakari, > -----Original Message----- > From: Sakari Ailus <sakari.ailus@iki.fi> > Sent: 25 January 2023 03:12 PM > To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com> > Cc: paul.j.murphy@intel.com; daniele.alessandrelli@intel.com; > mchehab@kernel.org; krzysztof.kozlowski+dt@linaro.org; > shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com; > kernel@pengutronix.de; linux-imx@nxp.com; linux-media@vger.kernel.org; > linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-arm- > kernel@lists.infradead.org > Subject: Re: [PATCH RESEND v10 5/5] media: i2c: imx334: update pixel and link > frequency > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > Hi Shravan, > > On Tue, Jan 24, 2023 at 05:34:02AM +0000, Shravan.Chippa@microchip.com > wrote: > > Hi Sakari, > > > > > -----Original Message----- > > > From: Sakari Ailus <sakari.ailus@iki.fi> > > > Sent: 24 January 2023 04:33 AM > > > To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com> > > > Cc: paul.j.murphy@intel.com; daniele.alessandrelli@intel.com; > > > mchehab@kernel.org; krzysztof.kozlowski+dt@linaro.org; > > > shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com; > > > kernel@pengutronix.de; linux-imx@nxp.com; > > > linux-media@vger.kernel.org; linux-kernel@vger.kernel.org; > > > devicetree@vger.kernel.org; linux-arm- kernel@lists.infradead.org > > > Subject: Re: [PATCH RESEND v10 5/5] media: i2c: imx334: update pixel > > > and link frequency > > > > > > EXTERNAL EMAIL: Do not click links or open attachments unless you > > > know the content is safe > > > > > > Hi Shravan, > > > > > > On Sat, Jan 21, 2023 at 09:07:13AM +0530, shravan kumar wrote: > > > > From: Shravan Chippa <shravan.chippa@microchip.com> > > > > > > > > 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. > > > > > > > > Add support to handle multiple link frequencies. > > > > > > > > Suggested-by: Sakari Ailus <sakari.ailus@iki.fi> > > > > Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com> > > > > --- > > > > drivers/media/i2c/imx334.c | 41 > > > > ++++++++++++++++++++++++++++---------- > > > > 1 file changed, 30 insertions(+), 11 deletions(-) > > > > > > > > diff --git a/drivers/media/i2c/imx334.c > > > > b/drivers/media/i2c/imx334.c index 309c706114d2..62b104eaa437 > > > > 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 > > > > #define IMX334_NUM_DATA_LANES 4 > > > > > > > > #define IMX334_REG_MIN 0x00 > > > > @@ -139,12 +140,14 @@ struct imx334 { > > > > u32 vblank; > > > > const struct imx334_mode *cur_mode; > > > > struct mutex mutex; > > > > + unsigned long menu_skip_mask; > > > > u32 cur_code; > > > > bool streaming; > > > > }; > > > > > > > > static const s64 link_freq[] = { > > > > - IMX334_LINK_FREQ, > > > > + IMX334_LINK_FREQ_891M, > > > > + IMX334_LINK_FREQ_445M, > > > > }; > > > > > > > > /* Sensor mode registers for 1920x1080@30fps */ @@ -468,7 +471,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 > > > > +601,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 +706,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; > > > > @@ -1047,7 +1057,7 @@ static int imx334_parse_hw_config(struct > > > > imx334 > > > *imx334) > > > > struct fwnode_handle *ep; > > > > unsigned long rate; > > > > int ret; > > > > - int i; > > > > + int i, j; > > > > > > unsigned int would be nicer. > > I will change. > > > > > > > > > > > if (!fwnode) > > > > return -ENXIO; > > > > @@ -1097,11 +1107,20 @@ static int imx334_parse_hw_config(struct > > > > imx334 > > > *imx334) > > > > goto done_endpoint_free; > > > > } > > > > > > > > - for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) > > > > - if (bus_cfg.link_frequencies[i] == IMX334_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]) { > > > > + set_bit(j, &imx334->menu_skip_mask); > > > > > > Is there a guarantee that you'll only be using the modes with the > > > listed frequencies? I don't see one but I might have missed it. > > > > If I understand it correctly, the question here is, the listed > > freqeunceis and modes are one to one mapped? Then yes. > > I don't see this being checked in imx334_set_pad_format(), for instance. > > If a frequency isn't in DT, the driver isn't supposed to be using it either. Yes, there is no check. But, if a frequency is not in DT, the driver will not add in menu items. So, the function imx334_set_pad_format() -> imx334_update_controls() fails, if we set the frequencies which are not there in the DT or menu items. Thanks, Shravan > > > > > Thanks. > > shravan > > > > > > > + break; > > > > + } > > > > + } > > > > + > > > > + if (j == ARRAY_SIZE(link_freq)) { > > > > + ret = dev_err_probe(imx334->dev, -EINVAL, > > > > + "no supported link freq > > > > + found\n"); > > > > goto done_endpoint_free; > > > > - > > > > - ret = -EINVAL; > > > > + } > > > > + } > > > > > > > > done_endpoint_free: > > > > v4l2_fwnode_endpoint_free(&bus_cfg); > > > > @@ -1232,10 +1251,10 @@ static int imx334_init_controls(struct > > > > imx334 > > > *imx334) > > > > imx334->link_freq_ctrl = v4l2_ctrl_new_int_menu(ctrl_hdlr, > > > > &imx334_ctrl_ops, > > > > V4L2_CID_LINK_FREQ, > > > > - ARRAY_SIZE(link_freq) - > > > > - 1, > > > > - mode->link_freq_idx, > > > > + > > > > + __fls(imx334->menu_skip_mask), > > > > + > > > > + __ffs(imx334->menu_skip_mask), > > > > link_freq); > > > > + > > > > if (imx334->link_freq_ctrl) > > > > imx334->link_freq_ctrl->flags |= > > > > V4L2_CTRL_FLAG_READ_ONLY; > > -- > Kind regards, > > Sakari Ailus
Hi Sakari, > -----Original Message----- > From: shravan Chippa - I35088 > Sent: 27 January 2023 05:40 AM > To: Sakari Ailus <sakari.ailus@iki.fi> > Cc: paul.j.murphy@intel.com; daniele.alessandrelli@intel.com; > mchehab@kernel.org; krzysztof.kozlowski+dt@linaro.org; > shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com; > kernel@pengutronix.de; linux-imx@nxp.com; linux-media@vger.kernel.org; > linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-arm- > kernel@lists.infradead.org > Subject: RE: [PATCH RESEND v10 5/5] media: i2c: imx334: update pixel and > link frequency > > Hi Sakari, > > > -----Original Message----- > > From: Sakari Ailus <sakari.ailus@iki.fi> > > Sent: 25 January 2023 03:12 PM > > To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com> > > Cc: paul.j.murphy@intel.com; daniele.alessandrelli@intel.com; > > mchehab@kernel.org; krzysztof.kozlowski+dt@linaro.org; > > shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com; > > kernel@pengutronix.de; linux-imx@nxp.com; linux- > media@vger.kernel.org; > > linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-arm- > > kernel@lists.infradead.org > > Subject: Re: [PATCH RESEND v10 5/5] media: i2c: imx334: update pixel > > and link frequency > > > > EXTERNAL EMAIL: Do not click links or open attachments unless you know > > the content is safe > > > > Hi Shravan, > > > > On Tue, Jan 24, 2023 at 05:34:02AM +0000, Shravan.Chippa@microchip.com > > wrote: > > > Hi Sakari, > > > > > > > -----Original Message----- > > > > From: Sakari Ailus <sakari.ailus@iki.fi> > > > > Sent: 24 January 2023 04:33 AM > > > > To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com> > > > > Cc: paul.j.murphy@intel.com; daniele.alessandrelli@intel.com; > > > > mchehab@kernel.org; krzysztof.kozlowski+dt@linaro.org; > > > > shawnguo@kernel.org; s.hauer@pengutronix.de; > festevam@gmail.com; > > > > kernel@pengutronix.de; linux-imx@nxp.com; > > > > linux-media@vger.kernel.org; linux-kernel@vger.kernel.org; > > > > devicetree@vger.kernel.org; linux-arm- kernel@lists.infradead.org > > > > Subject: Re: [PATCH RESEND v10 5/5] media: i2c: imx334: update > > > > pixel and link frequency > > > > > > > > EXTERNAL EMAIL: Do not click links or open attachments unless you > > > > know the content is safe > > > > > > > > Hi Shravan, > > > > > > > > On Sat, Jan 21, 2023 at 09:07:13AM +0530, shravan kumar wrote: > > > > > From: Shravan Chippa <shravan.chippa@microchip.com> > > > > > > > > > > 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. > > > > > > > > > > Add support to handle multiple link frequencies. > > > > > > > > > > Suggested-by: Sakari Ailus <sakari.ailus@iki.fi> > > > > > Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com> > > > > > --- > > > > > drivers/media/i2c/imx334.c | 41 > > > > > ++++++++++++++++++++++++++++---------- > > > > > 1 file changed, 30 insertions(+), 11 deletions(-) > > > > > > > > > > diff --git a/drivers/media/i2c/imx334.c > > > > > b/drivers/media/i2c/imx334.c index 309c706114d2..62b104eaa437 > > > > > 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 > > > > > #define IMX334_NUM_DATA_LANES 4 > > > > > > > > > > #define IMX334_REG_MIN 0x00 > > > > > @@ -139,12 +140,14 @@ struct imx334 { > > > > > u32 vblank; > > > > > const struct imx334_mode *cur_mode; > > > > > struct mutex mutex; > > > > > + unsigned long menu_skip_mask; > > > > > u32 cur_code; > > > > > bool streaming; > > > > > }; > > > > > > > > > > static const s64 link_freq[] = { > > > > > - IMX334_LINK_FREQ, > > > > > + IMX334_LINK_FREQ_891M, > > > > > + IMX334_LINK_FREQ_445M, > > > > > }; > > > > > > > > > > /* Sensor mode registers for 1920x1080@30fps */ @@ -468,7 > > > > > +471,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 > > > > > +601,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 +706,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; > > > > > @@ -1047,7 +1057,7 @@ static int imx334_parse_hw_config(struct > > > > > imx334 > > > > *imx334) > > > > > struct fwnode_handle *ep; > > > > > unsigned long rate; > > > > > int ret; > > > > > - int i; > > > > > + int i, j; > > > > > > > > unsigned int would be nicer. > > > I will change. > > > > > > > > > > > > > > if (!fwnode) > > > > > return -ENXIO; > > > > > @@ -1097,11 +1107,20 @@ static int imx334_parse_hw_config(struct > > > > > imx334 > > > > *imx334) > > > > > goto done_endpoint_free; > > > > > } > > > > > > > > > > - for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) > > > > > - if (bus_cfg.link_frequencies[i] == IMX334_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]) { > > > > > + set_bit(j, > > > > > + &imx334->menu_skip_mask); > > > > > > > > Is there a guarantee that you'll only be using the modes with the > > > > listed frequencies? I don't see one but I might have missed it. > > > > > > If I understand it correctly, the question here is, the listed > > > freqeunceis and modes are one to one mapped? Then yes. > > > > I don't see this being checked in imx334_set_pad_format(), for instance. > > > > If a frequency isn't in DT, the driver isn't supposed to be using it either. > > Yes, there is no check. > > But, if a frequency is not in DT, the driver will not add in menu items. > So, the function imx334_set_pad_format() -> imx334_update_controls() > fails, if we set the frequencies which are not there in the DT or menu items. > Are you ok with the above explanation or any changes you are expecting? Please do let me know if there are any changes needed. I am planning to send the next version. Thanks, Shravan > > Thanks, > Shravan > > > > > > > > > Thanks. > > > shravan > > > > > > > > > + break; > > > > > + } > > > > > + } > > > > > + > > > > > + if (j == ARRAY_SIZE(link_freq)) { > > > > > + ret = dev_err_probe(imx334->dev, -EINVAL, > > > > > + "no supported link > > > > > + freq found\n"); > > > > > goto done_endpoint_free; > > > > > - > > > > > - ret = -EINVAL; > > > > > + } > > > > > + } > > > > > > > > > > done_endpoint_free: > > > > > v4l2_fwnode_endpoint_free(&bus_cfg); > > > > > @@ -1232,10 +1251,10 @@ static int imx334_init_controls(struct > > > > > imx334 > > > > *imx334) > > > > > imx334->link_freq_ctrl = v4l2_ctrl_new_int_menu(ctrl_hdlr, > > > > > &imx334_ctrl_ops, > > > > > V4L2_CID_LINK_FREQ, > > > > > - ARRAY_SIZE(link_freq) - > > > > > - 1, > > > > > - mode->link_freq_idx, > > > > > + > > > > > + __fls(imx334->menu_skip_mask), > > > > > + > > > > > + __ffs(imx334->menu_skip_mask), > > > > > > > > > > link_freq); > > > > > + > > > > > if (imx334->link_freq_ctrl) > > > > > imx334->link_freq_ctrl->flags |= > > > > > V4L2_CTRL_FLAG_READ_ONLY; > > > > -- > > Kind regards, > > > > Sakari Ailus
diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c index 309c706114d2..62b104eaa437 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 #define IMX334_NUM_DATA_LANES 4 #define IMX334_REG_MIN 0x00 @@ -139,12 +140,14 @@ struct imx334 { u32 vblank; const struct imx334_mode *cur_mode; struct mutex mutex; + unsigned long menu_skip_mask; u32 cur_code; bool streaming; }; static const s64 link_freq[] = { - IMX334_LINK_FREQ, + IMX334_LINK_FREQ_891M, + IMX334_LINK_FREQ_445M, }; /* Sensor mode registers for 1920x1080@30fps */ @@ -468,7 +471,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 +601,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 +706,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; @@ -1047,7 +1057,7 @@ static int imx334_parse_hw_config(struct imx334 *imx334) struct fwnode_handle *ep; unsigned long rate; int ret; - int i; + int i, j; if (!fwnode) return -ENXIO; @@ -1097,11 +1107,20 @@ static int imx334_parse_hw_config(struct imx334 *imx334) goto done_endpoint_free; } - for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) - if (bus_cfg.link_frequencies[i] == IMX334_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]) { + set_bit(j, &imx334->menu_skip_mask); + break; + } + } + + if (j == ARRAY_SIZE(link_freq)) { + ret = dev_err_probe(imx334->dev, -EINVAL, + "no supported link freq found\n"); goto done_endpoint_free; - - ret = -EINVAL; + } + } done_endpoint_free: v4l2_fwnode_endpoint_free(&bus_cfg); @@ -1232,10 +1251,10 @@ static int imx334_init_controls(struct imx334 *imx334) imx334->link_freq_ctrl = v4l2_ctrl_new_int_menu(ctrl_hdlr, &imx334_ctrl_ops, V4L2_CID_LINK_FREQ, - ARRAY_SIZE(link_freq) - - 1, - mode->link_freq_idx, + __fls(imx334->menu_skip_mask), + __ffs(imx334->menu_skip_mask), link_freq); + if (imx334->link_freq_ctrl) imx334->link_freq_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;