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

[Kieran Bingham <kieran.bingham@ideasonboard.com>]

Quoting Matthias Fend (2025-12-17 12:21:28)
> Hi Dave,
> 
> thanks for your comment.
> 
> Am 17.12.2025 um 12:54 schrieb Dave Stevenson:
> > Hi Matthias
> > 
> > On Wed, 17 Dec 2025 at 07:41, Matthias Fend <matthias.fend@emfend.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.
> 
> This allows, for example, the registers to be changed when the image 
> sensor is actually used in streaming mode.
> 
> IMHO, this cannot be covered by i2ctransfer, as the device is used 
> exclusively by the driver.

I frequently modify registers while the device is streaming to debug and
investigate.

I use my colleague Tomi's rwmem tool though:

 - https://github.com/tomba/rwmem

But I don't think it does anything specifically special - it's still an
underlying i2c-transfer operation through /dev/i2c-x ?



> 
> > 
> > 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@emfend.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?
> 
> The only other I2C drivers that use pm* in {g,s}_register seem to be 
> imx283 and tc358746. Since both return 0 when the device is inactive, I 

Did you mean something other than imx283 here ?

--
Kieran

> figured there must be a reason for this and implemented it that way as well.
> 
> Thanks
>   ~Matthias
> 
> > 
> >> +
> >> +       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
> >>
> >>
>