| Message ID | 20230330115853.1628216-4-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Separate links and async sub-devices | expand |
Hi Sakari On Thu, Mar 30, 2023 at 02:58:38PM +0300, Sakari Ailus wrote: > V4L2 async sub-device matching originally used the device nodes only. > Endpoint nodes were taken into use instead as using the device nodes was > problematic for it was in some cases ambiguous which link might have been > in question. > > There is however no need to use endpoint nodes on both sides, as the async > sub-device's fwnode can always be trivially obtained using > fwnode_graph_get_remote_endpoint() when needed while what counts is > whether or not the link is between two device nodes, i.e. the device nodes > match. > As you know I'm a bit debated. Strict endpoint-matching requires one subdev to be registed per each endpoint, and this is tedious for drivers that have to register a subdev for each of its endpoints Allowing a subdev to be matched multiple times on different endpoints gives a way for lazy drivers to take a shortcut and simplify their topologies to a single subdev, when they would actually need more. Also, knowing where this series is going, I wonder if this wouldn't be a good time to enforce all async subdevices to be registered with an endpoint. From a very quick look at the drivers we have in mainline only a few still use v4l2_async_nf_add_fwnode() and only 4 of them (camss, rcar_drif, am437x-vpfe, vpif_capture, xilinx_vipp) use as match.fwnode what the remote port parent. I wonder if this would be a good occasione to enforce with a WARN() if !fwnode_graph_is_endpoint(fwnode) in v4l2_async_nf_add_fwnode() (or possibily, even remove that function and port all drivers to use vl2_async_nf_add_fwnode_remote(). I can help, if the above makes sense to you as well. > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/i2c/adv748x/adv748x-csi2.c | 3 - > drivers/media/i2c/max9286.c | 14 +--- > drivers/media/i2c/rdacm20.c | 15 +---- > drivers/media/i2c/rdacm21.c | 15 +---- > drivers/media/i2c/tc358746.c | 3 - > drivers/media/v4l2-core/v4l2-async.c | 86 ++++++------------------ > 6 files changed, 24 insertions(+), 112 deletions(-) > > diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c > index bd4f3fe0e309..3d830816243f 100644 > --- a/drivers/media/i2c/adv748x/adv748x-csi2.c > +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c > @@ -300,9 +300,6 @@ int adv748x_csi2_init(struct adv748x_state *state, struct adv748x_csi2 *tx) > MEDIA_ENT_F_VID_IF_BRIDGE, > is_txa(tx) ? "txa" : "txb"); > > - /* Ensure that matching is based upon the endpoint fwnodes */ > - tx->sd.fwnode = of_fwnode_handle(state->endpoints[tx->port]); > - > /* Register internal ops for incremental subdev registration */ > tx->sd.internal_ops = &adv748x_csi2_internal_ops; > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c > index 701038d6d19b..2d0f43e3fb9f 100644 > --- a/drivers/media/i2c/max9286.c > +++ b/drivers/media/i2c/max9286.c > @@ -1051,7 +1051,6 @@ static const struct v4l2_ctrl_ops max9286_ctrl_ops = { > static int max9286_v4l2_register(struct max9286_priv *priv) > { > struct device *dev = &priv->client->dev; > - struct fwnode_handle *ep; > int ret; > int i; > > @@ -1093,25 +1092,14 @@ static int max9286_v4l2_register(struct max9286_priv *priv) > if (ret) > goto err_async; > > - ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), MAX9286_SRC_PAD, > - 0, 0); > - if (!ep) { > - dev_err(dev, "Unable to retrieve endpoint on \"port@4\"\n"); > - ret = -ENOENT; > - goto err_async; > - } > - priv->sd.fwnode = ep; > - You should also remove the fwnode_handle_put() call from static void max9286_v4l2_unregister(struct max9286_priv *priv) { fwnode_handle_put(priv->sd.fwnode); v4l2_async_unregister_subdev(&priv->sd); max9286_v4l2_notifier_unregister(priv); } > ret = v4l2_async_register_subdev(&priv->sd); > if (ret < 0) { > dev_err(dev, "Unable to register subdevice\n"); > - goto err_put_node; > + goto err_async; > } > > return 0; > > -err_put_node: > - fwnode_handle_put(ep); > err_async: > v4l2_ctrl_handler_free(&priv->ctrls); > max9286_v4l2_notifier_unregister(priv); > diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c > index a2263fa825b5..ea1111152285 100644 > --- a/drivers/media/i2c/rdacm20.c > +++ b/drivers/media/i2c/rdacm20.c > @@ -567,7 +567,6 @@ static int rdacm20_initialize(struct rdacm20_device *dev) > static int rdacm20_probe(struct i2c_client *client) > { > struct rdacm20_device *dev; > - struct fwnode_handle *ep; > int ret; > > dev = devm_kzalloc(&client->dev, sizeof(*dev), GFP_KERNEL); > @@ -616,24 +615,12 @@ static int rdacm20_probe(struct i2c_client *client) > if (ret < 0) > goto error_free_ctrls; > > - ep = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev), NULL); > - if (!ep) { > - dev_err(&client->dev, > - "Unable to get endpoint in node %pOF\n", > - client->dev.of_node); > - ret = -ENOENT; > - goto error_free_ctrls; > - } > - dev->sd.fwnode = ep; Same for this driver and for rdacm21 > - > ret = v4l2_async_register_subdev(&dev->sd); > if (ret) > - goto error_put_node; > + goto error_free_ctrls; > > return 0; > > -error_put_node: > - fwnode_handle_put(ep); > error_free_ctrls: > v4l2_ctrl_handler_free(&dev->ctrls); > error: > diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c > index 9ccc56c30d3b..d67cfcb2e05a 100644 > --- a/drivers/media/i2c/rdacm21.c > +++ b/drivers/media/i2c/rdacm21.c > @@ -543,7 +543,6 @@ static int rdacm21_initialize(struct rdacm21_device *dev) > static int rdacm21_probe(struct i2c_client *client) > { > struct rdacm21_device *dev; > - struct fwnode_handle *ep; > int ret; > > dev = devm_kzalloc(&client->dev, sizeof(*dev), GFP_KERNEL); > @@ -588,24 +587,12 @@ static int rdacm21_probe(struct i2c_client *client) > if (ret < 0) > goto error_free_ctrls; > > - ep = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev), NULL); > - if (!ep) { > - dev_err(&client->dev, > - "Unable to get endpoint in node %pOF\n", > - client->dev.of_node); > - ret = -ENOENT; > - goto error_free_ctrls; > - } > - dev->sd.fwnode = ep; > - > ret = v4l2_async_register_subdev(&dev->sd); > if (ret) > - goto error_put_node; > + goto error_free_ctrls; > > return 0; > > -error_put_node: > - fwnode_handle_put(dev->sd.fwnode); > error_free_ctrls: > v4l2_ctrl_handler_free(&dev->ctrls); > error: > diff --git a/drivers/media/i2c/tc358746.c b/drivers/media/i2c/tc358746.c > index 4063754a6732..56f2b43d4edf 100644 > --- a/drivers/media/i2c/tc358746.c > +++ b/drivers/media/i2c/tc358746.c > @@ -1476,9 +1476,6 @@ static int tc358746_async_register(struct tc358746 *tc358746) > if (err) > goto err_cleanup; > > - tc358746->sd.fwnode = fwnode_graph_get_endpoint_by_id( > - dev_fwnode(tc358746->sd.dev), TC358746_SOURCE, 0, 0); > - > err = v4l2_async_register_subdev(&tc358746->sd); > if (err) > goto err_unregister; > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > index 6dd426c2ca68..13fe0bdc70b6 100644 > --- a/drivers/media/v4l2-core/v4l2-async.c > +++ b/drivers/media/v4l2-core/v4l2-async.c > @@ -86,84 +86,33 @@ match_fwnode_one(struct v4l2_async_notifier *notifier, > struct v4l2_subdev *sd, struct fwnode_handle *sd_fwnode, > struct v4l2_async_subdev *asd) > { > - struct fwnode_handle *other_fwnode; > - struct fwnode_handle *dev_fwnode; > - bool asd_fwnode_is_ep; > - bool sd_fwnode_is_ep; > - struct device *dev; > + struct fwnode_handle *asd_dev_fwnode; > + bool ret; > > dev_dbg(sd->dev, "async: fwnode match: need %pfw, trying %pfw\n", > sd_fwnode, asd->match.fwnode); > > - /* > - * Both the subdev and the async subdev can provide either an endpoint > - * fwnode or a device fwnode. Start with the simple case of direct > - * fwnode matching. > - */ > if (sd_fwnode == asd->match.fwnode) { > dev_dbg(sd->dev, "async: direct match found\n"); > return true; > } > > - /* > - * Otherwise, check if the sd fwnode and the asd fwnode refer to an > - * endpoint or a device. If they're of the same type, there's no match. > - * Technically speaking this checks if the nodes refer to a connected > - * endpoint, which is the simplest check that works for both OF and > - * ACPI. This won't make a difference, as drivers should not try to > - * match unconnected endpoints. > - */ > - sd_fwnode_is_ep = fwnode_graph_is_endpoint(sd_fwnode); > - asd_fwnode_is_ep = fwnode_graph_is_endpoint(asd->match.fwnode); > - > - if (sd_fwnode_is_ep == asd_fwnode_is_ep) { > - dev_dbg(sd->dev, "async: matching node types\n"); > + if (!fwnode_graph_is_endpoint(asd->match.fwnode)) { > + dev_dbg(sd->dev, > + "async: async subdev fwnode not endpoint, no match\n"); As per the previous patch I would just say "direct match failed"; > return false; > } > > - /* > - * The sd and asd fwnodes are of different types. Get the device fwnode > - * parent of the endpoint fwnode, and compare it with the other fwnode. > - */ > - if (sd_fwnode_is_ep) { > - dev_fwnode = fwnode_graph_get_port_parent(sd_fwnode); > - other_fwnode = asd->match.fwnode; > - } else { > - dev_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode); > - other_fwnode = sd_fwnode; > - } > - > - dev_dbg(sd->dev, "async: fwnode compat match, need %pfw, trying %pfw\n", > - dev_fwnode, other_fwnode); > + asd_dev_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode); > > - fwnode_handle_put(dev_fwnode); > + ret = sd_fwnode == asd_dev_fwnode; > > - if (dev_fwnode != other_fwnode) { > - dev_dbg(sd->dev, "async: compat match not found\n"); > - return false; > - } > + fwnode_handle_put(asd_dev_fwnode); > > - /* > - * We have a heterogeneous match. Retrieve the struct device of the side > - * that matched on a device fwnode to print its driver name. > - */ > - if (sd_fwnode_is_ep) > - dev = notifier->v4l2_dev ? notifier->v4l2_dev->dev > - : notifier->sd->dev; > - else > - dev = sd->dev; > - > - if (dev && dev->driver) { > - if (sd_fwnode_is_ep) > - dev_warn(dev, "Driver %s uses device fwnode, incorrect match may occur\n", > - dev->driver->name); > - dev_notice(dev, "Consider updating driver %s to match on endpoints\n", > - dev->driver->name); > - } > - > - dev_dbg(sd->dev, "async: compat match found\n"); > + dev_dbg(sd->dev, "async: device--endpoint match %sfound\n", double '--' However now that I think about it, a subdev will be matched agains a rather large number of async subdevs and most attempts will fail.. do we want a printout for each of them ? > + ret ? "" : "not "); > > - return true; > + return ret; > } > > static bool match_fwnode(struct v4l2_async_notifier *notifier, > @@ -804,12 +753,19 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd) > int ret; > > /* > - * No reference taken. The reference is held by the device > - * (struct v4l2_subdev.dev), and async sub-device does not > - * exist independently of the device at any point of time. > + * No reference taken. The reference is held by the device (struct > + * v4l2_subdev.dev), and async sub-device does not exist independently > + * of the device at any point of time. > + * > + * The async sub-device shall always be registered for its device node, > + * not the endpoint node. Issue a warning in that case. Once there is > + * certainty no driver no longer does this, remove the warning (and > + * compatibility code) below. > */ > if (!sd->fwnode && sd->dev) > sd->fwnode = dev_fwnode(sd->dev); > + else if (WARN_ON(fwnode_graph_is_endpoint(sd->fwnode))) > + sd->fwnode = fwnode_graph_get_port_parent(sd->fwnode); > > mutex_lock(&list_lock); > > -- > 2.30.2 >
Hi Jacopo, On Thu, Apr 13, 2023 at 06:50:04PM +0200, Jacopo Mondi wrote: > Hi Sakari > > On Thu, Mar 30, 2023 at 02:58:38PM +0300, Sakari Ailus wrote: > > V4L2 async sub-device matching originally used the device nodes only. > > Endpoint nodes were taken into use instead as using the device nodes was > > problematic for it was in some cases ambiguous which link might have been > > in question. > > > > There is however no need to use endpoint nodes on both sides, as the async > > sub-device's fwnode can always be trivially obtained using > > fwnode_graph_get_remote_endpoint() when needed while what counts is > > whether or not the link is between two device nodes, i.e. the device nodes > > match. > > > > As you know I'm a bit debated. > > Strict endpoint-matching requires one subdev to be registed per each > endpoint, and this is tedious for drivers that have to register a > subdev for each of its endpoints > > Allowing a subdev to be matched multiple times on different endpoints > gives a way for lazy drivers to take a shortcut and simplify their > topologies to a single subdev, when they would actually need more. I'd say this is really about interface design, not being "lazy". It depends on the sub-device. Ideally the framework should be also as easy for drivers drivers to use as possible. What is not supported, though, is multiple sub-devices with a single device node. Do we need that? At least I don't think I came across a driver that would. Although it would be relatively easy add this in form of a list of endpoints, if there's a need to. Also cc Niklas. > > Also, knowing where this series is going, I wonder if this wouldn't be > a good time to enforce all async subdevices to be registered with an > endpoint. From a very quick look at the drivers we have in mainline > only a few still use v4l2_async_nf_add_fwnode() and only 4 of them > (camss, rcar_drif, am437x-vpfe, vpif_capture, xilinx_vipp) use as > match.fwnode what the remote port parent. > > I wonder if this would be a good occasione to enforce with a WARN() if > !fwnode_graph_is_endpoint(fwnode) in v4l2_async_nf_add_fwnode() (or > possibily, even remove that function and port all drivers to use > vl2_async_nf_add_fwnode_remote(). I can help, if the above makes sense > to you as well. I prefer to keep things simple for drivers if possible, and this patch does that by removing a noticeable number of lines from a few drivers. > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > drivers/media/i2c/adv748x/adv748x-csi2.c | 3 - > > drivers/media/i2c/max9286.c | 14 +--- > > drivers/media/i2c/rdacm20.c | 15 +---- > > drivers/media/i2c/rdacm21.c | 15 +---- > > drivers/media/i2c/tc358746.c | 3 - > > drivers/media/v4l2-core/v4l2-async.c | 86 ++++++------------------ > > 6 files changed, 24 insertions(+), 112 deletions(-) > > > > diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c > > index bd4f3fe0e309..3d830816243f 100644 > > --- a/drivers/media/i2c/adv748x/adv748x-csi2.c > > +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c > > @@ -300,9 +300,6 @@ int adv748x_csi2_init(struct adv748x_state *state, struct adv748x_csi2 *tx) > > MEDIA_ENT_F_VID_IF_BRIDGE, > > is_txa(tx) ? "txa" : "txb"); > > > > - /* Ensure that matching is based upon the endpoint fwnodes */ > > - tx->sd.fwnode = of_fwnode_handle(state->endpoints[tx->port]); > > - > > /* Register internal ops for incremental subdev registration */ > > tx->sd.internal_ops = &adv748x_csi2_internal_ops; > > > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c > > index 701038d6d19b..2d0f43e3fb9f 100644 > > --- a/drivers/media/i2c/max9286.c > > +++ b/drivers/media/i2c/max9286.c > > @@ -1051,7 +1051,6 @@ static const struct v4l2_ctrl_ops max9286_ctrl_ops = { > > static int max9286_v4l2_register(struct max9286_priv *priv) > > { > > struct device *dev = &priv->client->dev; > > - struct fwnode_handle *ep; > > int ret; > > int i; > > > > @@ -1093,25 +1092,14 @@ static int max9286_v4l2_register(struct max9286_priv *priv) > > if (ret) > > goto err_async; > > > > - ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), MAX9286_SRC_PAD, > > - 0, 0); > > - if (!ep) { > > - dev_err(dev, "Unable to retrieve endpoint on \"port@4\"\n"); > > - ret = -ENOENT; > > - goto err_async; > > - } > > - priv->sd.fwnode = ep; > > - > > > You should also remove the fwnode_handle_put() call from > > static void max9286_v4l2_unregister(struct max9286_priv *priv) > { > fwnode_handle_put(priv->sd.fwnode); > v4l2_async_unregister_subdev(&priv->sd); > max9286_v4l2_notifier_unregister(priv); > } Thanks, I'll address these in v2. > > > ret = v4l2_async_register_subdev(&priv->sd); > > if (ret < 0) { > > dev_err(dev, "Unable to register subdevice\n"); > > - goto err_put_node; > > + goto err_async; > > } > > > > return 0; > > > > -err_put_node: > > - fwnode_handle_put(ep); > > err_async: > > v4l2_ctrl_handler_free(&priv->ctrls); > > max9286_v4l2_notifier_unregister(priv); > > diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c > > index a2263fa825b5..ea1111152285 100644 > > --- a/drivers/media/i2c/rdacm20.c > > +++ b/drivers/media/i2c/rdacm20.c > > @@ -567,7 +567,6 @@ static int rdacm20_initialize(struct rdacm20_device *dev) > > static int rdacm20_probe(struct i2c_client *client) > > { > > struct rdacm20_device *dev; > > - struct fwnode_handle *ep; > > int ret; > > > > dev = devm_kzalloc(&client->dev, sizeof(*dev), GFP_KERNEL); > > @@ -616,24 +615,12 @@ static int rdacm20_probe(struct i2c_client *client) > > if (ret < 0) > > goto error_free_ctrls; > > > > - ep = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev), NULL); > > - if (!ep) { > > - dev_err(&client->dev, > > - "Unable to get endpoint in node %pOF\n", > > - client->dev.of_node); > > - ret = -ENOENT; > > - goto error_free_ctrls; > > - } > > - dev->sd.fwnode = ep; > > Same for this driver and for rdacm21 > > > - > > ret = v4l2_async_register_subdev(&dev->sd); > > if (ret) > > - goto error_put_node; > > + goto error_free_ctrls; > > > > return 0; > > > > -error_put_node: > > - fwnode_handle_put(ep); > > error_free_ctrls: > > v4l2_ctrl_handler_free(&dev->ctrls); > > error: > > diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c > > index 9ccc56c30d3b..d67cfcb2e05a 100644 > > --- a/drivers/media/i2c/rdacm21.c > > +++ b/drivers/media/i2c/rdacm21.c > > @@ -543,7 +543,6 @@ static int rdacm21_initialize(struct rdacm21_device *dev) > > static int rdacm21_probe(struct i2c_client *client) > > { > > struct rdacm21_device *dev; > > - struct fwnode_handle *ep; > > int ret; > > > > dev = devm_kzalloc(&client->dev, sizeof(*dev), GFP_KERNEL); > > @@ -588,24 +587,12 @@ static int rdacm21_probe(struct i2c_client *client) > > if (ret < 0) > > goto error_free_ctrls; > > > > - ep = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev), NULL); > > - if (!ep) { > > - dev_err(&client->dev, > > - "Unable to get endpoint in node %pOF\n", > > - client->dev.of_node); > > - ret = -ENOENT; > > - goto error_free_ctrls; > > - } > > - dev->sd.fwnode = ep; > > - > > ret = v4l2_async_register_subdev(&dev->sd); > > if (ret) > > - goto error_put_node; > > + goto error_free_ctrls; > > > > return 0; > > > > -error_put_node: > > - fwnode_handle_put(dev->sd.fwnode); > > error_free_ctrls: > > v4l2_ctrl_handler_free(&dev->ctrls); > > error: > > diff --git a/drivers/media/i2c/tc358746.c b/drivers/media/i2c/tc358746.c > > index 4063754a6732..56f2b43d4edf 100644 > > --- a/drivers/media/i2c/tc358746.c > > +++ b/drivers/media/i2c/tc358746.c > > @@ -1476,9 +1476,6 @@ static int tc358746_async_register(struct tc358746 *tc358746) > > if (err) > > goto err_cleanup; > > > > - tc358746->sd.fwnode = fwnode_graph_get_endpoint_by_id( > > - dev_fwnode(tc358746->sd.dev), TC358746_SOURCE, 0, 0); > > - > > err = v4l2_async_register_subdev(&tc358746->sd); > > if (err) > > goto err_unregister; > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > > index 6dd426c2ca68..13fe0bdc70b6 100644 > > --- a/drivers/media/v4l2-core/v4l2-async.c > > +++ b/drivers/media/v4l2-core/v4l2-async.c > > @@ -86,84 +86,33 @@ match_fwnode_one(struct v4l2_async_notifier *notifier, > > struct v4l2_subdev *sd, struct fwnode_handle *sd_fwnode, > > struct v4l2_async_subdev *asd) > > { > > - struct fwnode_handle *other_fwnode; > > - struct fwnode_handle *dev_fwnode; > > - bool asd_fwnode_is_ep; > > - bool sd_fwnode_is_ep; > > - struct device *dev; > > + struct fwnode_handle *asd_dev_fwnode; > > + bool ret; > > > > dev_dbg(sd->dev, "async: fwnode match: need %pfw, trying %pfw\n", > > sd_fwnode, asd->match.fwnode); > > > > - /* > > - * Both the subdev and the async subdev can provide either an endpoint > > - * fwnode or a device fwnode. Start with the simple case of direct > > - * fwnode matching. > > - */ > > if (sd_fwnode == asd->match.fwnode) { > > dev_dbg(sd->dev, "async: direct match found\n"); > > return true; > > } > > > > - /* > > - * Otherwise, check if the sd fwnode and the asd fwnode refer to an > > - * endpoint or a device. If they're of the same type, there's no match. > > - * Technically speaking this checks if the nodes refer to a connected > > - * endpoint, which is the simplest check that works for both OF and > > - * ACPI. This won't make a difference, as drivers should not try to > > - * match unconnected endpoints. > > - */ > > - sd_fwnode_is_ep = fwnode_graph_is_endpoint(sd_fwnode); > > - asd_fwnode_is_ep = fwnode_graph_is_endpoint(asd->match.fwnode); > > - > > - if (sd_fwnode_is_ep == asd_fwnode_is_ep) { > > - dev_dbg(sd->dev, "async: matching node types\n"); > > + if (!fwnode_graph_is_endpoint(asd->match.fwnode)) { > > + dev_dbg(sd->dev, > > + "async: async subdev fwnode not endpoint, no match\n"); > > As per the previous patch I would just say "direct match failed"; > > > return false; > > } > > > > - /* > > - * The sd and asd fwnodes are of different types. Get the device fwnode > > - * parent of the endpoint fwnode, and compare it with the other fwnode. > > - */ > > - if (sd_fwnode_is_ep) { > > - dev_fwnode = fwnode_graph_get_port_parent(sd_fwnode); > > - other_fwnode = asd->match.fwnode; > > - } else { > > - dev_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode); > > - other_fwnode = sd_fwnode; > > - } > > - > > - dev_dbg(sd->dev, "async: fwnode compat match, need %pfw, trying %pfw\n", > > - dev_fwnode, other_fwnode); > > + asd_dev_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode); > > > > - fwnode_handle_put(dev_fwnode); > > + ret = sd_fwnode == asd_dev_fwnode; > > > > - if (dev_fwnode != other_fwnode) { > > - dev_dbg(sd->dev, "async: compat match not found\n"); > > - return false; > > - } > > + fwnode_handle_put(asd_dev_fwnode); > > > > - /* > > - * We have a heterogeneous match. Retrieve the struct device of the side > > - * that matched on a device fwnode to print its driver name. > > - */ > > - if (sd_fwnode_is_ep) > > - dev = notifier->v4l2_dev ? notifier->v4l2_dev->dev > > - : notifier->sd->dev; > > - else > > - dev = sd->dev; > > - > > - if (dev && dev->driver) { > > - if (sd_fwnode_is_ep) > > - dev_warn(dev, "Driver %s uses device fwnode, incorrect match may occur\n", > > - dev->driver->name); > > - dev_notice(dev, "Consider updating driver %s to match on endpoints\n", > > - dev->driver->name); > > - } > > - > > - dev_dbg(sd->dev, "async: compat match found\n"); > > + dev_dbg(sd->dev, "async: device--endpoint match %sfound\n", > > double '--' > > However now that I think about it, a subdev will be matched agains a > rather large number of async subdevs and most attempts will fail.. do > we want a printout for each of them ? Note that this will only happen if these debug prints are enabled. There was recently a case where they would have been useful. > > > > + ret ? "" : "not "); > > > > - return true; > > + return ret; > > } > > > > static bool match_fwnode(struct v4l2_async_notifier *notifier, > > @@ -804,12 +753,19 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd) > > int ret; > > > > /* > > - * No reference taken. The reference is held by the device > > - * (struct v4l2_subdev.dev), and async sub-device does not > > - * exist independently of the device at any point of time. > > + * No reference taken. The reference is held by the device (struct > > + * v4l2_subdev.dev), and async sub-device does not exist independently > > + * of the device at any point of time. > > + * > > + * The async sub-device shall always be registered for its device node, > > + * not the endpoint node. Issue a warning in that case. Once there is > > + * certainty no driver no longer does this, remove the warning (and > > + * compatibility code) below. > > */ > > if (!sd->fwnode && sd->dev) > > sd->fwnode = dev_fwnode(sd->dev); > > + else if (WARN_ON(fwnode_graph_is_endpoint(sd->fwnode))) > > + sd->fwnode = fwnode_graph_get_port_parent(sd->fwnode); > > > > mutex_lock(&list_lock); > >
Hi Sakari, On 2023-04-14 14:07:40 +0300, Sakari Ailus wrote: > Hi Jacopo, > > On Thu, Apr 13, 2023 at 06:50:04PM +0200, Jacopo Mondi wrote: > > Hi Sakari > > > > On Thu, Mar 30, 2023 at 02:58:38PM +0300, Sakari Ailus wrote: > > > V4L2 async sub-device matching originally used the device nodes only. > > > Endpoint nodes were taken into use instead as using the device nodes was > > > problematic for it was in some cases ambiguous which link might have been > > > in question. > > > > > > There is however no need to use endpoint nodes on both sides, as the async > > > sub-device's fwnode can always be trivially obtained using > > > fwnode_graph_get_remote_endpoint() when needed while what counts is > > > whether or not the link is between two device nodes, i.e. the device nodes > > > match. > > > > > > > As you know I'm a bit debated. > > > > Strict endpoint-matching requires one subdev to be registed per each > > endpoint, and this is tedious for drivers that have to register a > > subdev for each of its endpoints > > > > Allowing a subdev to be matched multiple times on different endpoints > > gives a way for lazy drivers to take a shortcut and simplify their > > topologies to a single subdev, when they would actually need more. > > I'd say this is really about interface design, not being "lazy". It depends > on the sub-device. Ideally the framework should be also as easy for drivers > drivers to use as possible. > > What is not supported, though, is multiple sub-devices with a single device > node. Do we need that? At least I don't think I came across a driver that > would. If I understand you correctly about multiple sub-device from a single device node, this exists today. The ADV748x driver have a single device node in DT and register multiple sub-devices, one for each source endpoint. The ADV748x have two CSI-2 transmitters, one 4-lane and one 1-lane as well as two different video capture "ports" one HDMI and one CVBS. Both capture ports can be active at the same time and routed internally inside the ADV748x to the two different CSI-2 transmitters. In order todo this the ADV748x register multiple subdevices and modifies the fwnode to be the endpoint instead of the device node. So the change in this patch for ADV748x driver would break that driver. > > Although it would be relatively easy add this in form of a list of > endpoints, if there's a need to. > > Also cc Niklas. > > > > > Also, knowing where this series is going, I wonder if this wouldn't be > > a good time to enforce all async subdevices to be registered with an > > endpoint. From a very quick look at the drivers we have in mainline > > only a few still use v4l2_async_nf_add_fwnode() and only 4 of them > > (camss, rcar_drif, am437x-vpfe, vpif_capture, xilinx_vipp) use as > > match.fwnode what the remote port parent. > > > > I wonder if this would be a good occasione to enforce with a WARN() if > > !fwnode_graph_is_endpoint(fwnode) in v4l2_async_nf_add_fwnode() (or > > possibily, even remove that function and port all drivers to use > > vl2_async_nf_add_fwnode_remote(). I can help, if the above makes sense > > to you as well. > > I prefer to keep things simple for drivers if possible, and this patch does > that by removing a noticeable number of lines from a few drivers. > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > --- > > > drivers/media/i2c/adv748x/adv748x-csi2.c | 3 - > > > drivers/media/i2c/max9286.c | 14 +--- > > > drivers/media/i2c/rdacm20.c | 15 +---- > > > drivers/media/i2c/rdacm21.c | 15 +---- > > > drivers/media/i2c/tc358746.c | 3 - > > > drivers/media/v4l2-core/v4l2-async.c | 86 ++++++------------------ > > > 6 files changed, 24 insertions(+), 112 deletions(-) > > > > > > diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c > > > index bd4f3fe0e309..3d830816243f 100644 > > > --- a/drivers/media/i2c/adv748x/adv748x-csi2.c > > > +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c > > > @@ -300,9 +300,6 @@ int adv748x_csi2_init(struct adv748x_state *state, struct adv748x_csi2 *tx) > > > MEDIA_ENT_F_VID_IF_BRIDGE, > > > is_txa(tx) ? "txa" : "txb"); > > > > > > - /* Ensure that matching is based upon the endpoint fwnodes */ > > > - tx->sd.fwnode = of_fwnode_handle(state->endpoints[tx->port]); I agree it's not nice, but dropping this would break this driver :-( Nacked-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > - > > > /* Register internal ops for incremental subdev registration */ > > > tx->sd.internal_ops = &adv748x_csi2_internal_ops; > > > > > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c > > > index 701038d6d19b..2d0f43e3fb9f 100644 > > > --- a/drivers/media/i2c/max9286.c > > > +++ b/drivers/media/i2c/max9286.c > > > @@ -1051,7 +1051,6 @@ static const struct v4l2_ctrl_ops max9286_ctrl_ops = { > > > static int max9286_v4l2_register(struct max9286_priv *priv) > > > { > > > struct device *dev = &priv->client->dev; > > > - struct fwnode_handle *ep; > > > int ret; > > > int i; > > > > > > @@ -1093,25 +1092,14 @@ static int max9286_v4l2_register(struct max9286_priv *priv) > > > if (ret) > > > goto err_async; > > > > > > - ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), MAX9286_SRC_PAD, > > > - 0, 0); > > > - if (!ep) { > > > - dev_err(dev, "Unable to retrieve endpoint on \"port@4\"\n"); > > > - ret = -ENOENT; > > > - goto err_async; > > > - } > > > - priv->sd.fwnode = ep; > > > - > > > > > > You should also remove the fwnode_handle_put() call from > > > > static void max9286_v4l2_unregister(struct max9286_priv *priv) > > { > > fwnode_handle_put(priv->sd.fwnode); > > v4l2_async_unregister_subdev(&priv->sd); > > max9286_v4l2_notifier_unregister(priv); > > } > > Thanks, I'll address these in v2. > > > > > > ret = v4l2_async_register_subdev(&priv->sd); > > > if (ret < 0) { > > > dev_err(dev, "Unable to register subdevice\n"); > > > - goto err_put_node; > > > + goto err_async; > > > } > > > > > > return 0; > > > > > > -err_put_node: > > > - fwnode_handle_put(ep); > > > err_async: > > > v4l2_ctrl_handler_free(&priv->ctrls); > > > max9286_v4l2_notifier_unregister(priv); > > > diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c > > > index a2263fa825b5..ea1111152285 100644 > > > --- a/drivers/media/i2c/rdacm20.c > > > +++ b/drivers/media/i2c/rdacm20.c > > > @@ -567,7 +567,6 @@ static int rdacm20_initialize(struct rdacm20_device *dev) > > > static int rdacm20_probe(struct i2c_client *client) > > > { > > > struct rdacm20_device *dev; > > > - struct fwnode_handle *ep; > > > int ret; > > > > > > dev = devm_kzalloc(&client->dev, sizeof(*dev), GFP_KERNEL); > > > @@ -616,24 +615,12 @@ static int rdacm20_probe(struct i2c_client *client) > > > if (ret < 0) > > > goto error_free_ctrls; > > > > > > - ep = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev), NULL); > > > - if (!ep) { > > > - dev_err(&client->dev, > > > - "Unable to get endpoint in node %pOF\n", > > > - client->dev.of_node); > > > - ret = -ENOENT; > > > - goto error_free_ctrls; > > > - } > > > - dev->sd.fwnode = ep; > > > > Same for this driver and for rdacm21 > > > > > - > > > ret = v4l2_async_register_subdev(&dev->sd); > > > if (ret) > > > - goto error_put_node; > > > + goto error_free_ctrls; > > > > > > return 0; > > > > > > -error_put_node: > > > - fwnode_handle_put(ep); > > > error_free_ctrls: > > > v4l2_ctrl_handler_free(&dev->ctrls); > > > error: > > > diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c > > > index 9ccc56c30d3b..d67cfcb2e05a 100644 > > > --- a/drivers/media/i2c/rdacm21.c > > > +++ b/drivers/media/i2c/rdacm21.c > > > @@ -543,7 +543,6 @@ static int rdacm21_initialize(struct rdacm21_device *dev) > > > static int rdacm21_probe(struct i2c_client *client) > > > { > > > struct rdacm21_device *dev; > > > - struct fwnode_handle *ep; > > > int ret; > > > > > > dev = devm_kzalloc(&client->dev, sizeof(*dev), GFP_KERNEL); > > > @@ -588,24 +587,12 @@ static int rdacm21_probe(struct i2c_client *client) > > > if (ret < 0) > > > goto error_free_ctrls; > > > > > > - ep = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev), NULL); > > > - if (!ep) { > > > - dev_err(&client->dev, > > > - "Unable to get endpoint in node %pOF\n", > > > - client->dev.of_node); > > > - ret = -ENOENT; > > > - goto error_free_ctrls; > > > - } > > > - dev->sd.fwnode = ep; > > > - > > > ret = v4l2_async_register_subdev(&dev->sd); > > > if (ret) > > > - goto error_put_node; > > > + goto error_free_ctrls; > > > > > > return 0; > > > > > > -error_put_node: > > > - fwnode_handle_put(dev->sd.fwnode); > > > error_free_ctrls: > > > v4l2_ctrl_handler_free(&dev->ctrls); > > > error: > > > diff --git a/drivers/media/i2c/tc358746.c b/drivers/media/i2c/tc358746.c > > > index 4063754a6732..56f2b43d4edf 100644 > > > --- a/drivers/media/i2c/tc358746.c > > > +++ b/drivers/media/i2c/tc358746.c > > > @@ -1476,9 +1476,6 @@ static int tc358746_async_register(struct tc358746 *tc358746) > > > if (err) > > > goto err_cleanup; > > > > > > - tc358746->sd.fwnode = fwnode_graph_get_endpoint_by_id( > > > - dev_fwnode(tc358746->sd.dev), TC358746_SOURCE, 0, 0); > > > - > > > err = v4l2_async_register_subdev(&tc358746->sd); > > > if (err) > > > goto err_unregister; > > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > > > index 6dd426c2ca68..13fe0bdc70b6 100644 > > > --- a/drivers/media/v4l2-core/v4l2-async.c > > > +++ b/drivers/media/v4l2-core/v4l2-async.c > > > @@ -86,84 +86,33 @@ match_fwnode_one(struct v4l2_async_notifier *notifier, > > > struct v4l2_subdev *sd, struct fwnode_handle *sd_fwnode, > > > struct v4l2_async_subdev *asd) > > > { > > > - struct fwnode_handle *other_fwnode; > > > - struct fwnode_handle *dev_fwnode; > > > - bool asd_fwnode_is_ep; > > > - bool sd_fwnode_is_ep; > > > - struct device *dev; > > > + struct fwnode_handle *asd_dev_fwnode; > > > + bool ret; > > > > > > dev_dbg(sd->dev, "async: fwnode match: need %pfw, trying %pfw\n", > > > sd_fwnode, asd->match.fwnode); > > > > > > - /* > > > - * Both the subdev and the async subdev can provide either an endpoint > > > - * fwnode or a device fwnode. Start with the simple case of direct > > > - * fwnode matching. > > > - */ > > > if (sd_fwnode == asd->match.fwnode) { > > > dev_dbg(sd->dev, "async: direct match found\n"); > > > return true; > > > } > > > > > > - /* > > > - * Otherwise, check if the sd fwnode and the asd fwnode refer to an > > > - * endpoint or a device. If they're of the same type, there's no match. > > > - * Technically speaking this checks if the nodes refer to a connected > > > - * endpoint, which is the simplest check that works for both OF and > > > - * ACPI. This won't make a difference, as drivers should not try to > > > - * match unconnected endpoints. > > > - */ > > > - sd_fwnode_is_ep = fwnode_graph_is_endpoint(sd_fwnode); > > > - asd_fwnode_is_ep = fwnode_graph_is_endpoint(asd->match.fwnode); > > > - > > > - if (sd_fwnode_is_ep == asd_fwnode_is_ep) { > > > - dev_dbg(sd->dev, "async: matching node types\n"); > > > + if (!fwnode_graph_is_endpoint(asd->match.fwnode)) { > > > + dev_dbg(sd->dev, > > > + "async: async subdev fwnode not endpoint, no match\n"); > > > > As per the previous patch I would just say "direct match failed"; > > > > > return false; > > > } > > > > > > - /* > > > - * The sd and asd fwnodes are of different types. Get the device fwnode > > > - * parent of the endpoint fwnode, and compare it with the other fwnode. > > > - */ > > > - if (sd_fwnode_is_ep) { > > > - dev_fwnode = fwnode_graph_get_port_parent(sd_fwnode); > > > - other_fwnode = asd->match.fwnode; > > > - } else { > > > - dev_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode); > > > - other_fwnode = sd_fwnode; > > > - } > > > - > > > - dev_dbg(sd->dev, "async: fwnode compat match, need %pfw, trying %pfw\n", > > > - dev_fwnode, other_fwnode); > > > + asd_dev_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode); > > > > > > - fwnode_handle_put(dev_fwnode); > > > + ret = sd_fwnode == asd_dev_fwnode; > > > > > > - if (dev_fwnode != other_fwnode) { > > > - dev_dbg(sd->dev, "async: compat match not found\n"); > > > - return false; > > > - } > > > + fwnode_handle_put(asd_dev_fwnode); > > > > > > - /* > > > - * We have a heterogeneous match. Retrieve the struct device of the side > > > - * that matched on a device fwnode to print its driver name. > > > - */ > > > - if (sd_fwnode_is_ep) > > > - dev = notifier->v4l2_dev ? notifier->v4l2_dev->dev > > > - : notifier->sd->dev; > > > - else > > > - dev = sd->dev; > > > - > > > - if (dev && dev->driver) { > > > - if (sd_fwnode_is_ep) > > > - dev_warn(dev, "Driver %s uses device fwnode, incorrect match may occur\n", > > > - dev->driver->name); > > > - dev_notice(dev, "Consider updating driver %s to match on endpoints\n", > > > - dev->driver->name); > > > - } > > > - > > > - dev_dbg(sd->dev, "async: compat match found\n"); > > > + dev_dbg(sd->dev, "async: device--endpoint match %sfound\n", > > > > double '--' > > > > However now that I think about it, a subdev will be matched agains a > > rather large number of async subdevs and most attempts will fail.. do > > we want a printout for each of them ? > > Note that this will only happen if these debug prints are enabled. There > was recently a case where they would have been useful. > > > > > > > > + ret ? "" : "not "); > > > > > > - return true; > > > + return ret; > > > } > > > > > > static bool match_fwnode(struct v4l2_async_notifier *notifier, > > > @@ -804,12 +753,19 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd) > > > int ret; > > > > > > /* > > > - * No reference taken. The reference is held by the device > > > - * (struct v4l2_subdev.dev), and async sub-device does not > > > - * exist independently of the device at any point of time. > > > + * No reference taken. The reference is held by the device (struct > > > + * v4l2_subdev.dev), and async sub-device does not exist independently > > > + * of the device at any point of time. > > > + * > > > + * The async sub-device shall always be registered for its device node, > > > + * not the endpoint node. Issue a warning in that case. Once there is > > > + * certainty no driver no longer does this, remove the warning (and > > > + * compatibility code) below. > > > */ > > > if (!sd->fwnode && sd->dev) > > > sd->fwnode = dev_fwnode(sd->dev); > > > + else if (WARN_ON(fwnode_graph_is_endpoint(sd->fwnode))) > > > + sd->fwnode = fwnode_graph_get_port_parent(sd->fwnode); > > > > > > mutex_lock(&list_lock); > > > > > -- > Kind regards, > > Sakari Ailus
Hi Niklas, On Mon, Apr 24, 2023 at 09:20:22PM +0200, Niklas Söderlund wrote: > Hi Sakari, > > On 2023-04-14 14:07:40 +0300, Sakari Ailus wrote: > > Hi Jacopo, > > > > On Thu, Apr 13, 2023 at 06:50:04PM +0200, Jacopo Mondi wrote: > > > Hi Sakari > > > > > > On Thu, Mar 30, 2023 at 02:58:38PM +0300, Sakari Ailus wrote: > > > > V4L2 async sub-device matching originally used the device nodes only. > > > > Endpoint nodes were taken into use instead as using the device nodes was > > > > problematic for it was in some cases ambiguous which link might have been > > > > in question. > > > > > > > > There is however no need to use endpoint nodes on both sides, as the async > > > > sub-device's fwnode can always be trivially obtained using > > > > fwnode_graph_get_remote_endpoint() when needed while what counts is > > > > whether or not the link is between two device nodes, i.e. the device nodes > > > > match. > > > > > > > > > > As you know I'm a bit debated. > > > > > > Strict endpoint-matching requires one subdev to be registed per each > > > endpoint, and this is tedious for drivers that have to register a > > > subdev for each of its endpoints > > > > > > Allowing a subdev to be matched multiple times on different endpoints > > > gives a way for lazy drivers to take a shortcut and simplify their > > > topologies to a single subdev, when they would actually need more. > > > > I'd say this is really about interface design, not being "lazy". It depends > > on the sub-device. Ideally the framework should be also as easy for drivers > > drivers to use as possible. > > > > What is not supported, though, is multiple sub-devices with a single device > > node. Do we need that? At least I don't think I came across a driver that > > would. > > If I understand you correctly about multiple sub-device from a single > device node, this exists today. The ADV748x driver have a single device > node in DT and register multiple sub-devices, one for each source > endpoint. > > The ADV748x have two CSI-2 transmitters, one 4-lane and one 1-lane as > well as two different video capture "ports" one HDMI and one CVBS. Both > capture ports can be active at the same time and routed internally > inside the ADV748x to the two different CSI-2 transmitters. > > In order todo this the ADV748x register multiple subdevices and modifies > the fwnode to be the endpoint instead of the device node. So the change > in this patch for ADV748x driver would break that driver. Ah, indeed. I guess I'll need to support that case as well then. It doesn't seem to be troublesome to implement, but I'm tempted making it a special case: every other driver would apparently be fine matching with device fwnode whereas doing endpoint-to-endpoint matching adds complexity to the drivers. This patch removes about 100 lines of rather ugly code largely from v4l2-async. There are other issues in the set with connection-subdev relations, I'll post v2 to address those as well.
Hi Sakari, On Mon, Apr 24, 2023 at 10:33:06PM +0300, Sakari Ailus wrote: > On Mon, Apr 24, 2023 at 09:20:22PM +0200, Niklas Söderlund wrote: > > On 2023-04-14 14:07:40 +0300, Sakari Ailus wrote: > > > On Thu, Apr 13, 2023 at 06:50:04PM +0200, Jacopo Mondi wrote: > > > > On Thu, Mar 30, 2023 at 02:58:38PM +0300, Sakari Ailus wrote: > > > > > V4L2 async sub-device matching originally used the device nodes only. > > > > > Endpoint nodes were taken into use instead as using the device nodes was > > > > > problematic for it was in some cases ambiguous which link might have been > > > > > in question. > > > > > > > > > > There is however no need to use endpoint nodes on both sides, as the async > > > > > sub-device's fwnode can always be trivially obtained using > > > > > fwnode_graph_get_remote_endpoint() when needed while what counts is > > > > > whether or not the link is between two device nodes, i.e. the device nodes > > > > > match. > > > > > > > > As you know I'm a bit debated. > > > > > > > > Strict endpoint-matching requires one subdev to be registed per each > > > > endpoint, and this is tedious for drivers that have to register a > > > > subdev for each of its endpoints > > > > > > > > Allowing a subdev to be matched multiple times on different endpoints > > > > gives a way for lazy drivers to take a shortcut and simplify their > > > > topologies to a single subdev, when they would actually need more. > > > > > > I'd say this is really about interface design, not being "lazy". It depends > > > on the sub-device. Ideally the framework should be also as easy for drivers > > > drivers to use as possible. > > > > > > What is not supported, though, is multiple sub-devices with a single device > > > node. Do we need that? At least I don't think I came across a driver that > > > would. > > > > If I understand you correctly about multiple sub-device from a single > > device node, this exists today. The ADV748x driver have a single device > > node in DT and register multiple sub-devices, one for each source > > endpoint. > > > > The ADV748x have two CSI-2 transmitters, one 4-lane and one 1-lane as > > well as two different video capture "ports" one HDMI and one CVBS. Both > > capture ports can be active at the same time and routed internally > > inside the ADV748x to the two different CSI-2 transmitters. > > > > In order todo this the ADV748x register multiple subdevices and modifies > > the fwnode to be the endpoint instead of the device node. So the change > > in this patch for ADV748x driver would break that driver. > > Ah, indeed. I guess I'll need to support that case as well then. It doesn't > seem to be troublesome to implement, but I'm tempted making it a special > case: every other driver would apparently be fine matching with device > fwnode whereas doing endpoint-to-endpoint matching adds complexity to the > drivers. This patch removes about 100 lines of rather ugly code largely > from v4l2-async. It's only 50 lines from v4l2-async, and I don't think the code is uglier than the rest of the file :-) In general, I prefer implementing tricky code in the framework and simplifying drivers. I think our goals align there, the framework should do the right thing by default for the majority of cases. However, as Niklas pointed out, endpoint matching is needed for drivers that register multiple subdevs with external connections (such as the adv742x), and that's exactly why endpoint matching was added in the first place. I think this needs to be kept. > There are other issues in the set with connection-subdev relations, I'll > post v2 to address those as well.
Hi Laurent, On Tue, Apr 25, 2023 at 04:37:42AM +0300, Laurent Pinchart wrote: > Hi Sakari, > > On Mon, Apr 24, 2023 at 10:33:06PM +0300, Sakari Ailus wrote: > > On Mon, Apr 24, 2023 at 09:20:22PM +0200, Niklas Söderlund wrote: > > > On 2023-04-14 14:07:40 +0300, Sakari Ailus wrote: > > > > On Thu, Apr 13, 2023 at 06:50:04PM +0200, Jacopo Mondi wrote: > > > > > On Thu, Mar 30, 2023 at 02:58:38PM +0300, Sakari Ailus wrote: > > > > > > V4L2 async sub-device matching originally used the device nodes only. > > > > > > Endpoint nodes were taken into use instead as using the device nodes was > > > > > > problematic for it was in some cases ambiguous which link might have been > > > > > > in question. > > > > > > > > > > > > There is however no need to use endpoint nodes on both sides, as the async > > > > > > sub-device's fwnode can always be trivially obtained using > > > > > > fwnode_graph_get_remote_endpoint() when needed while what counts is > > > > > > whether or not the link is between two device nodes, i.e. the device nodes > > > > > > match. > > > > > > > > > > As you know I'm a bit debated. > > > > > > > > > > Strict endpoint-matching requires one subdev to be registed per each > > > > > endpoint, and this is tedious for drivers that have to register a > > > > > subdev for each of its endpoints > > > > > > > > > > Allowing a subdev to be matched multiple times on different endpoints > > > > > gives a way for lazy drivers to take a shortcut and simplify their > > > > > topologies to a single subdev, when they would actually need more. > > > > > > > > I'd say this is really about interface design, not being "lazy". It depends > > > > on the sub-device. Ideally the framework should be also as easy for drivers > > > > drivers to use as possible. > > > > > > > > What is not supported, though, is multiple sub-devices with a single device > > > > node. Do we need that? At least I don't think I came across a driver that > > > > would. > > > > > > If I understand you correctly about multiple sub-device from a single > > > device node, this exists today. The ADV748x driver have a single device > > > node in DT and register multiple sub-devices, one for each source > > > endpoint. > > > > > > The ADV748x have two CSI-2 transmitters, one 4-lane and one 1-lane as > > > well as two different video capture "ports" one HDMI and one CVBS. Both > > > capture ports can be active at the same time and routed internally > > > inside the ADV748x to the two different CSI-2 transmitters. > > > > > > In order todo this the ADV748x register multiple subdevices and modifies > > > the fwnode to be the endpoint instead of the device node. So the change > > > in this patch for ADV748x driver would break that driver. > > > > Ah, indeed. I guess I'll need to support that case as well then. It doesn't > > seem to be troublesome to implement, but I'm tempted making it a special > > case: every other driver would apparently be fine matching with device > > fwnode whereas doing endpoint-to-endpoint matching adds complexity to the > > drivers. This patch removes about 100 lines of rather ugly code largely > > from v4l2-async. > > It's only 50 lines from v4l2-async, and I don't think the code is uglier > than the rest of the file :-) In general, I prefer implementing tricky > code in the framework and simplifying drivers. I think our goals align > there, the framework should do the right thing by default for the > majority of cases. However, as Niklas pointed out, endpoint matching is > needed for drivers that register multiple subdevs with external > connections (such as the adv742x), and that's exactly why endpoint > matching was added in the first place. I think this needs to be kept. I'm certainly fine with keeping functionality that driver needs and indeed did not intend to break it. However I'd like to simplify this for majority of drivers, this one can use additional APIs to get the job done. I'll address this in v2.
diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c index bd4f3fe0e309..3d830816243f 100644 --- a/drivers/media/i2c/adv748x/adv748x-csi2.c +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c @@ -300,9 +300,6 @@ int adv748x_csi2_init(struct adv748x_state *state, struct adv748x_csi2 *tx) MEDIA_ENT_F_VID_IF_BRIDGE, is_txa(tx) ? "txa" : "txb"); - /* Ensure that matching is based upon the endpoint fwnodes */ - tx->sd.fwnode = of_fwnode_handle(state->endpoints[tx->port]); - /* Register internal ops for incremental subdev registration */ tx->sd.internal_ops = &adv748x_csi2_internal_ops; diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c index 701038d6d19b..2d0f43e3fb9f 100644 --- a/drivers/media/i2c/max9286.c +++ b/drivers/media/i2c/max9286.c @@ -1051,7 +1051,6 @@ static const struct v4l2_ctrl_ops max9286_ctrl_ops = { static int max9286_v4l2_register(struct max9286_priv *priv) { struct device *dev = &priv->client->dev; - struct fwnode_handle *ep; int ret; int i; @@ -1093,25 +1092,14 @@ static int max9286_v4l2_register(struct max9286_priv *priv) if (ret) goto err_async; - ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), MAX9286_SRC_PAD, - 0, 0); - if (!ep) { - dev_err(dev, "Unable to retrieve endpoint on \"port@4\"\n"); - ret = -ENOENT; - goto err_async; - } - priv->sd.fwnode = ep; - ret = v4l2_async_register_subdev(&priv->sd); if (ret < 0) { dev_err(dev, "Unable to register subdevice\n"); - goto err_put_node; + goto err_async; } return 0; -err_put_node: - fwnode_handle_put(ep); err_async: v4l2_ctrl_handler_free(&priv->ctrls); max9286_v4l2_notifier_unregister(priv); diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c index a2263fa825b5..ea1111152285 100644 --- a/drivers/media/i2c/rdacm20.c +++ b/drivers/media/i2c/rdacm20.c @@ -567,7 +567,6 @@ static int rdacm20_initialize(struct rdacm20_device *dev) static int rdacm20_probe(struct i2c_client *client) { struct rdacm20_device *dev; - struct fwnode_handle *ep; int ret; dev = devm_kzalloc(&client->dev, sizeof(*dev), GFP_KERNEL); @@ -616,24 +615,12 @@ static int rdacm20_probe(struct i2c_client *client) if (ret < 0) goto error_free_ctrls; - ep = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev), NULL); - if (!ep) { - dev_err(&client->dev, - "Unable to get endpoint in node %pOF\n", - client->dev.of_node); - ret = -ENOENT; - goto error_free_ctrls; - } - dev->sd.fwnode = ep; - ret = v4l2_async_register_subdev(&dev->sd); if (ret) - goto error_put_node; + goto error_free_ctrls; return 0; -error_put_node: - fwnode_handle_put(ep); error_free_ctrls: v4l2_ctrl_handler_free(&dev->ctrls); error: diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c index 9ccc56c30d3b..d67cfcb2e05a 100644 --- a/drivers/media/i2c/rdacm21.c +++ b/drivers/media/i2c/rdacm21.c @@ -543,7 +543,6 @@ static int rdacm21_initialize(struct rdacm21_device *dev) static int rdacm21_probe(struct i2c_client *client) { struct rdacm21_device *dev; - struct fwnode_handle *ep; int ret; dev = devm_kzalloc(&client->dev, sizeof(*dev), GFP_KERNEL); @@ -588,24 +587,12 @@ static int rdacm21_probe(struct i2c_client *client) if (ret < 0) goto error_free_ctrls; - ep = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev), NULL); - if (!ep) { - dev_err(&client->dev, - "Unable to get endpoint in node %pOF\n", - client->dev.of_node); - ret = -ENOENT; - goto error_free_ctrls; - } - dev->sd.fwnode = ep; - ret = v4l2_async_register_subdev(&dev->sd); if (ret) - goto error_put_node; + goto error_free_ctrls; return 0; -error_put_node: - fwnode_handle_put(dev->sd.fwnode); error_free_ctrls: v4l2_ctrl_handler_free(&dev->ctrls); error: diff --git a/drivers/media/i2c/tc358746.c b/drivers/media/i2c/tc358746.c index 4063754a6732..56f2b43d4edf 100644 --- a/drivers/media/i2c/tc358746.c +++ b/drivers/media/i2c/tc358746.c @@ -1476,9 +1476,6 @@ static int tc358746_async_register(struct tc358746 *tc358746) if (err) goto err_cleanup; - tc358746->sd.fwnode = fwnode_graph_get_endpoint_by_id( - dev_fwnode(tc358746->sd.dev), TC358746_SOURCE, 0, 0); - err = v4l2_async_register_subdev(&tc358746->sd); if (err) goto err_unregister; diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c index 6dd426c2ca68..13fe0bdc70b6 100644 --- a/drivers/media/v4l2-core/v4l2-async.c +++ b/drivers/media/v4l2-core/v4l2-async.c @@ -86,84 +86,33 @@ match_fwnode_one(struct v4l2_async_notifier *notifier, struct v4l2_subdev *sd, struct fwnode_handle *sd_fwnode, struct v4l2_async_subdev *asd) { - struct fwnode_handle *other_fwnode; - struct fwnode_handle *dev_fwnode; - bool asd_fwnode_is_ep; - bool sd_fwnode_is_ep; - struct device *dev; + struct fwnode_handle *asd_dev_fwnode; + bool ret; dev_dbg(sd->dev, "async: fwnode match: need %pfw, trying %pfw\n", sd_fwnode, asd->match.fwnode); - /* - * Both the subdev and the async subdev can provide either an endpoint - * fwnode or a device fwnode. Start with the simple case of direct - * fwnode matching. - */ if (sd_fwnode == asd->match.fwnode) { dev_dbg(sd->dev, "async: direct match found\n"); return true; } - /* - * Otherwise, check if the sd fwnode and the asd fwnode refer to an - * endpoint or a device. If they're of the same type, there's no match. - * Technically speaking this checks if the nodes refer to a connected - * endpoint, which is the simplest check that works for both OF and - * ACPI. This won't make a difference, as drivers should not try to - * match unconnected endpoints. - */ - sd_fwnode_is_ep = fwnode_graph_is_endpoint(sd_fwnode); - asd_fwnode_is_ep = fwnode_graph_is_endpoint(asd->match.fwnode); - - if (sd_fwnode_is_ep == asd_fwnode_is_ep) { - dev_dbg(sd->dev, "async: matching node types\n"); + if (!fwnode_graph_is_endpoint(asd->match.fwnode)) { + dev_dbg(sd->dev, + "async: async subdev fwnode not endpoint, no match\n"); return false; } - /* - * The sd and asd fwnodes are of different types. Get the device fwnode - * parent of the endpoint fwnode, and compare it with the other fwnode. - */ - if (sd_fwnode_is_ep) { - dev_fwnode = fwnode_graph_get_port_parent(sd_fwnode); - other_fwnode = asd->match.fwnode; - } else { - dev_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode); - other_fwnode = sd_fwnode; - } - - dev_dbg(sd->dev, "async: fwnode compat match, need %pfw, trying %pfw\n", - dev_fwnode, other_fwnode); + asd_dev_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode); - fwnode_handle_put(dev_fwnode); + ret = sd_fwnode == asd_dev_fwnode; - if (dev_fwnode != other_fwnode) { - dev_dbg(sd->dev, "async: compat match not found\n"); - return false; - } + fwnode_handle_put(asd_dev_fwnode); - /* - * We have a heterogeneous match. Retrieve the struct device of the side - * that matched on a device fwnode to print its driver name. - */ - if (sd_fwnode_is_ep) - dev = notifier->v4l2_dev ? notifier->v4l2_dev->dev - : notifier->sd->dev; - else - dev = sd->dev; - - if (dev && dev->driver) { - if (sd_fwnode_is_ep) - dev_warn(dev, "Driver %s uses device fwnode, incorrect match may occur\n", - dev->driver->name); - dev_notice(dev, "Consider updating driver %s to match on endpoints\n", - dev->driver->name); - } - - dev_dbg(sd->dev, "async: compat match found\n"); + dev_dbg(sd->dev, "async: device--endpoint match %sfound\n", + ret ? "" : "not "); - return true; + return ret; } static bool match_fwnode(struct v4l2_async_notifier *notifier, @@ -804,12 +753,19 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd) int ret; /* - * No reference taken. The reference is held by the device - * (struct v4l2_subdev.dev), and async sub-device does not - * exist independently of the device at any point of time. + * No reference taken. The reference is held by the device (struct + * v4l2_subdev.dev), and async sub-device does not exist independently + * of the device at any point of time. + * + * The async sub-device shall always be registered for its device node, + * not the endpoint node. Issue a warning in that case. Once there is + * certainty no driver no longer does this, remove the warning (and + * compatibility code) below. */ if (!sd->fwnode && sd->dev) sd->fwnode = dev_fwnode(sd->dev); + else if (WARN_ON(fwnode_graph_is_endpoint(sd->fwnode))) + sd->fwnode = fwnode_graph_get_port_parent(sd->fwnode); mutex_lock(&list_lock);
V4L2 async sub-device matching originally used the device nodes only. Endpoint nodes were taken into use instead as using the device nodes was problematic for it was in some cases ambiguous which link might have been in question. There is however no need to use endpoint nodes on both sides, as the async sub-device's fwnode can always be trivially obtained using fwnode_graph_get_remote_endpoint() when needed while what counts is whether or not the link is between two device nodes, i.e. the device nodes match. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- drivers/media/i2c/adv748x/adv748x-csi2.c | 3 - drivers/media/i2c/max9286.c | 14 +--- drivers/media/i2c/rdacm20.c | 15 +---- drivers/media/i2c/rdacm21.c | 15 +---- drivers/media/i2c/tc358746.c | 3 - drivers/media/v4l2-core/v4l2-async.c | 86 ++++++------------------ 6 files changed, 24 insertions(+), 112 deletions(-)