[PATCH 0/3] Miscellaneous small things

[PATCH 0/3] Miscellaneous small things

[Sakari Ailus]

Hi folks,

This set does three things: refactor link creation in ipu3-cio2 driver by
using v4l2_fwnode_create_links_for_pad(), add debug prints in
v4l2_fwnode_create_links_for_pad() and drop an unneeded debug print in
media_relase().

Sakari Ailus (3):
  media: ipu3-cio2: Further clean up async subdev link creation
  media: v4l2-mc: Add debug prints for
    v4l2_fwnode_create_links_for_pad()
  media: mc: Drop useless debug print on file handle release

 drivers/media/mc/mc-devnode.c            |  1 -
 drivers/media/pci/intel/ipu3/ipu3-cio2.c | 22 +++++-----------------
 drivers/media/v4l2-core/v4l2-mc.c        | 21 +++++++++++++++++----
 3 files changed, 22 insertions(+), 22 deletions(-)

-- 
2.39.2

[PATCH 1/3] media: ipu3-cio2: Further clean up async subdev link creation

[Sakari Ailus]

Use v4l2_create_fwnode_links_to_pad() to create links from async
sub-devices to the CSI-2 receiver subdevs.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> # Renesas RZ/G2L SMARC
---
 drivers/media/pci/intel/ipu3/ipu3-cio2.c | 22 +++++-----------------
 1 file changed, 5 insertions(+), 17 deletions(-)

diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
index ed08bf4178f0..83e29c56fe33 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
@@ -28,6 +28,7 @@
 #include <media/v4l2-device.h>
 #include <media/v4l2-event.h>
 #include <media/v4l2-fwnode.h>
+#include <media/v4l2-mc.h>
 #include <media/v4l2-ioctl.h>
 #include <media/videobuf2-dma-sg.h>
 
@@ -1407,7 +1408,6 @@ static void cio2_notifier_unbind(struct v4l2_async_notifier *notifier,
 static int cio2_notifier_complete(struct v4l2_async_notifier *notifier)
 {
 	struct cio2_device *cio2 = to_cio2_device(notifier);
-	struct device *dev = &cio2->pci_dev->dev;
 	struct sensor_async_subdev *s_asd;
 	struct v4l2_async_connection *asd;
 	struct cio2_queue *q;
@@ -1417,23 +1417,10 @@ static int cio2_notifier_complete(struct v4l2_async_notifier *notifier)
 		s_asd = to_sensor_asd(asd);
 		q = &cio2->queue[s_asd->csi2.port];
 
-		ret = media_entity_get_fwnode_pad(&q->sensor->entity,
-						  s_asd->asd.match.fwnode,
-						  MEDIA_PAD_FL_SOURCE);
-		if (ret < 0) {
-			dev_err(dev, "no pad for endpoint %pfw (%d)\n",
-				s_asd->asd.match.fwnode, ret);
-			return ret;
-		}
-
-		ret = media_create_pad_link(&q->sensor->entity, ret,
-					    &q->subdev.entity, CIO2_PAD_SINK,
-					    0);
-		if (ret) {
-			dev_err(dev, "failed to create link for %s (endpoint %pfw, error %d)\n",
-				q->sensor->name, s_asd->asd.match.fwnode, ret);
+		ret = v4l2_create_fwnode_links_to_pad(asd->sd,
+						      &q->subdev_pads[CIO2_PAD_SINK], 0);
+		if (ret)
 			return ret;
-		}
 	}
 
 	return v4l2_device_register_subdev_nodes(&cio2->v4l2_dev);
@@ -1572,6 +1559,7 @@ static int cio2_queue_init(struct cio2_device *cio2, struct cio2_queue *q)
 	v4l2_subdev_init(subdev, &cio2_subdev_ops);
 	subdev->flags = V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
 	subdev->owner = THIS_MODULE;
+	subdev->dev = dev;
 	snprintf(subdev->name, sizeof(subdev->name),
 		 CIO2_ENTITY_NAME " %td", q - cio2->queue);
 	subdev->entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
-- 
2.39.2

Re: [PATCH 1/3] media: ipu3-cio2: Further clean up async subdev link creation

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Thu, Dec 21, 2023 at 03:58:20PM +0200, Sakari Ailus wrote:
> Use v4l2_create_fwnode_links_to_pad() to create links from async
> sub-devices to the CSI-2 receiver subdevs.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> # Renesas RZ/G2L SMARC

This doesn't look right, I don't think this patch could have been
meaningfully tested on a Renesas platform :-) With the line dropped,

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

