[2/4] media: i2c: imx214: Move controls init to separate function
Commit Message
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(-)
Comments
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!
>
@@ -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;