[PATCH v3 1/1] media: pci: ipu3-cio2: Obtain remote pad from endpoint

[PATCH v3 1/1] media: pci: ipu3-cio2: Obtain remote pad from endpoint

[Sakari Ailus]

Use the endpoint fwnode to find out the remote pad, instead of using the
first source pad found. Also improve error messages.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
Oh well. Hopefully this is final then.

since v2:
- Use remote fwnode for finding the remote pad, not local.

 drivers/media/pci/intel/ipu3/ipu3-cio2-main.c | 28 ++++++++-----------
 1 file changed, 12 insertions(+), 16 deletions(-)

diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
index 39a8022eec396..2743ecc9b8e4b 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
@@ -1418,31 +1418,27 @@ static int cio2_notifier_complete(struct v4l2_async_notifier *notifier)
 	struct sensor_async_subdev *s_asd;
 	struct v4l2_async_connection *asd;
 	struct cio2_queue *q;
-	unsigned int pad;
 	int ret;
 
 	list_for_each_entry(asd, &cio2->notifier.done_list, asc_entry) {
 		s_asd = to_sensor_asd(asd);
 		q = &cio2->queue[s_asd->csi2.port];
 
-		for (pad = 0; pad < q->sensor->entity.num_pads; pad++)
-			if (q->sensor->entity.pads[pad].flags &
-						MEDIA_PAD_FL_SOURCE)
-				break;
-
-		if (pad == q->sensor->entity.num_pads) {
-			dev_err(dev, "failed to find src pad for %s\n",
-				q->sensor->name);
-			return -ENXIO;
+		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, pad,
-				&q->subdev.entity, CIO2_PAD_SINK,
-				0);
+		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\n",
-				q->sensor->name);
+			dev_err(dev, "failed to create link for %s (endpoint %pfw, error %d)\n",
+				q->sensor->name, s_asd->asd.match.fwnode, ret);
 			return ret;
 		}
 	}
-- 
2.30.2

Re: [PATCH v3 1/1] media: pci: ipu3-cio2: Obtain remote pad from endpoint

[Bingbu Cao]

Reviewed-by: Bingbu Cao <bingbu.cao@intel.com>

On 5/15/23 8:21 PM, Sakari Ailus wrote:
> Use the endpoint fwnode to find out the remote pad, instead of using the
> first source pad found. Also improve error messages.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> Oh well. Hopefully this is final then.
> 
> since v2:
> - Use remote fwnode for finding the remote pad, not local.
> 
>  drivers/media/pci/intel/ipu3/ipu3-cio2-main.c | 28 ++++++++-----------
>  1 file changed, 12 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> index 39a8022eec396..2743ecc9b8e4b 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> @@ -1418,31 +1418,27 @@ static int cio2_notifier_complete(struct v4l2_async_notifier *notifier)
>  	struct sensor_async_subdev *s_asd;
>  	struct v4l2_async_connection *asd;
>  	struct cio2_queue *q;
> -	unsigned int pad;
>  	int ret;
>  
>  	list_for_each_entry(asd, &cio2->notifier.done_list, asc_entry) {
>  		s_asd = to_sensor_asd(asd);
>  		q = &cio2->queue[s_asd->csi2.port];
>  
> -		for (pad = 0; pad < q->sensor->entity.num_pads; pad++)
> -			if (q->sensor->entity.pads[pad].flags &
> -						MEDIA_PAD_FL_SOURCE)
> -				break;
> -
> -		if (pad == q->sensor->entity.num_pads) {
> -			dev_err(dev, "failed to find src pad for %s\n",
> -				q->sensor->name);
> -			return -ENXIO;
> +		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, pad,
> -				&q->subdev.entity, CIO2_PAD_SINK,
> -				0);
> +		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\n",
> -				q->sensor->name);
> +			dev_err(dev, "failed to create link for %s (endpoint %pfw, error %d)\n",
> +				q->sensor->name, s_asd->asd.match.fwnode, ret);
>  			return ret;
>  		}
>  	}
> 

-- 
Best regards,
Bingbu Cao

Re: [PATCH v3 1/1] media: pci: ipu3-cio2: Obtain remote pad from endpoint

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Mon, May 15, 2023 at 03:21:27PM +0300, Sakari Ailus wrote:
> Use the endpoint fwnode to find out the remote pad, instead of using the
> first source pad found. Also improve error messages.

The commit message should explain *why*. Once I know why, I'll review
the patch :-)

> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> Oh well. Hopefully this is final then.
> 
> since v2:
> - Use remote fwnode for finding the remote pad, not local.
> 
>  drivers/media/pci/intel/ipu3/ipu3-cio2-main.c | 28 ++++++++-----------
>  1 file changed, 12 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> index 39a8022eec396..2743ecc9b8e4b 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> @@ -1418,31 +1418,27 @@ static int cio2_notifier_complete(struct v4l2_async_notifier *notifier)
>  	struct sensor_async_subdev *s_asd;
>  	struct v4l2_async_connection *asd;
>  	struct cio2_queue *q;
> -	unsigned int pad;
>  	int ret;
>  
>  	list_for_each_entry(asd, &cio2->notifier.done_list, asc_entry) {
>  		s_asd = to_sensor_asd(asd);
>  		q = &cio2->queue[s_asd->csi2.port];
>  
> -		for (pad = 0; pad < q->sensor->entity.num_pads; pad++)
> -			if (q->sensor->entity.pads[pad].flags &
> -						MEDIA_PAD_FL_SOURCE)
> -				break;
> -
> -		if (pad == q->sensor->entity.num_pads) {
> -			dev_err(dev, "failed to find src pad for %s\n",
> -				q->sensor->name);
> -			return -ENXIO;
> +		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, pad,
> -				&q->subdev.entity, CIO2_PAD_SINK,
> -				0);
> +		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\n",
> -				q->sensor->name);
> +			dev_err(dev, "failed to create link for %s (endpoint %pfw, error %d)\n",
> +				q->sensor->name, s_asd->asd.match.fwnode, ret);
>  			return ret;
>  		}
>  	}

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 1/1] media: pci: ipu3-cio2: Obtain remote pad from endpoint

[Sakari Ailus]

Hi Laurent,

On Fri, Jun 02, 2023 at 12:12:12PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Mon, May 15, 2023 at 03:21:27PM +0300, Sakari Ailus wrote:
> > Use the endpoint fwnode to find out the remote pad, instead of using the
> > first source pad found. Also improve error messages.
> 
> The commit message should explain *why*. Once I know why, I'll review
> the patch :-)

I thought it'd be trivial. :-)

Using framework functions instead of open coding this in drivers, and using
the pad related to the endpoint fwnode instead of just the first pad found.

I'll add this to the commit message.

-- 
Sakari Ailus

Re: [PATCH v3 1/1] media: pci: ipu3-cio2: Obtain remote pad from endpoint

[Laurent Pinchart]

Hi Sakari,

On Fri, Jun 02, 2023 at 02:14:08PM +0000, Sakari Ailus wrote:
> On Fri, Jun 02, 2023 at 12:12:12PM +0300, Laurent Pinchart wrote:
> > On Mon, May 15, 2023 at 03:21:27PM +0300, Sakari Ailus wrote:
> > > Use the endpoint fwnode to find out the remote pad, instead of using the
> > > first source pad found. Also improve error messages.
> > 
> > The commit message should explain *why*. Once I know why, I'll review
> > the patch :-)
> 
> I thought it'd be trivial. :-)

It may appear trivial as a patch author, but reviewers don't have the
context, so they shouldn't be expected to guess the intent.

> Using framework functions instead of open coding this in drivers, and using
> the pad related to the endpoint fwnode instead of just the first pad found.
> 
> I'll add this to the commit message.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 1/1] media: pci: ipu3-cio2: Obtain remote pad from endpoint

[Sakari Ailus]

Hi Laurent,

On Fri, Jun 02, 2023 at 05:23:03PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Fri, Jun 02, 2023 at 02:14:08PM +0000, Sakari Ailus wrote:
> > On Fri, Jun 02, 2023 at 12:12:12PM +0300, Laurent Pinchart wrote:
> > > On Mon, May 15, 2023 at 03:21:27PM +0300, Sakari Ailus wrote:
> > > > Use the endpoint fwnode to find out the remote pad, instead of using the
> > > > first source pad found. Also improve error messages.
> > > 
> > > The commit message should explain *why*. Once I know why, I'll review
> > > the patch :-)
> > 
> > I thought it'd be trivial. :-)
> 
> It may appear trivial as a patch author, but reviewers don't have the
> context, so they shouldn't be expected to guess the intent.

This is actually already in the stage tree. Let's see what we decide to do
with that one.

-- 
Sakari Ailus

Re: [PATCH v3 1/1] media: pci: ipu3-cio2: Obtain remote pad from endpoint

[Sakari Ailus]

On Fri, Jun 02, 2023 at 02:14:08PM +0000, Sakari Ailus wrote:
> Hi Laurent,
> 
> On Fri, Jun 02, 2023 at 12:12:12PM +0300, Laurent Pinchart wrote:
> > Hi Sakari,
> > 
> > Thank you for the patch.
> > 
> > On Mon, May 15, 2023 at 03:21:27PM +0300, Sakari Ailus wrote:
> > > Use the endpoint fwnode to find out the remote pad, instead of using the
> > > first source pad found. Also improve error messages.
> > 
> > The commit message should explain *why*. Once I know why, I'll review
> > the patch :-)
> 
> I thought it'd be trivial. :-)
> 
> Using framework functions instead of open coding this in drivers, and using
> the pad related to the endpoint fwnode instead of just the first pad found.
> 
> I'll add this to the commit message.

Actually this is already in the media tree, let's see what we end up doing
with it.

-- 
Sakari Ailus