Re: [PATCH v4 4/6] media: rcar-vin: Prepare for unifying all v4l-async notifiers

["Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>]

Hej Sakari,

On 2025-06-07 11:13:55 +0000, Sakari Ailus wrote:
> Hejssan!
> 
> On Fri, Jun 06, 2025 at 03:50:00PM +0200, Niklas Söderlund wrote:
> > Hej Sakari,
> > 
> > Thanks for your feedback.
> 
> Var så god!
> 
> > 
> > On 2025-05-27 07:01:47 +0000, Sakari Ailus wrote:
> > > Hej Niklas,
> > > 
> > > On Wed, May 21, 2025 at 03:20:35PM +0200, Niklas Söderlund wrote:
> > > > The R-Car VIN driver is needless complex and uses more then one
> > > 
> > > s/needless\K/ly/
> > > 
> > > > v4l-async notifier to attach to all its subdevices. Prepare for unifying
> > > > them by moving rvin_parallel_parse_of() to where it needs to be when
> > > > they are unified.
> > > > 
> > > > The function is moved verbatim and there is no change in behavior.
> > > > 
> > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > > ---
> > > >  .../platform/renesas/rcar-vin/rcar-core.c     | 106 +++++++++---------
> > > >  1 file changed, 53 insertions(+), 53 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-core.c b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> > > > index d9ad56fb2aa9..60ec57d73a12 100644
> > > > --- a/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> > > > +++ b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> > > > @@ -337,6 +337,59 @@ static void rvin_group_notifier_cleanup(struct rvin_dev *vin)
> > > >  	}
> > > >  }
> > > >  
> > > > +static int rvin_parallel_parse_of(struct rvin_dev *vin)
> > > > +{
> > > > +	struct fwnode_handle *ep, *fwnode;
> > > > +	struct v4l2_fwnode_endpoint vep = {
> > > > +		.bus_type = V4L2_MBUS_UNKNOWN,
> > > > +	};
> > > > +	struct v4l2_async_connection *asc;
> > > > +	int ret;
> > > > +
> > > > +	ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(vin->dev), 0, 0, 0);
> > > > +	if (!ep)
> > > > +		return 0;
> > > > +
> > > > +	fwnode = fwnode_graph_get_remote_endpoint(ep);
> > > > +	ret = v4l2_fwnode_endpoint_parse(ep, &vep);
> > > > +	fwnode_handle_put(ep);
> > > > +	if (ret) {
> > > > +		vin_err(vin, "Failed to parse %pOF\n", to_of_node(fwnode));
> > > > +		ret = -EINVAL;
> > > > +		goto out;
> > > > +	}
> > > > +
> > > > +	switch (vep.bus_type) {
> > > > +	case V4L2_MBUS_PARALLEL:
> > > > +	case V4L2_MBUS_BT656:
> > > > +		vin_dbg(vin, "Found %s media bus\n",
> > > > +			vep.bus_type == V4L2_MBUS_PARALLEL ?
> > > > +			"PARALLEL" : "BT656");
> > > > +		vin->parallel.mbus_type = vep.bus_type;
> > > > +		vin->parallel.bus = vep.bus.parallel;
> > > > +		break;
> > > > +	default:
> > > > +		vin_err(vin, "Unknown media bus type\n");
> > > > +		ret = -EINVAL;
> > > > +		goto out;
> > > > +	}
> > > > +
> > > > +	asc = v4l2_async_nf_add_fwnode(&vin->notifier, fwnode,
> > > > +				       struct v4l2_async_connection);
> > > 
> > > If you use v4l2_async_nf_add_fwnode_remote() here, you can omit
> > > fwnode_graph_get_remote_endpoint() call above. Also the error handling
> > > becomes more simple.
> > 
> > Indeed it would, but I do use fwnode in the debug print at the end of 
> > the function. And I do find that print out use-full when debugging, so I 
> > would like to keep it.
> 
> The drivers really shouldn't have a need for this. How about adding that
> debug print to the V4L2 async framework instead? I think it might be useful
> for other drivers as well even though the information is available via
> debugfs (or sysfs?) already.

