[PATCH v2 00/11] media: ov5645: Add support for streams

[PATCH v2 00/11] media: ov5645: Add support for streams

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Hi All,

This patch series aims to add the below features,
- Support subdev active state
- Support for streams
- Support for virtual channel
- Code cleanup

Note, these patches are dependent on below:
1] https://patchwork.kernel.org/project/linux-media/patch/20240416193319.778192-27-sakari.ailus@linux.intel.com/
2] https://patchwork.kernel.org/project/linux-media/patch/20240416193319.778192-26-sakari.ailus@linux.intel.com/

RFC->v2
- Dropped setting of VC using routes
- Defaulted the native format to MEDIA_BUS_FMT_SBGGR8_1X8
- Fixed ov5645_enum_frame_size and ov5645_enum_mbus_code
  for internal image pad

RFC patch,
Link: https://lore.kernel.org/all/20240904210719.52466-1-prabhakar.mahadev-lad.rj@bp.renesas.com/

Test logs:
====================================

root@smarc-rzg2l:~# media-ctl -p
.....
- entity 4: ov5645 0-003c (2 pads, 1 link, 1 route)
            type V4L2 subdev subtype Sensor flags 0
            device node name /dev/v4l-subdev1
        routes:
                1/0 -> 0/0 [ACTIVE]
        pad0: SOURCE
                [stream:0 fmt:UYVY8_1X16/1920x1080 field:none colorspace:srgb
                 crop:(0,0)/1920x1080]
                -> "csi-10830400.csi2":0 [ENABLED,IMMUTABLE]
        pad1: SINK,0x8
                [stream:0 fmt:SBGGR8_1X8/2592x1944 field:none colorspace:srgb
                 crop:(0,0)/1920x1080]
.....

root@smarc-rzg2l:~# v4l2-ctl --device /dev/v4l-subdev1 --list-subdev-mbus-codes pad=0
ioctl: VIDIOC_SUBDEV_ENUM_MBUS_CODE (pad=0,stream=0)
    0x200f: MEDIA_BUS_FMT_UYVY8_1X16
root@smarc-rzg2l:~# v4l2-ctl --device /dev/v4l-subdev1 --list-subdev-mbus-codes pad=1
ioctl: VIDIOC_SUBDEV_ENUM_MBUS_CODE (pad=1,stream=0)
    0x3001: MEDIA_BUS_FMT_SBGGR8_1X8
root@smarc-rzg2l:~# v4l2-ctl --device /dev/v4l-subdev1 --list-subdev-framesizes pad=1,code=0x3001
ioctl: VIDIOC_SUBDEV_ENUM_FRAME_SIZE (pad=1,stream=0)
    Size Range: 2592x1944 - 2592x1944
root@smarc-rzg2l:~# v4l2-ctl --device /dev/v4l-subdev1 --list-subdev-framesizes pad=0,code=0x200f
ioctl: VIDIOC_SUBDEV_ENUM_FRAME_SIZE (pad=0,stream=0)
    Size Range: 1280x960 - 1280x960
    Size Range: 1920x1080 - 1920x1080
    Size Range: 2592x1944 - 2592x1944
root@smarc-rzg2l:~#

v4l2-compliance log:
-------------------
root@smarc-rzg2l:~# v4l2-compliance -u /dev/v4l-subdev1
v4l2-compliance 1.28.1-5233, 64 bits, 64-bit time_t
v4l2-compliance SHA: fc15e229d9d3 2024-07-23 19:22:15

Compliance test for device /dev/v[ 6347.789338] ov5645 0-003c: ================= START STATUS =================
4l-subdev1:

Driver In[ 6347.798197] ov5645 0-003c: ================== END STATUS ==================
fo:
    Driver version  : 6.11.0
    Capabilities   : 0x00000002
        Streams Support
    Client Capabilities: 0x0000000000000003
streams interval-uses-which
Required ioctls:
    test VIDIOC_SUDBEV_QUERYCAP: OK
    test invalid ioctls: OK

Allow for multiple opens:
    test second /dev/v4l-subdev1 open: OK
    test VIDIOC_SUBDEV_QUERYCAP: OK
    test for unlimited opens: OK

Debug ioctls:
    test VIDIOC_LOG_STATUS: OK (Not Supported)

Input ioctls:
    test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
    test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
    test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
    test VIDIOC_ENUMAUDIO: OK (Not Supported)
    test VIDIOC_G/S/ENUMINPUT: OK (Not Supported)
    test VIDIOC_G/S_AUDIO: OK (Not Supported)
    Inputs: 0 Audio Inputs: 0 Tuners: 0

Output ioctls:
    test VIDIOC_G/S_MODULATOR: OK (Not Supported)
    test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
    test VIDIOC_ENUMAUDOUT: OK (Not Supported)
    test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
    test VIDIOC_G/S_AUDOUT: OK (Not Supported)
    Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
    test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
    test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
    test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
    test VIDIOC_G/S_EDID: OK (Not Supported)

Sub-Device routing ioctls:
    test Try VIDIOC_SUBDEV_G_ROUTING/VIDIOC_SUBDEV_S_ROUTING: OK
    test Active VIDIOC_SUBDEV_G_ROUTING/VIDIOC_SUBDEV_S_ROUTING: OK

Control ioctls:
    test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
    test VIDIOC_QUERYCTRL: OK
    test VIDIOC_G/S_CTRL: OK
    test VIDIOC_G/S/TRY_EXT_CTRLS: OK
    test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK
    test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
    Standard Controls: 12 Private Controls: 0

Format ioctls:
    test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK (Not Supported)
    test VIDIOC_G/S_PARM: OK (Not Supported)
    test VIDIOC_G_FBUF: OK (Not Supported)
    test VIDIOC_G_FMT: OK (Not Supported)
    test VIDIOC_TRY_FMT: OK (Not Supported)
    test VIDIOC_S_FMT: OK (Not Supported)
    test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
    test Cropping: OK (Not Supported)
    test Composing: OK (Not Supported)
    test Scaling: OK (Not Supported)

Codec ioctls:
    test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
    test VIDIOC_G_ENC_INDEX: OK (Not Supported)
    test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)

Buffer ioctls:
    test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK (Not Supported)
    test CREATE_BUFS maximum buffers: OK
    test VIDIOC_REMOVE_BUFS: OK
    test VIDIOC_EXPBUF: OK (Not Supported)
    test Requests: OK (Not Supported)

Total for device /dev/v4l-subdev1: 47, Succeeded: 47, Failed: 0, Warnings: 0

Lad Prabhakar (11):
  media: i2c: ov5645: Add V4L2_SUBDEV_FL_HAS_EVENTS and subscribe hooks
  media: i2c: ov5645: Use local `dev` pointer for subdev device
    assignment
  media: i2c: ov5645: Enable runtime PM after
    v4l2_async_register_subdev()
  media: i2c: ov5645: Use dev_err_probe instead of dev_err
  media: i2c: ov5645: Use v4l2_async_register_subdev_sensor()
  media: i2c: ov5645: Drop `power_lock` mutex
  media: i2c: ov5645: Use subdev active state
  media: i2c: ov5645: Switch to {enable,disable}_streams
  media: i2c: ov5645: Add internal image sink pad
  media: i2c: ov5645: Report internal routes to userspace
  media: i2c: ov5645: Report streams using frame descriptors

 drivers/media/i2c/ov5645.c | 433 ++++++++++++++++++++-----------------
 1 file changed, 240 insertions(+), 193 deletions(-)

-- 
2.34.1

[PATCH v2 01/11] media: i2c: ov5645: Add V4L2_SUBDEV_FL_HAS_EVENTS and subscribe hooks

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

The OV5645 sensor exposes controls, so the V4L2_SUBDEV_FL_HAS_EVENTS flag
should be set and implement subscribe_event and unsubscribe_event hooks.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/media/i2c/ov5645.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index 019979f553b1..6eedd0310b02 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -29,6 +29,7 @@
 #include <linux/slab.h>
 #include <linux/types.h>
 #include <media/v4l2-ctrls.h>
+#include <media/v4l2-event.h>
 #include <media/v4l2-fwnode.h>
 #include <media/v4l2-subdev.h>
 
@@ -1042,7 +1043,13 @@ static const struct v4l2_subdev_pad_ops ov5645_subdev_pad_ops = {
 	.get_selection = ov5645_get_selection,
 };
 
+static const struct v4l2_subdev_core_ops ov5645_core_ops = {
+	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
+	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
+};
+
 static const struct v4l2_subdev_ops ov5645_subdev_ops = {
+	.core = &ov5645_core_ops,
 	.video = &ov5645_video_ops,
 	.pad = &ov5645_subdev_pad_ops,
 };
@@ -1178,7 +1185,7 @@ static int ov5645_probe(struct i2c_client *client)
 
 	v4l2_i2c_subdev_init(&ov5645->sd, client, &ov5645_subdev_ops);
 	ov5645->sd.internal_ops = &ov5645_internal_ops;
-	ov5645->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+	ov5645->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
 	ov5645->pad.flags = MEDIA_PAD_FL_SOURCE;
 	ov5645->sd.dev = &client->dev;
 	ov5645->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
-- 
2.34.1

Re: [PATCH v2 01/11] media: i2c: ov5645: Add V4L2_SUBDEV_FL_HAS_EVENTS and subscribe hooks

[Laurent Pinchart]

Hi Prabhakar,

Thank you for the patch.

On Tue, Sep 10, 2024 at 06:06:00PM +0100, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> The OV5645 sensor exposes controls, so the V4L2_SUBDEV_FL_HAS_EVENTS flag
> should be set and implement subscribe_event and unsubscribe_event hooks.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> ---
>  drivers/media/i2c/ov5645.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> index 019979f553b1..6eedd0310b02 100644
> --- a/drivers/media/i2c/ov5645.c
> +++ b/drivers/media/i2c/ov5645.c
> @@ -29,6 +29,7 @@
>  #include <linux/slab.h>
>  #include <linux/types.h>
>  #include <media/v4l2-ctrls.h>
> +#include <media/v4l2-event.h>
>  #include <media/v4l2-fwnode.h>
>  #include <media/v4l2-subdev.h>
>  
> @@ -1042,7 +1043,13 @@ static const struct v4l2_subdev_pad_ops ov5645_subdev_pad_ops = {
>  	.get_selection = ov5645_get_selection,
>  };
>  
> +static const struct v4l2_subdev_core_ops ov5645_core_ops = {
> +	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
> +	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
> +};
> +
>  static const struct v4l2_subdev_ops ov5645_subdev_ops = {
> +	.core = &ov5645_core_ops,
>  	.video = &ov5645_video_ops,
>  	.pad = &ov5645_subdev_pad_ops,
>  };
> @@ -1178,7 +1185,7 @@ static int ov5645_probe(struct i2c_client *client)
>  
>  	v4l2_i2c_subdev_init(&ov5645->sd, client, &ov5645_subdev_ops);
>  	ov5645->sd.internal_ops = &ov5645_internal_ops;
> -	ov5645->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +	ov5645->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
>  	ov5645->pad.flags = MEDIA_PAD_FL_SOURCE;
>  	ov5645->sd.dev = &client->dev;
>  	ov5645->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> -- 
> 2.34.1
> 

-- 
Regards,

Laurent Pinchart

[PATCH v2 02/11] media: i2c: ov5645: Use local `dev` pointer for subdev device assignment

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

While assigning the subdev device pointer, use the local `dev` pointer
which is already extracted from the `i2c_client` pointer.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/media/i2c/ov5645.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index 6eedd0310b02..ab3a419df2df 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -1187,7 +1187,7 @@ static int ov5645_probe(struct i2c_client *client)
 	ov5645->sd.internal_ops = &ov5645_internal_ops;
 	ov5645->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
 	ov5645->pad.flags = MEDIA_PAD_FL_SOURCE;
-	ov5645->sd.dev = &client->dev;
+	ov5645->sd.dev = dev;
 	ov5645->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
 
 	ret = media_entity_pads_init(&ov5645->sd.entity, 1, &ov5645->pad);
-- 
2.34.1

Re: [PATCH v2 02/11] media: i2c: ov5645: Use local `dev` pointer for subdev device assignment

[Laurent Pinchart]

On Tue, Sep 10, 2024 at 06:06:01PM +0100, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> While assigning the subdev device pointer, use the local `dev` pointer
> which is already extracted from the `i2c_client` pointer.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> ---
>  drivers/media/i2c/ov5645.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> index 6eedd0310b02..ab3a419df2df 100644
> --- a/drivers/media/i2c/ov5645.c
> +++ b/drivers/media/i2c/ov5645.c
> @@ -1187,7 +1187,7 @@ static int ov5645_probe(struct i2c_client *client)
>  	ov5645->sd.internal_ops = &ov5645_internal_ops;
>  	ov5645->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
>  	ov5645->pad.flags = MEDIA_PAD_FL_SOURCE;
> -	ov5645->sd.dev = &client->dev;
> +	ov5645->sd.dev = dev;
>  	ov5645->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
>  
>  	ret = media_entity_pads_init(&ov5645->sd.entity, 1, &ov5645->pad);

-- 
Regards,

Laurent Pinchart

[PATCH v2 03/11] media: i2c: ov5645: Enable runtime PM after v4l2_async_register_subdev()

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

To simplify the probe error path, enable runtime PM after the
v4l2_async_register_subdev() call.

This change ensures that runtime PM is only enabled once the subdevice
registration is successful, avoiding unnecessary cleanup in the error
path.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/media/i2c/ov5645.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index ab3a419df2df..78b86438c798 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -1239,18 +1239,17 @@ static int ov5645_probe(struct i2c_client *client)
 		goto power_down;
 	}
 
-	pm_runtime_set_active(dev);
-	pm_runtime_get_noresume(dev);
-	pm_runtime_enable(dev);
-
 	ov5645_init_state(&ov5645->sd, NULL);
 
 	ret = v4l2_async_register_subdev(&ov5645->sd);
 	if (ret < 0) {
 		dev_err(dev, "could not register v4l2 device\n");
-		goto err_pm_runtime;
+		goto power_down;
 	}
 
+	pm_runtime_set_active(dev);
+	pm_runtime_get_noresume(dev);
+	pm_runtime_enable(dev);
 	pm_runtime_set_autosuspend_delay(dev, 1000);
 	pm_runtime_use_autosuspend(dev);
 	pm_runtime_mark_last_busy(dev);
@@ -1258,9 +1257,6 @@ static int ov5645_probe(struct i2c_client *client)
 
 	return 0;
 
-err_pm_runtime:
-	pm_runtime_disable(dev);
-	pm_runtime_put_noidle(dev);
 power_down:
 	ov5645_set_power_off(dev);
 free_entity:
-- 
2.34.1

Re: [PATCH v2 03/11] media: i2c: ov5645: Enable runtime PM after v4l2_async_register_subdev()

[Laurent Pinchart]

Hi Prabhakar,

Thank you for the patch.

On Tue, Sep 10, 2024 at 06:06:02PM +0100, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> To simplify the probe error path, enable runtime PM after the
> v4l2_async_register_subdev() call.
> 
> This change ensures that runtime PM is only enabled once the subdevice
> registration is successful, avoiding unnecessary cleanup in the error
> path.

The subdev could start being used as soon as it gets registered, so
runtime PM initialization should happen before
v4l2_async_register_subdev().

> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  drivers/media/i2c/ov5645.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> index ab3a419df2df..78b86438c798 100644
> --- a/drivers/media/i2c/ov5645.c
> +++ b/drivers/media/i2c/ov5645.c
> @@ -1239,18 +1239,17 @@ static int ov5645_probe(struct i2c_client *client)
>  		goto power_down;
>  	}
>  
> -	pm_runtime_set_active(dev);
> -	pm_runtime_get_noresume(dev);
> -	pm_runtime_enable(dev);
> -
>  	ov5645_init_state(&ov5645->sd, NULL);
>  
>  	ret = v4l2_async_register_subdev(&ov5645->sd);
>  	if (ret < 0) {
>  		dev_err(dev, "could not register v4l2 device\n");
> -		goto err_pm_runtime;
> +		goto power_down;
>  	}
>  
> +	pm_runtime_set_active(dev);
> +	pm_runtime_get_noresume(dev);
> +	pm_runtime_enable(dev);
>  	pm_runtime_set_autosuspend_delay(dev, 1000);
>  	pm_runtime_use_autosuspend(dev);
>  	pm_runtime_mark_last_busy(dev);
> @@ -1258,9 +1257,6 @@ static int ov5645_probe(struct i2c_client *client)
>  
>  	return 0;
>  
> -err_pm_runtime:
> -	pm_runtime_disable(dev);
> -	pm_runtime_put_noidle(dev);
>  power_down:
>  	ov5645_set_power_off(dev);
>  free_entity:

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 03/11] media: i2c: ov5645: Enable runtime PM after v4l2_async_register_subdev()

[Lad, Prabhakar]

Hi Laurent,

Thank you for the review.

On Tue, Sep 24, 2024 at 11:38 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Prabhakar,
>
> Thank you for the patch.
>
> On Tue, Sep 10, 2024 at 06:06:02PM +0100, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > To simplify the probe error path, enable runtime PM after the
> > v4l2_async_register_subdev() call.
> >
> > This change ensures that runtime PM is only enabled once the subdevice
> > registration is successful, avoiding unnecessary cleanup in the error
> > path.
>
> The subdev could start being used as soon as it gets registered, so
> runtime PM initialization should happen before
> v4l2_async_register_subdev().
>
Agreed, I will drop this patch.

Cheers,
Prabhakar

[PATCH v2 04/11] media: i2c: ov5645: Use dev_err_probe instead of dev_err

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Drop dev_err() and use the dev_err_probe() helper on probe path.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/media/i2c/ov5645.c | 74 +++++++++++++++-----------------------
 1 file changed, 28 insertions(+), 46 deletions(-)

diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index 78b86438c798..9e6ff1f1b9ac 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -1076,51 +1076,37 @@ static int ov5645_probe(struct i2c_client *client)
 	ov5645->dev = dev;
 
 	endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1);
-	if (!endpoint) {
-		dev_err(dev, "endpoint node not found\n");
-		return -EINVAL;
-	}
+	if (!endpoint)
+		return dev_err_probe(dev, -EINVAL, "endpoint node not found\n");
 
 	ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(endpoint),
 					 &ov5645->ep);
 
 	of_node_put(endpoint);
 
-	if (ret < 0) {
-		dev_err(dev, "parsing endpoint node failed\n");
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "parsing endpoint node failed\n");
 
