linux-kernel - Re: [PATCH v7 2/3] media: i2c: adv748x: add adv748x driver

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20170718084128.e3eaio6ua2qf346g@valkosipuli.retiisi.org.uk>
Date:   Tue, 18 Jul 2017 11:41:28 +0300
From:   Sakari Ailus <sakari.ailus@....fi>
To:     Kieran Bingham <kbingham@...nel.org>
Cc:     linux-renesas-soc@...r.kernel.org, linux-media@...r.kernel.org,
        linux-kernel@...r.kernel.org, laurent.pinchart@...asonboard.com,
        kieran.bingham@...asonboard.com, niklas.soderlund@...natech.se,
        hans.verkuil@...co.com,
        Kieran Bingham <kieran.bingham+renesas@...asonboard.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Steve Longerbeam <slongerbeam@...il.com>,
        Pavel Machek <pavel@....cz>,
        Rajmohan Mani <rajmohan.mani@...el.com>,
        Ramesh Shanmugasundaram <ramesh.shanmugasundaram@...renesas.com>,
        Todor Tomov <todor.tomov@...aro.org>,
        Ramiro Oliveira <Ramiro.Oliveira@...opsys.com>,
        Robert Jarzmik <robert.jarzmik@...e.fr>,
        Hyungwoo Yang <hyungwoo.yang@...el.com>
Subject: Re: [PATCH v7 2/3] media: i2c: adv748x: add adv748x driver

Hi Kieran,

A few more minor matters that you might want to address on top of Hans's
pull request.

On Thu, Jul 06, 2017 at 12:01:16PM +0100, Kieran Bingham wrote:
...
> +static int adv748x_afe_g_input_status(struct v4l2_subdev *sd, u32 *status)
> +{
> +	struct adv748x_afe *afe = adv748x_sd_to_afe(sd);
> +	struct adv748x_state *state = adv748x_afe_to_state(afe);
> +	int ret;
> +
> +	mutex_lock(&state->mutex);
> +
> +	ret = adv748x_afe_status(afe, status, NULL);
> +
> +	mutex_unlock(&state->mutex);

A newline here would be nice.

> +	return ret;
> +}

...

> +int adv748x_csi2_set_pixelrate(struct v4l2_subdev *sd, s64 rate)
> +{
> +	struct v4l2_ctrl *ctrl;
> +
> +	ctrl = v4l2_ctrl_find(sd->ctrl_handler, V4L2_CID_PIXEL_RATE);

It'd be much nicer to store the control pointer to your device's own struct
and use it. No need to look it up or check whether it was found.

> +	if (!ctrl)
> +		return -EINVAL;
> +
> +	return v4l2_ctrl_s_ctrl_int64(ctrl, rate);
> +}

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@....fi	XMPP: sailus@...iisi.org.uk