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

[Bingbu Cao <bingbu.cao@linux.intel.com>]

Sakari,

Thanks for your patch.

On 5/8/23 2:41 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>
> ---
> since v1:
> 
> - Drop pad variable.
> 
> - Wrap long lines.
> 
>  drivers/media/pci/intel/ipu3/ipu3-cio2-main.c | 31 +++++++++----------
>  1 file changed, 15 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 3c84cb1216320..c5d00c929bfac 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> @@ -1373,6 +1373,7 @@ static const struct v4l2_subdev_ops cio2_subdev_ops = {
>  struct sensor_async_subdev {
>  	struct v4l2_async_subdev asd;
>  	struct csi2_bus_info csi2;
> +	struct fwnode_endpoint endpoint;
>  };
>  
>  #define to_sensor_asd(asd)	container_of(asd, struct sensor_async_subdev, asd)
> @@ -1417,31 +1418,28 @@ static int cio2_notifier_complete(struct v4l2_async_notifier *notifier)
>  	struct sensor_async_subdev *s_asd;
>  	struct v4l2_async_subdev *asd;
>  	struct cio2_queue *q;
> -	unsigned int pad;
>  	int ret;
>  
>  	list_for_each_entry(asd, &cio2->notifier.asd_list, asd_list) {
>  		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->endpoint.local_fwnode,
> +						  MEDIA_PAD_FL_SOURCE);
> +		if (ret < 0) {
> +			dev_err(dev, "no endpoint for %pfw (%d)\n",
> +				s_asd->endpoint.local_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);

Is there some cases that sensor has multiple source pads?

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

Over 80, but it looks fine ;).

> +				q->sensor->name, s_asd->endpoint.local_fwnode,
> +				ret);
>  			return ret;
>  		}
>  	}
> @@ -1485,6 +1483,7 @@ static int cio2_parse_firmware(struct cio2_device *cio2)
>  			goto err_parse;
>  		}
>  
> +		s_asd->endpoint = vep.base;
>  		s_asd->csi2.port = vep.base.port;
>  		s_asd->csi2.lanes = vep.bus.mipi_csi2.num_data_lanes;
>  
> 

-- 
Best regards,
Bingbu Cao