-	if (ov5645->ep.bus_type != V4L2_MBUS_CSI2_DPHY) {
-		dev_err(dev, "invalid bus type, must be CSI2\n");
-		return -EINVAL;
-	}
+	if (ov5645->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
+		return dev_err_probe(dev, -EINVAL, "invalid bus type, must be CSI2\n");
 
 	/* get system clock (xclk) */
 	ov5645->xclk = devm_clk_get(dev, NULL);
-	if (IS_ERR(ov5645->xclk)) {
-		dev_err(dev, "could not get xclk");
-		return PTR_ERR(ov5645->xclk);
-	}
+	if (IS_ERR(ov5645->xclk))
+		return dev_err_probe(dev, PTR_ERR(ov5645->xclk), "could not get xclk");
 
 	ret = of_property_read_u32(dev->of_node, "clock-frequency", &xclk_freq);
-	if (ret) {
-		dev_err(dev, "could not get xclk frequency\n");
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(dev, ret, "could not get xclk frequency\n");
 
 	/* external clock must be 24MHz, allow 1% tolerance */
-	if (xclk_freq < 23760000 || xclk_freq > 24240000) {
-		dev_err(dev, "external clock frequency %u is not supported\n",
-			xclk_freq);
-		return -EINVAL;
-	}
+	if (xclk_freq < 23760000 || xclk_freq > 24240000)
+		return dev_err_probe(dev, -EINVAL, "external clock frequency %u is not supported\n",
+				     xclk_freq);
 
 	ret = clk_set_rate(ov5645->xclk, xclk_freq);
-	if (ret) {
-		dev_err(dev, "could not set xclk frequency\n");
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(dev, ret, "could not set xclk frequency\n");
 
 	for (i = 0; i < OV5645_NUM_SUPPLIES; i++)
 		ov5645->supplies[i].supply = ov5645_supply_name[i];
@@ -1131,16 +1117,12 @@ static int ov5645_probe(struct i2c_client *client)
 		return ret;
 
 	ov5645->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_HIGH);
-	if (IS_ERR(ov5645->enable_gpio)) {
-		dev_err(dev, "cannot get enable gpio\n");
-		return PTR_ERR(ov5645->enable_gpio);
-	}
+	if (IS_ERR(ov5645->enable_gpio))
+		return dev_err_probe(dev, PTR_ERR(ov5645->enable_gpio), "cannot get enable gpio\n");
 
 	ov5645->rst_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
-	if (IS_ERR(ov5645->rst_gpio)) {
-		dev_err(dev, "cannot get reset gpio\n");
-		return PTR_ERR(ov5645->rst_gpio);
-	}
+	if (IS_ERR(ov5645->rst_gpio))
+		return dev_err_probe(dev, PTR_ERR(ov5645->rst_gpio), "cannot get reset gpio\n");
 
 	mutex_init(&ov5645->power_lock);
 
@@ -1177,9 +1159,9 @@ static int ov5645_probe(struct i2c_client *client)
 	ov5645->sd.ctrl_handler = &ov5645->ctrls;
 
 	if (ov5645->ctrls.error) {
-		dev_err(dev, "%s: control initialization error %d\n",
-		       __func__, ov5645->ctrls.error);
 		ret = ov5645->ctrls.error;
+		dev_err_probe(dev, ret, "%s: control initialization error %d\n",
+			      __func__, ov5645->ctrls.error);
 		goto free_ctrl;
 	}
 
@@ -1192,7 +1174,7 @@ static int ov5645_probe(struct i2c_client *client)
 
 	ret = media_entity_pads_init(&ov5645->sd.entity, 1, &ov5645->pad);
 	if (ret < 0) {
-		dev_err(dev, "could not register media entity\n");
+		dev_err_probe(dev, ret, "could not register media entity\n");
 		goto free_ctrl;
 	}
 
@@ -1202,14 +1184,14 @@ static int ov5645_probe(struct i2c_client *client)
 
 	ret = ov5645_read_reg(ov5645, OV5645_CHIP_ID_HIGH, &chip_id_high);
 	if (ret < 0 || chip_id_high != OV5645_CHIP_ID_HIGH_BYTE) {
-		dev_err(dev, "could not read ID high\n");
 		ret = -ENODEV;
+		dev_err_probe(dev, ret, "could not read ID high\n");
 		goto power_down;
 	}
 	ret = ov5645_read_reg(ov5645, OV5645_CHIP_ID_LOW, &chip_id_low);
 	if (ret < 0 || chip_id_low != OV5645_CHIP_ID_LOW_BYTE) {
-		dev_err(dev, "could not read ID low\n");
 		ret = -ENODEV;
+		dev_err_probe(dev, ret, "could not read ID low\n");
 		goto power_down;
 	}
 
@@ -1218,24 +1200,24 @@ static int ov5645_probe(struct i2c_client *client)
 	ret = ov5645_read_reg(ov5645, OV5645_AEC_PK_MANUAL,
 			      &ov5645->aec_pk_manual);
 	if (ret < 0) {
-		dev_err(dev, "could not read AEC/AGC mode\n");
 		ret = -ENODEV;
+		dev_err_probe(dev, ret, "could not read AEC/AGC mode\n");
 		goto power_down;
 	}
 
 	ret = ov5645_read_reg(ov5645, OV5645_TIMING_TC_REG20,
 			      &ov5645->timing_tc_reg20);
 	if (ret < 0) {
-		dev_err(dev, "could not read vflip value\n");
 		ret = -ENODEV;
+		dev_err_probe(dev, ret, "could not read vflip value\n");
 		goto power_down;
 	}
 
 	ret = ov5645_read_reg(ov5645, OV5645_TIMING_TC_REG21,
 			      &ov5645->timing_tc_reg21);
 	if (ret < 0) {
-		dev_err(dev, "could not read hflip value\n");
 		ret = -ENODEV;
+		dev_err_probe(dev, ret, "could not read hflip value\n");
 		goto power_down;
 	}
 
@@ -1243,7 +1225,7 @@ static int ov5645_probe(struct i2c_client *client)
 
 	ret = v4l2_async_register_subdev(&ov5645->sd);
 	if (ret < 0) {
-		dev_err(dev, "could not register v4l2 device\n");
+		dev_err_probe(dev, ret, "could not register v4l2 device\n");
 		goto power_down;
 	}
 
-- 
2.34.1

Re: [PATCH v2 04/11] media: i2c: ov5645: Use dev_err_probe instead of dev_err

[Laurent Pinchart]

Hi Prabhakar,

Thank you for the patch.

On Tue, Sep 10, 2024 at 06:06:03PM +0100, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Drop dev_err() and use the dev_err_probe() helper on probe path.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  drivers/media/i2c/ov5645.c | 74 +++++++++++++++-----------------------
>  1 file changed, 28 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> index 78b86438c798..9e6ff1f1b9ac 100644
> --- a/drivers/media/i2c/ov5645.c
> +++ b/drivers/media/i2c/ov5645.c
> @@ -1076,51 +1076,37 @@ static int ov5645_probe(struct i2c_client *client)
>  	ov5645->dev = dev;
>  
>  	endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1);
> -	if (!endpoint) {
> -		dev_err(dev, "endpoint node not found\n");
> -		return -EINVAL;
> -	}
> +	if (!endpoint)
> +		return dev_err_probe(dev, -EINVAL, "endpoint node not found\n");
>  
>  	ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(endpoint),
>  					 &ov5645->ep);
>  
>  	of_node_put(endpoint);
>  
> -	if (ret < 0) {
> -		dev_err(dev, "parsing endpoint node failed\n");
> -		return ret;
> -	}
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "parsing endpoint node failed\n");
>  
> -	if (ov5645->ep.bus_type != V4L2_MBUS_CSI2_DPHY) {
> -		dev_err(dev, "invalid bus type, must be CSI2\n");
> -		return -EINVAL;
> -	}
> +	if (ov5645->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
> +		return dev_err_probe(dev, -EINVAL, "invalid bus type, must be CSI2\n");
>  
>  	/* get system clock (xclk) */
>  	ov5645->xclk = devm_clk_get(dev, NULL);
> -	if (IS_ERR(ov5645->xclk)) {
> -		dev_err(dev, "could not get xclk");
> -		return PTR_ERR(ov5645->xclk);
> -	}
> +	if (IS_ERR(ov5645->xclk))
> +		return dev_err_probe(dev, PTR_ERR(ov5645->xclk), "could not get xclk");
>  
>  	ret = of_property_read_u32(dev->of_node, "clock-frequency", &xclk_freq);
> -	if (ret) {
> -		dev_err(dev, "could not get xclk frequency\n");
> -		return ret;
> -	}
> +	if (ret)
> +		return dev_err_probe(dev, ret, "could not get xclk frequency\n");
>  
>  	/* external clock must be 24MHz, allow 1% tolerance */
> -	if (xclk_freq < 23760000 || xclk_freq > 24240000) {
> -		dev_err(dev, "external clock frequency %u is not supported\n",
> -			xclk_freq);
> -		return -EINVAL;
> -	}
> +	if (xclk_freq < 23760000 || xclk_freq > 24240000)
> +		return dev_err_probe(dev, -EINVAL, "external clock frequency %u is not supported\n",
> +				     xclk_freq);
>  
>  	ret = clk_set_rate(ov5645->xclk, xclk_freq);
> -	if (ret) {
> -		dev_err(dev, "could not set xclk frequency\n");
> -		return ret;
> -	}
> +	if (ret)
> +		return dev_err_probe(dev, ret, "could not set xclk frequency\n");
>  
>  	for (i = 0; i < OV5645_NUM_SUPPLIES; i++)
>  		ov5645->supplies[i].supply = ov5645_supply_name[i];
> @@ -1131,16 +1117,12 @@ static int ov5645_probe(struct i2c_client *client)
>  		return ret;
>  
>  	ov5645->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_HIGH);
> -	if (IS_ERR(ov5645->enable_gpio)) {
> -		dev_err(dev, "cannot get enable gpio\n");
> -		return PTR_ERR(ov5645->enable_gpio);
> -	}
> +	if (IS_ERR(ov5645->enable_gpio))
> +		return dev_err_probe(dev, PTR_ERR(ov5645->enable_gpio), "cannot get enable gpio\n");

Those lines are getting long. We usually try to wrap at 80 columns for
sensor drivers:

		return dev_err_probe(dev, PTR_ERR(ov5645->enable_gpio),
				     "cannot get enable gpio\n");

Same elsewhere. I'll let Sakari decide.

>  
>  	ov5645->rst_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> -	if (IS_ERR(ov5645->rst_gpio)) {
> -		dev_err(dev, "cannot get reset gpio\n");
> -		return PTR_ERR(ov5645->rst_gpio);
> -	}
> +	if (IS_ERR(ov5645->rst_gpio))
> +		return dev_err_probe(dev, PTR_ERR(ov5645->rst_gpio), "cannot get reset gpio\n");
>  
>  	mutex_init(&ov5645->power_lock);
>  
> @@ -1177,9 +1159,9 @@ static int ov5645_probe(struct i2c_client *client)
>  	ov5645->sd.ctrl_handler = &ov5645->ctrls;
>  
>  	if (ov5645->ctrls.error) {
> -		dev_err(dev, "%s: control initialization error %d\n",
> -		       __func__, ov5645->ctrls.error);
>  		ret = ov5645->ctrls.error;
> +		dev_err_probe(dev, ret, "%s: control initialization error %d\n",
> +			      __func__, ov5645->ctrls.error);
>  		goto free_ctrl;
>  	}
>  
> @@ -1192,7 +1174,7 @@ static int ov5645_probe(struct i2c_client *client)
>  
>  	ret = media_entity_pads_init(&ov5645->sd.entity, 1, &ov5645->pad);
>  	if (ret < 0) {
> -		dev_err(dev, "could not register media entity\n");
> +		dev_err_probe(dev, ret, "could not register media entity\n");
>  		goto free_ctrl;
>  	}
>  
> @@ -1202,14 +1184,14 @@ static int ov5645_probe(struct i2c_client *client)
>  
>  	ret = ov5645_read_reg(ov5645, OV5645_CHIP_ID_HIGH, &chip_id_high);
>  	if (ret < 0 || chip_id_high != OV5645_CHIP_ID_HIGH_BYTE) {
> -		dev_err(dev, "could not read ID high\n");
>  		ret = -ENODEV;
> +		dev_err_probe(dev, ret, "could not read ID high\n");
>  		goto power_down;
>  	}
>  	ret = ov5645_read_reg(ov5645, OV5645_CHIP_ID_LOW, &chip_id_low);
>  	if (ret < 0 || chip_id_low != OV5645_CHIP_ID_LOW_BYTE) {
> -		dev_err(dev, "could not read ID low\n");
>  		ret = -ENODEV;
> +		dev_err_probe(dev, ret, "could not read ID low\n");
>  		goto power_down;
>  	}
>  
> @@ -1218,24 +1200,24 @@ static int ov5645_probe(struct i2c_client *client)
>  	ret = ov5645_read_reg(ov5645, OV5645_AEC_PK_MANUAL,
>  			      &ov5645->aec_pk_manual);
>  	if (ret < 0) {
> -		dev_err(dev, "could not read AEC/AGC mode\n");
>  		ret = -ENODEV;
> +		dev_err_probe(dev, ret, "could not read AEC/AGC mode\n");
>  		goto power_down;
>  	}
>  
>  	ret = ov5645_read_reg(ov5645, OV5645_TIMING_TC_REG20,
>  			      &ov5645->timing_tc_reg20);
>  	if (ret < 0) {
> -		dev_err(dev, "could not read vflip value\n");
>  		ret = -ENODEV;
> +		dev_err_probe(dev, ret, "could not read vflip value\n");
>  		goto power_down;
>  	}
>  
>  	ret = ov5645_read_reg(ov5645, OV5645_TIMING_TC_REG21,
>  			      &ov5645->timing_tc_reg21);
>  	if (ret < 0) {
> -		dev_err(dev, "could not read hflip value\n");
>  		ret = -ENODEV;
> +		dev_err_probe(dev, ret, "could not read hflip value\n");
>  		goto power_down;
>  	}
>  
> @@ -1243,7 +1225,7 @@ static int ov5645_probe(struct i2c_client *client)
>  
>  	ret = v4l2_async_register_subdev(&ov5645->sd);
>  	if (ret < 0) {
> -		dev_err(dev, "could not register v4l2 device\n");
> +		dev_err_probe(dev, ret, "could not register v4l2 device\n");
>  		goto power_down;
>  	}
>  

The probe function looks really young, I think it would benefit from
being broken down in multiple functions.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 04/11] media: i2c: ov5645: Use dev_err_probe instead of dev_err

[Lad, Prabhakar]

Hi Laurent,

Thank you for the review.

On Tue, Sep 24, 2024 at 11:44 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Prabhakar,
>
> Thank you for the patch.
>
> On Tue, Sep 10, 2024 at 06:06:03PM +0100, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Drop dev_err() and use the dev_err_probe() helper on probe path.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  drivers/media/i2c/ov5645.c | 74 +++++++++++++++-----------------------
> >  1 file changed, 28 insertions(+), 46 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> > index 78b86438c798..9e6ff1f1b9ac 100644
> > --- a/drivers/media/i2c/ov5645.c
> > +++ b/drivers/media/i2c/ov5645.c
> > @@ -1076,51 +1076,37 @@ static int ov5645_probe(struct i2c_client *client)
> >       ov5645->dev = dev;
> >
> >       endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1);
> > -     if (!endpoint) {
> > -             dev_err(dev, "endpoint node not found\n");
> > -             return -EINVAL;
> > -     }
> > +     if (!endpoint)
> > +             return dev_err_probe(dev, -EINVAL, "endpoint node not found\n");
> >
> >       ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(endpoint),
> >                                        &ov5645->ep);
> >
> >       of_node_put(endpoint);
> >
> > -     if (ret < 0) {
> > -             dev_err(dev, "parsing endpoint node failed\n");
> > -             return ret;
> > -     }
> > +     if (ret < 0)
> > +             return dev_err_probe(dev, ret, "parsing endpoint node failed\n");
> >
> > -     if (ov5645->ep.bus_type != V4L2_MBUS_CSI2_DPHY) {
> > -             dev_err(dev, "invalid bus type, must be CSI2\n");
> > -             return -EINVAL;
> > -     }
> > +     if (ov5645->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
> > +             return dev_err_probe(dev, -EINVAL, "invalid bus type, must be CSI2\n");
> >
> >       /* get system clock (xclk) */
> >       ov5645->xclk = devm_clk_get(dev, NULL);
> > -     if (IS_ERR(ov5645->xclk)) {
> > -             dev_err(dev, "could not get xclk");
> > -             return PTR_ERR(ov5645->xclk);
> > -     }
> > +     if (IS_ERR(ov5645->xclk))
> > +             return dev_err_probe(dev, PTR_ERR(ov5645->xclk), "could not get xclk");
> >
> >       ret = of_property_read_u32(dev->of_node, "clock-frequency", &xclk_freq);
> > -     if (ret) {
> > -             dev_err(dev, "could not get xclk frequency\n");
> > -             return ret;
> > -     }
> > +     if (ret)
> > +             return dev_err_probe(dev, ret, "could not get xclk frequency\n");
> >
> >       /* external clock must be 24MHz, allow 1% tolerance */
> > -     if (xclk_freq < 23760000 || xclk_freq > 24240000) {
> > -             dev_err(dev, "external clock frequency %u is not supported\n",
> > -                     xclk_freq);
> > -             return -EINVAL;
> > -     }
> > +     if (xclk_freq < 23760000 || xclk_freq > 24240000)
> > +             return dev_err_probe(dev, -EINVAL, "external clock frequency %u is not supported\n",
> > +                                  xclk_freq);
> >
> >       ret = clk_set_rate(ov5645->xclk, xclk_freq);
> > -     if (ret) {
> > -             dev_err(dev, "could not set xclk frequency\n");
> > -             return ret;
> > -     }
> > +     if (ret)
> > +             return dev_err_probe(dev, ret, "could not set xclk frequency\n");
> >
> >       for (i = 0; i < OV5645_NUM_SUPPLIES; i++)
> >               ov5645->supplies[i].supply = ov5645_supply_name[i];
> > @@ -1131,16 +1117,12 @@ static int ov5645_probe(struct i2c_client *client)
> >               return ret;
> >
> >       ov5645->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_HIGH);
> > -     if (IS_ERR(ov5645->enable_gpio)) {
> > -             dev_err(dev, "cannot get enable gpio\n");
> > -             return PTR_ERR(ov5645->enable_gpio);
> > -     }
> > +     if (IS_ERR(ov5645->enable_gpio))
> > +             return dev_err_probe(dev, PTR_ERR(ov5645->enable_gpio), "cannot get enable gpio\n");
>
> Those lines are getting long. We usually try to wrap at 80 columns for
> sensor drivers:
>
As there will be a v3 anyway, I'll wrap it to 80 columns.