> ---
>  drivers/media/pci/intel/ipu3/ipu3-cio2.c | 22 +++++-----------------
>  1 file changed, 5 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> index ed08bf4178f0..83e29c56fe33 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> @@ -28,6 +28,7 @@
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-event.h>
>  #include <media/v4l2-fwnode.h>
> +#include <media/v4l2-mc.h>
>  #include <media/v4l2-ioctl.h>
>  #include <media/videobuf2-dma-sg.h>
>  
> @@ -1407,7 +1408,6 @@ static void cio2_notifier_unbind(struct v4l2_async_notifier *notifier,
>  static int cio2_notifier_complete(struct v4l2_async_notifier *notifier)
>  {
>  	struct cio2_device *cio2 = to_cio2_device(notifier);
> -	struct device *dev = &cio2->pci_dev->dev;
>  	struct sensor_async_subdev *s_asd;
>  	struct v4l2_async_connection *asd;
>  	struct cio2_queue *q;
> @@ -1417,23 +1417,10 @@ static int cio2_notifier_complete(struct v4l2_async_notifier *notifier)
>  		s_asd = to_sensor_asd(asd);
>  		q = &cio2->queue[s_asd->csi2.port];
>  
> -		ret = media_entity_get_fwnode_pad(&q->sensor->entity,
> -						  s_asd->asd.match.fwnode,
> -						  MEDIA_PAD_FL_SOURCE);
> -		if (ret < 0) {
> -			dev_err(dev, "no pad for endpoint %pfw (%d)\n",
> -				s_asd->asd.match.fwnode, ret);
> -			return ret;
> -		}
> -
> -		ret = media_create_pad_link(&q->sensor->entity, ret,
> -					    &q->subdev.entity, CIO2_PAD_SINK,
> -					    0);
> -		if (ret) {
> -			dev_err(dev, "failed to create link for %s (endpoint %pfw, error %d)\n",
> -				q->sensor->name, s_asd->asd.match.fwnode, ret);
> +		ret = v4l2_create_fwnode_links_to_pad(asd->sd,
> +						      &q->subdev_pads[CIO2_PAD_SINK], 0);
> +		if (ret)
>  			return ret;
> -		}
>  	}
>  
>  	return v4l2_device_register_subdev_nodes(&cio2->v4l2_dev);
> @@ -1572,6 +1559,7 @@ static int cio2_queue_init(struct cio2_device *cio2, struct cio2_queue *q)
>  	v4l2_subdev_init(subdev, &cio2_subdev_ops);
>  	subdev->flags = V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
>  	subdev->owner = THIS_MODULE;
> +	subdev->dev = dev;
>  	snprintf(subdev->name, sizeof(subdev->name),
>  		 CIO2_ENTITY_NAME " %td", q - cio2->queue);
>  	subdev->entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/3] media: ipu3-cio2: Further clean up async subdev link creation

[Sakari Ailus]

Hi Laurent,

On Thu, Dec 21, 2023 at 06:57:23PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Thu, Dec 21, 2023 at 03:58:20PM +0200, Sakari Ailus wrote:
> > Use v4l2_create_fwnode_links_to_pad() to create links from async
> > sub-devices to the CSI-2 receiver subdevs.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> # Renesas RZ/G2L SMARC
> 
> This doesn't look right, I don't think this patch could have been
> meaningfully tested on a Renesas platform :-) With the line dropped,

Oops. I intended to remove this but forgot... The patch used to be in a
branch that hosted a patchset tested by Prabhakar and adding the Tested-by:
mistakenly was applied to this patch by a script.

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

Thanks!

-- 
Regards,

Sakari Ailus

[PATCH 2/3] media: v4l2-mc: Add debug prints for v4l2_fwnode_create_links_for_pad()

[Sakari Ailus]

Add relevant debug prints for v4l2_fwnode_create_links_for_pad(). This
should help debugging when things go wrong.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> # Renesas RZ/G2L SMARC
---
 drivers/media/v4l2-core/v4l2-mc.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-mc.c b/drivers/media/v4l2-core/v4l2-mc.c
