[PATCH 0/3] drivers: media: imx283 extensions

[PATCH 0/3] drivers: media: imx283 extensions

[Matthias Fend]

Add support for selecting the MIPI clock mode (continuous, non-continuous)
and general register debugging.

Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
---
Matthias Fend (3):
      media: dt-bindings: imx283: add clock-noncontinuous
      media: i2c: imx283: add support for non-continuous MIPI clock mode
      media: i2c: imx283: implement {g,s}_register

 .../devicetree/bindings/media/i2c/sony,imx283.yaml |  2 +
 drivers/media/i2c/imx283.c                         | 56 ++++++++++++++++++++++
 2 files changed, 58 insertions(+)
---
base-commit: f7231cff1f3ff8259bef02dc4999bc132abf29cf
change-id: 20251216-imx283-ext-5968a658dbe6

Best regards,
-- 
Matthias Fend <matthias.fend@emfend.at>

[PATCH 1/3] media: dt-bindings: imx283: add clock-noncontinuous

[Matthias Fend]

Add the optional clock-noncontinuous endpoint property that allows enabling
MIPI CSI-2 non-continuous clock operations.

Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
---
 Documentation/devicetree/bindings/media/i2c/sony,imx283.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx283.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx283.yaml
index e4f49f1435a5c2e6e1507d250662ea6ecbf3c7dc..a91695f5618767ac851e5bc72b347a21da77c52d 100644
--- a/Documentation/devicetree/bindings/media/i2c/sony,imx283.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/sony,imx283.yaml
@@ -59,6 +59,7 @@ properties:
                   - const: 3
                   - const: 4
 
+          clock-noncontinuous: true
           link-frequencies: true
 
         required:
@@ -99,6 +100,7 @@ examples:
                 imx283: endpoint {
                     remote-endpoint = <&cam>;
                     data-lanes = <1 2 3 4>;
+                    clock-noncontinuous;
                     link-frequencies = /bits/ 64 <360000000>;
                 };
             };

-- 
2.34.1

Re: [PATCH 1/3] media: dt-bindings: imx283: add clock-noncontinuous

[Krzysztof Kozlowski]

On 17/12/2025 08:06, Matthias Fend wrote:
> Add the optional clock-noncontinuous endpoint property that allows enabling
> MIPI CSI-2 non-continuous clock operations.
> 
> Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
> ---
>  Documentation/devicetree/bindings/media/i2c/sony,imx283.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx283.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx283.yaml
> index e4f49f1435a5c2e6e1507d250662ea6ecbf3c7dc..a91695f5618767ac851e5bc72b347a21da77c52d 100644
> --- a/Documentation/devicetree/bindings/media/i2c/sony,imx283.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx283.yaml
> @@ -59,6 +59,7 @@ properties:
>                    - const: 3
>                    - const: 4
>  
> +          clock-noncontinuous: true

Drop, it's already there via referenced schema.

>            link-frequencies: true
>  
>          required:
> @@ -99,6 +100,7 @@ examples:
>                  imx283: endpoint {
>                      remote-endpoint = <&cam>;
>                      data-lanes = <1 2 3 4>;
> +                    clock-noncontinuous;

And updating example just for this is rather churn.

>                      link-frequencies = /bits/ 64 <360000000>;
>                  };
>              };
> 


Best regards,
Krzysztof

Re: [PATCH 1/3] media: dt-bindings: imx283: add clock-noncontinuous

[Matthias Fend]

Hi Krzysztof,

thanks for your feedback.