>                 return dev_err_probe(dev, PTR_ERR(ov5645->enable_gpio),
>                                      "cannot get enable gpio\n");
>
> Same elsewhere. I'll let Sakari decide.
>
> >
> >       ov5645->rst_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> > -     if (IS_ERR(ov5645->rst_gpio)) {
> > -             dev_err(dev, "cannot get reset gpio\n");
> > -             return PTR_ERR(ov5645->rst_gpio);
> > -     }
> > +     if (IS_ERR(ov5645->rst_gpio))
> > +             return dev_err_probe(dev, PTR_ERR(ov5645->rst_gpio), "cannot get reset gpio\n");
> >
> >       mutex_init(&ov5645->power_lock);
> >
> > @@ -1177,9 +1159,9 @@ static int ov5645_probe(struct i2c_client *client)
> >       ov5645->sd.ctrl_handler = &ov5645->ctrls;
> >
> >       if (ov5645->ctrls.error) {
> > -             dev_err(dev, "%s: control initialization error %d\n",
> > -                    __func__, ov5645->ctrls.error);
> >               ret = ov5645->ctrls.error;
> > +             dev_err_probe(dev, ret, "%s: control initialization error %d\n",
> > +                           __func__, ov5645->ctrls.error);
> >               goto free_ctrl;
> >       }
> >
> > @@ -1192,7 +1174,7 @@ static int ov5645_probe(struct i2c_client *client)
> >
> >       ret = media_entity_pads_init(&ov5645->sd.entity, 1, &ov5645->pad);
> >       if (ret < 0) {
> > -             dev_err(dev, "could not register media entity\n");
> > +             dev_err_probe(dev, ret, "could not register media entity\n");
> >               goto free_ctrl;
> >       }
> >
> > @@ -1202,14 +1184,14 @@ static int ov5645_probe(struct i2c_client *client)
> >
> >       ret = ov5645_read_reg(ov5645, OV5645_CHIP_ID_HIGH, &chip_id_high);
> >       if (ret < 0 || chip_id_high != OV5645_CHIP_ID_HIGH_BYTE) {
> > -             dev_err(dev, "could not read ID high\n");
> >               ret = -ENODEV;
> > +             dev_err_probe(dev, ret, "could not read ID high\n");
> >               goto power_down;
> >       }
> >       ret = ov5645_read_reg(ov5645, OV5645_CHIP_ID_LOW, &chip_id_low);
> >       if (ret < 0 || chip_id_low != OV5645_CHIP_ID_LOW_BYTE) {
> > -             dev_err(dev, "could not read ID low\n");
> >               ret = -ENODEV;
> > +             dev_err_probe(dev, ret, "could not read ID low\n");
> >               goto power_down;
> >       }
> >
> > @@ -1218,24 +1200,24 @@ static int ov5645_probe(struct i2c_client *client)
> >       ret = ov5645_read_reg(ov5645, OV5645_AEC_PK_MANUAL,
> >                             &ov5645->aec_pk_manual);
> >       if (ret < 0) {
> > -             dev_err(dev, "could not read AEC/AGC mode\n");
> >               ret = -ENODEV;
> > +             dev_err_probe(dev, ret, "could not read AEC/AGC mode\n");
> >               goto power_down;
> >       }
> >
> >       ret = ov5645_read_reg(ov5645, OV5645_TIMING_TC_REG20,
> >                             &ov5645->timing_tc_reg20);
> >       if (ret < 0) {
> > -             dev_err(dev, "could not read vflip value\n");
> >               ret = -ENODEV;
> > +             dev_err_probe(dev, ret, "could not read vflip value\n");
> >               goto power_down;
> >       }
> >
> >       ret = ov5645_read_reg(ov5645, OV5645_TIMING_TC_REG21,
> >                             &ov5645->timing_tc_reg21);
> >       if (ret < 0) {
> > -             dev_err(dev, "could not read hflip value\n");
> >               ret = -ENODEV;
> > +             dev_err_probe(dev, ret, "could not read hflip value\n");
> >               goto power_down;
> >       }
> >
> > @@ -1243,7 +1225,7 @@ static int ov5645_probe(struct i2c_client *client)
> >
> >       ret = v4l2_async_register_subdev(&ov5645->sd);
> >       if (ret < 0) {
> > -             dev_err(dev, "could not register v4l2 device\n");
> > +             dev_err_probe(dev, ret, "could not register v4l2 device\n");
> >               goto power_down;
> >       }
> >
>
> The probe function looks really young, I think it would benefit from
> being broken down in multiple functions.
>
I will add this once this initial series gets accepted.

Cheers,
Prabhakar

[PATCH v2 05/11] media: i2c: ov5645: Use v4l2_async_register_subdev_sensor()

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Make use v4l2_async_register_subdev_sensor() helper to register
the subdev.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/media/i2c/ov5645.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index 9e6ff1f1b9ac..45687d004004 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -1223,7 +1223,7 @@ static int ov5645_probe(struct i2c_client *client)
 
 	ov5645_init_state(&ov5645->sd, NULL);
 
-	ret = v4l2_async_register_subdev(&ov5645->sd);
+	ret = v4l2_async_register_subdev_sensor(&ov5645->sd);
 	if (ret < 0) {
 		dev_err_probe(dev, ret, "could not register v4l2 device\n");
 		goto power_down;
-- 
2.34.1

Re: [PATCH v2 05/11] media: i2c: ov5645: Use v4l2_async_register_subdev_sensor()

[Laurent Pinchart]

On Tue, Sep 10, 2024 at 06:06:04PM +0100, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Make use v4l2_async_register_subdev_sensor() helper to register
> the subdev.

The commit message should explain why.

> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> ---
>  drivers/media/i2c/ov5645.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> index 9e6ff1f1b9ac..45687d004004 100644
> --- a/drivers/media/i2c/ov5645.c
> +++ b/drivers/media/i2c/ov5645.c
> @@ -1223,7 +1223,7 @@ static int ov5645_probe(struct i2c_client *client)
>  
>  	ov5645_init_state(&ov5645->sd, NULL);
>  
> -	ret = v4l2_async_register_subdev(&ov5645->sd);
> +	ret = v4l2_async_register_subdev_sensor(&ov5645->sd);
>  	if (ret < 0) {
>  		dev_err_probe(dev, ret, "could not register v4l2 device\n");
>  		goto power_down;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 05/11] media: i2c: ov5645: Use v4l2_async_register_subdev_sensor()

[Lad, Prabhakar]

Hi Laurent,

Thank you for the review.

On Tue, Sep 24, 2024 at 11:45 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Tue, Sep 10, 2024 at 06:06:04PM +0100, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Make use v4l2_async_register_subdev_sensor() helper to register
> > the subdev.
>
> The commit message should explain why.
>
Sure I'll update the commit message.

Cheers,
Prabhakar

[PATCH v2 06/11] media: i2c: ov5645: Drop `power_lock` mutex

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Drop mutex while applying controls and just rely on
pm_runtime_get_if_in_use() call.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/media/i2c/ov5645.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index 45687d004004..25c60afcc0ec 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -106,8 +106,6 @@ struct ov5645 {
 	u8 timing_tc_reg20;
 	u8 timing_tc_reg21;
 
-	struct mutex power_lock; /* lock to protect power state */
-
 	struct gpio_desc *enable_gpio;
 	struct gpio_desc *rst_gpio;
 };
@@ -782,11 +780,8 @@ static int ov5645_s_ctrl(struct v4l2_ctrl *ctrl)
 					     struct ov5645, ctrls);
 	int ret;
 
-	mutex_lock(&ov5645->power_lock);
-	if (!pm_runtime_get_if_in_use(ov5645->dev)) {
-		mutex_unlock(&ov5645->power_lock);
+	if (!pm_runtime_get_if_in_use(ov5645->dev))
 		return 0;
-	}
 
 	switch (ctrl->id) {
 	case V4L2_CID_SATURATION:
@@ -817,7 +812,6 @@ static int ov5645_s_ctrl(struct v4l2_ctrl *ctrl)
 
 	pm_runtime_mark_last_busy(ov5645->dev);
 	pm_runtime_put_autosuspend(ov5645->dev);
-	mutex_unlock(&ov5645->power_lock);
 
 	return ret;
 }
@@ -1124,8 +1118,6 @@ static int ov5645_probe(struct i2c_client *client)
 	if (IS_ERR(ov5645->rst_gpio))
 		return dev_err_probe(dev, PTR_ERR(ov5645->rst_gpio), "cannot get reset gpio\n");
 
-	mutex_init(&ov5645->power_lock);
-
 	v4l2_ctrl_handler_init(&ov5645->ctrls, 9);
 	v4l2_ctrl_new_std(&ov5645->ctrls, &ov5645_ctrl_ops,
 			  V4L2_CID_SATURATION, -4, 4, 1, 0);
@@ -1245,7 +1237,6 @@ static int ov5645_probe(struct i2c_client *client)
 	media_entity_cleanup(&ov5645->sd.entity);
 free_ctrl:
 	v4l2_ctrl_handler_free(&ov5645->ctrls);
-	mutex_destroy(&ov5645->power_lock);
 
 	return ret;
 }
@@ -1262,7 +1253,6 @@ static void ov5645_remove(struct i2c_client *client)
 	if (!pm_runtime_status_suspended(ov5645->dev))
 		ov5645_set_power_off(ov5645->dev);
 	pm_runtime_set_suspended(ov5645->dev);
-	mutex_destroy(&ov5645->power_lock);
 }
 
 static const struct i2c_device_id ov5645_id[] = {
-- 
2.34.1

Re: [PATCH v2 06/11] media: i2c: ov5645: Drop `power_lock` mutex

[Laurent Pinchart]

On Tue, Sep 10, 2024 at 06:06:05PM +0100, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Drop mutex while applying controls and just rely on
> pm_runtime_get_if_in_use() call.

Here too the commit message should explain why the mutex can be dropped.
Given that it is only used in .s_ctrl(), and the control framework
serializes calls to the function, the mutex is not needed.

> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> ---
>  drivers/media/i2c/ov5645.c | 12 +-----------
>  1 file changed, 1 insertion(+), 11 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> index 45687d004004..25c60afcc0ec 100644
> --- a/drivers/media/i2c/ov5645.c
> +++ b/drivers/media/i2c/ov5645.c
> @@ -106,8 +106,6 @@ struct ov5645 {
>  	u8 timing_tc_reg20;
>  	u8 timing_tc_reg21;
>  
> -	struct mutex power_lock; /* lock to protect power state */
> -
>  	struct gpio_desc *enable_gpio;
>  	struct gpio_desc *rst_gpio;
>  };
> @@ -782,11 +780,8 @@ static int ov5645_s_ctrl(struct v4l2_ctrl *ctrl)
>  					     struct ov5645, ctrls);
>  	int ret;
>  
> -	mutex_lock(&ov5645->power_lock);
> -	if (!pm_runtime_get_if_in_use(ov5645->dev)) {
> -		mutex_unlock(&ov5645->power_lock);
> +	if (!pm_runtime_get_if_in_use(ov5645->dev))
>  		return 0;
> -	}
>  
>  	switch (ctrl->id) {
>  	case V4L2_CID_SATURATION:
> @@ -817,7 +812,6 @@ static int ov5645_s_ctrl(struct v4l2_ctrl *ctrl)
>  
>  	pm_runtime_mark_last_busy(ov5645->dev);
>  	pm_runtime_put_autosuspend(ov5645->dev);
> -	mutex_unlock(&ov5645->power_lock);
>  
>  	return ret;
>  }
> @@ -1124,8 +1118,6 @@ static int ov5645_probe(struct i2c_client *client)
>  	if (IS_ERR(ov5645->rst_gpio))
>  		return dev_err_probe(dev, PTR_ERR(ov5645->rst_gpio), "cannot get reset gpio\n");
>  
> -	mutex_init(&ov5645->power_lock);
> -
>  	v4l2_ctrl_handler_init(&ov5645->ctrls, 9);
>  	v4l2_ctrl_new_std(&ov5645->ctrls, &ov5645_ctrl_ops,
>  			  V4L2_CID_SATURATION, -4, 4, 1, 0);
> @@ -1245,7 +1237,6 @@ static int ov5645_probe(struct i2c_client *client)
>  	media_entity_cleanup(&ov5645->sd.entity);
>  free_ctrl:
>  	v4l2_ctrl_handler_free(&ov5645->ctrls);
> -	mutex_destroy(&ov5645->power_lock);
>  
>  	return ret;
>  }
> @@ -1262,7 +1253,6 @@ static void ov5645_remove(struct i2c_client *client)
>  	if (!pm_runtime_status_suspended(ov5645->dev))
>  		ov5645_set_power_off(ov5645->dev);
>  	pm_runtime_set_suspended(ov5645->dev);
> -	mutex_destroy(&ov5645->power_lock);
>  }
>  
>  static const struct i2c_device_id ov5645_id[] = {

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 06/11] media: i2c: ov5645: Drop `power_lock` mutex

[Lad, Prabhakar]

Hi Laurent,

Thank you for the review.

On Tue, Sep 24, 2024 at 11:46 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Tue, Sep 10, 2024 at 06:06:05PM +0100, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Drop mutex while applying controls and just rely on
> > pm_runtime_get_if_in_use() call.
>
> Here too the commit message should explain why the mutex can be dropped.
> Given that it is only used in .s_ctrl(), and the control framework
> serializes calls to the function, the mutex is not needed.
>
Agreed, I'll update the commit message as above and send a v3.

Cheers,
Prabhakar

[PATCH v2 07/11] media: i2c: ov5645: Use subdev active state

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Port the ov5645 sensor driver to use the subdev active state.

Move all the format configuration to the subdevice state and simplify
the format handling, locking and initialization.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/media/i2c/ov5645.c | 109 +++++++++++++------------------------
 1 file changed, 39 insertions(+), 70 deletions(-)

diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index 25c60afcc0ec..9497ec737cb7 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -89,7 +89,6 @@ struct ov5645 {
 	struct v4l2_subdev sd;
 	struct media_pad pad;
 	struct v4l2_fwnode_endpoint ep;
-	struct v4l2_mbus_framefmt fmt;
 	struct v4l2_rect crop;
 	struct clk *xclk;
 
@@ -850,49 +849,6 @@ static int ov5645_enum_frame_size(struct v4l2_subdev *subdev,
 	return 0;
 }
 
-static struct v4l2_mbus_framefmt *
-__ov5645_get_pad_format(struct ov5645 *ov5645,
-			struct v4l2_subdev_state *sd_state,
-			unsigned int pad,
-			enum v4l2_subdev_format_whence which)
-{
-	switch (which) {
-	case V4L2_SUBDEV_FORMAT_TRY:
-		return v4l2_subdev_state_get_format(sd_state, pad);
-	case V4L2_SUBDEV_FORMAT_ACTIVE:
-		return &ov5645->fmt;
-	default:
-		return NULL;
-	}
-}
-
-static int ov5645_get_format(struct v4l2_subdev *sd,
-			     struct v4l2_subdev_state *sd_state,
-			     struct v4l2_subdev_format *format)
-{
-	struct ov5645 *ov5645 = to_ov5645(sd);
-
-	format->format = *__ov5645_get_pad_format(ov5645, sd_state,
-						  format->pad,
-						  format->which);
-	return 0;
-}
-
-static struct v4l2_rect *
-__ov5645_get_pad_crop(struct ov5645 *ov5645,
-		      struct v4l2_subdev_state *sd_state,
-		      unsigned int pad, enum v4l2_subdev_format_whence which)
-{
-	switch (which) {
-	case V4L2_SUBDEV_FORMAT_TRY:
-		return v4l2_subdev_state_get_crop(sd_state, pad);
-	case V4L2_SUBDEV_FORMAT_ACTIVE:
-		return &ov5645->crop;
-	default:
-		return NULL;
-	}
-}
-
 static int ov5645_set_format(struct v4l2_subdev *sd,
 			     struct v4l2_subdev_state *sd_state,
 			     struct v4l2_subdev_format *format)
@@ -903,33 +859,30 @@ static int ov5645_set_format(struct v4l2_subdev *sd,
 	const struct ov5645_mode_info *new_mode;
 	int ret;
 
-	__crop = __ov5645_get_pad_crop(ov5645, sd_state, format->pad,
-				       format->which);
-
+	__crop = v4l2_subdev_state_get_crop(sd_state, 0);
 	new_mode = v4l2_find_nearest_size(ov5645_mode_info_data,
-			       ARRAY_SIZE(ov5645_mode_info_data),
-			       width, height,
-			       format->format.width, format->format.height);
+					  ARRAY_SIZE(ov5645_mode_info_data),
+					  width, height, format->format.width,
+					  format->format.height);
 
 	__crop->width = new_mode->width;
 	__crop->height = new_mode->height;
 
 	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
-		ret = v4l2_ctrl_s_ctrl_int64(ov5645->pixel_clock,
-					     new_mode->pixel_clock);
+		ret = __v4l2_ctrl_s_ctrl_int64(ov5645->pixel_clock,
+					       new_mode->pixel_clock);
 		if (ret < 0)
 			return ret;
 
-		ret = v4l2_ctrl_s_ctrl(ov5645->link_freq,
-				       new_mode->link_freq);
+		ret = __v4l2_ctrl_s_ctrl(ov5645->link_freq,
+					 new_mode->link_freq);
 		if (ret < 0)
 			return ret;
 
 		ov5645->current_mode = new_mode;
 	}
 
-	__format = __ov5645_get_pad_format(ov5645, sd_state, format->pad,
-					   format->which);
+	__format = v4l2_subdev_state_get_format(sd_state, 0);
 	__format->width = __crop->width;
 	__format->height = __crop->height;
 	__format->code = MEDIA_BUS_FMT_UYVY8_1X16;
