linux-kernel - Re: [PATCH 3/3] media: i2c: imx283: implement {g,s}

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAPY8ntCiOJb9iyFDYS_wxhteoHL7vMFpEF8gVwrf2qeFd-Fssw@mail.gmail.com>
Date: Wed, 17 Dec 2025 11:54:38 +0000
From: Dave Stevenson <dave.stevenson@...pberrypi.com>
To: Matthias Fend <matthias.fend@...end.at>
Cc: Kieran Bingham <kieran.bingham@...asonboard.com>, Umang Jain <uajain@...lia.com>, 
	Mauro Carvalho Chehab <mchehab@...nel.org>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Sakari Ailus <sakari.ailus@...ux.intel.com>, linux-media@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] media: i2c: imx283: implement {g,s}_register

Hi Matthias

On Wed, 17 Dec 2025 at 07:41, Matthias Fend <matthias.fend@...end.at> wrote:
>
> Implement {g,s}_register to support advanced V4L2 debug functionality.

Is there any real benefit to providing access via {g,s}_register
rather than using i2ctransfer -f ? The I2C framework ensures that each
transfer is atomic as long as it is formed into one transaction
request.

IMHO The only place these are really needed is with devices such as
the adv7180 family which have a bank and page addressing scheme, and
the driver is caching the last accessed bank.

> Signed-off-by: Matthias Fend <matthias.fend@...end.at>
> ---
>  drivers/media/i2c/imx283.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
>
> diff --git a/drivers/media/i2c/imx283.c b/drivers/media/i2c/imx283.c
> index 7a6ab2941ea985401b21d60163b58e980cf31ddc..d8ccde0a1587259f39a10984c517cc57d323b6bc 100644
> --- a/drivers/media/i2c/imx283.c
> +++ b/drivers/media/i2c/imx283.c
> @@ -1295,7 +1295,51 @@ static const struct v4l2_subdev_internal_ops imx283_internal_ops = {
>         .init_state = imx283_init_state,
>  };
>
> +#ifdef CONFIG_VIDEO_ADV_DEBUG
> +static int imx283_g_register(struct v4l2_subdev *sd,
> +                            struct v4l2_dbg_register *reg)
> +{
> +       struct imx283 *imx283 = to_imx283(sd);
> +       u64 val;
> +       int ret;
> +
> +       if (!pm_runtime_get_if_active(imx283->dev))
> +               return 0;

Returning no error if the device is powered down feels wrong. How is
the caller meant to differentiate between powered down and the
register actually containing 0?

> +
> +       ret = cci_read(imx283->cci, CCI_REG8(reg->reg), &val, NULL);
> +       reg->val = val;
> +
> +       pm_runtime_put(imx283->dev);
> +
> +       return ret;
> +}
> +
> +static int imx283_s_register(struct v4l2_subdev *sd,
> +                            const struct v4l2_dbg_register *reg)
> +{
> +       struct imx283 *imx283 = to_imx283(sd);
> +       int ret;
> +
> +       if (!pm_runtime_get_if_active(imx283->dev))
> +               return 0;

Ditto here. The caller is told the value was written, but it wasn't.

Thanks.
  Dave

> +
> +       ret = cci_write(imx283->cci, CCI_REG8(reg->reg), reg->val, NULL);
> +
> +       pm_runtime_put(imx283->dev);
> +
> +       return ret;
> +}
> +#endif
> +
> +static const struct v4l2_subdev_core_ops imx283_core_ops = {
> +#ifdef CONFIG_VIDEO_ADV_DEBUG
> +       .g_register = imx283_g_register,
> +       .s_register = imx283_s_register,
> +#endif
> +};
> +
>  static const struct v4l2_subdev_ops imx283_subdev_ops = {
> +       .core = &imx283_core_ops,
>         .video = &imx283_video_ops,
>         .pad = &imx283_pad_ops,
>  };
>
> --
> 2.34.1
>
>