Re: [PATCH v3 4/4] device property: Move fwnode graph ops to firmware specific locations

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Hi Rob,

Rob Herring wrote:
On Thu, Mar 16, 2017 at 8:34 AM, Sakari Ailus
<sakari.ailus@xxxxxxxxxxxxxxx> wrote:
Move firmware specific implementations of the fwnode graph operations to
firmware specific locations.

Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
---
 drivers/acpi/property.c | 61 ++++++++++++++++++++++++++++++++
 drivers/base/property.c | 92 +++----------------------------------------------
 drivers/of/base.c       | 70 +++++++++++++++++++++++++++++++++++++
 include/linux/fwnode.h  | 16 +++++++++
 4 files changed, 152 insertions(+), 87 deletions(-)

As I've mentioned before, this is just propagating APIs I would like
to get rid of. See my series cleaning up DRM users[1].

Why would you want to get rid of the existing APIs? They do fit well for the purpose they're currently used for on V4L2, at least. That could be because the current API was implemented *for* V4L2. If you'd like to add a new one, then at least I'd very much like to keep what exists at the moment.

Please see e.g. isp_fwnodes_parse() in drivers/media/platform/omap3isp/isp.c . That function is generic enough to be moved to the V4L2 framework, with a driver callback to handle driver specific matters.


diff --git a/drivers/of/base.c b/drivers/of/base.c
index e483dd1..a8686a8 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -2624,6 +2624,71 @@ static struct fwnode_handle *of_fwnode_get_named_child_node(
        return NULL;
 }

+static struct fwnode_handle *of_fwnode_graph_get_next_endpoint(
+       struct fwnode_handle *fwnode, struct fwnode_handle *prev)
+{
+       struct device_node *node;
+
+       node = of_graph_get_next_endpoint(to_of_node(fwnode),
+                                         to_of_node(prev));
+       if (node)
+               return of_fwnode_handle(node);
+
+       return NULL;
+}

For example, most callers of of_graph_get_next_endpoint really just
want port=0, endpoint=0 and then they want the remote node. As the
assignment of port and endpoint numbers are fixed for a device,
drivers should be explicit about which port/endpoint they want. The
actual handle to either the port or endpoint nodes are mostly
irrelevant. We only really need the endpoint node handles when we have
properties in the endpoints.

I certainly don't mind adding a new API if there are use cases the existing one does not serve well. But please don't remove the enumeration API. It's simply way more practical.

If you have one or two ports and an endpoint in each, perhaps it's ok just to try check if one more more of them are around. Some devices may have e.g. six ports today and I don't think that number will go down in the future, it's rather going to increase.

The caller can also find the endpoints without knowing the endpoint numbers in advance. In a number of devices' DT binding documentation it is not explicitly stated that endpoint IDs have a special meaning. I'm not fully certain everything would keep on working fine if the drivers that previously did not care about endpoint numbers would start e.g. requiring zero from now on.

An analogous interface is the _ENUM IOCTLs in V4L2, with the difference that it's towards the user space.

--
Kind regards,

Sakari Ailus
sakari.ailus@xxxxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux