mbox series

[0/5] v4l2-fwnode: Remove v4l2_async_notifier_parse_fwnode_endpoints_by_port() helper

Message ID 20201125164450.2056963-1-niklas.soderlund+renesas@ragnatech.se
Headers show
Series v4l2-fwnode: Remove v4l2_async_notifier_parse_fwnode_endpoints_by_port() helper | expand

Message

Niklas Söderlund Nov. 25, 2020, 4:44 p.m. UTC
Hello,

This series aims to remove a V4L2 helper that provides a too simple 
implementation, v4l2_async_notifier_parse_fwnode_endpoints_by_port().  
Instead drivers shall implement what the helper does directly to get 
greater control of the process. There is only one user left in tree of 
this interface, R-Car VIN.

This series starts therefor to rework the R-Car VIN driver to not depend 
on the helper. And in the process a small fwnode imbalance is fixed.  
After the last user of the helper is reworked the series removes the 
helper to prevent usage of it to resurface.

This series is based on-top of the latest media tree and is tested on 
Renesas R-Car M3-N and Koelsch without any regressions or change in 
behavior detected.

Niklas Söderlund (5):
  rcar-vin: Only dynamically allocate v4l2_async_subdev
  rcar-vin: Rework parallel firmware parsing
  rcar-vin: Use v4l2_async_subdev instead of fwnode_handle to match
    subdevices
  rcar-vin: Rework CSI-2 firmware parsing
  v4l2-fwnode: Remove
    v4l2_async_notifier_parse_fwnode_endpoints_by_port()

 drivers/media/platform/rcar-vin/rcar-core.c | 156 ++++++++++++--------
 drivers/media/platform/rcar-vin/rcar-dma.c  |  16 +-
 drivers/media/platform/rcar-vin/rcar-v4l2.c |  12 +-
 drivers/media/platform/rcar-vin/rcar-vin.h  |   8 +-
 drivers/media/v4l2-core/v4l2-fwnode.c       |  14 --
 include/media/v4l2-fwnode.h                 |  53 -------
 6 files changed, 113 insertions(+), 146 deletions(-)

Comments

Sakari Ailus Nov. 25, 2020, 5:21 p.m. UTC | #1
Hejssan Niklas,

Tack för detta hop av lappar! De är verkligen jättebehagliga!

On Wed, Nov 25, 2020 at 05:44:49PM +0100, Niklas Söderlund wrote:
> Rework the CSI-2 firmware parsing code to not use the soon to be
> removed v4l2_async_notifier_parse_fwnode_endpoints_by_port() helper. The
> change only aims to prepare for the removing of the old helper and there
> are no functional change.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 67 ++++++++++++---------
>  1 file changed, 40 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index 830ab0865967310b..98bff765b02e67d9 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -812,37 +812,48 @@ static const struct v4l2_async_notifier_operations rvin_group_notify_ops = {
>  	.complete = rvin_group_notify_complete,
>  };
>  
> -static int rvin_mc_parse_of_endpoint(struct device *dev,
> -				     struct v4l2_fwnode_endpoint *vep,
> -				     struct v4l2_async_subdev *asd)
> +static int rvin_mc_parse_of(struct rvin_dev *vin, unsigned int id)
>  {
> -	struct rvin_dev *vin = dev_get_drvdata(dev);
> -	int ret = 0;
> +	struct fwnode_handle *ep, *fwnode;
> +	struct v4l2_fwnode_endpoint vep = {
> +		.bus_type = V4L2_MBUS_CSI2_DPHY,
> +	};

This would fit on a single line.

> +	struct v4l2_async_subdev *asd;
> +	int ret;
>  
> -	if (vep->base.port != 1 || vep->base.id >= RVIN_CSI_MAX)
> -		return -EINVAL;
> +	ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(vin->dev), 1, id, 0);

You could use the FWNODE_GRAPH_ENDPOINT_NEXT flag to get the next endpoint
instead of specifying its number. Whichever works better...

