| Message ID | 20231023-imx214-v1-2-b33f1bbd1fcf@apitzsch.eu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: i2c: imx214: Extend with sensor size and firmware information | expand |
Hi Andre' On Mon, Oct 23, 2023 at 11:47:51PM +0200, André Apitzsch wrote: > Code refinement, no functional changes. > > Signed-off-by: André Apitzsch <git@apitzsch.eu> > --- > drivers/media/i2c/imx214.c | 111 ++++++++++++++++++++++++++------------------- > 1 file changed, 64 insertions(+), 47 deletions(-) > > diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c > index 9218c149d4c8..554fc4733128 100644 > --- a/drivers/media/i2c/imx214.c > +++ b/drivers/media/i2c/imx214.c > @@ -695,6 +695,68 @@ static const struct v4l2_ctrl_ops imx214_ctrl_ops = { > .s_ctrl = imx214_set_ctrl, > }; > > +static int imx214_ctrls_init(struct imx214 *imx214) > +{ > + static const s64 link_freq[] = { > + IMX214_DEFAULT_LINK_FREQ > + }; > + static const struct v4l2_area unit_size = { > + .width = 1120, > + .height = 1120, > + }; > + struct v4l2_ctrl_handler *ctrl_hdlr; > + int ret; > + > + ctrl_hdlr = &imx214->ctrls; > + ret = v4l2_ctrl_handler_init(&imx214->ctrls, 3); I know it was already like this, but you could take occasion to pre-allocate enough control slots. I count 4 here, plus the 2 parsed from system firware in the next patch. You can change this here and mention it in the commit message or with a separate patch on top. Up to you! > + if (ret) > + return ret; > + > + imx214->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, NULL, > + V4L2_CID_PIXEL_RATE, 0, > + IMX214_DEFAULT_PIXEL_RATE, 1, > + IMX214_DEFAULT_PIXEL_RATE); > + > + imx214->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr, NULL, > + V4L2_CID_LINK_FREQ, > + ARRAY_SIZE(link_freq) - 1, > + 0, link_freq); > + if (imx214->link_freq) > + imx214->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY; > + > + /* > + * WARNING! > + * Values obtained reverse engineering blobs and/or devices. > + * Ranges and functionality might be wrong. > + * > + * Sony, please release some register set documentation for the > + * device. > + * > + * Yours sincerely, Ricardo. > + */ > + imx214->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops, > + V4L2_CID_EXPOSURE, > + IMX214_EXPOSURE_MIN, > + IMX214_EXPOSURE_MAX, > + IMX214_EXPOSURE_STEP, > + IMX214_EXPOSURE_DEFAULT); > + > + imx214->unit_size = v4l2_ctrl_new_std_compound(ctrl_hdlr, > + NULL, > + V4L2_CID_UNIT_CELL_SIZE, > + v4l2_ctrl_ptr_create((void *)&unit_size)); > + > + ret = ctrl_hdlr->error; > + if (ret) { > + v4l2_ctrl_handler_free(ctrl_hdlr); > + return dev_err_probe(imx214->dev, ret, "failed to add controls\n"); dev_err_probe won't help I think, or could ctrl_hdr->error be -EPROBE_DEFER ? Not a big deal though! All minor comments, with these addressed Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > + } > + > + imx214->sd.ctrl_handler = ctrl_hdlr; > + > + return 0; > +}; > + > #define MAX_CMD 4 > static int imx214_write_table(struct imx214 *imx214, > const struct reg_8 table[]) > @@ -918,13 +980,6 @@ static int imx214_probe(struct i2c_client *client) > { > struct device *dev = &client->dev; > struct imx214 *imx214; > - static const s64 link_freq[] = { > - IMX214_DEFAULT_LINK_FREQ, > - }; > - static const struct v4l2_area unit_size = { > - .width = 1120, > - .height = 1120, > - }; > int ret; > > ret = imx214_parse_fwnode(dev); > @@ -979,48 +1034,10 @@ static int imx214_probe(struct i2c_client *client) > pm_runtime_enable(imx214->dev); > pm_runtime_idle(imx214->dev); > > - v4l2_ctrl_handler_init(&imx214->ctrls, 3); > - > - imx214->pixel_rate = v4l2_ctrl_new_std(&imx214->ctrls, NULL, > - V4L2_CID_PIXEL_RATE, 0, > - IMX214_DEFAULT_PIXEL_RATE, 1, > - IMX214_DEFAULT_PIXEL_RATE); > - imx214->link_freq = v4l2_ctrl_new_int_menu(&imx214->ctrls, NULL, > - V4L2_CID_LINK_FREQ, > - ARRAY_SIZE(link_freq) - 1, > - 0, link_freq); > - if (imx214->link_freq) > - imx214->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY; > - > - /* > - * WARNING! > - * Values obtained reverse engineering blobs and/or devices. > - * Ranges and functionality might be wrong. > - * > - * Sony, please release some register set documentation for the > - * device. > - * > - * Yours sincerely, Ricardo. > - */ > - imx214->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops, > - V4L2_CID_EXPOSURE, > - IMX214_EXPOSURE_MIN, > - IMX214_EXPOSURE_MAX, > - IMX214_EXPOSURE_STEP, > - IMX214_EXPOSURE_DEFAULT); > - > - imx214->unit_size = v4l2_ctrl_new_std_compound(&imx214->ctrls, > - NULL, > - V4L2_CID_UNIT_CELL_SIZE, > - v4l2_ctrl_ptr_create((void *)&unit_size)); > - ret = imx214->ctrls.error; > - if (ret) { > - dev_err(&client->dev, "%s control init failed (%d)\n", > - __func__, ret); > + ret = imx214_ctrls_init(imx214); > + if (ret < 0) > goto free_ctrl; > - } > > - imx214->sd.ctrl_handler = &imx214->ctrls; > mutex_init(&imx214->mutex); > imx214->ctrls.lock = &imx214->mutex; > > > -- > 2.42.0 >
Am Dienstag, dem 24.10.2023 um 09:22 +0200 schrieb Jacopo Mondi: > Hi Andre' > > On Mon, Oct 23, 2023 at 11:47:51PM +0200, André Apitzsch wrote: > > Code refinement, no functional changes. > > > > Signed-off-by: André Apitzsch <git@apitzsch.eu> > > --- > > drivers/media/i2c/imx214.c | 111 ++++++++++++++++++++++++++------- > > ------------ > > 1 file changed, 64 insertions(+), 47 deletions(-) > > > > diff --git a/drivers/media/i2c/imx214.c > > b/drivers/media/i2c/imx214.c > > index 9218c149d4c8..554fc4733128 100644 > > --- a/drivers/media/i2c/imx214.c > > +++ b/drivers/media/i2c/imx214.c > > @@ -695,6 +695,68 @@ static const struct v4l2_ctrl_ops > > imx214_ctrl_ops = { > > .s_ctrl = imx214_set_ctrl, > > }; > > > > +static int imx214_ctrls_init(struct imx214 *imx214) > > +{ > > + static const s64 link_freq[] = { > > + IMX214_DEFAULT_LINK_FREQ > > + }; > > + static const struct v4l2_area unit_size = { > > + .width = 1120, > > + .height = 1120, > > + }; > > + struct v4l2_ctrl_handler *ctrl_hdlr; > > + int ret; > > + > > + ctrl_hdlr = &imx214->ctrls; > > + ret = v4l2_ctrl_handler_init(&imx214->ctrls, 3); > > I know it was already like this, but you could take occasion to > pre-allocate enough control slots. I count 4 here, plus the 2 parsed > from system firware in the next patch. > > You can change this here and mention it in the commit message or with > a separate patch on top. Up to you! > I will add it to the next patch ("Read orientation and rotation from system firmware"). As it should be increased there anyway. Hope that's fine. > > > + if (ret) > > + return ret; > > + > > + imx214->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, NULL, > > + > > V4L2_CID_PIXEL_RATE, 0, > > + > > IMX214_DEFAULT_PIXEL_RATE, 1, > > + > > IMX214_DEFAULT_PIXEL_RATE); > > + > > + imx214->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr, > > NULL, > > + > > V4L2_CID_LINK_FREQ, > > + > > ARRAY_SIZE(link_freq) - 1, > > + 0, link_freq); > > + if (imx214->link_freq) > > + imx214->link_freq->flags |= > > V4L2_CTRL_FLAG_READ_ONLY; > > + > > + /* > > + * WARNING! > > + * Values obtained reverse engineering blobs and/or > > devices. > > + * Ranges and functionality might be wrong. > > + * > > + * Sony, please release some register set documentation > > for the > > + * device. > > + * > > + * Yours sincerely, Ricardo. > > + */ > > + imx214->exposure = v4l2_ctrl_new_std(ctrl_hdlr, > > &imx214_ctrl_ops, > > + V4L2_CID_EXPOSURE, > > + IMX214_EXPOSURE_MIN, > > + IMX214_EXPOSURE_MAX, > > + IMX214_EXPOSURE_STEP, > > + > > IMX214_EXPOSURE_DEFAULT); > > + > > + imx214->unit_size = v4l2_ctrl_new_std_compound(ctrl_hdlr, > > + NULL, > > + V4L2_CID_UNIT_CELL_SIZE, > > + v4l2_ctrl_ptr_create((void > > *)&unit_size)); > > + > > + ret = ctrl_hdlr->error; > > + if (ret) { > > + v4l2_ctrl_handler_free(ctrl_hdlr); > > + return dev_err_probe(imx214->dev, ret, "failed to > > add controls\n"); > > dev_err_probe won't help I think, or could ctrl_hdr->error be > -EPROBE_DEFER ? Not a big deal though! dev_err_probe() is used by imx415 (the latest added imx* driver). That's why I used it, too. André > > All minor comments, with these addressed > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > Thanks > j >
Hi Andre On Mon, Oct 23, 2023 at 11:49 PM André Apitzsch <git@apitzsch.eu> wrote: > > Code refinement, no functional changes. > > Signed-off-by: André Apitzsch <git@apitzsch.eu> With Jacopos comments (don't use de_err_probe()) Reviewed-by: Ricardo Ribalda <ribalda@chromium.org> > + ret = imx214_ctrls_init(imx214); > + if (ret < 0) > goto free_ctrl; It seems like we can mutex_destroy a non inited mutex. Could you send a follow-up patch to fix that? Thanks!
Hi Ricardo, Am Freitag, dem 27.10.2023 um 14:25 +0200 schrieb Ricardo Ribalda Delgado: > Hi Andre > On Mon, Oct 23, 2023 at 11:49 PM André Apitzsch <git@apitzsch.eu> > wrote: > > > > Code refinement, no functional changes. > > > > Signed-off-by: André Apitzsch <git@apitzsch.eu> > > With Jacopos comments (don't use de_err_probe()) > Reviewed-by: Ricardo Ribalda <ribalda@chromium.org> > > > + ret = imx214_ctrls_init(imx214); > > + if (ret < 0) > > goto free_ctrl; > > It seems like we can mutex_destroy a non inited mutex. Could you send > a follow-up patch to fix that? > Sorry, I don't get it. Could you explain what you mean. Thanks. > Thanks!
Hi André On Fri, Oct 27, 2023 at 11:23 PM André Apitzsch <git@apitzsch.eu> wrote: > > Hi Ricardo, > > Am Freitag, dem 27.10.2023 um 14:25 +0200 schrieb Ricardo Ribalda > Delgado: > > Hi Andre > > On Mon, Oct 23, 2023 at 11:49 PM André Apitzsch <git@apitzsch.eu> > > wrote: > > > > > > Code refinement, no functional changes. > > > > > > Signed-off-by: André Apitzsch <git@apitzsch.eu> > > > > With Jacopos comments (don't use de_err_probe()) > > Reviewed-by: Ricardo Ribalda <ribalda@chromium.org> > > > > > + ret = imx214_ctrls_init(imx214); > > > + if (ret < 0) > > > goto free_ctrl; > > > > It seems like we can mutex_destroy a non inited mutex. Could you send > > a follow-up patch to fix that? > > > Sorry, I don't get it. Could you explain what you mean. Thanks. > If the controls are initialized incorrectly we will jump to free_ctrl in line 1046, which calls mutex_destroy(&imx214->mutex); But that mutex initialized in line 1050. You did not introduce the bug, but since you have the hardware and are sending the other patches it would be great if you could add a new patch to fix it :) Thanks! > > Thanks! >
diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c index 9218c149d4c8..554fc4733128 100644 --- a/drivers/media/i2c/imx214.c +++ b/drivers/media/i2c/imx214.c @@ -695,6 +695,68 @@ static const struct v4l2_ctrl_ops imx214_ctrl_ops = { .s_ctrl = imx214_set_ctrl, }; +static int imx214_ctrls_init(struct imx214 *imx214) +{ + static const s64 link_freq[] = { + IMX214_DEFAULT_LINK_FREQ + }; + static const struct v4l2_area unit_size = { + .width = 1120, + .height = 1120, + }; + struct v4l2_ctrl_handler *ctrl_hdlr; + int ret; + + ctrl_hdlr = &imx214->ctrls; + ret = v4l2_ctrl_handler_init(&imx214->ctrls, 3); + if (ret) + return ret; + + imx214->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, NULL, + V4L2_CID_PIXEL_RATE, 0, + IMX214_DEFAULT_PIXEL_RATE, 1, + IMX214_DEFAULT_PIXEL_RATE); + + imx214->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr, NULL, + V4L2_CID_LINK_FREQ, + ARRAY_SIZE(link_freq) - 1, + 0, link_freq); + if (imx214->link_freq) + imx214->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY; + + /* + * WARNING! + * Values obtained reverse engineering blobs and/or devices. + * Ranges and functionality might be wrong. + * + * Sony, please release some register set documentation for the + * device. + * + * Yours sincerely, Ricardo. + */ + imx214->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops, + V4L2_CID_EXPOSURE, + IMX214_EXPOSURE_MIN, + IMX214_EXPOSURE_MAX, + IMX214_EXPOSURE_STEP, + IMX214_EXPOSURE_DEFAULT); + + imx214->unit_size = v4l2_ctrl_new_std_compound(ctrl_hdlr, + NULL, + V4L2_CID_UNIT_CELL_SIZE, + v4l2_ctrl_ptr_create((void *)&unit_size)); + + ret = ctrl_hdlr->error; + if (ret) { + v4l2_ctrl_handler_free(ctrl_hdlr); + return dev_err_probe(imx214->dev, ret, "failed to add controls\n"); + } + + imx214->sd.ctrl_handler = ctrl_hdlr; + + return 0; +}; + #define MAX_CMD 4 static int imx214_write_table(struct imx214 *imx214, const struct reg_8 table[]) @@ -918,13 +980,6 @@ static int imx214_probe(struct i2c_client *client) { struct device *dev = &client->dev; struct imx214 *imx214; - static const s64 link_freq[] = { - IMX214_DEFAULT_LINK_FREQ, - }; - static const struct v4l2_area unit_size = { - .width = 1120, - .height = 1120, - }; int ret; ret = imx214_parse_fwnode(dev); @@ -979,48 +1034,10 @@ static int imx214_probe(struct i2c_client *client) pm_runtime_enable(imx214->dev); pm_runtime_idle(imx214->dev); - v4l2_ctrl_handler_init(&imx214->ctrls, 3); - - imx214->pixel_rate = v4l2_ctrl_new_std(&imx214->ctrls, NULL, - V4L2_CID_PIXEL_RATE, 0, - IMX214_DEFAULT_PIXEL_RATE, 1, - IMX214_DEFAULT_PIXEL_RATE); - imx214->link_freq = v4l2_ctrl_new_int_menu(&imx214->ctrls, NULL, - V4L2_CID_LINK_FREQ, - ARRAY_SIZE(link_freq) - 1, - 0, link_freq); - if (imx214->link_freq) - imx214->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY; - - /* - * WARNING! - * Values obtained reverse engineering blobs and/or devices. - * Ranges and functionality might be wrong. - * - * Sony, please release some register set documentation for the - * device. - * - * Yours sincerely, Ricardo. - */ - imx214->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops, - V4L2_CID_EXPOSURE, - IMX214_EXPOSURE_MIN, - IMX214_EXPOSURE_MAX, - IMX214_EXPOSURE_STEP, - IMX214_EXPOSURE_DEFAULT); - - imx214->unit_size = v4l2_ctrl_new_std_compound(&imx214->ctrls, - NULL, - V4L2_CID_UNIT_CELL_SIZE, - v4l2_ctrl_ptr_create((void *)&unit_size)); - ret = imx214->ctrls.error; - if (ret) { - dev_err(&client->dev, "%s control init failed (%d)\n", - __func__, ret); + ret = imx214_ctrls_init(imx214); + if (ret < 0) goto free_ctrl; - } - imx214->sd.ctrl_handler = &imx214->ctrls; mutex_init(&imx214->mutex); imx214->ctrls.lock = &imx214->mutex;
Code refinement, no functional changes. Signed-off-by: André Apitzsch <git@apitzsch.eu> --- drivers/media/i2c/imx214.c | 111 ++++++++++++++++++++++++++------------------- 1 file changed, 64 insertions(+), 47 deletions(-)