Re: [PATCH v6 06/17] media: v4l2-fwnode: Add a convenience function for registering subdevs with notifiers
From: Steve Longerbeam
Date: Thu Sep 13 2018 - 20:58:20 EST
Hi Jacopo,
On 09/13/2018 05:58 AM, jacopo mondi wrote:
Hi Sakari,
On Thu, Sep 13, 2018 at 03:44:25PM +0300, Sakari Ailus wrote:
Hi Jacopo,
On Thu, Sep 13, 2018 at 12:37:27PM +0200, jacopo mondi wrote:
Hi Steve,
On Mon, Jul 09, 2018 at 03:39:06PM -0700, Steve Longerbeam wrote:
Adds v4l2_async_register_fwnode_subdev(), which is a convenience function
for parsing a sub-device's fwnode port endpoints for connected remote
sub-devices, registering a sub-device notifier, and then registering
the sub-device itself.
Signed-off-by: Steve Longerbeam <steve_longerbeam@xxxxxxxxxx>
---
Changes since v5:
- add call to v4l2_async_notifier_init().
Changes since v4:
- none
Changes since v3:
- remove support for port sub-devices, such sub-devices will have to
role their own.
Changes since v2:
- fix error-out path in v4l2_async_register_fwnode_subdev() that forgot
to put device.
Changes since v1:
- add #include <media/v4l2-subdev.h> to v4l2-fwnode.h for
'struct v4l2_subdev' declaration.
---
drivers/media/v4l2-core/v4l2-fwnode.c | 64 +++++++++++++++++++++++++++++++++++
include/media/v4l2-fwnode.h | 38 +++++++++++++++++++++
2 files changed, 102 insertions(+)
diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
index 67ad333..94d867a 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c
@@ -872,6 +872,70 @@ int v4l2_async_register_subdev_sensor_common(struct v4l2_subdev *sd)
}
EXPORT_SYMBOL_GPL(v4l2_async_register_subdev_sensor_common);
+int v4l2_async_register_fwnode_subdev(
The meat of this function is to register a subdev with a notifier,
so I would make it clear in the function name which is otherwise
misleading
Yes, I struggled with how to name this function without making it
ridiculously long.
+ struct v4l2_subdev *sd, size_t asd_struct_size,
+ unsigned int *ports, unsigned int num_ports,
+ int (*parse_endpoint)(struct device *dev,
+ struct v4l2_fwnode_endpoint *vep,
+ struct v4l2_async_subdev *asd))
+{
+ struct v4l2_async_notifier *notifier;
+ struct device *dev = sd->dev;
+ struct fwnode_handle *fwnode;
+ int ret;
+
+ if (WARN_ON(!dev))
+ return -ENODEV;
+
+ fwnode = dev_fwnode(dev);
+ if (!fwnode_device_is_available(fwnode))
+ return -ENODEV;
+
+ notifier = kzalloc(sizeof(*notifier), GFP_KERNEL);
+ if (!notifier)
+ return -ENOMEM;
+
+ v4l2_async_notifier_init(notifier);
+
+ if (!ports) {
+ ret = v4l2_async_notifier_parse_fwnode_endpoints(
+ dev, notifier, asd_struct_size, parse_endpoint);
+ if (ret < 0)
+ goto out_cleanup;
+ } else {
+ unsigned int i;
+
+ for (i = 0; i < num_ports; i++) {
It's not particularly exciting to iterate on pointers received from
callers without checking for num_ports first.
The loop is not executed if num_ports is zero, so I don't see a problem
with that.
I know this is internal drivers API and failures are meant to be
catched early in development, but what if the actual number of ports
identifiers is < then the num_ports parameter?
see below.
Also the caller has to allocate an array of "ports" and keep track of it
just to pass it to this function
I agree that it is cumbersome to require callers to allocate a ports
array. Perhaps the ports array and num_ports could be replaced by a
u64 bit mask, but that would limit port ID's to 0 - 63.
Steve
and I don't see a way to set the
notifier's ops before the notifier gets registered here below.
True; this can be seen as an omission but quite a few drivers have no need
for this either. It could be added later on --- I think it'd make perfect
sense.
In a 'notifier configuration' structure that gather these and existing
function parameters together as you suggested...
+ ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port(
+ dev, notifier, asd_struct_size,
+ ports[i], parse_endpoint);
+ if (ret < 0)
+ goto out_cleanup;
+ }
+ }
+
+ ret = v4l2_async_subdev_notifier_register(sd, notifier);
+ if (ret < 0)
+ goto out_cleanup;
+
+ ret = v4l2_async_register_subdev(sd);
+ if (ret < 0)
+ goto out_unregister;
+
+ sd->subdev_notifier = notifier;
This is set already by v4l2_async_subdev_notifier_register()
The same pattern is actually present in
v4l2_async_register_subdev_sensor_common(). It's used in unregistration
that can only happen after the registration, i.e. this function, has
completed.
In general, I have doubts this function is really needed. It requires
the caller to reserve memory just to pass down a list of intergers,
and there is no way to set subdev ops.
Could you have a look at how drivers/media/platform/rcar-vin/rcar-csi2.c
registers a subdevice and an associated notifier and see if in your
opinion it can be implemented in the same way in your imx csi/csi2 driver,
or you still like this one most?
I was actually thinking of changing this later on a bit. I came to think of
this after picking up the patchset to my tree... oh well.
This function is meant for cases where you have multiple ports. That's not
working very nicely at the moment, and even with my patches, you can't pass
default configuration to e.g. v4l2_async_notifier_parse_fwnode_endpoints().
So there's definitely work to do. I'd like to move the details of parsing
out of drivers; every driver is doing almost the same but just in a little
bit different way.
I see...
The arguments should to be put into a struct. That way we get rid of a very
long series of hard-to-read function arguments, as well as we don't need to
change every caller when the function gets something new and interesting to
do.
Right now the entire patchset is so big (40 patches) that I'd prefer to get
it in unless serious issues are found, and proceed the development on top.
Sure, please go ahead and thanks for the reply.
Cheers
j
--
Kind regards,
Sakari Ailus
sakari.ailus@xxxxxxxxxxxxxxx