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 <[email protected]>
---
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(
+ 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++) {
+ 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;
+
+ return 0;
+
+out_unregister:
+ v4l2_async_notifier_unregister(notifier);
+out_cleanup:
+ v4l2_async_notifier_cleanup(notifier);
+ kfree(notifier);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(v4l2_async_register_fwnode_subdev);
+
MODULE_LICENSE("GPL");
MODULE_AUTHOR("Sakari Ailus <[email protected]>");
MODULE_AUTHOR("Sylwester Nawrocki <[email protected]>");
diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
index ea7a8b2..031ebb0 100644
--- a/include/media/v4l2-fwnode.h
+++ b/include/media/v4l2-fwnode.h
@@ -23,6 +23,7 @@
#include <linux/types.h>
#include <media/v4l2-mediabus.h>
+#include <media/v4l2-subdev.h>
struct fwnode_handle;
struct v4l2_async_notifier;
@@ -360,4 +361,41 @@ int v4l2_async_notifier_parse_fwnode_endpoints_by_port(
int v4l2_async_notifier_parse_fwnode_sensor_common(
struct device *dev, struct v4l2_async_notifier *notifier);
+/**
+ * v4l2_async_register_fwnode_subdev - registers a sub-device to the
+ * asynchronous sub-device framework
+ * and parses fwnode endpoints
+ *
+ * @sd: pointer to struct &v4l2_subdev
+ * @asd_struct_size: size of the driver's async sub-device struct, including
+ * sizeof(struct v4l2_async_subdev). The &struct
+ * v4l2_async_subdev shall be the first member of
+ * the driver's async sub-device struct, i.e. both
+ * begin at the same memory address.
+ * @ports: array of port id's to parse for fwnode endpoints. If NULL, will
+ * parse all ports owned by the sub-device.
+ * @num_ports: number of ports in @ports array. Ignored if @ports is NULL.
+ * @parse_endpoint: Driver's callback function called on each V4L2 fwnode
+ * endpoint. Optional.
+ *
+ * This function is just like v4l2_async_register_subdev() with the
+ * exception that calling it will also allocate a notifier for the
+ * sub-device, parse the sub-device's firmware node endpoints using
+ * v4l2_async_notifier_parse_fwnode_endpoints() or
+ * v4l2_async_notifier_parse_fwnode_endpoints_by_port(), and
+ * registers the sub-device notifier. The sub-device is similarly
+ * unregistered by calling v4l2_async_unregister_subdev().
+ *
+ * While registered, the subdev module is marked as in-use.
+ *
+ * An error is returned if the module is no longer loaded on any attempts
+ * to register it.
+ */
+int v4l2_async_register_fwnode_subdev(
+ 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));
+
#endif /* _V4L2_FWNODE_H */
--
2.7.4
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 <[email protected]>
> ---
> 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
> + 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.
Also the caller has to allocate an array of "ports" and keep track of it
just to pass it to this function and I don't see a way to set the
notifier's ops before the notifier gets registered here below.
> + 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()
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?
Thanks
j
> +
> + return 0;
> +
> +out_unregister:
> + v4l2_async_notifier_unregister(notifier);
> +out_cleanup:
> + v4l2_async_notifier_cleanup(notifier);
> + kfree(notifier);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_async_register_fwnode_subdev);
> +
> MODULE_LICENSE("GPL");
> MODULE_AUTHOR("Sakari Ailus <[email protected]>");
> MODULE_AUTHOR("Sylwester Nawrocki <[email protected]>");
> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> index ea7a8b2..031ebb0 100644
> --- a/include/media/v4l2-fwnode.h
> +++ b/include/media/v4l2-fwnode.h
> @@ -23,6 +23,7 @@
> #include <linux/types.h>
>
> #include <media/v4l2-mediabus.h>
> +#include <media/v4l2-subdev.h>
>
> struct fwnode_handle;
> struct v4l2_async_notifier;
> @@ -360,4 +361,41 @@ int v4l2_async_notifier_parse_fwnode_endpoints_by_port(
> int v4l2_async_notifier_parse_fwnode_sensor_common(
> struct device *dev, struct v4l2_async_notifier *notifier);
>
> +/**
> + * v4l2_async_register_fwnode_subdev - registers a sub-device to the
> + * asynchronous sub-device framework
> + * and parses fwnode endpoints
> + *
> + * @sd: pointer to struct &v4l2_subdev
> + * @asd_struct_size: size of the driver's async sub-device struct, including
> + * sizeof(struct v4l2_async_subdev). The &struct
> + * v4l2_async_subdev shall be the first member of
> + * the driver's async sub-device struct, i.e. both
> + * begin at the same memory address.
> + * @ports: array of port id's to parse for fwnode endpoints. If NULL, will
> + * parse all ports owned by the sub-device.
> + * @num_ports: number of ports in @ports array. Ignored if @ports is NULL.
> + * @parse_endpoint: Driver's callback function called on each V4L2 fwnode
> + * endpoint. Optional.
> + *
> + * This function is just like v4l2_async_register_subdev() with the
> + * exception that calling it will also allocate a notifier for the
> + * sub-device, parse the sub-device's firmware node endpoints using
> + * v4l2_async_notifier_parse_fwnode_endpoints() or
> + * v4l2_async_notifier_parse_fwnode_endpoints_by_port(), and
> + * registers the sub-device notifier. The sub-device is similarly
> + * unregistered by calling v4l2_async_unregister_subdev().
> + *
> + * While registered, the subdev module is marked as in-use.
> + *
> + * An error is returned if the module is no longer loaded on any attempts
> + * to register it.
> + */
> +int v4l2_async_register_fwnode_subdev(
> + 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));
> +
> #endif /* _V4L2_FWNODE_H */
> --
> 2.7.4
>
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 <[email protected]>
> > ---
> > 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
>
> > + 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.
>
> Also the caller has to allocate an array of "ports" and keep track of it
> just to pass it to this function 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.
>
> > + 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.
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.
--
Kind regards,
Sakari Ailus
[email protected]
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 <[email protected]>
> > > ---
> > > 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
> >
> > > + 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?
> >
> > Also the caller has to allocate an array of "ports" and keep track of it
> > just to pass it to this function 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
> [email protected]
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 <[email protected]>
>>>> ---
>>>> 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
>> [email protected]