| Message ID | 20241021-imx214-v2-7-fbd23e99541e@apitzsch.eu |
|---|---|
| State | New |
| Headers | show |
| Series | media: i2c: imx214: Miscellaneous cleanups and improvements | expand |
On Mon, Oct 21, 2024 at 12:14 AM André Apitzsch via B4 Relay <devnull+git.apitzsch.eu@kernel.org> wrote: > > From: André Apitzsch <git@apitzsch.eu> > > The imx214 camera is capable of either two-lane or four-lane operation. > > Currently only the four-lane mode is supported, as proper pixel rates > and link frequences for the two-lane mode are unknown. > > Signed-off-by: André Apitzsch <git@apitzsch.eu> > --- > drivers/media/i2c/imx214.c | 26 +++++++++++++++++++------- > 1 file changed, 19 insertions(+), 7 deletions(-) > > diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c > index 0c83149bcc3e3b833a087d26104eb7dfaafdf904..497baad616ad7374a92a3da2b7c1096b1d72a0c7 100644 > --- a/drivers/media/i2c/imx214.c > +++ b/drivers/media/i2c/imx214.c > @@ -199,7 +199,6 @@ struct imx214 { > > /*From imx214_mode_tbls.h*/ > static const struct cci_reg_sequence mode_4096x2304[] = { > - { IMX214_REG_CSI_LANE_MODE, IMX214_CSI_4_LANE_MODE }, > { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF }, > { IMX214_REG_HDR_RES_REDUCTION, IMX214_HDR_RES_REDU_THROUGH }, > { IMX214_REG_EXPOSURE_RATIO, 1 }, > @@ -272,7 +271,6 @@ static const struct cci_reg_sequence mode_4096x2304[] = { > }; > > static const struct cci_reg_sequence mode_1920x1080[] = { > - { IMX214_REG_CSI_LANE_MODE, IMX214_CSI_4_LANE_MODE }, > { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF }, > { IMX214_REG_HDR_RES_REDUCTION, IMX214_HDR_RES_REDU_THROUGH }, > { IMX214_REG_EXPOSURE_RATIO, 1 }, > @@ -791,6 +789,13 @@ static int imx214_start_streaming(struct imx214 *imx214) > return ret; > } > > + ret = cci_write(imx214->regmap, IMX214_REG_CSI_LANE_MODE, > + IMX214_CSI_4_LANE_MODE, NULL); > + if (ret) { > + dev_err(imx214->dev, "%s failed to configure lanes\n", __func__); > + return ret; > + } > + > ret = cci_multi_reg_write(imx214->regmap, imx214->cur_mode->reg_table, > imx214->cur_mode->num_of_regs, NULL); > if (ret < 0) { > @@ -932,7 +937,7 @@ static int imx214_get_regulators(struct device *dev, struct imx214 *imx214) > imx214->supplies); > } > > -static int imx214_parse_fwnode(struct device *dev) > +static int imx214_parse_fwnode(struct device *dev, struct imx214 *imx214) We don't seem to use imx214 in the function. You probably do not want to add this change. > { > struct fwnode_handle *endpoint; > struct v4l2_fwnode_endpoint bus_cfg = { > @@ -951,6 +956,13 @@ static int imx214_parse_fwnode(struct device *dev) > goto done; > } > > + /* Check the number of MIPI CSI2 data lanes */ > + if (bus_cfg.bus.mipi_csi2.num_data_lanes != 4) { > + dev_err_probe(dev, -EINVAL, > + "only 4 data lanes are currently supported\n"); > + goto done; > + } > + > for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) > if (bus_cfg.link_frequencies[i] == IMX214_DEFAULT_LINK_FREQ) > break; > @@ -975,14 +987,14 @@ static int imx214_probe(struct i2c_client *client) > struct imx214 *imx214; > int ret; > > - ret = imx214_parse_fwnode(dev); > - if (ret) > - return ret; > - > imx214 = devm_kzalloc(dev, sizeof(*imx214), GFP_KERNEL); > if (!imx214) > return -ENOMEM; > > + ret = imx214_parse_fwnode(dev, imx214); > + if (ret) > + return ret; I am not against changing the order... but the commit message does not mention it. > + > imx214->dev = dev; > > imx214->xclk = devm_clk_get(dev, NULL); > > -- > 2.47.0 > >
Hi Ricardo, Am Mittwoch, dem 30.10.2024 um 12:38 +0100 schrieb Ricardo Ribalda Delgado: > On Mon, Oct 21, 2024 at 12:14 AM André Apitzsch via B4 Relay > <devnull+git.apitzsch.eu@kernel.org> wrote: > > > > From: André Apitzsch <git@apitzsch.eu> > > > > The imx214 camera is capable of either two-lane or four-lane > > operation. > > > > Currently only the four-lane mode is supported, as proper pixel > > rates > > and link frequences for the two-lane mode are unknown. > > > > Signed-off-by: André Apitzsch <git@apitzsch.eu> > > --- > > drivers/media/i2c/imx214.c | 26 +++++++++++++++++++------- > > 1 file changed, 19 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/media/i2c/imx214.c > > b/drivers/media/i2c/imx214.c > > index > > 0c83149bcc3e3b833a087d26104eb7dfaafdf904..497baad616ad7374a92a3da2b > > 7c1096b1d72a0c7 100644 > > --- a/drivers/media/i2c/imx214.c > > +++ b/drivers/media/i2c/imx214.c > > @@ -199,7 +199,6 @@ struct imx214 { > > > > /*From imx214_mode_tbls.h*/ > > static const struct cci_reg_sequence mode_4096x2304[] = { > > - { IMX214_REG_CSI_LANE_MODE, IMX214_CSI_4_LANE_MODE }, > > { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF }, > > { IMX214_REG_HDR_RES_REDUCTION, IMX214_HDR_RES_REDU_THROUGH > > }, > > { IMX214_REG_EXPOSURE_RATIO, 1 }, > > @@ -272,7 +271,6 @@ static const struct cci_reg_sequence > > mode_4096x2304[] = { > > }; > > > > static const struct cci_reg_sequence mode_1920x1080[] = { > > - { IMX214_REG_CSI_LANE_MODE, IMX214_CSI_4_LANE_MODE }, > > { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF }, > > { IMX214_REG_HDR_RES_REDUCTION, IMX214_HDR_RES_REDU_THROUGH > > }, > > { IMX214_REG_EXPOSURE_RATIO, 1 }, > > @@ -791,6 +789,13 @@ static int imx214_start_streaming(struct > > imx214 *imx214) > > return ret; > > } > > > > + ret = cci_write(imx214->regmap, IMX214_REG_CSI_LANE_MODE, > > + IMX214_CSI_4_LANE_MODE, NULL); > > + if (ret) { > > + dev_err(imx214->dev, "%s failed to configure > > lanes\n", __func__); > > + return ret; > > + } > > + > > ret = cci_multi_reg_write(imx214->regmap, imx214->cur_mode- > > >reg_table, > > imx214->cur_mode->num_of_regs, > > NULL); > > if (ret < 0) { > > @@ -932,7 +937,7 @@ static int imx214_get_regulators(struct device > > *dev, struct imx214 *imx214) > > imx214->supplies); > > } > > > > -static int imx214_parse_fwnode(struct device *dev) > > +static int imx214_parse_fwnode(struct device *dev, struct imx214 > > *imx214) > We don't seem to use imx214 in the function. You probably do not want > to add this change. > > { > > struct fwnode_handle *endpoint; > > struct v4l2_fwnode_endpoint bus_cfg = { > > @@ -951,6 +956,13 @@ static int imx214_parse_fwnode(struct device > > *dev) > > goto done; > > } > > > > + /* Check the number of MIPI CSI2 data lanes */ > > + if (bus_cfg.bus.mipi_csi2.num_data_lanes != 4) { > > + dev_err_probe(dev, -EINVAL, > > + "only 4 data lanes are currently > > supported\n"); > > + goto done; > > + } > > + > > for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) > > if (bus_cfg.link_frequencies[i] == > > IMX214_DEFAULT_LINK_FREQ) > > break; > > @@ -975,14 +987,14 @@ static int imx214_probe(struct i2c_client > > *client) > > struct imx214 *imx214; > > int ret; > > > > - ret = imx214_parse_fwnode(dev); > > - if (ret) > > - return ret; > > - > > imx214 = devm_kzalloc(dev, sizeof(*imx214), GFP_KERNEL); > > if (!imx214) > > return -ENOMEM; > > > > + ret = imx214_parse_fwnode(dev, imx214); > > + if (ret) > > + return ret; > I am not against changing the order... but the commit message does > not mention it. > I'm not sure how to argue why the order should be changed, now that the imx214 argument is gone. I'll restore the original order. It can be undone, when actually needed. Best regards, André > > + > > imx214->dev = dev; > > > > imx214->xclk = devm_clk_get(dev, NULL); > > > > -- > > 2.47.0 > > > >
diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c index 0c83149bcc3e3b833a087d26104eb7dfaafdf904..497baad616ad7374a92a3da2b7c1096b1d72a0c7 100644 --- a/drivers/media/i2c/imx214.c +++ b/drivers/media/i2c/imx214.c @@ -199,7 +199,6 @@ struct imx214 { /*From imx214_mode_tbls.h*/ static const struct cci_reg_sequence mode_4096x2304[] = { - { IMX214_REG_CSI_LANE_MODE, IMX214_CSI_4_LANE_MODE }, { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF }, { IMX214_REG_HDR_RES_REDUCTION, IMX214_HDR_RES_REDU_THROUGH }, { IMX214_REG_EXPOSURE_RATIO, 1 }, @@ -272,7 +271,6 @@ static const struct cci_reg_sequence mode_4096x2304[] = { }; static const struct cci_reg_sequence mode_1920x1080[] = { - { IMX214_REG_CSI_LANE_MODE, IMX214_CSI_4_LANE_MODE }, { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF }, { IMX214_REG_HDR_RES_REDUCTION, IMX214_HDR_RES_REDU_THROUGH }, { IMX214_REG_EXPOSURE_RATIO, 1 }, @@ -791,6 +789,13 @@ static int imx214_start_streaming(struct imx214 *imx214) return ret; } + ret = cci_write(imx214->regmap, IMX214_REG_CSI_LANE_MODE, + IMX214_CSI_4_LANE_MODE, NULL); + if (ret) { + dev_err(imx214->dev, "%s failed to configure lanes\n", __func__); + return ret; + } + ret = cci_multi_reg_write(imx214->regmap, imx214->cur_mode->reg_table, imx214->cur_mode->num_of_regs, NULL); if (ret < 0) { @@ -932,7 +937,7 @@ static int imx214_get_regulators(struct device *dev, struct imx214 *imx214) imx214->supplies); } -static int imx214_parse_fwnode(struct device *dev) +static int imx214_parse_fwnode(struct device *dev, struct imx214 *imx214) { struct fwnode_handle *endpoint; struct v4l2_fwnode_endpoint bus_cfg = { @@ -951,6 +956,13 @@ static int imx214_parse_fwnode(struct device *dev) goto done; } + /* Check the number of MIPI CSI2 data lanes */ + if (bus_cfg.bus.mipi_csi2.num_data_lanes != 4) { + dev_err_probe(dev, -EINVAL, + "only 4 data lanes are currently supported\n"); + goto done; + } + for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) if (bus_cfg.link_frequencies[i] == IMX214_DEFAULT_LINK_FREQ) break; @@ -975,14 +987,14 @@ static int imx214_probe(struct i2c_client *client) struct imx214 *imx214; int ret; - ret = imx214_parse_fwnode(dev); - if (ret) - return ret; - imx214 = devm_kzalloc(dev, sizeof(*imx214), GFP_KERNEL); if (!imx214) return -ENOMEM; + ret = imx214_parse_fwnode(dev, imx214); + if (ret) + return ret; + imx214->dev = dev; imx214->xclk = devm_clk_get(dev, NULL);