Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
Date:   Tue, 24 Oct 2023 09:22:37 +0200
From:   Jacopo Mondi <jacopo.mondi@ideasonboard.com>
To:     =?utf-8?B?QW5kcsOp?= Apitzsch <git@apitzsch.eu>
Cc:     Ricardo Ribalda <ribalda@kernel.org>,
        Sakari Ailus <sakari.ailus@linux.intel.com>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
        ~postmarketos/upstreaming@lists.sr.ht
Subject: Re: [PATCH 2/4] media: i2c: imx214: Move controls init to separate
 function
Message-ID: <kdp6brzqdllnyokwsalkpsbjl6cfnpv62gojfp3xoilsurphke@qqqa4gufb7xf>
References: <20231023-imx214-v1-0-b33f1bbd1fcf@apitzsch.eu>
 <20231023-imx214-v1-2-b33f1bbd1fcf@apitzsch.eu>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20231023-imx214-v1-2-b33f1bbd1fcf@apitzsch.eu>
Precedence: bulk

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
>