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

[Matthias Fend <matthias.fend@emfend.at>]

Hi Kieran,

thanks for your reply.

Am 17.12.2025 um 13:42 schrieb Kieran Bingham:
> 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 ?

Thanks for the hint - I didn't know that tool yet.

With the '-f' option, it's actually possible to use i2ctransfer as well.

> 
> 
> 
>>
>>>
>>> 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 ?

True, the IMX283 is obviously not a good reference in this respect :)

However, if there's agreement that implementing {g,s}_register for this 
driver isn't sensible, I'll just drop this commit.

Thanks
  ~Matthias

> 
> --
> 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
>>>>
>>>>
>>