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

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

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.

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