| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <kdp6brzqdllnyokwsalkpsbjl6cfnpv62gojfp3xoilsurphke@qqqa4gufb7xf>
Date: Tue, 24 Oct 2023 09:22:37 +0200
From: Jacopo Mondi <jacopo.mondi@...asonboard.com>
To: André Apitzsch <git@...tzsch.eu>
Cc: Ricardo Ribalda <ribalda@...nel.org>,
Sakari Ailus <sakari.ailus@...ux.intel.com>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
~postmarketos/upstreaming@...ts.sr.ht
Subject: Re: [PATCH 2/4] media: i2c: imx214: Move controls init to separate
function
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@...tzsch.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@...asonboard.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
>
Powered by blists - more mailing lists