Am 17.12.2025 um 13:03 schrieb Krzysztof Kozlowski:
> On 17/12/2025 08:06, Matthias Fend wrote:
>> Add the optional clock-noncontinuous endpoint property that allows enabling
>> MIPI CSI-2 non-continuous clock operations.
>>
>> Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
>> ---
>>   Documentation/devicetree/bindings/media/i2c/sony,imx283.yaml | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx283.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx283.yaml
>> index e4f49f1435a5c2e6e1507d250662ea6ecbf3c7dc..a91695f5618767ac851e5bc72b347a21da77c52d 100644
>> --- a/Documentation/devicetree/bindings/media/i2c/sony,imx283.yaml
>> +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx283.yaml
>> @@ -59,6 +59,7 @@ properties:
>>                     - const: 3
>>                     - const: 4
>>   
>> +          clock-noncontinuous: true
> 
> Drop, it's already there via referenced schema.
> 
>>             link-frequencies: true
>>   
>>           required:
>> @@ -99,6 +100,7 @@ examples:
>>                   imx283: endpoint {
>>                       remote-endpoint = <&cam>;
>>                       data-lanes = <1 2 3 4>;
>> +                    clock-noncontinuous;
> 
> And updating example just for this is rather churn.

I thought it was worth the change because the example now reflects the 
original (before this series) behavior of the sensor.

Do you think I should remove the entire commit from the series?

Thanks
  ~Matthias

> 
>>                       link-frequencies = /bits/ 64 <360000000>;
>>                   };
>>               };
>>
> 
> 
> Best regards,
> Krzysztof

Re: [PATCH 1/3] media: dt-bindings: imx283: add clock-noncontinuous

[Krzysztof Kozlowski]

