| 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 |
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))
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 >
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
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
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(-)