@@ -944,11 +897,15 @@ static int ov5645_set_format(struct v4l2_subdev *sd,
 static int ov5645_init_state(struct v4l2_subdev *subdev,
 			     struct v4l2_subdev_state *sd_state)
 {
-	struct v4l2_subdev_format fmt = { 0 };
-
-	fmt.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
-	fmt.format.width = 1920;
-	fmt.format.height = 1080;
+	struct v4l2_subdev_format fmt = {
+		.which = V4L2_SUBDEV_FORMAT_TRY,
+		.pad = 0,
+		.format = {
+			.code = MEDIA_BUS_FMT_UYVY8_1X16,
+			.width = ov5645_mode_info_data[1].width,
+			.height = ov5645_mode_info_data[1].height,
+		},
+	};
 
 	ov5645_set_format(subdev, sd_state, &fmt);
 
@@ -959,25 +916,27 @@ static int ov5645_get_selection(struct v4l2_subdev *sd,
 			   struct v4l2_subdev_state *sd_state,
 			   struct v4l2_subdev_selection *sel)
 {
-	struct ov5645 *ov5645 = to_ov5645(sd);
-
 	if (sel->target != V4L2_SEL_TGT_CROP)
 		return -EINVAL;
 
-	sel->r = *__ov5645_get_pad_crop(ov5645, sd_state, sel->pad,
-					sel->which);
+	sel->r = *v4l2_subdev_state_get_crop(sd_state, 0);
 	return 0;
 }
 
 static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
 {
 	struct ov5645 *ov5645 = to_ov5645(subdev);
+	struct v4l2_subdev_state *state;
 	int ret;
 
+	state = v4l2_subdev_lock_and_get_active_state(&ov5645->sd);
+
 	if (enable) {
 		ret = pm_runtime_resume_and_get(ov5645->dev);
-		if (ret < 0)
+		if (ret < 0) {
+			v4l2_subdev_unlock_state(state);
 			return ret;
+		}
 
 		ret = ov5645_set_register_array(ov5645,
 					ov5645->current_mode->data,
@@ -988,7 +947,7 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
 				ov5645->current_mode->height);
 			goto err_rpm_put;
 		}
-		ret = v4l2_ctrl_handler_setup(&ov5645->ctrls);
+		ret = __v4l2_ctrl_handler_setup(&ov5645->ctrls);
 		if (ret < 0) {
 			dev_err(ov5645->dev, "could not sync v4l2 controls\n");
 			goto err_rpm_put;
@@ -1013,6 +972,7 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
 		goto stream_off_rpm_put;
 	}
 
+	v4l2_subdev_unlock_state(state);
 	return 0;
 
 err_rpm_put:
@@ -1022,6 +982,7 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
 stream_off_rpm_put:
 	pm_runtime_mark_last_busy(ov5645->dev);
 	pm_runtime_put_autosuspend(ov5645->dev);
+	v4l2_subdev_unlock_state(state);
 	return ret;
 }
 
@@ -1032,7 +993,7 @@ static const struct v4l2_subdev_video_ops ov5645_video_ops = {
 static const struct v4l2_subdev_pad_ops ov5645_subdev_pad_ops = {
 	.enum_mbus_code = ov5645_enum_mbus_code,
 	.enum_frame_size = ov5645_enum_frame_size,
-	.get_fmt = ov5645_get_format,
+	.get_fmt = v4l2_subdev_get_fmt,
 	.set_fmt = ov5645_set_format,
 	.get_selection = ov5645_get_selection,
 };
@@ -1213,12 +1174,17 @@ static int ov5645_probe(struct i2c_client *client)
 		goto power_down;
 	}
 
-	ov5645_init_state(&ov5645->sd, NULL);
+	ov5645->sd.state_lock = ov5645->ctrls.lock;
+	ret = v4l2_subdev_init_finalize(&ov5645->sd);
+	if (ret < 0) {
+		dev_err_probe(dev, ret, "subdev init error\n");
+		goto power_down;
+	}
 
 	ret = v4l2_async_register_subdev_sensor(&ov5645->sd);
 	if (ret < 0) {
 		dev_err_probe(dev, ret, "could not register v4l2 device\n");
-		goto power_down;
+		goto error_subdev_cleanup;
 	}
 
 	pm_runtime_set_active(dev);
@@ -1231,6 +1197,8 @@ static int ov5645_probe(struct i2c_client *client)
 
 	return 0;
 
+error_subdev_cleanup:
+	v4l2_subdev_cleanup(&ov5645->sd);
 power_down:
 	ov5645_set_power_off(dev);
 free_entity:
@@ -1247,6 +1215,7 @@ static void ov5645_remove(struct i2c_client *client)
 	struct ov5645 *ov5645 = to_ov5645(sd);
 
 	v4l2_async_unregister_subdev(&ov5645->sd);
+	v4l2_subdev_cleanup(sd);
 	media_entity_cleanup(&ov5645->sd.entity);
 	v4l2_ctrl_handler_free(&ov5645->ctrls);
 	pm_runtime_disable(ov5645->dev);
-- 
2.34.1

Re: [PATCH v2 07/11] media: i2c: ov5645: Use subdev active state

[Laurent Pinchart]

Hi Prabhakar,

Thank you for the patch.

On Tue, Sep 10, 2024 at 06:06:06PM +0100, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Port the ov5645 sensor driver to use the subdev active state.
> 
> Move all the format configuration to the subdevice state and simplify
> the format handling, locking and initialization.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> ---
>  drivers/media/i2c/ov5645.c | 109 +++++++++++++------------------------
>  1 file changed, 39 insertions(+), 70 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> index 25c60afcc0ec..9497ec737cb7 100644
> --- a/drivers/media/i2c/ov5645.c
> +++ b/drivers/media/i2c/ov5645.c
> @@ -89,7 +89,6 @@ struct ov5645 {
>  	struct v4l2_subdev sd;
>  	struct media_pad pad;
>  	struct v4l2_fwnode_endpoint ep;
> -	struct v4l2_mbus_framefmt fmt;
>  	struct v4l2_rect crop;
>  	struct clk *xclk;
>  
> @@ -850,49 +849,6 @@ static int ov5645_enum_frame_size(struct v4l2_subdev *subdev,
>  	return 0;
>  }
>  
> -static struct v4l2_mbus_framefmt *
> -__ov5645_get_pad_format(struct ov5645 *ov5645,
> -			struct v4l2_subdev_state *sd_state,
> -			unsigned int pad,
> -			enum v4l2_subdev_format_whence which)
> -{
> -	switch (which) {
> -	case V4L2_SUBDEV_FORMAT_TRY:
> -		return v4l2_subdev_state_get_format(sd_state, pad);
> -	case V4L2_SUBDEV_FORMAT_ACTIVE:
> -		return &ov5645->fmt;
> -	default:
> -		return NULL;
> -	}
> -}
> -
> -static int ov5645_get_format(struct v4l2_subdev *sd,
> -			     struct v4l2_subdev_state *sd_state,
> -			     struct v4l2_subdev_format *format)
> -{
> -	struct ov5645 *ov5645 = to_ov5645(sd);
> -
> -	format->format = *__ov5645_get_pad_format(ov5645, sd_state,
> -						  format->pad,
> -						  format->which);
> -	return 0;
> -}
> -
> -static struct v4l2_rect *
> -__ov5645_get_pad_crop(struct ov5645 *ov5645,
> -		      struct v4l2_subdev_state *sd_state,
> -		      unsigned int pad, enum v4l2_subdev_format_whence which)
> -{
> -	switch (which) {
> -	case V4L2_SUBDEV_FORMAT_TRY:
> -		return v4l2_subdev_state_get_crop(sd_state, pad);
> -	case V4L2_SUBDEV_FORMAT_ACTIVE:
> -		return &ov5645->crop;
> -	default:
> -		return NULL;
> -	}
> -}
> -
>  static int ov5645_set_format(struct v4l2_subdev *sd,
>  			     struct v4l2_subdev_state *sd_state,
>  			     struct v4l2_subdev_format *format)
> @@ -903,33 +859,30 @@ static int ov5645_set_format(struct v4l2_subdev *sd,
>  	const struct ov5645_mode_info *new_mode;
>  	int ret;
>  
> -	__crop = __ov5645_get_pad_crop(ov5645, sd_state, format->pad,
> -				       format->which);
> -
> +	__crop = v4l2_subdev_state_get_crop(sd_state, 0);
>  	new_mode = v4l2_find_nearest_size(ov5645_mode_info_data,
> -			       ARRAY_SIZE(ov5645_mode_info_data),
> -			       width, height,
> -			       format->format.width, format->format.height);
> +					  ARRAY_SIZE(ov5645_mode_info_data),
> +					  width, height, format->format.width,
> +					  format->format.height);
>  
>  	__crop->width = new_mode->width;
>  	__crop->height = new_mode->height;
>  
>  	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> -		ret = v4l2_ctrl_s_ctrl_int64(ov5645->pixel_clock,
> -					     new_mode->pixel_clock);
> +		ret = __v4l2_ctrl_s_ctrl_int64(ov5645->pixel_clock,
> +					       new_mode->pixel_clock);
>  		if (ret < 0)
>  			return ret;
>  
> -		ret = v4l2_ctrl_s_ctrl(ov5645->link_freq,
> -				       new_mode->link_freq);
> +		ret = __v4l2_ctrl_s_ctrl(ov5645->link_freq,
> +					 new_mode->link_freq);
>  		if (ret < 0)
>  			return ret;
>  
>  		ov5645->current_mode = new_mode;
>  	}
>  
> -	__format = __ov5645_get_pad_format(ov5645, sd_state, format->pad,
> -					   format->which);
> +	__format = v4l2_subdev_state_get_format(sd_state, 0);
>  	__format->width = __crop->width;
>  	__format->height = __crop->height;
>  	__format->code = MEDIA_BUS_FMT_UYVY8_1X16;
> @@ -944,11 +897,15 @@ static int ov5645_set_format(struct v4l2_subdev *sd,
>  static int ov5645_init_state(struct v4l2_subdev *subdev,
>  			     struct v4l2_subdev_state *sd_state)
>  {
> -	struct v4l2_subdev_format fmt = { 0 };
> -
> -	fmt.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
> -	fmt.format.width = 1920;
> -	fmt.format.height = 1080;
> +	struct v4l2_subdev_format fmt = {
> +		.which = V4L2_SUBDEV_FORMAT_TRY,
> +		.pad = 0,
> +		.format = {
> +			.code = MEDIA_BUS_FMT_UYVY8_1X16,
> +			.width = ov5645_mode_info_data[1].width,
> +			.height = ov5645_mode_info_data[1].height,
> +		},
> +	};
>  
>  	ov5645_set_format(subdev, sd_state, &fmt);
>  
> @@ -959,25 +916,27 @@ static int ov5645_get_selection(struct v4l2_subdev *sd,
>  			   struct v4l2_subdev_state *sd_state,
>  			   struct v4l2_subdev_selection *sel)
>  {
> -	struct ov5645 *ov5645 = to_ov5645(sd);
> -
>  	if (sel->target != V4L2_SEL_TGT_CROP)
>  		return -EINVAL;
>  
> -	sel->r = *__ov5645_get_pad_crop(ov5645, sd_state, sel->pad,
> -					sel->which);
> +	sel->r = *v4l2_subdev_state_get_crop(sd_state, 0);
>  	return 0;
>  }
>  
>  static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
>  {
>  	struct ov5645 *ov5645 = to_ov5645(subdev);
> +	struct v4l2_subdev_state *state;
>  	int ret;
>  
> +	state = v4l2_subdev_lock_and_get_active_state(&ov5645->sd);
> +
>  	if (enable) {
>  		ret = pm_runtime_resume_and_get(ov5645->dev);
> -		if (ret < 0)
> +		if (ret < 0) {
> +			v4l2_subdev_unlock_state(state);
>  			return ret;
> +		}
>  
>  		ret = ov5645_set_register_array(ov5645,
>  					ov5645->current_mode->data,
> @@ -988,7 +947,7 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
>  				ov5645->current_mode->height);
>  			goto err_rpm_put;
>  		}
> -		ret = v4l2_ctrl_handler_setup(&ov5645->ctrls);
> +		ret = __v4l2_ctrl_handler_setup(&ov5645->ctrls);
>  		if (ret < 0) {
>  			dev_err(ov5645->dev, "could not sync v4l2 controls\n");
>  			goto err_rpm_put;
> @@ -1013,6 +972,7 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
>  		goto stream_off_rpm_put;
>  	}
>  
> +	v4l2_subdev_unlock_state(state);
>  	return 0;
>  
>  err_rpm_put:
> @@ -1022,6 +982,7 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
>  stream_off_rpm_put:
>  	pm_runtime_mark_last_busy(ov5645->dev);
>  	pm_runtime_put_autosuspend(ov5645->dev);
> +	v4l2_subdev_unlock_state(state);
>  	return ret;
>  }
>  
> @@ -1032,7 +993,7 @@ static const struct v4l2_subdev_video_ops ov5645_video_ops = {
>  static const struct v4l2_subdev_pad_ops ov5645_subdev_pad_ops = {
>  	.enum_mbus_code = ov5645_enum_mbus_code,
>  	.enum_frame_size = ov5645_enum_frame_size,
> -	.get_fmt = ov5645_get_format,
> +	.get_fmt = v4l2_subdev_get_fmt,
>  	.set_fmt = ov5645_set_format,
>  	.get_selection = ov5645_get_selection,
>  };
> @@ -1213,12 +1174,17 @@ static int ov5645_probe(struct i2c_client *client)
>  		goto power_down;
>  	}
>  
> -	ov5645_init_state(&ov5645->sd, NULL);
> +	ov5645->sd.state_lock = ov5645->ctrls.lock;
> +	ret = v4l2_subdev_init_finalize(&ov5645->sd);
> +	if (ret < 0) {
> +		dev_err_probe(dev, ret, "subdev init error\n");
> +		goto power_down;
> +	}
>  
>  	ret = v4l2_async_register_subdev_sensor(&ov5645->sd);
>  	if (ret < 0) {
>  		dev_err_probe(dev, ret, "could not register v4l2 device\n");
> -		goto power_down;
> +		goto error_subdev_cleanup;
>  	}
>  
>  	pm_runtime_set_active(dev);
> @@ -1231,6 +1197,8 @@ static int ov5645_probe(struct i2c_client *client)
>  
>  	return 0;
>  
> +error_subdev_cleanup:
> +	v4l2_subdev_cleanup(&ov5645->sd);
>  power_down:
>  	ov5645_set_power_off(dev);
>  free_entity:
> @@ -1247,6 +1215,7 @@ static void ov5645_remove(struct i2c_client *client)
>  	struct ov5645 *ov5645 = to_ov5645(sd);
>  
>  	v4l2_async_unregister_subdev(&ov5645->sd);
> +	v4l2_subdev_cleanup(sd);
>  	media_entity_cleanup(&ov5645->sd.entity);
>  	v4l2_ctrl_handler_free(&ov5645->ctrls);
>  	pm_runtime_disable(ov5645->dev);

-- 
Regards,

Laurent Pinchart

[PATCH v2 08/11] media: i2c: ov5645: Switch to {enable,disable}_streams

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Switch from s_stream to enable_streams and disable_streams callbacks.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/media/i2c/ov5645.c | 90 +++++++++++++++++++-------------------
 1 file changed, 46 insertions(+), 44 deletions(-)

diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index 9497ec737cb7..dc93514608ee 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -923,71 +923,71 @@ static int ov5645_get_selection(struct v4l2_subdev *sd,
 	return 0;
 }
 
-static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
+static int ov5645_enable_streams(struct v4l2_subdev *sd,
+				 struct v4l2_subdev_state *state, u32 pad,
+				 u64 streams_mask)
 {
-	struct ov5645 *ov5645 = to_ov5645(subdev);
-	struct v4l2_subdev_state *state;
+	struct ov5645 *ov5645 = to_ov5645(sd);
 	int ret;
 
-	state = v4l2_subdev_lock_and_get_active_state(&ov5645->sd);
-
-	if (enable) {
-		ret = pm_runtime_resume_and_get(ov5645->dev);
-		if (ret < 0) {
-			v4l2_subdev_unlock_state(state);
-			return ret;
-		}
+	ret = pm_runtime_resume_and_get(ov5645->dev);
+	if (ret < 0)
+		return ret;
 
-		ret = ov5645_set_register_array(ov5645,
+	ret = ov5645_set_register_array(ov5645,
 					ov5645->current_mode->data,
 					ov5645->current_mode->data_size);
-		if (ret < 0) {
-			dev_err(ov5645->dev, "could not set mode %dx%d\n",
-				ov5645->current_mode->width,
-				ov5645->current_mode->height);
-			goto err_rpm_put;
-		}
-		ret = __v4l2_ctrl_handler_setup(&ov5645->ctrls);
-		if (ret < 0) {
-			dev_err(ov5645->dev, "could not sync v4l2 controls\n");
-			goto err_rpm_put;
-		}
-
-		ret = ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x45);
-		if (ret < 0)
-			goto err_rpm_put;
-
-		ret = ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
-				       OV5645_SYSTEM_CTRL0_START);
-		if (ret < 0)
-			goto err_rpm_put;
-	} else {
-		ret = ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x40);
-		if (ret < 0)
-			goto stream_off_rpm_put;
+	if (ret < 0) {
+		dev_err(ov5645->dev, "could not set mode %dx%d\n",
+			ov5645->current_mode->width,
+			ov5645->current_mode->height);
+		goto err_rpm_put;
+	}
+	ret = __v4l2_ctrl_handler_setup(&ov5645->ctrls);
+	if (ret < 0) {
+		dev_err(ov5645->dev, "could not sync v4l2 controls\n");
+		goto err_rpm_put;
+	}
 
-		ret = ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
-				       OV5645_SYSTEM_CTRL0_STOP);
+	ret = ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x45);
+	if (ret < 0)
+		goto err_rpm_put;
 
-		goto stream_off_rpm_put;
-	}
+	ret = ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
+			       OV5645_SYSTEM_CTRL0_START);
+	if (ret < 0)
+		goto err_rpm_put;
 
-	v4l2_subdev_unlock_state(state);
 	return 0;
 
 err_rpm_put:
 	pm_runtime_put_sync(ov5645->dev);
 	return ret;
+}
+
+static int ov5645_disable_streams(struct v4l2_subdev *sd,
+				  struct v4l2_subdev_state *state, u32 pad,
+				  u64 streams_mask)
+{
+	struct ov5645 *ov5645 = to_ov5645(sd);
+	int ret;
+
+	ret = ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x40);
+	if (ret < 0)
+		goto rpm_put;
+
+	ret = ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
+			       OV5645_SYSTEM_CTRL0_STOP);
 
-stream_off_rpm_put:
+rpm_put:
 	pm_runtime_mark_last_busy(ov5645->dev);
 	pm_runtime_put_autosuspend(ov5645->dev);