On 17/12/2025 13:26, Matthias Fend wrote:
> Hi Krzysztof,
> 
> thanks for your feedback.
> 
> Am 17.12.2025 um 13:03 schrieb Krzysztof Kozlowski:
>> On 17/12/2025 08:06, Matthias Fend wrote:
>>> Add the optional clock-noncontinuous endpoint property that allows enabling
>>> MIPI CSI-2 non-continuous clock operations.
>>>
>>> Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
>>> ---
>>>   Documentation/devicetree/bindings/media/i2c/sony,imx283.yaml | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx283.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx283.yaml
>>> index e4f49f1435a5c2e6e1507d250662ea6ecbf3c7dc..a91695f5618767ac851e5bc72b347a21da77c52d 100644
>>> --- a/Documentation/devicetree/bindings/media/i2c/sony,imx283.yaml
>>> +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx283.yaml
>>> @@ -59,6 +59,7 @@ properties:
>>>                     - const: 3
>>>                     - const: 4
>>>   
>>> +          clock-noncontinuous: true
>>
>> Drop, it's already there via referenced schema.
>>
>>>             link-frequencies: true
>>>   
>>>           required:
>>> @@ -99,6 +100,7 @@ examples:
>>>                   imx283: endpoint {
>>>                       remote-endpoint = <&cam>;
>>>                       data-lanes = <1 2 3 4>;
>>> +                    clock-noncontinuous;
>>
>> And updating example just for this is rather churn.
> 
> I thought it was worth the change because the example now reflects the 
> original (before this series) behavior of the sensor.

I don't understand this, so let's clarify - extend the example only if
this is necessary, IOW existing code is wrong.

Your commit msg does not indicate anything wrong about existing code.

Best regards,
Krzysztof

[PATCH 2/3] media: i2c: imx283: add support for non-continuous MIPI clock mode

[Matthias Fend]

Add support for selecting between continuous and non-continuous MIPI clock
mode.

Previously, the CSI-2 non-continuous clock endpoint flag was ignored and
the sensor was always configured for non-continuous clock mode. For
existing device tree nodes that do not have this property enabled, this
update will therefore change the actual clock mode.

Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
---
 drivers/media/i2c/imx283.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/media/i2c/imx283.c b/drivers/media/i2c/imx283.c
index 8ab63ad8f385f6e2a2d7432feff0af09a5356dc4..7a6ab2941ea985401b21d60163b58e980cf31ddc 100644
--- a/drivers/media/i2c/imx283.c
+++ b/drivers/media/i2c/imx283.c
@@ -149,6 +149,9 @@
 #define IMX283_REG_PLSTMG02		CCI_REG8(0x36aa)
 #define   IMX283_PLSTMG02_VAL		0x00
 
+#define IMX283_REG_MIPI_CLK		CCI_REG8(0x3a43)
+#define   IMX283_MIPI_CLK_NONCONTINUOUS	BIT(0)
+
 #define IMX283_REG_EBD_X_OUT_SIZE	CCI_REG16_LE(0x3a54)
 
 /* Test pattern generator */
@@ -565,6 +568,7 @@ struct imx283 {
 	struct v4l2_ctrl *hblank;
 	struct v4l2_ctrl *vflip;
 
+	bool mipi_clk_noncontinuous;
 	unsigned long link_freq_bitmap;
 
 	u16 hmax;
@@ -988,6 +992,7 @@ static int imx283_set_pad_format(struct v4l2_subdev *sd,
 static int imx283_standby_cancel(struct imx283 *imx283)
 {
 	unsigned int link_freq_idx;
+	u8 mipi_clk;
 	int ret = 0;
 
 	cci_write(imx283->cci, IMX283_REG_STANDBY,
@@ -1007,6 +1012,10 @@ static int imx283_standby_cancel(struct imx283 *imx283)
 	/* Enable PLL */
 	cci_write(imx283->cci, IMX283_REG_STBPL, IMX283_STBPL_NORMAL, &ret);
 
+	/* Configure MIPI clock mode */
+	mipi_clk = imx283->mipi_clk_noncontinuous ? IMX283_MIPI_CLK_NONCONTINUOUS : 0;
+	cci_write(imx283->cci, IMX283_REG_MIPI_CLK, mipi_clk, &ret);
+
 	/* Configure the MIPI link speed */
 	link_freq_idx = __ffs(imx283->link_freq_bitmap);
 	cci_multi_reg_write(imx283->cci, link_freq_reglist[link_freq_idx].regs,
@@ -1426,6 +1435,9 @@ static int imx283_parse_endpoint(struct imx283 *imx283)
 		goto done_endpoint_free;
 	}
 
+	imx283->mipi_clk_noncontinuous =
+		bus_cfg.bus.mipi_csi2.flags & V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK;
+
 	ret = v4l2_link_freq_to_bitmap(imx283->dev, bus_cfg.link_frequencies,
 				       bus_cfg.nr_of_link_frequencies,
 				       link_frequencies, ARRAY_SIZE(link_frequencies),

-- 
2.34.1

Re: [PATCH 2/3] media: i2c: imx283: add support for non-continuous MIPI clock mode

[Kieran Bingham]

Quoting Matthias Fend (2025-12-17 07:06:01)
> Add support for selecting between continuous and non-continuous MIPI clock
> mode.
> 
> Previously, the CSI-2 non-continuous clock endpoint flag was ignored and
> the sensor was always configured for non-continuous clock mode. For
> existing device tree nodes that do not have this property enabled, this
> update will therefore change the actual clock mode.

So this means that not specifying non-continous will now enforce
continuous mode on existing devices ?

Are there any implications to that? I know there are quite a few users
of the sensor on Raspberry Pi for instance.

I think it should be fine though.

> Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
> ---
>  drivers/media/i2c/imx283.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/media/i2c/imx283.c b/drivers/media/i2c/imx283.c
> index 8ab63ad8f385f6e2a2d7432feff0af09a5356dc4..7a6ab2941ea985401b21d60163b58e980cf31ddc 100644
> --- a/drivers/media/i2c/imx283.c
> +++ b/drivers/media/i2c/imx283.c
> @@ -149,6 +149,9 @@
>  #define IMX283_REG_PLSTMG02            CCI_REG8(0x36aa)
>  #define   IMX283_PLSTMG02_VAL          0x00
>  
> +#define IMX283_REG_MIPI_CLK            CCI_REG8(0x3a43)
> +#define   IMX283_MIPI_CLK_NONCONTINUOUS        BIT(0)

I can't find this in my datasheet, so no specific comment on the
register I'm afraid. Did you get this from the vendor or is it an
assumption from other sony drivers?

I assume you can measurably detect that this register impacts the clock
on a CSI scope or such perhaps from the receiver?

So even with all that I think this appears to be correct. I'll test it
more when I can but otherwise:


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> +
>  #define IMX283_REG_EBD_X_OUT_SIZE      CCI_REG16_LE(0x3a54)
>  
>  /* Test pattern generator */
> @@ -565,6 +568,7 @@ struct imx283 {
>         struct v4l2_ctrl *hblank;
>         struct v4l2_ctrl *vflip;
>  
> +       bool mipi_clk_noncontinuous;
>         unsigned long link_freq_bitmap;
>  
>         u16 hmax;
> @@ -988,6 +992,7 @@ static int imx283_set_pad_format(struct v4l2_subdev *sd,
>  static int imx283_standby_cancel(struct imx283 *imx283)
>  {
>         unsigned int link_freq_idx;
> +       u8 mipi_clk;
>         int ret = 0;
>  
>         cci_write(imx283->cci, IMX283_REG_STANDBY,
> @@ -1007,6 +1012,10 @@ static int imx283_standby_cancel(struct imx283 *imx283)
>         /* Enable PLL */
>         cci_write(imx283->cci, IMX283_REG_STBPL, IMX283_STBPL_NORMAL, &ret);
>  
> +       /* Configure MIPI clock mode */
> +       mipi_clk = imx283->mipi_clk_noncontinuous ? IMX283_MIPI_CLK_NONCONTINUOUS : 0;
> +       cci_write(imx283->cci, IMX283_REG_MIPI_CLK, mipi_clk, &ret);
> +
>         /* Configure the MIPI link speed */
>         link_freq_idx = __ffs(imx283->link_freq_bitmap);
>         cci_multi_reg_write(imx283->cci, link_freq_reglist[link_freq_idx].regs,
> @@ -1426,6 +1435,9 @@ static int imx283_parse_endpoint(struct imx283 *imx283)
>                 goto done_endpoint_free;
>         }
>  
> +       imx283->mipi_clk_noncontinuous =
> +               bus_cfg.bus.mipi_csi2.flags & V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK;
> +
>         ret = v4l2_link_freq_to_bitmap(imx283->dev, bus_cfg.link_frequencies,
>                                        bus_cfg.nr_of_link_frequencies,
>                                        link_frequencies, ARRAY_SIZE(link_frequencies),
> 
> -- 
> 2.34.1
>

Re: [PATCH 2/3] media: i2c: imx283: add support for non-continuous MIPI clock mode

[Matthias Fend]

Hi Kieran,

thanks for your feedback.

Am 30.12.2025 um 11:44 schrieb Kieran Bingham:
> Quoting Matthias Fend (2025-12-17 07:06:01)
>> Add support for selecting between continuous and non-continuous MIPI clock
>> mode.
>>
>> Previously, the CSI-2 non-continuous clock endpoint flag was ignored and
>> the sensor was always configured for non-continuous clock mode. For
>> existing device tree nodes that do not have this property enabled, this
>> update will therefore change the actual clock mode.
> 
> So this means that not specifying non-continous will now enforce
> continuous mode on existing devices ?

Yes.

> 
> Are there any implications to that? I know there are quite a few users
> of the sensor on Raspberry Pi for instance.

This shouldn't cause any problems. Several sensors already use different 
clock modes. However, it can't be completely ruled out that some MIPI 
receivers might have issues with it.

At least on an i.MX8MP, both modes work without any problems.

> 
> I think it should be fine though.
> 
>> Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
>> ---
>>   drivers/media/i2c/imx283.c | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/media/i2c/imx283.c b/drivers/media/i2c/imx283.c
>> index 8ab63ad8f385f6e2a2d7432feff0af09a5356dc4..7a6ab2941ea985401b21d60163b58e980cf31ddc 100644
>> --- a/drivers/media/i2c/imx283.c
>> +++ b/drivers/media/i2c/imx283.c
>> @@ -149,6 +149,9 @@
>>   #define IMX283_REG_PLSTMG02            CCI_REG8(0x36aa)
>>   #define   IMX283_PLSTMG02_VAL          0x00
>>   
>> +#define IMX283_REG_MIPI_CLK            CCI_REG8(0x3a43)
>> +#define   IMX283_MIPI_CLK_NONCONTINUOUS        BIT(0)
> 
> I can't find this in my datasheet, so no specific comment on the
> register I'm afraid. Did you get this from the vendor or is it an
> assumption from other sony drivers?

This was a hint from the vendor.

> 
> I assume you can measurably detect that this register impacts the clock
> on a CSI scope or such perhaps from the receiver?

Yes, on an scope you can clearly see whether the clock switches to the 
low-power state during the inactive phases.

Some MIPI receivers (such as the i.MX8MP) also have a status flag that 
indicates the current ULP state of the clock lane.

By increasing the vblank, this register can also be used to determine 
quite reliable whether the clock lane sometimes switches to the ULP state.

> 
> So even with all that I think this appears to be correct. I'll test it
> more when I can but otherwise:

Thanks!

~Matthias

> 
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
>> +
>>   #define IMX283_REG_EBD_X_OUT_SIZE      CCI_REG16_LE(0x3a54)
>>   
>>   /* Test pattern generator */
>> @@ -565,6 +568,7 @@ struct imx283 {
>>          struct v4l2_ctrl *hblank;
>>          struct v4l2_ctrl *vflip;
>>   
>> +       bool mipi_clk_noncontinuous;
>>          unsigned long link_freq_bitmap;
>>   
>>          u16 hmax;
>> @@ -988,6 +992,7 @@ static int imx283_set_pad_format(struct v4l2_subdev *sd,
>>   static int imx283_standby_cancel(struct imx283 *imx283)
>>   {
>>          unsigned int link_freq_idx;
>> +       u8 mipi_clk;
>>          int ret = 0;
>>   
>>          cci_write(imx283->cci, IMX283_REG_STANDBY,
>> @@ -1007,6 +1012,10 @@ static int imx283_standby_cancel(struct imx283 *imx283)
>>          /* Enable PLL */
>>          cci_write(imx283->cci, IMX283_REG_STBPL, IMX283_STBPL_NORMAL, &ret);
>>   
>> +       /* Configure MIPI clock mode */
>> +       mipi_clk = imx283->mipi_clk_noncontinuous ? IMX283_MIPI_CLK_NONCONTINUOUS : 0;
>> +       cci_write(imx283->cci, IMX283_REG_MIPI_CLK, mipi_clk, &ret);
>> +
>>          /* Configure the MIPI link speed */
>>          link_freq_idx = __ffs(imx283->link_freq_bitmap);
>>          cci_multi_reg_write(imx283->cci, link_freq_reglist[link_freq_idx].regs,
>> @@ -1426,6 +1435,9 @@ static int imx283_parse_endpoint(struct imx283 *imx283)
>>                  goto done_endpoint_free;
>>          }
>>   
>> +       imx283->mipi_clk_noncontinuous =
>> +               bus_cfg.bus.mipi_csi2.flags & V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK;
>> +
>>          ret = v4l2_link_freq_to_bitmap(imx283->dev, bus_cfg.link_frequencies,
>>                                         bus_cfg.nr_of_link_frequencies,
>>                                         link_frequencies, ARRAY_SIZE(link_frequencies),
>>
>> -- 
>> 2.34.1
>>

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

[Matthias Fend]

Implement {g,s}_register to support advanced V4L2 debug functionality.

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

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

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

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?

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

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

[Matthias Fend]

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

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

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



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

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

[Matthias Fend]

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

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

[Dave Stevenson]

Hi Matthias

On Wed, 17 Dec 2025 at 14:02, Matthias Fend <matthias.fend@emfend.at> wrote:
>
> * Spam *
> 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.

That's why I said using i2ctransfer -f :-) The force option is "sudo"
when it comes to I2C commands.

I'm in the same boat as Kieran in fairly frequently modifying
registers whilst streaming, and then frequently getting annoyed when
the driver puts the value back again!

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

Dropping it would get my vote.
The functionality is duplicated by "i2ctransfer -f ..." or rwmem, and
then you've got one fewer abstraction. If the sensor is powered down
when you try accessing it, then you'll get the i2c error code back.

Thanks
  Dave

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