That is a good idea, I will try to find time and move the debug prints 
(I have other similar ones in the driver) to the framework in follow up 
work.

Or if debugfs is enough for my needs I can drop them all together! My 
primary use-case is to get a grips of what's going on from bug reports 
sent to me so I can't always poke around on a live system.

> 
> > 
> > Laurent's suggestion of using __free(fwnode_handle) can instead be used 
> > to make error handling easier, and since it would be needed for the ep 
> > variable anyhow I think I will try that.
> 
> Sounds good to me.
> 
> > 
> > > 
> > > > +	if (IS_ERR(asc)) {
> > > > +		ret = PTR_ERR(asc);
> > > > +		goto out;
> > > > +	}
> > > > +
> > > > +	vin->parallel.asc = asc;
> > > > +
> > > > +	vin_dbg(vin, "Add parallel OF device %pOF\n", to_of_node(fwnode));
> 
> Please use %pfw instead for the fwnode (at least for the possible V4L2
> async patch).

ack.

> 
> > > > +out:
> > > > +	fwnode_handle_put(fwnode);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > >  static int rvin_group_notifier_init(struct rvin_dev *vin, unsigned int port,
> > > >  				    unsigned int max_id)
> > > >  {
> > > > @@ -635,59 +688,6 @@ static const struct v4l2_async_notifier_operations rvin_parallel_notify_ops = {
> > > >  	.complete = rvin_parallel_notify_complete,
> > > >  };
> > > >  
> > > > -static int rvin_parallel_parse_of(struct rvin_dev *vin)
> > > > -{
> > > > -	struct fwnode_handle *ep, *fwnode;
> > > > -	struct v4l2_fwnode_endpoint vep = {
> > > > -		.bus_type = V4L2_MBUS_UNKNOWN,
> > > > -	};
> > > > -	struct v4l2_async_connection *asc;
> > > > -	int ret;
> > > > -
> > > > -	ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(vin->dev), 0, 0, 0);
> > > > -	if (!ep)
> > > > -		return 0;
> > > > -
> > > > -	fwnode = fwnode_graph_get_remote_endpoint(ep);
> > > > -	ret = v4l2_fwnode_endpoint_parse(ep, &vep);
> > > > -	fwnode_handle_put(ep);
> > > > -	if (ret) {
> > > > -		vin_err(vin, "Failed to parse %pOF\n", to_of_node(fwnode));
> > > > -		ret = -EINVAL;
> > > > -		goto out;
> > > > -	}
> > > > -
> > > > -	switch (vep.bus_type) {
> > > > -	case V4L2_MBUS_PARALLEL:
> > > > -	case V4L2_MBUS_BT656:
> > > > -		vin_dbg(vin, "Found %s media bus\n",
> > > > -			vep.bus_type == V4L2_MBUS_PARALLEL ?
> > > > -			"PARALLEL" : "BT656");
> > > > -		vin->parallel.mbus_type = vep.bus_type;
> > > > -		vin->parallel.bus = vep.bus.parallel;
> > > > -		break;
> > > > -	default:
> > > > -		vin_err(vin, "Unknown media bus type\n");
> > > > -		ret = -EINVAL;
> > > > -		goto out;
> > > > -	}
> > > > -
> > > > -	asc = v4l2_async_nf_add_fwnode(&vin->notifier, fwnode,
> > > > -				       struct v4l2_async_connection);
> > > > -	if (IS_ERR(asc)) {
> > > > -		ret = PTR_ERR(asc);
> > > > -		goto out;
> > > > -	}
> > > > -
> > > > -	vin->parallel.asc = asc;
> > > > -
> > > > -	vin_dbg(vin, "Add parallel OF device %pOF\n", to_of_node(fwnode));
> > > > -out:
> > > > -	fwnode_handle_put(fwnode);
> > > > -
> > > > -	return ret;
> > > > -}
> > > > -
> > > >  static void rvin_parallel_cleanup(struct rvin_dev *vin)
> > > >  {
> > > >  	v4l2_async_nf_unregister(&vin->notifier);
> 
> -- 
> Med vänliga hälsningar,
> 
> Sakari Ailus

-- 
Kind Regards,
Niklas Söderlund