index 52d349e72b8c..b1a6246580ec 100644
--- a/drivers/media/v4l2-core/v4l2-mc.c
+++ b/drivers/media/v4l2-core/v4l2-mc.c
@@ -337,12 +337,18 @@ int v4l2_create_fwnode_links_to_pad(struct v4l2_subdev *src_sd,
 		src_idx = media_entity_get_fwnode_pad(&src_sd->entity,
 						      endpoint,
 						      MEDIA_PAD_FL_SOURCE);
-		if (src_idx < 0)
+		if (src_idx < 0) {
+			dev_dbg(src_sd->dev, "no pad found for %pfw\n",
+				endpoint);
 			continue;
+		}
 
 		remote_ep = fwnode_graph_get_remote_endpoint(endpoint);
-		if (!remote_ep)
+		if (!remote_ep) {
+			dev_dbg(src_sd->dev, "no remote ep found for %pfw\n",
+				endpoint);
 			continue;
+		}
 
 		/*
 		 * ask the sink to verify it owns the remote endpoint,
@@ -353,8 +359,12 @@ int v4l2_create_fwnode_links_to_pad(struct v4l2_subdev *src_sd,
 						       MEDIA_PAD_FL_SINK);
 		fwnode_handle_put(remote_ep);
 
-		if (sink_idx < 0 || sink_idx != sink->index)
+		if (sink_idx < 0 || sink_idx != sink->index) {
+			dev_dbg(src_sd->dev,
+				"sink pad index mismatch or error (was %d, expected %u)\n",
+				sink_idx, sink->index);
 			continue;
+		}
 
 		/*
 		 * the source endpoint corresponds to one of its source pads,
@@ -367,8 +377,11 @@ int v4l2_create_fwnode_links_to_pad(struct v4l2_subdev *src_sd,
 		src = &src_sd->entity.pads[src_idx];
 
 		/* skip if link already exists */
-		if (media_entity_find_link(src, sink))
+		if (media_entity_find_link(src, sink)) {
+			dev_dbg(src_sd->dev, "link from %pfw to %pfw exists\n",
+				endpoint, remote_ep);
 			continue;
+		}
 
 		dev_dbg(src_sd->dev, "creating link %s:%d -> %s:%d\n",
 			src_sd->entity.name, src_idx,
-- 
2.39.2

Re: [PATCH 2/3] media: v4l2-mc: Add debug prints for v4l2_fwnode_create_links_for_pad()

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Thu, Dec 21, 2023 at 03:58:21PM +0200, Sakari Ailus wrote:
> Add relevant debug prints for v4l2_fwnode_create_links_for_pad(). This
> should help debugging when things go wrong.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> # Renesas RZ/G2L SMARC
> ---
>  drivers/media/v4l2-core/v4l2-mc.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-mc.c b/drivers/media/v4l2-core/v4l2-mc.c
> index 52d349e72b8c..b1a6246580ec 100644
> --- a/drivers/media/v4l2-core/v4l2-mc.c
> +++ b/drivers/media/v4l2-core/v4l2-mc.c
> @@ -337,12 +337,18 @@ int v4l2_create_fwnode_links_to_pad(struct v4l2_subdev *src_sd,
>  		src_idx = media_entity_get_fwnode_pad(&src_sd->entity,
>  						      endpoint,
>  						      MEDIA_PAD_FL_SOURCE);
> -		if (src_idx < 0)
> +		if (src_idx < 0) {
> +			dev_dbg(src_sd->dev, "no pad found for %pfw\n",
> +				endpoint);
>  			continue;
> +		}
>  
>  		remote_ep = fwnode_graph_get_remote_endpoint(endpoint);
> -		if (!remote_ep)
> +		if (!remote_ep) {
> +			dev_dbg(src_sd->dev, "no remote ep found for %pfw\n",
> +				endpoint);
>  			continue;
> +		}
>  
>  		/*
>  		 * ask the sink to verify it owns the remote endpoint,
> @@ -353,8 +359,12 @@ int v4l2_create_fwnode_links_to_pad(struct v4l2_subdev *src_sd,
>  						       MEDIA_PAD_FL_SINK);
>  		fwnode_handle_put(remote_ep);
>  
> -		if (sink_idx < 0 || sink_idx != sink->index)
> +		if (sink_idx < 0 || sink_idx != sink->index) {
> +			dev_dbg(src_sd->dev,
> +				"sink pad index mismatch or error (was %d, expected %u)\n",

s/was/is/

> +				sink_idx, sink->index);
>  			continue;
> +		}
>  
>  		/*
>  		 * the source endpoint corresponds to one of its source pads,
> @@ -367,8 +377,11 @@ int v4l2_create_fwnode_links_to_pad(struct v4l2_subdev *src_sd,
>  		src = &src_sd->entity.pads[src_idx];
>  
>  		/* skip if link already exists */
> -		if (media_entity_find_link(src, sink))
> +		if (media_entity_find_link(src, sink)) {
> +			dev_dbg(src_sd->dev, "link from %pfw to %pfw exists\n",
> +				endpoint, remote_ep);

Should this be printed using the entity names and pad numbers instead,
as done below ?

>  			continue;
> +		}
>  
>  		dev_dbg(src_sd->dev, "creating link %s:%d -> %s:%d\n",
>  			src_sd->entity.name, src_idx,

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/3] media: v4l2-mc: Add debug prints for v4l2_fwnode_create_links_for_pad()

[Sakari Ailus]

Hi Laurent,

Thanks for the review.

On Thu, Dec 21, 2023 at 06:45:34PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Thu, Dec 21, 2023 at 03:58:21PM +0200, Sakari Ailus wrote:
> > Add relevant debug prints for v4l2_fwnode_create_links_for_pad(). This
> > should help debugging when things go wrong.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> # Renesas RZ/G2L SMARC
> > ---
> >  drivers/media/v4l2-core/v4l2-mc.c | 21 +++++++++++++++++----
> >  1 file changed, 17 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-mc.c b/drivers/media/v4l2-core/v4l2-mc.c
> > index 52d349e72b8c..b1a6246580ec 100644
> > --- a/drivers/media/v4l2-core/v4l2-mc.c
> > +++ b/drivers/media/v4l2-core/v4l2-mc.c
> > @@ -337,12 +337,18 @@ int v4l2_create_fwnode_links_to_pad(struct v4l2_subdev *src_sd,
> >  		src_idx = media_entity_get_fwnode_pad(&src_sd->entity,
> >  						      endpoint,
> >  						      MEDIA_PAD_FL_SOURCE);
> > -		if (src_idx < 0)
> > +		if (src_idx < 0) {
> > +			dev_dbg(src_sd->dev, "no pad found for %pfw\n",
> > +				endpoint);
> >  			continue;
> > +		}
> >  
> >  		remote_ep = fwnode_graph_get_remote_endpoint(endpoint);
> > -		if (!remote_ep)
> > +		if (!remote_ep) {
> > +			dev_dbg(src_sd->dev, "no remote ep found for %pfw\n",
> > +				endpoint);
> >  			continue;
> > +		}
> >  
> >  		/*
> >  		 * ask the sink to verify it owns the remote endpoint,
> > @@ -353,8 +359,12 @@ int v4l2_create_fwnode_links_to_pad(struct v4l2_subdev *src_sd,
> >  						       MEDIA_PAD_FL_SINK);
> >  		fwnode_handle_put(remote_ep);
> >  
> > -		if (sink_idx < 0 || sink_idx != sink->index)
> > +		if (sink_idx < 0 || sink_idx != sink->index) {
> > +			dev_dbg(src_sd->dev,
> > +				"sink pad index mismatch or error (was %d, expected %u)\n",
> 
> s/was/is/
> 
> > +				sink_idx, sink->index);
> >  			continue;
> > +		}
> >  
> >  		/*
> >  		 * the source endpoint corresponds to one of its source pads,
> > @@ -367,8 +377,11 @@ int v4l2_create_fwnode_links_to_pad(struct v4l2_subdev *src_sd,
> >  		src = &src_sd->entity.pads[src_idx];
> >  
> >  		/* skip if link already exists */
> > -		if (media_entity_find_link(src, sink))
> > +		if (media_entity_find_link(src, sink)) {
> > +			dev_dbg(src_sd->dev, "link from %pfw to %pfw exists\n",
> > +				endpoint, remote_ep);
> 
> Should this be printed using the entity names and pad numbers instead,
> as done below ?

I'll address these in v2.

> 
> >  			continue;
> > +		}
> >  
> >  		dev_dbg(src_sd->dev, "creating link %s:%d -> %s:%d\n",
> >  			src_sd->entity.name, src_idx,

-- 
Regards,

Sakari Ailus

[PATCH 3/3] media: mc: Drop useless debug print on file handle release

[Sakari Ailus]

Drop a debug print in media_release(), which is a release callback for a
file handle. Printing a debug message here is simply not necessary.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/mc/mc-devnode.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
index 680fbb3a9340..9c8fe9335dc1 100644
--- a/drivers/media/mc/mc-devnode.c
+++ b/drivers/media/mc/mc-devnode.c
@@ -190,7 +190,6 @@ static int media_release(struct inode *inode, struct file *filp)
 	   return value is ignored. */
 	put_device(&devnode->dev);
 
-	pr_debug("%s: Media Release\n", __func__);
 	return 0;
 }
 
-- 
2.39.2

Re: [PATCH 3/3] media: mc: Drop useless debug print on file handle release

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Thu, Dec 21, 2023 at 03:58:22PM +0200, Sakari Ailus wrote:
> Drop a debug print in media_release(), which is a release callback for a
> file handle. Printing a debug message here is simply not necessary.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

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

> ---
>  drivers/media/mc/mc-devnode.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
> index 680fbb3a9340..9c8fe9335dc1 100644
> --- a/drivers/media/mc/mc-devnode.c
> +++ b/drivers/media/mc/mc-devnode.c
> @@ -190,7 +190,6 @@ static int media_release(struct inode *inode, struct file *filp)
>  	   return value is ignored. */
>  	put_device(&devnode->dev);
>  
> -	pr_debug("%s: Media Release\n", __func__);
>  	return 0;
>  }
>  

-- 
Regards,

Laurent Pinchart