> +	if (!ep)
> +		return 0;
>  
> -	if (!of_device_is_available(to_of_node(asd->match.fwnode))) {
> +	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;
> +	}
> +
> +	if (!of_device_is_available(to_of_node(fwnode))) {

fwnode is an endpoint here, not the device node.

But there's no need for this check either, as
fwnode_graph_get_endpoint_by_id(), by default, only returns endpoints that
are connected to available device's endpoints. So you can remove this
check.

>  		vin_dbg(vin, "OF device %pOF disabled, ignoring\n",
> -			to_of_node(asd->match.fwnode));
> -		return -ENOTCONN;
> -	}
> -
> -	mutex_lock(&vin->group->lock);
> -
> -	if (vin->group->csi[vep->base.id].asd) {
> -		vin_dbg(vin, "OF device %pOF already handled\n",
> -			to_of_node(asd->match.fwnode));
> +			to_of_node(fwnode));
>  		ret = -ENOTCONN;
>  		goto out;
>  	}
>  
> -	vin->group->csi[vep->base.id].asd = asd;
> +	asd = v4l2_async_notifier_add_fwnode_subdev(&vin->group->notifier,
> +						    fwnode, sizeof(*asd));
> +	if (IS_ERR(asd)) {
> +		ret = PTR_ERR(asd);
> +		goto out;
> +	}
> +
> +	vin->group->csi[vep.base.id].asd = asd;
>  
>  	vin_dbg(vin, "Add group OF device %pOF to slot %u\n",
> -		to_of_node(asd->match.fwnode), vep->base.id);
> +		to_of_node(fwnode), vep.base.id);
>  out:
> -	mutex_unlock(&vin->group->lock);
> +	fwnode_handle_put(fwnode);
>  
>  	return ret;
>  }
> @@ -850,7 +861,7 @@ static int rvin_mc_parse_of_endpoint(struct device *dev,
>  static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
>  {
>  	unsigned int count = 0, vin_mask = 0;
> -	unsigned int i;
> +	unsigned int i, id;
>  	int ret;
>  
>  	mutex_lock(&vin->group->lock);
> @@ -881,12 +892,14 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
>  		if (!(vin_mask & BIT(i)))
>  			continue;
>  
> -		ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port(
> -				vin->group->vin[i]->dev, &vin->group->notifier,
> -				sizeof(struct v4l2_async_subdev), 1,
> -				rvin_mc_parse_of_endpoint);
> -		if (ret)
> -			return ret;
> +		for (id = 0; id < RVIN_CSI_MAX; id++) {
> +			if (vin->group->csi[id].asd)
> +				continue;
> +
> +			ret = rvin_mc_parse_of(vin->group->vin[i], id);
> +			if (ret)
> +				return ret;
> +		}
>  	}
>  
>  	if (list_empty(&vin->group->notifier.asd_list))
Jacopo Mondi Nov. 26, 2020, 10:12 a.m. UTC | #2
Hi Niklas, Sakari,

On Wed, Nov 25, 2020 at 05:44:45PM +0100, Niklas Söderlund wrote:
> Hello,

>

> This series aims to remove a V4L2 helper that provides a too simple

> implementation, v4l2_async_notifier_parse_fwnode_endpoints_by_port().

> Instead drivers shall implement what the helper does directly to get

> greater control of the process. There is only one user left in tree of

> this interface, R-Car VIN.


What is the plan going forward ?
removing v4l2_async_notifier_parse_fwnode_endpoints_by_port() here
then remove v4l2_async_notifier_parse_fwnode_endpoints() as it has a
single user in mainline too ?

Are we standardizing all platform drivers to use
v4l2_async_notifier_add_fwnode_subdev() and perform fwnode.match
initialization by themselves or is there a plan to replace
v4l2_async_notifier_parse_fwnode_endpoints*() with something else ?

Thanks
  j

>

> This series starts therefor to rework the R-Car VIN driver to not depend

> on the helper. And in the process a small fwnode imbalance is fixed.

> After the last user of the helper is reworked the series removes the

> helper to prevent usage of it to resurface.

>

> This series is based on-top of the latest media tree and is tested on

> Renesas R-Car M3-N and Koelsch without any regressions or change in

> behavior detected.

>

> Niklas Söderlund (5):

>   rcar-vin: Only dynamically allocate v4l2_async_subdev

>   rcar-vin: Rework parallel firmware parsing

>   rcar-vin: Use v4l2_async_subdev instead of fwnode_handle to match

>     subdevices

>   rcar-vin: Rework CSI-2 firmware parsing

>   v4l2-fwnode: Remove

>     v4l2_async_notifier_parse_fwnode_endpoints_by_port()

>

>  drivers/media/platform/rcar-vin/rcar-core.c | 156 ++++++++++++--------

>  drivers/media/platform/rcar-vin/rcar-dma.c  |  16 +-

>  drivers/media/platform/rcar-vin/rcar-v4l2.c |  12 +-

>  drivers/media/platform/rcar-vin/rcar-vin.h  |   8 +-

>  drivers/media/v4l2-core/v4l2-fwnode.c       |  14 --

>  include/media/v4l2-fwnode.h                 |  53 -------

>  6 files changed, 113 insertions(+), 146 deletions(-)

>

> --

> 2.29.2

>
Sakari Ailus Nov. 26, 2020, 10:22 a.m. UTC | #3
Hi Jacopo,

On Thu, Nov 26, 2020 at 11:12:51AM +0100, Jacopo Mondi wrote:
> Hi Niklas, Sakari,

> 

> On Wed, Nov 25, 2020 at 05:44:45PM +0100, Niklas Söderlund wrote:

> > Hello,

> >

> > This series aims to remove a V4L2 helper that provides a too simple

> > implementation, v4l2_async_notifier_parse_fwnode_endpoints_by_port().

> > Instead drivers shall implement what the helper does directly to get

> > greater control of the process. There is only one user left in tree of

> > this interface, R-Car VIN.

> 

> What is the plan going forward ?

> removing v4l2_async_notifier_parse_fwnode_endpoints_by_port() here

> then remove v4l2_async_notifier_parse_fwnode_endpoints() as it has a

> single user in mainline too ?

> 

> Are we standardizing all platform drivers to use

> v4l2_async_notifier_add_fwnode_subdev() and perform fwnode.match

> initialization by themselves or is there a plan to replace


Yes, please.

> v4l2_async_notifier_parse_fwnode_endpoints*() with something else ?


That's always been somewhat clunky and required special cases. The other
option, i.e. what this patchset does, is straightforward as well as allows
setting defaults in drivers, and admittedly, comes with a little bit of
extra code in drivers in areas that are driver specific. The old functions
such as v4l2_async_notifier_parse_fwnode_endpoints() just pretended they
were not...

-- 
Regards,

Sakari Ailus
Jacopo Mondi Nov. 27, 2020, 11:19 a.m. UTC | #4
Hi Sakari,

On Thu, Nov 26, 2020 at 12:22:05PM +0200, Sakari Ailus wrote:
> Hi Jacopo,

>

> On Thu, Nov 26, 2020 at 11:12:51AM +0100, Jacopo Mondi wrote:

> > Hi Niklas, Sakari,

> >

> > On Wed, Nov 25, 2020 at 05:44:45PM +0100, Niklas Söderlund wrote:

> > > Hello,

> > >

> > > This series aims to remove a V4L2 helper that provides a too simple

> > > implementation, v4l2_async_notifier_parse_fwnode_endpoints_by_port().

> > > Instead drivers shall implement what the helper does directly to get

> > > greater control of the process. There is only one user left in tree of

> > > this interface, R-Car VIN.

> >

> > What is the plan going forward ?

> > removing v4l2_async_notifier_parse_fwnode_endpoints_by_port() here

> > then remove v4l2_async_notifier_parse_fwnode_endpoints() as it has a

> > single user in mainline too ?

> >

> > Are we standardizing all platform drivers to use

> > v4l2_async_notifier_add_fwnode_subdev() and perform fwnode.match

> > initialization by themselves or is there a plan to replace

>

> Yes, please.

>

> > v4l2_async_notifier_parse_fwnode_endpoints*() with something else ?

>

> That's always been somewhat clunky and required special cases. The other

> option, i.e. what this patchset does, is straightforward as well as allows

> setting defaults in drivers, and admittedly, comes with a little bit of

> extra code in drivers in areas that are driver specific. The old functions

> such as v4l2_async_notifier_parse_fwnode_endpoints() just pretended they

> were not...


Agreed in full :)
(At the expense of a little extra code in drivers)

>

> --

> Regards,

>

> Sakari Ailus