-	v4l2_subdev_unlock_state(state);
+
 	return ret;
 }
 
 static const struct v4l2_subdev_video_ops ov5645_video_ops = {
-	.s_stream = ov5645_s_stream,
+	.s_stream = v4l2_subdev_s_stream_helper,
 };
 
 static const struct v4l2_subdev_pad_ops ov5645_subdev_pad_ops = {
@@ -996,6 +996,8 @@ static const struct v4l2_subdev_pad_ops ov5645_subdev_pad_ops = {
 	.get_fmt = v4l2_subdev_get_fmt,
 	.set_fmt = ov5645_set_format,
 	.get_selection = ov5645_get_selection,
+	.enable_streams = ov5645_enable_streams,
+	.disable_streams = ov5645_disable_streams,
 };
 
 static const struct v4l2_subdev_core_ops ov5645_core_ops = {
-- 
2.34.1

Re: [PATCH v2 08/11] media: i2c: ov5645: Switch to {enable,disable}_streams

[Laurent Pinchart]

Hi Prabhakar,

Thank you for the patch.

On Tue, Sep 10, 2024 at 06:06:07PM +0100, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Switch from s_stream to enable_streams and disable_streams callbacks.

Here too you should explain why. The "why" is the most important part of
any commit message.

> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> ---
>  drivers/media/i2c/ov5645.c | 90 +++++++++++++++++++-------------------
>  1 file changed, 46 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> index 9497ec737cb7..dc93514608ee 100644
> --- a/drivers/media/i2c/ov5645.c
> +++ b/drivers/media/i2c/ov5645.c
> @@ -923,71 +923,71 @@ static int ov5645_get_selection(struct v4l2_subdev *sd,
>  	return 0;
>  }
>  
> -static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
> +static int ov5645_enable_streams(struct v4l2_subdev *sd,
> +				 struct v4l2_subdev_state *state, u32 pad,
> +				 u64 streams_mask)
>  {
> -	struct ov5645 *ov5645 = to_ov5645(subdev);
> -	struct v4l2_subdev_state *state;
> +	struct ov5645 *ov5645 = to_ov5645(sd);
>  	int ret;
>  
> -	state = v4l2_subdev_lock_and_get_active_state(&ov5645->sd);
> -
> -	if (enable) {
> -		ret = pm_runtime_resume_and_get(ov5645->dev);
> -		if (ret < 0) {
> -			v4l2_subdev_unlock_state(state);
> -			return ret;
> -		}
> +	ret = pm_runtime_resume_and_get(ov5645->dev);
> +	if (ret < 0)
> +		return ret;
>  
> -		ret = ov5645_set_register_array(ov5645,
> +	ret = ov5645_set_register_array(ov5645,
>  					ov5645->current_mode->data,
>  					ov5645->current_mode->data_size);
> -		if (ret < 0) {
> -			dev_err(ov5645->dev, "could not set mode %dx%d\n",
> -				ov5645->current_mode->width,
> -				ov5645->current_mode->height);
> -			goto err_rpm_put;
> -		}
> -		ret = __v4l2_ctrl_handler_setup(&ov5645->ctrls);
> -		if (ret < 0) {
> -			dev_err(ov5645->dev, "could not sync v4l2 controls\n");
> -			goto err_rpm_put;
> -		}
> -
> -		ret = ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x45);
> -		if (ret < 0)
> -			goto err_rpm_put;
> -
> -		ret = ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
> -				       OV5645_SYSTEM_CTRL0_START);
> -		if (ret < 0)
> -			goto err_rpm_put;
> -	} else {
> -		ret = ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x40);
> -		if (ret < 0)
> -			goto stream_off_rpm_put;
> +	if (ret < 0) {
> +		dev_err(ov5645->dev, "could not set mode %dx%d\n",
> +			ov5645->current_mode->width,
> +			ov5645->current_mode->height);
> +		goto err_rpm_put;
> +	}
> +	ret = __v4l2_ctrl_handler_setup(&ov5645->ctrls);
> +	if (ret < 0) {
> +		dev_err(ov5645->dev, "could not sync v4l2 controls\n");
> +		goto err_rpm_put;
> +	}
>  
> -		ret = ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
> -				       OV5645_SYSTEM_CTRL0_STOP);
> +	ret = ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x45);
> +	if (ret < 0)
> +		goto err_rpm_put;
>  
> -		goto stream_off_rpm_put;
> -	}
> +	ret = ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
> +			       OV5645_SYSTEM_CTRL0_START);
> +	if (ret < 0)
> +		goto err_rpm_put;
>  
> -	v4l2_subdev_unlock_state(state);
>  	return 0;
>  
>  err_rpm_put:
>  	pm_runtime_put_sync(ov5645->dev);
>  	return ret;
> +}
> +
> +static int ov5645_disable_streams(struct v4l2_subdev *sd,
> +				  struct v4l2_subdev_state *state, u32 pad,
> +				  u64 streams_mask)
> +{
> +	struct ov5645 *ov5645 = to_ov5645(sd);
> +	int ret;
> +
> +	ret = ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x40);
> +	if (ret < 0)
> +		goto rpm_put;
> +
> +	ret = ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
> +			       OV5645_SYSTEM_CTRL0_STOP);
>  
> -stream_off_rpm_put:
> +rpm_put:
>  	pm_runtime_mark_last_busy(ov5645->dev);
>  	pm_runtime_put_autosuspend(ov5645->dev);
> -	v4l2_subdev_unlock_state(state);
> +
>  	return ret;
>  }
>  
>  static const struct v4l2_subdev_video_ops ov5645_video_ops = {
> -	.s_stream = ov5645_s_stream,
> +	.s_stream = v4l2_subdev_s_stream_helper,
>  };
>  
>  static const struct v4l2_subdev_pad_ops ov5645_subdev_pad_ops = {
> @@ -996,6 +996,8 @@ static const struct v4l2_subdev_pad_ops ov5645_subdev_pad_ops = {
>  	.get_fmt = v4l2_subdev_get_fmt,
>  	.set_fmt = ov5645_set_format,
>  	.get_selection = ov5645_get_selection,
> +	.enable_streams = ov5645_enable_streams,
> +	.disable_streams = ov5645_disable_streams,
>  };
>  
>  static const struct v4l2_subdev_core_ops ov5645_core_ops = {

-- 
Regards,

Laurent Pinchart

[PATCH v2 09/11] media: i2c: ov5645: Add internal image sink pad

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Use the newly added internal pad API to expose the internal
configuration of the sensor to userspace in a standard manner.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/media/i2c/ov5645.c | 107 +++++++++++++++++++++++++++----------
 1 file changed, 79 insertions(+), 28 deletions(-)

diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index dc93514608ee..255c6395a268 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -60,6 +60,10 @@
 #define OV5645_SDE_SAT_U		0x5583
 #define OV5645_SDE_SAT_V		0x5584
 
+#define OV5645_NATIVE_FORMAT	MEDIA_BUS_FMT_SBGGR8_1X8
+#define OV5645_NATIVE_WIDTH	2592
+#define OV5645_NATIVE_HEIGHT	1944
+
 /* regulator supplies */
 static const char * const ov5645_supply_name[] = {
 	"vdddo", /* Digital I/O (1.8V) supply */
@@ -69,6 +73,12 @@ static const char * const ov5645_supply_name[] = {
 
 #define OV5645_NUM_SUPPLIES ARRAY_SIZE(ov5645_supply_name)
 
+enum ov5645_pad_ids {
+	OV5645_PAD_SOURCE,
+	OV5645_PAD_IMAGE,
+	OV5645_NUM_PADS,
+};
+
 struct reg_value {
 	u16 reg;
 	u8 val;
@@ -87,7 +97,7 @@ struct ov5645 {
 	struct i2c_client *i2c_client;
 	struct device *dev;
 	struct v4l2_subdev sd;
-	struct media_pad pad;
+	struct media_pad pads[OV5645_NUM_PADS];
 	struct v4l2_fwnode_endpoint ep;
 	struct v4l2_rect crop;
 	struct clk *xclk;
@@ -826,7 +836,10 @@ static int ov5645_enum_mbus_code(struct v4l2_subdev *sd,
 	if (code->index > 0)
 		return -EINVAL;
 
-	code->code = MEDIA_BUS_FMT_UYVY8_1X16;
+	if (code->pad == OV5645_PAD_IMAGE)
+		code->code = OV5645_NATIVE_FORMAT;
+	else
+		code->code = MEDIA_BUS_FMT_UYVY8_1X16;
 
 	return 0;
 }
@@ -835,16 +848,24 @@ static int ov5645_enum_frame_size(struct v4l2_subdev *subdev,
 				  struct v4l2_subdev_state *sd_state,
 				  struct v4l2_subdev_frame_size_enum *fse)
 {
-	if (fse->code != MEDIA_BUS_FMT_UYVY8_1X16)
-		return -EINVAL;
-
-	if (fse->index >= ARRAY_SIZE(ov5645_mode_info_data))
-		return -EINVAL;
-
-	fse->min_width = ov5645_mode_info_data[fse->index].width;
-	fse->max_width = ov5645_mode_info_data[fse->index].width;
-	fse->min_height = ov5645_mode_info_data[fse->index].height;
-	fse->max_height = ov5645_mode_info_data[fse->index].height;
+	if (fse->pad == OV5645_PAD_IMAGE) {
+		if (fse->code != OV5645_NATIVE_FORMAT || fse->index > 0)
+			return -EINVAL;
+
+		fse->min_width = OV5645_NATIVE_WIDTH;
+		fse->max_width = OV5645_NATIVE_WIDTH;
+		fse->min_height = OV5645_NATIVE_HEIGHT;
+		fse->max_height = OV5645_NATIVE_HEIGHT;
+	} else {
+		if (fse->code != MEDIA_BUS_FMT_UYVY8_1X16 ||
+		    fse->index >= ARRAY_SIZE(ov5645_mode_info_data))
+			return -EINVAL;
+
+		fse->min_width = ov5645_mode_info_data[fse->index].width;
+		fse->max_width = ov5645_mode_info_data[fse->index].width;
+		fse->min_height = ov5645_mode_info_data[fse->index].height;
+		fse->max_height = ov5645_mode_info_data[fse->index].height;
+	}
 
 	return 0;
 }
@@ -855,18 +876,55 @@ static int ov5645_set_format(struct v4l2_subdev *sd,
 {
 	struct ov5645 *ov5645 = to_ov5645(sd);
 	struct v4l2_mbus_framefmt *__format;
+	struct v4l2_rect *compose;
 	struct v4l2_rect *__crop;
 	const struct ov5645_mode_info *new_mode;
 	int ret;
 
-	__crop = v4l2_subdev_state_get_crop(sd_state, 0);
+	if (format->pad != OV5645_PAD_SOURCE)
+		return v4l2_subdev_get_fmt(sd, sd_state, format);
+
 	new_mode = v4l2_find_nearest_size(ov5645_mode_info_data,
 					  ARRAY_SIZE(ov5645_mode_info_data),
 					  width, height, format->format.width,
 					  format->format.height);
-
-	__crop->width = new_mode->width;
-	__crop->height = new_mode->height;
+	format->format.code = MEDIA_BUS_FMT_UYVY8_1X16;
+	format->format.width = new_mode->width;
+	format->format.height = new_mode->height;
+	format->format.field = V4L2_FIELD_NONE;
+	format->format.colorspace = V4L2_COLORSPACE_SRGB;
+	format->format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
+	format->format.quantization = V4L2_QUANTIZATION_DEFAULT;
+	format->format.xfer_func = V4L2_XFER_FUNC_DEFAULT;
+
+	__format = v4l2_subdev_state_get_format(sd_state, OV5645_PAD_IMAGE);
+	*__format = format->format;
+	__format->code = OV5645_NATIVE_FORMAT;
+	__format->width = OV5645_NATIVE_WIDTH;
+	__format->height = OV5645_NATIVE_HEIGHT;
+
+	__crop = v4l2_subdev_state_get_crop(sd_state, OV5645_PAD_IMAGE);
+	__crop->width = format->format.width;
+	__crop->height = format->format.height;
+
+	/*
+	 * The compose rectangle models binning, its size is the sensor output
+	 * size.
+	 */
+	compose = v4l2_subdev_state_get_compose(sd_state, OV5645_PAD_IMAGE);
+	compose->left = 0;
+	compose->top = 0;
+	compose->width = format->format.width;
+	compose->height = format->format.height;
+
+	__crop = v4l2_subdev_state_get_crop(sd_state, OV5645_PAD_SOURCE);
+	__crop->left = 0;
+	__crop->top = 0;
+	__crop->width = format->format.width;
+	__crop->height = format->format.height;
+
+	__format = v4l2_subdev_state_get_format(sd_state, OV5645_PAD_SOURCE);
+	*__format = format->format;
 
 	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
 		ret = __v4l2_ctrl_s_ctrl_int64(ov5645->pixel_clock,
@@ -882,14 +940,6 @@ static int ov5645_set_format(struct v4l2_subdev *sd,
 		ov5645->current_mode = new_mode;
 	}
 
-	__format = v4l2_subdev_state_get_format(sd_state, 0);
-	__format->width = __crop->width;
-	__format->height = __crop->height;
-	__format->code = MEDIA_BUS_FMT_UYVY8_1X16;
-	__format->field = V4L2_FIELD_NONE;
-	__format->colorspace = V4L2_COLORSPACE_SRGB;
-
-	format->format = *__format;
 
 	return 0;
 }
@@ -899,7 +949,7 @@ static int ov5645_init_state(struct v4l2_subdev *subdev,
 {
 	struct v4l2_subdev_format fmt = {
 		.which = V4L2_SUBDEV_FORMAT_TRY,
-		.pad = 0,
+		.pad = OV5645_PAD_SOURCE,
 		.format = {
 			.code = MEDIA_BUS_FMT_UYVY8_1X16,
 			.width = ov5645_mode_info_data[1].width,
@@ -919,7 +969,7 @@ static int ov5645_get_selection(struct v4l2_subdev *sd,
 	if (sel->target != V4L2_SEL_TGT_CROP)
 		return -EINVAL;
 
-	sel->r = *v4l2_subdev_state_get_crop(sd_state, 0);
+	sel->r = *v4l2_subdev_state_get_crop(sd_state, sel->pad);
 	return 0;
 }
 
@@ -1123,11 +1173,12 @@ static int ov5645_probe(struct i2c_client *client)
 	v4l2_i2c_subdev_init(&ov5645->sd, client, &ov5645_subdev_ops);
 	ov5645->sd.internal_ops = &ov5645_internal_ops;
 	ov5645->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
-	ov5645->pad.flags = MEDIA_PAD_FL_SOURCE;
+	ov5645->pads[OV5645_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
+	ov5645->pads[OV5645_PAD_IMAGE].flags = MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_INTERNAL;
 	ov5645->sd.dev = dev;
 	ov5645->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
 
-	ret = media_entity_pads_init(&ov5645->sd.entity, 1, &ov5645->pad);
+	ret = media_entity_pads_init(&ov5645->sd.entity, ARRAY_SIZE(ov5645->pads), ov5645->pads);
 	if (ret < 0) {
 		dev_err_probe(dev, ret, "could not register media entity\n");
 		goto free_ctrl;
-- 
2.34.1

Re: [PATCH v2 09/11] media: i2c: ov5645: Add internal image sink pad

[Laurent Pinchart]

Hi Prabhakar,

Thank you for the patch.

On Tue, Sep 10, 2024 at 06:06:08PM +0100, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Use the newly added internal pad API to expose the internal
> configuration of the sensor to userspace in a standard manner.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  drivers/media/i2c/ov5645.c | 107 +++++++++++++++++++++++++++----------
>  1 file changed, 79 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> index dc93514608ee..255c6395a268 100644
> --- a/drivers/media/i2c/ov5645.c
> +++ b/drivers/media/i2c/ov5645.c
> @@ -60,6 +60,10 @@
>  #define OV5645_SDE_SAT_U		0x5583
>  #define OV5645_SDE_SAT_V		0x5584
>  
> +#define OV5645_NATIVE_FORMAT	MEDIA_BUS_FMT_SBGGR8_1X8
> +#define OV5645_NATIVE_WIDTH	2592
> +#define OV5645_NATIVE_HEIGHT	1944
> +
>  /* regulator supplies */
>  static const char * const ov5645_supply_name[] = {
>  	"vdddo", /* Digital I/O (1.8V) supply */
> @@ -69,6 +73,12 @@ static const char * const ov5645_supply_name[] = {
>  
>  #define OV5645_NUM_SUPPLIES ARRAY_SIZE(ov5645_supply_name)
>  
> +enum ov5645_pad_ids {
> +	OV5645_PAD_SOURCE,
> +	OV5645_PAD_IMAGE,
> +	OV5645_NUM_PADS,
> +};
> +
>  struct reg_value {
>  	u16 reg;
>  	u8 val;
> @@ -87,7 +97,7 @@ struct ov5645 {
>  	struct i2c_client *i2c_client;
>  	struct device *dev;
>  	struct v4l2_subdev sd;
> -	struct media_pad pad;
> +	struct media_pad pads[OV5645_NUM_PADS];
>  	struct v4l2_fwnode_endpoint ep;
>  	struct v4l2_rect crop;
>  	struct clk *xclk;
> @@ -826,7 +836,10 @@ static int ov5645_enum_mbus_code(struct v4l2_subdev *sd,
>  	if (code->index > 0)
>  		return -EINVAL;
>  
> -	code->code = MEDIA_BUS_FMT_UYVY8_1X16;
> +	if (code->pad == OV5645_PAD_IMAGE)
> +		code->code = OV5645_NATIVE_FORMAT;
> +	else
> +		code->code = MEDIA_BUS_FMT_UYVY8_1X16;
>  
>  	return 0;
>  }
> @@ -835,16 +848,24 @@ static int ov5645_enum_frame_size(struct v4l2_subdev *subdev,
>  				  struct v4l2_subdev_state *sd_state,
>  				  struct v4l2_subdev_frame_size_enum *fse)
>  {
> -	if (fse->code != MEDIA_BUS_FMT_UYVY8_1X16)
> -		return -EINVAL;
> -
> -	if (fse->index >= ARRAY_SIZE(ov5645_mode_info_data))
> -		return -EINVAL;
> -
> -	fse->min_width = ov5645_mode_info_data[fse->index].width;
> -	fse->max_width = ov5645_mode_info_data[fse->index].width;
> -	fse->min_height = ov5645_mode_info_data[fse->index].height;
> -	fse->max_height = ov5645_mode_info_data[fse->index].height;
> +	if (fse->pad == OV5645_PAD_IMAGE) {
> +		if (fse->code != OV5645_NATIVE_FORMAT || fse->index > 0)
> +			return -EINVAL;
> +
> +		fse->min_width = OV5645_NATIVE_WIDTH;
> +		fse->max_width = OV5645_NATIVE_WIDTH;
> +		fse->min_height = OV5645_NATIVE_HEIGHT;
> +		fse->max_height = OV5645_NATIVE_HEIGHT;
> +	} else {
> +		if (fse->code != MEDIA_BUS_FMT_UYVY8_1X16 ||

This will be interesting. With internal pads we will introduce the
concept of a "subdev model", which will formally document how the V4L2
subdev configuration items (formats, selection rectangles, ...) maps to
hardware features. Sakari is working on the definition of a subdev model
for raw sensors, that should catter for the needs of raw sensors without
a bayer scaler (the vast majority of them). To use internal pads with a
non-raw sensor, we'll have to define a model. It may be more
challenging/complicated to do so, as the YUV sensor features are less
standardized, but it will be an interesting development.

> +		    fse->index >= ARRAY_SIZE(ov5645_mode_info_data))
> +			return -EINVAL;
> +
> +		fse->min_width = ov5645_mode_info_data[fse->index].width;
> +		fse->max_width = ov5645_mode_info_data[fse->index].width;
> +		fse->min_height = ov5645_mode_info_data[fse->index].height;
> +		fse->max_height = ov5645_mode_info_data[fse->index].height;
> +	}
>  
>  	return 0;
>  }
> @@ -855,18 +876,55 @@ static int ov5645_set_format(struct v4l2_subdev *sd,
>  {
>  	struct ov5645 *ov5645 = to_ov5645(sd);
>  	struct v4l2_mbus_framefmt *__format;
> +	struct v4l2_rect *compose;
>  	struct v4l2_rect *__crop;

While at it, could you rename __crop to crop ?

>  	const struct ov5645_mode_info *new_mode;
>  	int ret;
>  
> -	__crop = v4l2_subdev_state_get_crop(sd_state, 0);
> +	if (format->pad != OV5645_PAD_SOURCE)
> +		return v4l2_subdev_get_fmt(sd, sd_state, format);
> +
>  	new_mode = v4l2_find_nearest_size(ov5645_mode_info_data,
>  					  ARRAY_SIZE(ov5645_mode_info_data),
>  					  width, height, format->format.width,
>  					  format->format.height);
> -
> -	__crop->width = new_mode->width;
> -	__crop->height = new_mode->height;
> +	format->format.code = MEDIA_BUS_FMT_UYVY8_1X16;
> +	format->format.width = new_mode->width;
> +	format->format.height = new_mode->height;
> +	format->format.field = V4L2_FIELD_NONE;
> +	format->format.colorspace = V4L2_COLORSPACE_SRGB;
> +	format->format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> +	format->format.quantization = V4L2_QUANTIZATION_DEFAULT;
> +	format->format.xfer_func = V4L2_XFER_FUNC_DEFAULT;

Drivers are not supposed to return DEFAULT values, you should pick
appropriate values.

> +
> +	__format = v4l2_subdev_state_get_format(sd_state, OV5645_PAD_IMAGE);

Maybe __format could also become fmt.

> +	*__format = format->format;
> +	__format->code = OV5645_NATIVE_FORMAT;
> +	__format->width = OV5645_NATIVE_WIDTH;
> +	__format->height = OV5645_NATIVE_HEIGHT;
> +
> +	__crop = v4l2_subdev_state_get_crop(sd_state, OV5645_PAD_IMAGE);
> +	__crop->width = format->format.width;
> +	__crop->height = format->format.height;
> +
> +	/*
> +	 * The compose rectangle models binning, its size is the sensor output
> +	 * size.
> +	 */
> +	compose = v4l2_subdev_state_get_compose(sd_state, OV5645_PAD_IMAGE);
> +	compose->left = 0;
> +	compose->top = 0;
> +	compose->width = format->format.width;
> +	compose->height = format->format.height;
> +
> +	__crop = v4l2_subdev_state_get_crop(sd_state, OV5645_PAD_SOURCE);
> +	__crop->left = 0;
> +	__crop->top = 0;
> +	__crop->width = format->format.width;
> +	__crop->height = format->format.height;
> +
> +	__format = v4l2_subdev_state_get_format(sd_state, OV5645_PAD_SOURCE);
> +	*__format = format->format;
>  
>  	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
>  		ret = __v4l2_ctrl_s_ctrl_int64(ov5645->pixel_clock,
> @@ -882,14 +940,6 @@ static int ov5645_set_format(struct v4l2_subdev *sd,
>  		ov5645->current_mode = new_mode;
>  	}
>  
> -	__format = v4l2_subdev_state_get_format(sd_state, 0);
> -	__format->width = __crop->width;
> -	__format->height = __crop->height;
> -	__format->code = MEDIA_BUS_FMT_UYVY8_1X16;
> -	__format->field = V4L2_FIELD_NONE;
> -	__format->colorspace = V4L2_COLORSPACE_SRGB;
> -
> -	format->format = *__format;
>  
>  	return 0;
>  }
> @@ -899,7 +949,7 @@ static int ov5645_init_state(struct v4l2_subdev *subdev,
>  {
>  	struct v4l2_subdev_format fmt = {
>  		.which = V4L2_SUBDEV_FORMAT_TRY,
> -		.pad = 0,
> +		.pad = OV5645_PAD_SOURCE,
>  		.format = {
>  			.code = MEDIA_BUS_FMT_UYVY8_1X16,
>  			.width = ov5645_mode_info_data[1].width,
> @@ -919,7 +969,7 @@ static int ov5645_get_selection(struct v4l2_subdev *sd,
>  	if (sel->target != V4L2_SEL_TGT_CROP)
>  		return -EINVAL;
>  
> -	sel->r = *v4l2_subdev_state_get_crop(sd_state, 0);
> +	sel->r = *v4l2_subdev_state_get_crop(sd_state, sel->pad);

Now there's a compose rectangle, you should support getting it.

>  	return 0;
>  }
>  
> @@ -1123,11 +1173,12 @@ static int ov5645_probe(struct i2c_client *client)
>  	v4l2_i2c_subdev_init(&ov5645->sd, client, &ov5645_subdev_ops);
>  	ov5645->sd.internal_ops = &ov5645_internal_ops;
>  	ov5645->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
> -	ov5645->pad.flags = MEDIA_PAD_FL_SOURCE;
> +	ov5645->pads[OV5645_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
> +	ov5645->pads[OV5645_PAD_IMAGE].flags = MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_INTERNAL;
>  	ov5645->sd.dev = dev;
>  	ov5645->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
>  
> -	ret = media_entity_pads_init(&ov5645->sd.entity, 1, &ov5645->pad);
> +	ret = media_entity_pads_init(&ov5645->sd.entity, ARRAY_SIZE(ov5645->pads), ov5645->pads);

Line wrap.

>  	if (ret < 0) {
>  		dev_err_probe(dev, ret, "could not register media entity\n");
>  		goto free_ctrl;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 09/11] media: i2c: ov5645: Add internal image sink pad

[Sakari Ailus]

Hi Laurent, Prabhakar,

On Wed, Sep 25, 2024 at 07:21:53PM +0300, Laurent Pinchart wrote:
> Hi Prabhakar,
> 
> Thank you for the patch.
> 
> On Tue, Sep 10, 2024 at 06:06:08PM +0100, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > 
> > Use the newly added internal pad API to expose the internal
> > configuration of the sensor to userspace in a standard manner.
> > 
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  drivers/media/i2c/ov5645.c | 107 +++++++++++++++++++++++++++----------
> >  1 file changed, 79 insertions(+), 28 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> > index dc93514608ee..255c6395a268 100644
> > --- a/drivers/media/i2c/ov5645.c
> > +++ b/drivers/media/i2c/ov5645.c
> > @@ -60,6 +60,10 @@
> >  #define OV5645_SDE_SAT_U		0x5583
> >  #define OV5645_SDE_SAT_V		0x5584
> >  
> > +#define OV5645_NATIVE_FORMAT	MEDIA_BUS_FMT_SBGGR8_1X8
> > +#define OV5645_NATIVE_WIDTH	2592
> > +#define OV5645_NATIVE_HEIGHT	1944
> > +
> >  /* regulator supplies */
> >  static const char * const ov5645_supply_name[] = {
> >  	"vdddo", /* Digital I/O (1.8V) supply */
> > @@ -69,6 +73,12 @@ static const char * const ov5645_supply_name[] = {
> >  
> >  #define OV5645_NUM_SUPPLIES ARRAY_SIZE(ov5645_supply_name)
> >  
> > +enum ov5645_pad_ids {
> > +	OV5645_PAD_SOURCE,
> > +	OV5645_PAD_IMAGE,
> > +	OV5645_NUM_PADS,
> > +};
> > +
> >  struct reg_value {
> >  	u16 reg;
> >  	u8 val;
> > @@ -87,7 +97,7 @@ struct ov5645 {
> >  	struct i2c_client *i2c_client;
> >  	struct device *dev;
> >  	struct v4l2_subdev sd;
> > -	struct media_pad pad;
> > +	struct media_pad pads[OV5645_NUM_PADS];
> >  	struct v4l2_fwnode_endpoint ep;
> >  	struct v4l2_rect crop;
> >  	struct clk *xclk;
> > @@ -826,7 +836,10 @@ static int ov5645_enum_mbus_code(struct v4l2_subdev *sd,
> >  	if (code->index > 0)
> >  		return -EINVAL;
> >  
> > -	code->code = MEDIA_BUS_FMT_UYVY8_1X16;
> > +	if (code->pad == OV5645_PAD_IMAGE)
> > +		code->code = OV5645_NATIVE_FORMAT;
> > +	else
> > +		code->code = MEDIA_BUS_FMT_UYVY8_1X16;
> >  
> >  	return 0;
> >  }
> > @@ -835,16 +848,24 @@ static int ov5645_enum_frame_size(struct v4l2_subdev *subdev,
> >  				  struct v4l2_subdev_state *sd_state,
> >  				  struct v4l2_subdev_frame_size_enum *fse)
> >  {
> > -	if (fse->code != MEDIA_BUS_FMT_UYVY8_1X16)
> > -		return -EINVAL;
> > -
> > -	if (fse->index >= ARRAY_SIZE(ov5645_mode_info_data))
> > -		return -EINVAL;
> > -
> > -	fse->min_width = ov5645_mode_info_data[fse->index].width;
> > -	fse->max_width = ov5645_mode_info_data[fse->index].width;
> > -	fse->min_height = ov5645_mode_info_data[fse->index].height;
> > -	fse->max_height = ov5645_mode_info_data[fse->index].height;
> > +	if (fse->pad == OV5645_PAD_IMAGE) {
> > +		if (fse->code != OV5645_NATIVE_FORMAT || fse->index > 0)
> > +			return -EINVAL;
> > +
> > +		fse->min_width = OV5645_NATIVE_WIDTH;
> > +		fse->max_width = OV5645_NATIVE_WIDTH;
> > +		fse->min_height = OV5645_NATIVE_HEIGHT;
> > +		fse->max_height = OV5645_NATIVE_HEIGHT;
> > +	} else {
> > +		if (fse->code != MEDIA_BUS_FMT_UYVY8_1X16 ||
> 
> This will be interesting. With internal pads we will introduce the
> concept of a "subdev model", which will formally document how the V4L2
> subdev configuration items (formats, selection rectangles, ...) maps to
> hardware features. Sakari is working on the definition of a subdev model
> for raw sensors, that should catter for the needs of raw sensors without
> a bayer scaler (the vast majority of them). To use internal pads with a
> non-raw sensor, we'll have to define a model. It may be more
> challenging/complicated to do so, as the YUV sensor features are less
> standardized, but it will be an interesting development.

I think I'll make the sub-device model a bitmask, to allow implementing
more than one at the same time.

I'll try to remember to cc you to the patchset when I'll send it, likely
next week.

> 
> > +		    fse->index >= ARRAY_SIZE(ov5645_mode_info_data))
> > +			return -EINVAL;
> > +
> > +		fse->min_width = ov5645_mode_info_data[fse->index].width;
> > +		fse->max_width = ov5645_mode_info_data[fse->index].width;
> > +		fse->min_height = ov5645_mode_info_data[fse->index].height;
> > +		fse->max_height = ov5645_mode_info_data[fse->index].height;
> > +	}
> >  
> >  	return 0;
> >  }
> > @@ -855,18 +876,55 @@ static int ov5645_set_format(struct v4l2_subdev *sd,
> >  {
> >  	struct ov5645 *ov5645 = to_ov5645(sd);
> >  	struct v4l2_mbus_framefmt *__format;
> > +	struct v4l2_rect *compose;
> >  	struct v4l2_rect *__crop;
> 
> While at it, could you rename __crop to crop ?
> 
> >  	const struct ov5645_mode_info *new_mode;
> >  	int ret;
> >  
> > -	__crop = v4l2_subdev_state_get_crop(sd_state, 0);
> > +	if (format->pad != OV5645_PAD_SOURCE)
> > +		return v4l2_subdev_get_fmt(sd, sd_state, format);
> > +
> >  	new_mode = v4l2_find_nearest_size(ov5645_mode_info_data,
> >  					  ARRAY_SIZE(ov5645_mode_info_data),
> >  					  width, height, format->format.width,
> >  					  format->format.height);
> > -
> > -	__crop->width = new_mode->width;
> > -	__crop->height = new_mode->height;
> > +	format->format.code = MEDIA_BUS_FMT_UYVY8_1X16;
> > +	format->format.width = new_mode->width;
> > +	format->format.height = new_mode->height;
> > +	format->format.field = V4L2_FIELD_NONE;
> > +	format->format.colorspace = V4L2_COLORSPACE_SRGB;
> > +	format->format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> > +	format->format.quantization = V4L2_QUANTIZATION_DEFAULT;
> > +	format->format.xfer_func = V4L2_XFER_FUNC_DEFAULT;
> 
> Drivers are not supposed to return DEFAULT values, you should pick
> appropriate values.
> 
> > +
> > +	__format = v4l2_subdev_state_get_format(sd_state, OV5645_PAD_IMAGE);
> 
> Maybe __format could also become fmt.
> 
> > +	*__format = format->format;
> > +	__format->code = OV5645_NATIVE_FORMAT;
> > +	__format->width = OV5645_NATIVE_WIDTH;
> > +	__format->height = OV5645_NATIVE_HEIGHT;
> > +
> > +	__crop = v4l2_subdev_state_get_crop(sd_state, OV5645_PAD_IMAGE);
> > +	__crop->width = format->format.width;
> > +	__crop->height = format->format.height;
> > +
> > +	/*
> > +	 * The compose rectangle models binning, its size is the sensor output
> > +	 * size.
> > +	 */
> > +	compose = v4l2_subdev_state_get_compose(sd_state, OV5645_PAD_IMAGE);
> > +	compose->left = 0;
> > +	compose->top = 0;
> > +	compose->width = format->format.width;
> > +	compose->height = format->format.height;
> > +
> > +	__crop = v4l2_subdev_state_get_crop(sd_state, OV5645_PAD_SOURCE);
> > +	__crop->left = 0;
> > +	__crop->top = 0;
> > +	__crop->width = format->format.width;
> > +	__crop->height = format->format.height;
> > +
> > +	__format = v4l2_subdev_state_get_format(sd_state, OV5645_PAD_SOURCE);
> > +	*__format = format->format;
> >  
> >  	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> >  		ret = __v4l2_ctrl_s_ctrl_int64(ov5645->pixel_clock,
> > @@ -882,14 +940,6 @@ static int ov5645_set_format(struct v4l2_subdev *sd,
> >  		ov5645->current_mode = new_mode;
> >  	}
> >  
> > -	__format = v4l2_subdev_state_get_format(sd_state, 0);
> > -	__format->width = __crop->width;
> > -	__format->height = __crop->height;
> > -	__format->code = MEDIA_BUS_FMT_UYVY8_1X16;
> > -	__format->field = V4L2_FIELD_NONE;
> > -	__format->colorspace = V4L2_COLORSPACE_SRGB;
> > -
> > -	format->format = *__format;
> >  
> >  	return 0;
> >  }
> > @@ -899,7 +949,7 @@ static int ov5645_init_state(struct v4l2_subdev *subdev,
> >  {
> >  	struct v4l2_subdev_format fmt = {
> >  		.which = V4L2_SUBDEV_FORMAT_TRY,
> > -		.pad = 0,
> > +		.pad = OV5645_PAD_SOURCE,
> >  		.format = {
> >  			.code = MEDIA_BUS_FMT_UYVY8_1X16,
> >  			.width = ov5645_mode_info_data[1].width,
> > @@ -919,7 +969,7 @@ static int ov5645_get_selection(struct v4l2_subdev *sd,
> >  	if (sel->target != V4L2_SEL_TGT_CROP)
> >  		return -EINVAL;
> >  
> > -	sel->r = *v4l2_subdev_state_get_crop(sd_state, 0);
> > +	sel->r = *v4l2_subdev_state_get_crop(sd_state, sel->pad);
> 
> Now there's a compose rectangle, you should support getting it.
> 
> >  	return 0;
> >  }
> >  
> > @@ -1123,11 +1173,12 @@ static int ov5645_probe(struct i2c_client *client)
> >  	v4l2_i2c_subdev_init(&ov5645->sd, client, &ov5645_subdev_ops);
> >  	ov5645->sd.internal_ops = &ov5645_internal_ops;
> >  	ov5645->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
> > -	ov5645->pad.flags = MEDIA_PAD_FL_SOURCE;
> > +	ov5645->pads[OV5645_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
> > +	ov5645->pads[OV5645_PAD_IMAGE].flags = MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_INTERNAL;
> >  	ov5645->sd.dev = dev;
> >  	ov5645->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> >  
> > -	ret = media_entity_pads_init(&ov5645->sd.entity, 1, &ov5645->pad);
> > +	ret = media_entity_pads_init(&ov5645->sd.entity, ARRAY_SIZE(ov5645->pads), ov5645->pads);
> 
> Line wrap.

It's good to run:

	scripts/checkpatch.pl --strict --max-line-length=80

on the patches.

> 
> >  	if (ret < 0) {
> >  		dev_err_probe(dev, ret, "could not register media entity\n");
> >  		goto free_ctrl;
> 

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v2 09/11] media: i2c: ov5645: Add internal image sink pad

[Lad, Prabhakar]

Hi Sakari,

On Thu, Sep 26, 2024 at 4:49 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Laurent, Prabhakar,
>
> On Wed, Sep 25, 2024 at 07:21:53PM +0300, Laurent Pinchart wrote:
> > Hi Prabhakar,
> >
> > Thank you for the patch.
> >
> > On Tue, Sep 10, 2024 at 06:06:08PM +0100, Prabhakar wrote:
> > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > >
> > > Use the newly added internal pad API to expose the internal
> > > configuration of the sensor to userspace in a standard manner.
> > >
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > ---
> > >  drivers/media/i2c/ov5645.c | 107 +++++++++++++++++++++++++++----------
> > >  1 file changed, 79 insertions(+), 28 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> > > index dc93514608ee..255c6395a268 100644
> > > --- a/drivers/media/i2c/ov5645.c
> > > +++ b/drivers/media/i2c/ov5645.c
> > > @@ -60,6 +60,10 @@
> > >  #define OV5645_SDE_SAT_U           0x5583
> > >  #define OV5645_SDE_SAT_V           0x5584
> > >
> > > +#define OV5645_NATIVE_FORMAT       MEDIA_BUS_FMT_SBGGR8_1X8
> > > +#define OV5645_NATIVE_WIDTH        2592
> > > +#define OV5645_NATIVE_HEIGHT       1944
> > > +
> > >  /* regulator supplies */
> > >  static const char * const ov5645_supply_name[] = {
> > >     "vdddo", /* Digital I/O (1.8V) supply */
> > > @@ -69,6 +73,12 @@ static const char * const ov5645_supply_name[] = {
> > >
> > >  #define OV5645_NUM_SUPPLIES ARRAY_SIZE(ov5645_supply_name)
> > >
> > > +enum ov5645_pad_ids {
> > > +   OV5645_PAD_SOURCE,
> > > +   OV5645_PAD_IMAGE,
> > > +   OV5645_NUM_PADS,
> > > +};
> > > +
> > >  struct reg_value {
> > >     u16 reg;
> > >     u8 val;
> > > @@ -87,7 +97,7 @@ struct ov5645 {
> > >     struct i2c_client *i2c_client;
> > >     struct device *dev;
> > >     struct v4l2_subdev sd;
> > > -   struct media_pad pad;
> > > +   struct media_pad pads[OV5645_NUM_PADS];
> > >     struct v4l2_fwnode_endpoint ep;
> > >     struct v4l2_rect crop;
> > >     struct clk *xclk;
> > > @@ -826,7 +836,10 @@ static int ov5645_enum_mbus_code(struct v4l2_subdev *sd,
> > >     if (code->index > 0)
> > >             return -EINVAL;
> > >
> > > -   code->code = MEDIA_BUS_FMT_UYVY8_1X16;
> > > +   if (code->pad == OV5645_PAD_IMAGE)
> > > +           code->code = OV5645_NATIVE_FORMAT;
> > > +   else
> > > +           code->code = MEDIA_BUS_FMT_UYVY8_1X16;
> > >
> > >     return 0;
> > >  }
> > > @@ -835,16 +848,24 @@ static int ov5645_enum_frame_size(struct v4l2_subdev *subdev,
> > >                               struct v4l2_subdev_state *sd_state,
> > >                               struct v4l2_subdev_frame_size_enum *fse)
> > >  {
> > > -   if (fse->code != MEDIA_BUS_FMT_UYVY8_1X16)
> > > -           return -EINVAL;
> > > -
> > > -   if (fse->index >= ARRAY_SIZE(ov5645_mode_info_data))
> > > -           return -EINVAL;
> > > -
> > > -   fse->min_width = ov5645_mode_info_data[fse->index].width;
> > > -   fse->max_width = ov5645_mode_info_data[fse->index].width;
> > > -   fse->min_height = ov5645_mode_info_data[fse->index].height;
> > > -   fse->max_height = ov5645_mode_info_data[fse->index].height;
> > > +   if (fse->pad == OV5645_PAD_IMAGE) {
> > > +           if (fse->code != OV5645_NATIVE_FORMAT || fse->index > 0)
> > > +                   return -EINVAL;
> > > +
> > > +           fse->min_width = OV5645_NATIVE_WIDTH;
> > > +           fse->max_width = OV5645_NATIVE_WIDTH;
> > > +           fse->min_height = OV5645_NATIVE_HEIGHT;
> > > +           fse->max_height = OV5645_NATIVE_HEIGHT;
> > > +   } else {
> > > +           if (fse->code != MEDIA_BUS_FMT_UYVY8_1X16 ||
> >
> > This will be interesting. With internal pads we will introduce the
> > concept of a "subdev model", which will formally document how the V4L2
> > subdev configuration items (formats, selection rectangles, ...) maps to
> > hardware features. Sakari is working on the definition of a subdev model
> > for raw sensors, that should catter for the needs of raw sensors without
> > a bayer scaler (the vast majority of them). To use internal pads with a
> > non-raw sensor, we'll have to define a model. It may be more
> > challenging/complicated to do so, as the YUV sensor features are less
> > standardized, but it will be an interesting development.
>
> I think I'll make the sub-device model a bitmask, to allow implementing
> more than one at the same time.
>
> I'll try to remember to cc you to the patchset when I'll send it, likely
> next week.
>
Great, I'll withhold sending a v3.

> >
> > > +               fse->index >= ARRAY_SIZE(ov5645_mode_info_data))
> > > +                   return -EINVAL;
> > > +
> > > +           fse->min_width = ov5645_mode_info_data[fse->index].width;
> > > +           fse->max_width = ov5645_mode_info_data[fse->index].width;
> > > +           fse->min_height = ov5645_mode_info_data[fse->index].height;
> > > +           fse->max_height = ov5645_mode_info_data[fse->index].height;
> > > +   }
> > >
> > >     return 0;
> > >  }
> > > @@ -855,18 +876,55 @@ static int ov5645_set_format(struct v4l2_subdev *sd,
> > >  {
> > >     struct ov5645 *ov5645 = to_ov5645(sd);
> > >     struct v4l2_mbus_framefmt *__format;
> > > +   struct v4l2_rect *compose;
> > >     struct v4l2_rect *__crop;
> >
> > While at it, could you rename __crop to crop ?
> >
> > >     const struct ov5645_mode_info *new_mode;
> > >     int ret;
> > >
> > > -   __crop = v4l2_subdev_state_get_crop(sd_state, 0);
> > > +   if (format->pad != OV5645_PAD_SOURCE)
> > > +           return v4l2_subdev_get_fmt(sd, sd_state, format);
> > > +
> > >     new_mode = v4l2_find_nearest_size(ov5645_mode_info_data,
> > >                                       ARRAY_SIZE(ov5645_mode_info_data),
> > >                                       width, height, format->format.width,
> > >                                       format->format.height);
> > > -
> > > -   __crop->width = new_mode->width;
> > > -   __crop->height = new_mode->height;
> > > +   format->format.code = MEDIA_BUS_FMT_UYVY8_1X16;
> > > +   format->format.width = new_mode->width;
> > > +   format->format.height = new_mode->height;
> > > +   format->format.field = V4L2_FIELD_NONE;
> > > +   format->format.colorspace = V4L2_COLORSPACE_SRGB;
> > > +   format->format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> > > +   format->format.quantization = V4L2_QUANTIZATION_DEFAULT;
> > > +   format->format.xfer_func = V4L2_XFER_FUNC_DEFAULT;
> >
> > Drivers are not supposed to return DEFAULT values, you should pick
> > appropriate values.
> >
> > > +
> > > +   __format = v4l2_subdev_state_get_format(sd_state, OV5645_PAD_IMAGE);
> >
> > Maybe __format could also become fmt.
> >
> > > +   *__format = format->format;
> > > +   __format->code = OV5645_NATIVE_FORMAT;
> > > +   __format->width = OV5645_NATIVE_WIDTH;
> > > +   __format->height = OV5645_NATIVE_HEIGHT;
> > > +
> > > +   __crop = v4l2_subdev_state_get_crop(sd_state, OV5645_PAD_IMAGE);
> > > +   __crop->width = format->format.width;
> > > +   __crop->height = format->format.height;
> > > +
> > > +   /*
> > > +    * The compose rectangle models binning, its size is the sensor output
> > > +    * size.
> > > +    */
> > > +   compose = v4l2_subdev_state_get_compose(sd_state, OV5645_PAD_IMAGE);
> > > +   compose->left = 0;
> > > +   compose->top = 0;
> > > +   compose->width = format->format.width;
> > > +   compose->height = format->format.height;
> > > +
> > > +   __crop = v4l2_subdev_state_get_crop(sd_state, OV5645_PAD_SOURCE);
> > > +   __crop->left = 0;
> > > +   __crop->top = 0;
> > > +   __crop->width = format->format.width;
> > > +   __crop->height = format->format.height;
> > > +
> > > +   __format = v4l2_subdev_state_get_format(sd_state, OV5645_PAD_SOURCE);
> > > +   *__format = format->format;
> > >
> > >     if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > >             ret = __v4l2_ctrl_s_ctrl_int64(ov5645->pixel_clock,
> > > @@ -882,14 +940,6 @@ static int ov5645_set_format(struct v4l2_subdev *sd,
> > >             ov5645->current_mode = new_mode;
> > >     }
> > >
> > > -   __format = v4l2_subdev_state_get_format(sd_state, 0);
> > > -   __format->width = __crop->width;
> > > -   __format->height = __crop->height;
> > > -   __format->code = MEDIA_BUS_FMT_UYVY8_1X16;
> > > -   __format->field = V4L2_FIELD_NONE;
> > > -   __format->colorspace = V4L2_COLORSPACE_SRGB;
> > > -
> > > -   format->format = *__format;
> > >
> > >     return 0;
> > >  }
> > > @@ -899,7 +949,7 @@ static int ov5645_init_state(struct v4l2_subdev *subdev,
> > >  {
> > >     struct v4l2_subdev_format fmt = {
> > >             .which = V4L2_SUBDEV_FORMAT_TRY,
> > > -           .pad = 0,
> > > +           .pad = OV5645_PAD_SOURCE,
> > >             .format = {
> > >                     .code = MEDIA_BUS_FMT_UYVY8_1X16,
> > >                     .width = ov5645_mode_info_data[1].width,
> > > @@ -919,7 +969,7 @@ static int ov5645_get_selection(struct v4l2_subdev *sd,
> > >     if (sel->target != V4L2_SEL_TGT_CROP)
> > >             return -EINVAL;
> > >
> > > -   sel->r = *v4l2_subdev_state_get_crop(sd_state, 0);
> > > +   sel->r = *v4l2_subdev_state_get_crop(sd_state, sel->pad);
> >
> > Now there's a compose rectangle, you should support getting it.
> >
> > >     return 0;
> > >  }
> > >
> > > @@ -1123,11 +1173,12 @@ static int ov5645_probe(struct i2c_client *client)
> > >     v4l2_i2c_subdev_init(&ov5645->sd, client, &ov5645_subdev_ops);
> > >     ov5645->sd.internal_ops = &ov5645_internal_ops;
> > >     ov5645->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
> > > -   ov5645->pad.flags = MEDIA_PAD_FL_SOURCE;
> > > +   ov5645->pads[OV5645_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
> > > +   ov5645->pads[OV5645_PAD_IMAGE].flags = MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_INTERNAL;
> > >     ov5645->sd.dev = dev;
> > >     ov5645->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> > >
> > > -   ret = media_entity_pads_init(&ov5645->sd.entity, 1, &ov5645->pad);
> > > +   ret = media_entity_pads_init(&ov5645->sd.entity, ARRAY_SIZE(ov5645->pads), ov5645->pads);
> >
> > Line wrap.
>
> It's good to run:
>
>         scripts/checkpatch.pl --strict --max-line-length=80
>
> on the patches.
>
Thanks for the pointer.

Cheers,
Prabhakar

[PATCH v2 10/11] media: i2c: ov5645: Report internal routes to userspace

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Usage of internal pads creates a route internal to the subdev, and the
V4L2 camera sensor API requires such routes to be reported to userspace.
Create the route in the .init_state() operation.

Internal routing support requires stream support, so set the
V4L2_SUBDEV_FL_HAS_STREAMS flag and switch formats and selection
rectangles access from pads to streams. As the route is immutable,
there's no need to implement the .set_routing() operation, and we can
hardcode the sink and source stream IDs to 0.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/media/i2c/ov5645.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index 255c6395a268..7f1133292ffc 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -947,15 +947,36 @@ static int ov5645_set_format(struct v4l2_subdev *sd,
 static int ov5645_init_state(struct v4l2_subdev *subdev,
 			     struct v4l2_subdev_state *sd_state)
 {
+	struct v4l2_subdev_route routes[1] = {
+		{
+			.sink_pad = OV5645_PAD_IMAGE,
+			.sink_stream = 0,
+			.source_pad = OV5645_PAD_SOURCE,
+			.source_stream = 0,
+			.flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE |
+				 V4L2_SUBDEV_ROUTE_FL_IMMUTABLE,
+		},
+	};
+	struct v4l2_subdev_krouting routing = {
+		.len_routes = ARRAY_SIZE(routes),
+		.num_routes = ARRAY_SIZE(routes),
+		.routes = routes,
+	};
 	struct v4l2_subdev_format fmt = {
 		.which = V4L2_SUBDEV_FORMAT_TRY,
 		.pad = OV5645_PAD_SOURCE,
+		.stream = 0,
 		.format = {
 			.code = MEDIA_BUS_FMT_UYVY8_1X16,
 			.width = ov5645_mode_info_data[1].width,
 			.height = ov5645_mode_info_data[1].height,
 		},
 	};
+	int ret;
+
+	ret = v4l2_subdev_set_routing(subdev, sd_state, &routing);
+	if (ret)
+		return ret;
 
 	ov5645_set_format(subdev, sd_state, &fmt);
 
@@ -1172,7 +1193,8 @@ static int ov5645_probe(struct i2c_client *client)
 
 	v4l2_i2c_subdev_init(&ov5645->sd, client, &ov5645_subdev_ops);
 	ov5645->sd.internal_ops = &ov5645_internal_ops;
-	ov5645->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
+	ov5645->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS |
+			    V4L2_SUBDEV_FL_STREAMS;
 	ov5645->pads[OV5645_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
 	ov5645->pads[OV5645_PAD_IMAGE].flags = MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_INTERNAL;
 	ov5645->sd.dev = dev;
-- 
2.34.1

[PATCH v2 11/11] media: i2c: ov5645: Report streams using frame descriptors

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Implement the .get_frame_desc() subdev operation to report information
about streams to the connected CSI-2 receiver. This is required to let
the CSI-2 receiver driver know about virtual channels and data types for
each stream.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/media/i2c/ov5645.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index 7f1133292ffc..c24eb6e7a7b5 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -28,6 +28,7 @@
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 #include <linux/types.h>
+#include <media/mipi-csi2.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-event.h>
 #include <media/v4l2-fwnode.h>
@@ -829,6 +830,32 @@ static const struct v4l2_ctrl_ops ov5645_ctrl_ops = {
 	.s_ctrl = ov5645_s_ctrl,
 };
 
+static int ov5645_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
+				 struct v4l2_mbus_frame_desc *fd)
+{
+	const struct v4l2_mbus_framefmt *fmt;
+	struct v4l2_subdev_state *state;
+
+	if (pad != OV5645_PAD_SOURCE)
+		return -EINVAL;
+
+	state = v4l2_subdev_lock_and_get_active_state(sd);
+	fmt = v4l2_subdev_state_get_format(state, OV5645_PAD_SOURCE, 0);
+	v4l2_subdev_unlock_state(state);
+
+	fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
+	fd->num_entries = 1;
+
+	memset(fd->entry, 0, sizeof(fd->entry));
+
+	fd->entry[0].pixelcode = fmt->code;
+	fd->entry[0].stream = 0;
+	fd->entry[0].bus.csi2.vc = 0;
+	fd->entry[0].bus.csi2.dt = MIPI_CSI2_DT_YUV422_8B;
+
+	return 0;
+}
+
 static int ov5645_enum_mbus_code(struct v4l2_subdev *sd,
 				 struct v4l2_subdev_state *sd_state,
 				 struct v4l2_subdev_mbus_code_enum *code)
@@ -1062,6 +1089,7 @@ static const struct v4l2_subdev_video_ops ov5645_video_ops = {
 };
 
 static const struct v4l2_subdev_pad_ops ov5645_subdev_pad_ops = {
+	.get_frame_desc = ov5645_get_frame_desc,
 	.enum_mbus_code = ov5645_enum_mbus_code,
 	.enum_frame_size = ov5645_enum_frame_size,
 	.get_fmt = v4l2_subdev_get_fmt,
-- 
2.34.1

Re: [PATCH v2 11/11] media: i2c: ov5645: Report streams using frame descriptors

[Laurent Pinchart]

Hi Prabhakar,

Thank you for the patch.

On Tue, Sep 10, 2024 at 06:06:10PM +0100, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Implement the .get_frame_desc() subdev operation to report information
> about streams to the connected CSI-2 receiver. This is required to let
> the CSI-2 receiver driver know about virtual channels and data types for
> each stream.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  drivers/media/i2c/ov5645.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> index 7f1133292ffc..c24eb6e7a7b5 100644
> --- a/drivers/media/i2c/ov5645.c
> +++ b/drivers/media/i2c/ov5645.c
> @@ -28,6 +28,7 @@
>  #include <linux/regulator/consumer.h>
>  #include <linux/slab.h>
>  #include <linux/types.h>
> +#include <media/mipi-csi2.h>
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-event.h>
>  #include <media/v4l2-fwnode.h>
> @@ -829,6 +830,32 @@ static const struct v4l2_ctrl_ops ov5645_ctrl_ops = {
>  	.s_ctrl = ov5645_s_ctrl,
>  };
>  
> +static int ov5645_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> +				 struct v4l2_mbus_frame_desc *fd)
> +{
> +	const struct v4l2_mbus_framefmt *fmt;
> +	struct v4l2_subdev_state *state;
> +
> +	if (pad != OV5645_PAD_SOURCE)
> +		return -EINVAL;
> +
> +	state = v4l2_subdev_lock_and_get_active_state(sd);
> +	fmt = v4l2_subdev_state_get_format(state, OV5645_PAD_SOURCE, 0);
> +	v4l2_subdev_unlock_state(state);

Once you unlock the state fmt could change, so you should instead do

	state = v4l2_subdev_lock_and_get_active_state(sd);
	code = v4l2_subdev_state_get_format(state, OV5645_PAD_SOURCE, 0)->code;
	v4l2_subdev_unlock_state(state);

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> +
> +	fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
> +	fd->num_entries = 1;
> +
> +	memset(fd->entry, 0, sizeof(fd->entry));
> +
> +	fd->entry[0].pixelcode = fmt->code;
> +	fd->entry[0].stream = 0;
> +	fd->entry[0].bus.csi2.vc = 0;
> +	fd->entry[0].bus.csi2.dt = MIPI_CSI2_DT_YUV422_8B;
> +
> +	return 0;
> +}
> +
>  static int ov5645_enum_mbus_code(struct v4l2_subdev *sd,
>  				 struct v4l2_subdev_state *sd_state,
>  				 struct v4l2_subdev_mbus_code_enum *code)
> @@ -1062,6 +1089,7 @@ static const struct v4l2_subdev_video_ops ov5645_video_ops = {
>  };
>  
>  static const struct v4l2_subdev_pad_ops ov5645_subdev_pad_ops = {
> +	.get_frame_desc = ov5645_get_frame_desc,
>  	.enum_mbus_code = ov5645_enum_mbus_code,
>  	.enum_frame_size = ov5645_enum_frame_size,
>  	.get_fmt = v4l2_subdev_get_fmt,

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 11/11] media: i2c: ov5645: Report streams using frame descriptors

[Lad, Prabhakar]

Hi Laurent,

Thank you for the review.

On Wed, Sep 25, 2024 at 5:26 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Prabhakar,
>
> Thank you for the patch.
>
> On Tue, Sep 10, 2024 at 06:06:10PM +0100, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Implement the .get_frame_desc() subdev operation to report information
> > about streams to the connected CSI-2 receiver. This is required to let
> > the CSI-2 receiver driver know about virtual channels and data types for
> > each stream.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  drivers/media/i2c/ov5645.c | 28 ++++++++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> >
> > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> > index 7f1133292ffc..c24eb6e7a7b5 100644
> > --- a/drivers/media/i2c/ov5645.c
> > +++ b/drivers/media/i2c/ov5645.c
> > @@ -28,6 +28,7 @@
> >  #include <linux/regulator/consumer.h>
> >  #include <linux/slab.h>
> >  #include <linux/types.h>
> > +#include <media/mipi-csi2.h>
> >  #include <media/v4l2-ctrls.h>
> >  #include <media/v4l2-event.h>
> >  #include <media/v4l2-fwnode.h>
> > @@ -829,6 +830,32 @@ static const struct v4l2_ctrl_ops ov5645_ctrl_ops = {
> >       .s_ctrl = ov5645_s_ctrl,
> >  };
> >
> > +static int ov5645_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> > +                              struct v4l2_mbus_frame_desc *fd)
> > +{
> > +     const struct v4l2_mbus_framefmt *fmt;
> > +     struct v4l2_subdev_state *state;
> > +
> > +     if (pad != OV5645_PAD_SOURCE)
> > +             return -EINVAL;
> > +
> > +     state = v4l2_subdev_lock_and_get_active_state(sd);
> > +     fmt = v4l2_subdev_state_get_format(state, OV5645_PAD_SOURCE, 0);
> > +     v4l2_subdev_unlock_state(state);
>
> Once you unlock the state fmt could change, so you should instead do
>
>         state = v4l2_subdev_lock_and_get_active_state(sd);
>         code = v4l2_subdev_state_get_format(state, OV5645_PAD_SOURCE, 0)->code;
>         v4l2_subdev_unlock_state(state);
>
Ok, I will update the above.

Cheers,
Prabhakar

> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>
> > +
> > +     fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
> > +     fd->num_entries = 1;
> > +
> > +     memset(fd->entry, 0, sizeof(fd->entry));
> > +
> > +     fd->entry[0].pixelcode = fmt->code;
> > +     fd->entry[0].stream = 0;
> > +     fd->entry[0].bus.csi2.vc = 0;
> > +     fd->entry[0].bus.csi2.dt = MIPI_CSI2_DT_YUV422_8B;
> > +
> > +     return 0;
> > +}
> > +
> >  static int ov5645_enum_mbus_code(struct v4l2_subdev *sd,
> >                                struct v4l2_subdev_state *sd_state,
> >                                struct v4l2_subdev_mbus_code_enum *code)
> > @@ -1062,6 +1089,7 @@ static const struct v4l2_subdev_video_ops ov5645_video_ops = {
> >  };
> >
> >  static const struct v4l2_subdev_pad_ops ov5645_subdev_pad_ops = {
> > +     .get_frame_desc = ov5645_get_frame_desc,
> >       .enum_mbus_code = ov5645_enum_mbus_code,
> >       .enum_frame_size = ov5645_enum_frame_size,
> >       .get_fmt = v4l2_subdev_get_fmt,
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH v2 11/11] media: i2c: ov5645: Report streams using frame descriptors

[Sakari Ailus]

Hi Prabhakar,

Thanks for the set. It looks largely very nice to me, after addressing
Laurent's comments. A few comments here and possibly on other patches...

On Tue, Sep 10, 2024 at 06:06:10PM +0100, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Implement the .get_frame_desc() subdev operation to report information
> about streams to the connected CSI-2 receiver. This is required to let
> the CSI-2 receiver driver know about virtual channels and data types for
> each stream.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  drivers/media/i2c/ov5645.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> index 7f1133292ffc..c24eb6e7a7b5 100644
> --- a/drivers/media/i2c/ov5645.c
> +++ b/drivers/media/i2c/ov5645.c
> @@ -28,6 +28,7 @@
>  #include <linux/regulator/consumer.h>
>  #include <linux/slab.h>
>  #include <linux/types.h>
> +#include <media/mipi-csi2.h>
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-event.h>
>  #include <media/v4l2-fwnode.h>
> @@ -829,6 +830,32 @@ static const struct v4l2_ctrl_ops ov5645_ctrl_ops = {
>  	.s_ctrl = ov5645_s_ctrl,
>  };
>  
> +static int ov5645_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> +				 struct v4l2_mbus_frame_desc *fd)
> +{
> +	const struct v4l2_mbus_framefmt *fmt;
> +	struct v4l2_subdev_state *state;
> +
> +	if (pad != OV5645_PAD_SOURCE)
> +		return -EINVAL;

As you have a single source pad, and pretty much all sensor drivers will, I
think it'd be nice to add a check for this (that it's not an internal pad)
to the caller side in v4l2-subdev.c. And of course drop this one.

> +
> +	state = v4l2_subdev_lock_and_get_active_state(sd);
> +	fmt = v4l2_subdev_state_get_format(state, OV5645_PAD_SOURCE, 0);
> +	v4l2_subdev_unlock_state(state);
> +
> +	fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
> +	fd->num_entries = 1;
> +
> +	memset(fd->entry, 0, sizeof(fd->entry));
> +
> +	fd->entry[0].pixelcode = fmt->code;
> +	fd->entry[0].stream = 0;
> +	fd->entry[0].bus.csi2.vc = 0;
> +	fd->entry[0].bus.csi2.dt = MIPI_CSI2_DT_YUV422_8B;
> +
> +	return 0;
> +}
> +
>  static int ov5645_enum_mbus_code(struct v4l2_subdev *sd,
>  				 struct v4l2_subdev_state *sd_state,
>  				 struct v4l2_subdev_mbus_code_enum *code)
> @@ -1062,6 +1089,7 @@ static const struct v4l2_subdev_video_ops ov5645_video_ops = {
>  };
>  
>  static const struct v4l2_subdev_pad_ops ov5645_subdev_pad_ops = {
> +	.get_frame_desc = ov5645_get_frame_desc,
>  	.enum_mbus_code = ov5645_enum_mbus_code,
>  	.enum_frame_size = ov5645_enum_frame_size,
>  	.get_fmt = v4l2_subdev_get_fmt,
> 

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v2 11/11] media: i2c: ov5645: Report streams using frame descriptors

[Laurent Pinchart]

On Thu, Sep 26, 2024 at 03:45:26PM +0000, Sakari Ailus wrote:
> Hi Prabhakar,
> 
> Thanks for the set. It looks largely very nice to me, after addressing
> Laurent's comments. A few comments here and possibly on other patches...
> 
> On Tue, Sep 10, 2024 at 06:06:10PM +0100, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > 
> > Implement the .get_frame_desc() subdev operation to report information
> > about streams to the connected CSI-2 receiver. This is required to let
> > the CSI-2 receiver driver know about virtual channels and data types for
> > each stream.
> > 
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  drivers/media/i2c/ov5645.c | 28 ++++++++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> > 
> > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> > index 7f1133292ffc..c24eb6e7a7b5 100644
> > --- a/drivers/media/i2c/ov5645.c
> > +++ b/drivers/media/i2c/ov5645.c
> > @@ -28,6 +28,7 @@
> >  #include <linux/regulator/consumer.h>
> >  #include <linux/slab.h>
> >  #include <linux/types.h>
> > +#include <media/mipi-csi2.h>
> >  #include <media/v4l2-ctrls.h>
> >  #include <media/v4l2-event.h>
> >  #include <media/v4l2-fwnode.h>
> > @@ -829,6 +830,32 @@ static const struct v4l2_ctrl_ops ov5645_ctrl_ops = {
> >  	.s_ctrl = ov5645_s_ctrl,
> >  };
> >  
> > +static int ov5645_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> > +				 struct v4l2_mbus_frame_desc *fd)
> > +{
> > +	const struct v4l2_mbus_framefmt *fmt;
> > +	struct v4l2_subdev_state *state;
> > +
> > +	if (pad != OV5645_PAD_SOURCE)
> > +		return -EINVAL;
> 
> As you have a single source pad, and pretty much all sensor drivers will, I
> think it'd be nice to add a check for this (that it's not an internal pad)
> to the caller side in v4l2-subdev.c. And of course drop this one.

What check would you add, just verifying that the pad is a source pad ?

> > +
> > +	state = v4l2_subdev_lock_and_get_active_state(sd);
> > +	fmt = v4l2_subdev_state_get_format(state, OV5645_PAD_SOURCE, 0);
> > +	v4l2_subdev_unlock_state(state);
> > +
> > +	fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
> > +	fd->num_entries = 1;
> > +
> > +	memset(fd->entry, 0, sizeof(fd->entry));
> > +
> > +	fd->entry[0].pixelcode = fmt->code;
> > +	fd->entry[0].stream = 0;
> > +	fd->entry[0].bus.csi2.vc = 0;
> > +	fd->entry[0].bus.csi2.dt = MIPI_CSI2_DT_YUV422_8B;
> > +
> > +	return 0;
> > +}
> > +
> >  static int ov5645_enum_mbus_code(struct v4l2_subdev *sd,
> >  				 struct v4l2_subdev_state *sd_state,
> >  				 struct v4l2_subdev_mbus_code_enum *code)
> > @@ -1062,6 +1089,7 @@ static const struct v4l2_subdev_video_ops ov5645_video_ops = {
> >  };
> >  
> >  static const struct v4l2_subdev_pad_ops ov5645_subdev_pad_ops = {
> > +	.get_frame_desc = ov5645_get_frame_desc,
> >  	.enum_mbus_code = ov5645_enum_mbus_code,
> >  	.enum_frame_size = ov5645_enum_frame_size,
> >  	.get_fmt = v4l2_subdev_get_fmt,
> > 

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 11/11] media: i2c: ov5645: Report streams using frame descriptors

[Sakari Ailus]

On Thu, Sep 26, 2024 at 08:48:19PM +0300, Laurent Pinchart wrote:
> On Thu, Sep 26, 2024 at 03:45:26PM +0000, Sakari Ailus wrote:
> > Hi Prabhakar,
> > 
> > Thanks for the set. It looks largely very nice to me, after addressing
> > Laurent's comments. A few comments here and possibly on other patches...
> > 
> > On Tue, Sep 10, 2024 at 06:06:10PM +0100, Prabhakar wrote:
> > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > 
> > > Implement the .get_frame_desc() subdev operation to report information
> > > about streams to the connected CSI-2 receiver. This is required to let
> > > the CSI-2 receiver driver know about virtual channels and data types for
> > > each stream.
> > > 
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > ---
> > >  drivers/media/i2c/ov5645.c | 28 ++++++++++++++++++++++++++++
> > >  1 file changed, 28 insertions(+)
> > > 
> > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> > > index 7f1133292ffc..c24eb6e7a7b5 100644
> > > --- a/drivers/media/i2c/ov5645.c
> > > +++ b/drivers/media/i2c/ov5645.c
> > > @@ -28,6 +28,7 @@
> > >  #include <linux/regulator/consumer.h>
> > >  #include <linux/slab.h>
> > >  #include <linux/types.h>
> > > +#include <media/mipi-csi2.h>
> > >  #include <media/v4l2-ctrls.h>
> > >  #include <media/v4l2-event.h>
> > >  #include <media/v4l2-fwnode.h>
> > > @@ -829,6 +830,32 @@ static const struct v4l2_ctrl_ops ov5645_ctrl_ops = {
> > >  	.s_ctrl = ov5645_s_ctrl,
> > >  };
> > >  
> > > +static int ov5645_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> > > +				 struct v4l2_mbus_frame_desc *fd)
> > > +{
> > > +	const struct v4l2_mbus_framefmt *fmt;
> > > +	struct v4l2_subdev_state *state;
> > > +
> > > +	if (pad != OV5645_PAD_SOURCE)
> > > +		return -EINVAL;
> > 
> > As you have a single source pad, and pretty much all sensor drivers will, I
> > think it'd be nice to add a check for this (that it's not an internal pad)
> > to the caller side in v4l2-subdev.c. And of course drop this one.
> 
> What check would you add, just verifying that the pad is a source pad ?

I think you could add that, too, besides the absence of the internal flag.

-- 
Sakari Ailus

Re: [PATCH v2 11/11] media: i2c: ov5645: Report streams using frame descriptors

[Lad, Prabhakar]

Hi Sakari and Laurent,

On Thu, Sep 26, 2024 at 7:57 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> On Thu, Sep 26, 2024 at 08:48:19PM +0300, Laurent Pinchart wrote:
> > On Thu, Sep 26, 2024 at 03:45:26PM +0000, Sakari Ailus wrote:
> > > Hi Prabhakar,
> > >
> > > Thanks for the set. It looks largely very nice to me, after addressing
> > > Laurent's comments. A few comments here and possibly on other patches...
> > >
> > > On Tue, Sep 10, 2024 at 06:06:10PM +0100, Prabhakar wrote:
> > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > >
> > > > Implement the .get_frame_desc() subdev operation to report information
> > > > about streams to the connected CSI-2 receiver. This is required to let
> > > > the CSI-2 receiver driver know about virtual channels and data types for
> > > > each stream.
> > > >
> > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > ---
> > > >  drivers/media/i2c/ov5645.c | 28 ++++++++++++++++++++++++++++
> > > >  1 file changed, 28 insertions(+)
> > > >
> > > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> > > > index 7f1133292ffc..c24eb6e7a7b5 100644
> > > > --- a/drivers/media/i2c/ov5645.c
> > > > +++ b/drivers/media/i2c/ov5645.c
> > > > @@ -28,6 +28,7 @@
> > > >  #include <linux/regulator/consumer.h>
> > > >  #include <linux/slab.h>
> > > >  #include <linux/types.h>
> > > > +#include <media/mipi-csi2.h>
> > > >  #include <media/v4l2-ctrls.h>
> > > >  #include <media/v4l2-event.h>
> > > >  #include <media/v4l2-fwnode.h>
> > > > @@ -829,6 +830,32 @@ static const struct v4l2_ctrl_ops ov5645_ctrl_ops = {
> > > >   .s_ctrl = ov5645_s_ctrl,
> > > >  };
> > > >
> > > > +static int ov5645_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> > > > +                          struct v4l2_mbus_frame_desc *fd)
> > > > +{
> > > > + const struct v4l2_mbus_framefmt *fmt;
> > > > + struct v4l2_subdev_state *state;
> > > > +
> > > > + if (pad != OV5645_PAD_SOURCE)
> > > > +         return -EINVAL;
> > >
> > > As you have a single source pad, and pretty much all sensor drivers will, I
> > > think it'd be nice to add a check for this (that it's not an internal pad)
> > > to the caller side in v4l2-subdev.c. And of course drop this one.
> >
> > What check would you add, just verifying that the pad is a source pad ?
>
> I think you could add that, too, besides the absence of the internal flag.
>
Checking only for the source flag should suffice, as the
MEDIA_PAD_FL_INTERNAL flag cannot be set for a source pad because
media_entity_pads_init() enforces this restriction.

Do you agree?

Cheers,
Prabhakar

Re: [PATCH v2 11/11] media: i2c: ov5645: Report streams using frame descriptors

[Sakari Ailus]

Hi Prabhakar,

On Fri, Sep 27, 2024 at 04:31:31PM +0100, Lad, Prabhakar wrote:
> Hi Sakari and Laurent,
> 
> On Thu, Sep 26, 2024 at 7:57 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > On Thu, Sep 26, 2024 at 08:48:19PM +0300, Laurent Pinchart wrote:
> > > On Thu, Sep 26, 2024 at 03:45:26PM +0000, Sakari Ailus wrote:
> > > > Hi Prabhakar,
> > > >
> > > > Thanks for the set. It looks largely very nice to me, after addressing
> > > > Laurent's comments. A few comments here and possibly on other patches...
> > > >
> > > > On Tue, Sep 10, 2024 at 06:06:10PM +0100, Prabhakar wrote:
> > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > >
> > > > > Implement the .get_frame_desc() subdev operation to report information
> > > > > about streams to the connected CSI-2 receiver. This is required to let
> > > > > the CSI-2 receiver driver know about virtual channels and data types for
> > > > > each stream.
> > > > >
> > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > ---
> > > > >  drivers/media/i2c/ov5645.c | 28 ++++++++++++++++++++++++++++
> > > > >  1 file changed, 28 insertions(+)
> > > > >
> > > > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> > > > > index 7f1133292ffc..c24eb6e7a7b5 100644
> > > > > --- a/drivers/media/i2c/ov5645.c
> > > > > +++ b/drivers/media/i2c/ov5645.c
> > > > > @@ -28,6 +28,7 @@
> > > > >  #include <linux/regulator/consumer.h>
> > > > >  #include <linux/slab.h>
> > > > >  #include <linux/types.h>
> > > > > +#include <media/mipi-csi2.h>
> > > > >  #include <media/v4l2-ctrls.h>
> > > > >  #include <media/v4l2-event.h>
> > > > >  #include <media/v4l2-fwnode.h>
> > > > > @@ -829,6 +830,32 @@ static const struct v4l2_ctrl_ops ov5645_ctrl_ops = {
> > > > >   .s_ctrl = ov5645_s_ctrl,
> > > > >  };
> > > > >
> > > > > +static int ov5645_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> > > > > +                          struct v4l2_mbus_frame_desc *fd)
> > > > > +{
> > > > > + const struct v4l2_mbus_framefmt *fmt;
> > > > > + struct v4l2_subdev_state *state;
> > > > > +
> > > > > + if (pad != OV5645_PAD_SOURCE)
> > > > > +         return -EINVAL;
> > > >
> > > > As you have a single source pad, and pretty much all sensor drivers will, I
> > > > think it'd be nice to add a check for this (that it's not an internal pad)
> > > > to the caller side in v4l2-subdev.c. And of course drop this one.
> > >
> > > What check would you add, just verifying that the pad is a source pad ?
> >
> > I think you could add that, too, besides the absence of the internal flag.
> >
> Checking only for the source flag should suffice, as the
> MEDIA_PAD_FL_INTERNAL flag cannot be set for a source pad because
> media_entity_pads_init() enforces this restriction.
> 
> Do you agree?

Works for me.

-- 
Sakari Ailus