| Message ID | 20201125164450.2056963-3-niklas.soderlund+renesas@ragnatech.se (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | v4l2-fwnode: Remove v4l2_async_notifier_parse_fwnode_endpoints_by_port() helper | expand |
Hi Niklas, On Wed, Nov 25, 2020 at 05:44:47PM +0100, Niklas Söderlund wrote: > Rework the parallel 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. nit: 'changes' as you use 'are' > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > --- > drivers/media/platform/rcar-vin/rcar-core.c | 50 +++++++++++++++------ > 1 file changed, 36 insertions(+), 14 deletions(-) > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c > index 07f250bfc0051135..396ff5531359f3f4 100644 > --- a/drivers/media/platform/rcar-vin/rcar-core.c > +++ b/drivers/media/platform/rcar-vin/rcar-core.c > @@ -604,32 +604,56 @@ static const struct v4l2_async_notifier_operations rvin_parallel_notify_ops = { > .complete = rvin_parallel_notify_complete, > }; > > -static int rvin_parallel_parse_v4l2(struct device *dev, > - struct v4l2_fwnode_endpoint *vep, > - struct v4l2_async_subdev *asd) > +static int rvin_parallel_parse_of(struct rvin_dev *vin) > { > - struct rvin_dev *vin = dev_get_drvdata(dev); > + struct fwnode_handle *ep, *fwnode; > + struct v4l2_fwnode_endpoint vep = { > + .bus_type = V4L2_MBUS_UNKNOWN, > + }; Afaict bus autodiscovery is kind of deprectated. Unfortunately we don't enforce the "bus-type" property presence in DTS, so it's very hard to identify mis-configurations and set defaults (which we know and should set). I think this calls for a slight rework of this part with an associated bindings update. We assume DTS are correct, so this is not pressing, but currently we don't have any validation in place. For later, anyway. > + struct v4l2_async_subdev *asd; > + int ret; > > - if (vep->base.port || vep->base.id) > - return -ENOTCONN; > + ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(vin->dev), 0, 0, 0); get_next_endpoint() would do the same afaict, but being explicit is probably better > + if (!ep) > + return 0; > > - vin->parallel.mbus_type = vep->bus_type; > + fwnode = fwnode_graph_get_remote_endpoint(ep); We're matching a parallel subdevice, which usually registers its async subdev on the device node, not on endpoints. In facts v4l2_async_notifier_fwnode_parse_endpoint() which is in the call path of the v4l2_async_notifier_parse_fwnode_endpoints_by_port() you are removing does: asd->match.fwnode = fwnode_graph_get_remote_port_parent(endpoint); We now have match_fwnode() that adjusts endpoints to be matched against the remote's parent but this still feels like a workaround as most subdevs in mainline (all but adv748x?) still match on device node. I wonder how many system would actually break if we change v4l2_async_register_subdev[_sensor_common]() to use the first available endpoint as match target. > + 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 (vin->parallel.mbus_type) { > + switch (vep.bus_type) { > case V4L2_MBUS_PARALLEL: > case V4L2_MBUS_BT656: > vin_dbg(vin, "Found %s media bus\n", > - vin->parallel.mbus_type == V4L2_MBUS_PARALLEL ? > + vep.bus_type == V4L2_MBUS_PARALLEL ? > "PARALLEL" : "BT656"); > - vin->parallel.bus = vep->bus.parallel; > + vin->parallel.mbus_type = vep.bus_type; > + vin->parallel.bus = vep.bus.parallel; > break; > default: > vin_err(vin, "Unknown media bus type\n"); > - return -EINVAL; > + ret = -EINVAL; > + goto out; > + } > + > + asd = v4l2_async_notifier_add_fwnode_subdev(&vin->notifier, fwnode, > + sizeof(*asd)); > + if (IS_ERR(asd)) { > + ret = PTR_ERR(asd); > + goto out; > } > > vin->parallel.asd = asd; > > + vin_dbg(vin, "Add parallel OF device %pOF\n", to_of_node(fwnode)); You could omit OF as it is implied :) > +out: > + fwnode_handle_put(fwnode); > + > return 0; > } > > @@ -639,9 +663,7 @@ static int rvin_parallel_init(struct rvin_dev *vin) > > v4l2_async_notifier_init(&vin->notifier); > > - ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port( > - vin->dev, &vin->notifier, sizeof(*vin->parallel.asd), > - 0, rvin_parallel_parse_v4l2); > + ret = rvin_parallel_parse_of(vin); Patch is very nice, I like this direction Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org> Thanks j > if (ret) > return ret; > > -- > 2.29.2 >
diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c index 07f250bfc0051135..396ff5531359f3f4 100644 --- a/drivers/media/platform/rcar-vin/rcar-core.c +++ b/drivers/media/platform/rcar-vin/rcar-core.c @@ -604,32 +604,56 @@ static const struct v4l2_async_notifier_operations rvin_parallel_notify_ops = { .complete = rvin_parallel_notify_complete, }; -static int rvin_parallel_parse_v4l2(struct device *dev, - struct v4l2_fwnode_endpoint *vep, - struct v4l2_async_subdev *asd) +static int rvin_parallel_parse_of(struct rvin_dev *vin) { - struct rvin_dev *vin = dev_get_drvdata(dev); + struct fwnode_handle *ep, *fwnode; + struct v4l2_fwnode_endpoint vep = { + .bus_type = V4L2_MBUS_UNKNOWN, + }; + struct v4l2_async_subdev *asd; + int ret; - if (vep->base.port || vep->base.id) - return -ENOTCONN; + ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(vin->dev), 0, 0, 0); + if (!ep) + return 0; - vin->parallel.mbus_type = vep->bus_type; + 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 (vin->parallel.mbus_type) { + switch (vep.bus_type) { case V4L2_MBUS_PARALLEL: case V4L2_MBUS_BT656: vin_dbg(vin, "Found %s media bus\n", - vin->parallel.mbus_type == V4L2_MBUS_PARALLEL ? + vep.bus_type == V4L2_MBUS_PARALLEL ? "PARALLEL" : "BT656"); - vin->parallel.bus = vep->bus.parallel; + vin->parallel.mbus_type = vep.bus_type; + vin->parallel.bus = vep.bus.parallel; break; default: vin_err(vin, "Unknown media bus type\n"); - return -EINVAL; + ret = -EINVAL; + goto out; + } + + asd = v4l2_async_notifier_add_fwnode_subdev(&vin->notifier, fwnode, + sizeof(*asd)); + if (IS_ERR(asd)) { + ret = PTR_ERR(asd); + goto out; } vin->parallel.asd = asd; + vin_dbg(vin, "Add parallel OF device %pOF\n", to_of_node(fwnode)); +out: + fwnode_handle_put(fwnode); + return 0; } @@ -639,9 +663,7 @@ static int rvin_parallel_init(struct rvin_dev *vin) v4l2_async_notifier_init(&vin->notifier); - ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port( - vin->dev, &vin->notifier, sizeof(*vin->parallel.asd), - 0, rvin_parallel_parse_v4l2); + ret = rvin_parallel_parse_of(vin); if (ret) return ret;
Rework the parallel 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 | 50 +++++++++++++++------ 1 file changed, 36 insertions(+), 14 deletions(-)