Re: [PATCH v7 04/18] v4l: fwnode: Support generic parsing of graph endpoints in a device

[Sakari Ailus]

On Mon, Sep 04, 2017 at 07:37:09PM +0200, Hans Verkuil wrote:
> On 09/04/2017 05:54 PM, Sakari Ailus wrote:
> >>> +/**
> >>> + * v4l2_async_notifier_parse_fwnode_endpoints - Parse V4L2 fwnode 
> >>> endpoints in a
> >>> + *   device node
> >>> + * @dev: the device the endpoints of which are to be parsed
> >>> + * @notifier: notifier for @dev
> >>> + * @asd_struct_size: size of the driver's async sub-device struct, 
> >>> including
> >>> + *sizeof(struct v4l2_async_subdev). The 
> >>> + *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.
> >>> + * @parse_endpoint: Driver's callback function called on each V4L2 fwnode
> >>> + *   endpoint. Optional.
> >>> + *   Return: %0 on success
> >>> + *   %-ENOTCONN if the endpoint is to be skipped 
> >>> but this
> >>> + *  should not be considered as an 
> >>> error
> >>> + *   %-EINVAL if the endpoint configuration is 
> >>> invalid
> >>> + *
> >>> + * Parse the fwnode endpoints of the @dev device and populate the async 
> >>> sub-
> >>> + * devices array of the notifier. The @parse_endpoint callback function 
> >>> is
> >>> + * called for each endpoint with the corresponding async sub-device 
> >>> pointer to
> >>> + * let the caller initialize the driver-specific part of the async 
> >>> sub-device
> >>> + * structure.
> >>> + *
> >>> + * The notifier memory shall be zeroed before this function is called on 
> >>> the
> >>> + * notifier.
> >>> + *
> >>> + * This function may not be called on a registered notifier and may be 
> >>> called on
> >>> + * a notifier only once. When using this function, the user may not 
> >>> access the
> >>> + * notifier's subdevs array nor change notifier's num_subdevs field, 
> >>> these are
> >>> + * reserved for the framework's internal use only.
> >>
> >> I don't think the sentence "When using...only" makes any sense. How would 
> >> you
> >> even be able to access any of the notifier fields? You don't have access 
> >> to it
> >> from the parse_endpoint callback.
> > 
> > Not from the parse_endpoint callback. The notifier is first set up by the
> > driver, and this text applies to that --- whether or not parse_endpoint is
> > given.
> 
> What you mean is "After calling this function..." since 
> v4l2_async_notifier_release()
> needs this to release all the memory.

Right, that's a good point.

notifier->subdevs may be allocated by the driver as well, so
v4l2_async_notifier_release() must take that into account.
notifier->max_subdevs should be good for that.

-- 
Sakari Ailus
e-mail: sakari.ai...@iki.fi

Re: [PATCH v7 04/18] v4l: fwnode: Support generic parsing of graph endpoints in a device

[Hans Verkuil]

On 09/04/2017 05:54 PM, Sakari Ailus wrote:
>>> +/**
>>> + * v4l2_async_notifier_parse_fwnode_endpoints - Parse V4L2 fwnode 
>>> endpoints in a
>>> + * device node
>>> + * @dev: the device the endpoints of which are to be parsed
>>> + * @notifier: notifier for @dev
>>> + * @asd_struct_size: size of the driver's async sub-device struct, 
>>> including
>>> + *  sizeof(struct v4l2_async_subdev). The 
>>> + *  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.
>>> + * @parse_endpoint: Driver's callback function called on each V4L2 fwnode
>>> + * endpoint. Optional.
>>> + * Return: %0 on success
>>> + * %-ENOTCONN if the endpoint is to be skipped but this
>>> + *should not be considered as an error
>>> + * %-EINVAL if the endpoint configuration is invalid
>>> + *
>>> + * Parse the fwnode endpoints of the @dev device and populate the async 
>>> sub-
>>> + * devices array of the notifier. The @parse_endpoint callback function is
>>> + * called for each endpoint with the corresponding async sub-device 
>>> pointer to
>>> + * let the caller initialize the driver-specific part of the async 
>>> sub-device
>>> + * structure.
>>> + *
>>> + * The notifier memory shall be zeroed before this function is called on 
>>> the
>>> + * notifier.
>>> + *
>>> + * This function may not be called on a registered notifier and may be 
>>> called on
>>> + * a notifier only once. When using this function, the user may not access 
>>> the
>>> + * notifier's subdevs array nor change notifier's num_subdevs field, these 
>>> are
>>> + * reserved for the framework's internal use only.
>>
>> I don't think the sentence "When using...only" makes any sense. How would you
>> even be able to access any of the notifier fields? You don't have access to 
>> it
>> from the parse_endpoint callback.
> 
> Not from the parse_endpoint callback. The notifier is first set up by the
> driver, and this text applies to that --- whether or not parse_endpoint is
> given.

What you mean is "After calling this function..." since 
v4l2_async_notifier_release()
needs this to release all the memory.

I'll take another look at this text when I see v8.

Regards,

Hans

> 
>>
>> I think it can just be dropped.
>>
>>> + *
>>> + * The @struct v4l2_fwnode_endpoint passed to the callback function
>>> + * @parse_endpoint is released once the function is finished. If there is 
>>> a need
>>> + * to retain that configuration, the user needs to allocate memory for it.
>>> + *
>>> + * Any notifier populated using this function must be released with a call 
>>> to
>>> + * v4l2_async_notifier_release() after it has been unregistered and the 
>>> async
>>> + * sub-devices are no longer in use.
>>> + *
>>> + * Return: %0 on success, including when no async sub-devices are found
>>> + *%-ENOMEM if memory allocation failed
>>> + *%-EINVAL if graph or endpoint parsing failed
>>> + *Other error codes as returned by @parse_endpoint
>>> + */
>>> +int v4l2_async_notifier_parse_fwnode_endpoints(
>>> +   struct device *dev, struct v4l2_async_notifier *notifier,
>>> +   size_t asd_struct_size,
>>> +   int (*parse_endpoint)(struct device *dev,
>>> + struct v4l2_fwnode_endpoint *vep,
>>> + struct v4l2_async_subdev *asd));
>>> +
>>>  #endif /* _V4L2_FWNODE_H */
>>>
> 

Re: [PATCH v7 04/18] v4l: fwnode: Support generic parsing of graph endpoints in a device

[Sakari Ailus]

Hi Hans,

On Mon, Sep 04, 2017 at 03:19:10PM +0200, Hans Verkuil wrote:
> On 09/03/2017 07:49 PM, Sakari Ailus wrote:
> > The current practice is that drivers iterate over their endpoints and
> > parse each endpoint separately. This is very similar in a number of
> > drivers, implement a generic function for the job. Driver specific matters
> > can be taken into account in the driver specific callback.
> > 
> > Signed-off-by: Sakari Ailus 
> > ---
> >  drivers/media/v4l2-core/v4l2-async.c  |  16 
> >  drivers/media/v4l2-core/v4l2-fwnode.c | 136 
> > ++
> >  include/media/v4l2-async.h|  24 +-
> >  include/media/v4l2-fwnode.h   |  53 +
> >  4 files changed, 227 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c 
> > b/drivers/media/v4l2-core/v4l2-async.c
> > index 851f128eba22..6740a241d4a1 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -22,6 +22,7 @@
> >  
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  
> >  static bool match_i2c(struct v4l2_subdev *sd, struct v4l2_async_subdev 
> > *asd)
> > @@ -278,6 +279,21 @@ void v4l2_async_notifier_unregister(struct 
> > v4l2_async_notifier *notifier)
> >  }
> >  EXPORT_SYMBOL(v4l2_async_notifier_unregister);
> >  
> > +void v4l2_async_notifier_release(struct v4l2_async_notifier *notifier)
> > +{
> > +   unsigned int i;
> > +
> > +   for (i = 0; i < notifier->num_subdevs; i++)
> > +   kfree(notifier->subdevs[i]);
> > +
> > +   notifier->max_subdevs = 0;
> > +   notifier->num_subdevs = 0;
> > +
> > +   kvfree(notifier->subdevs);
> > +   notifier->subdevs = NULL;
> > +}
> > +EXPORT_SYMBOL_GPL(v4l2_async_notifier_release);
> > +
> >  int v4l2_async_register_subdev(struct v4l2_subdev *sd)
> >  {
> > struct v4l2_async_notifier *notifier;
> > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c 
> > b/drivers/media/v4l2-core/v4l2-fwnode.c
> > index 706f9e7b90f1..f8c7a9bc6a58 100644
> > --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > @@ -19,6 +19,7 @@
> >   */
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -26,6 +27,7 @@
> >  #include 
> >  #include 
> >  
> > +#include 
> >  #include 
> >  
> >  enum v4l2_fwnode_bus_type {
> > @@ -313,6 +315,140 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link 
> > *link)
> >  }
> >  EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);
> >  
> > +static int v4l2_async_notifier_realloc(struct v4l2_async_notifier 
> > *notifier,
> > +  unsigned int max_subdevs)
> > +{
> > +   struct v4l2_async_subdev **subdevs;
> > +
> > +   if (max_subdevs <= notifier->max_subdevs)
> > +   return 0;
> > +
> > +   subdevs = kvmalloc_array(
> > +   max_subdevs, sizeof(*notifier->subdevs),
> > +   GFP_KERNEL | __GFP_ZERO);
> > +   if (!subdevs)
> > +   return -ENOMEM;
> > +
> > +   if (notifier->subdevs) {
> > +   memcpy(subdevs, notifier->subdevs,
> > +  sizeof(*subdevs) * notifier->num_subdevs);
> > +
> > +   kvfree(notifier->subdevs);
> > +   }
> > +
> > +   notifier->subdevs = subdevs;
> > +   notifier->max_subdevs = max_subdevs;
> > +
> > +   return 0;
> > +}
> > +
> > +static int v4l2_async_notifier_fwnode_parse_endpoint(
> > +   struct device *dev, struct v4l2_async_notifier *notifier,
> > +   struct fwnode_handle *endpoint, unsigned int asd_struct_size,
> > +   int (*parse_endpoint)(struct device *dev,
> > +   struct v4l2_fwnode_endpoint *vep,
> > +   struct v4l2_async_subdev *asd))
> > +{
> > +   struct v4l2_async_subdev *asd;
> > +   struct v4l2_fwnode_endpoint *vep;
> > +   int ret = 0;
> > +
> > +   asd = kzalloc(asd_struct_size, GFP_KERNEL);
> > +   if (!asd)
> > +   return -ENOMEM;
> > +
> > +   asd->match.fwnode.fwnode =
> > +   fwnode_graph_get_remote_port_parent(endpoint);
> > +   if (!asd->match.fwnode.fwnode) {
> > +   dev_warn(dev, "bad remote port parent\n");
> > +   ret = -EINVAL;
> > +   goto out_err;
> > +   }
> > +
> > +   /* Ignore endpoints the parsing of which failed. */
> > +   vep = v4l2_fwnode_endpoint_alloc_parse(endpoint);
> > +   if (IS_ERR(vep)) {
> > +   ret = PTR_ERR(vep);
> > +   dev_warn(dev, "unable to parse V4L2 fwnode endpoint (%d)\n",
> > +ret);
> > +   goto out_err;
> > +   }
> > +
> > +   ret = parse_endpoint ? parse_endpoint(dev, vep, asd) : 0;
> 
> How likely is it that parse_endpoint will be NULL? I ask because if it is
> common, then you could put everything from v4l2_fwnode_endpoint_alloc_parse
> until just before 'asd->match_type = V4L2_ASYNC_MATCH_FWNODE;' under
> 'if (parse_endpoint) {'.
> 
> The disadvantage is that you won't see parse errors, but on the other hand
> nobody 

Re: [PATCH v7 04/18] v4l: fwnode: Support generic parsing of graph endpoints in a device

[Hans Verkuil]

On 09/03/2017 07:49 PM, Sakari Ailus wrote:
> The current practice is that drivers iterate over their endpoints and
> parse each endpoint separately. This is very similar in a number of
> drivers, implement a generic function for the job. Driver specific matters
> can be taken into account in the driver specific callback.
> 
> Signed-off-by: Sakari Ailus 
> ---
>  drivers/media/v4l2-core/v4l2-async.c  |  16 
>  drivers/media/v4l2-core/v4l2-fwnode.c | 136 
> ++
>  include/media/v4l2-async.h|  24 +-
>  include/media/v4l2-fwnode.h   |  53 +
>  4 files changed, 227 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c 
> b/drivers/media/v4l2-core/v4l2-async.c
> index 851f128eba22..6740a241d4a1 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -22,6 +22,7 @@
>  
>  #include 
>  #include 
> +#include 
>  #include 
>  
>  static bool match_i2c(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
> @@ -278,6 +279,21 @@ void v4l2_async_notifier_unregister(struct 
> v4l2_async_notifier *notifier)
>  }
>  EXPORT_SYMBOL(v4l2_async_notifier_unregister);
>  
> +void v4l2_async_notifier_release(struct v4l2_async_notifier *notifier)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < notifier->num_subdevs; i++)
> + kfree(notifier->subdevs[i]);
> +
> + notifier->max_subdevs = 0;
> + notifier->num_subdevs = 0;
> +
> + kvfree(notifier->subdevs);
> + notifier->subdevs = NULL;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_async_notifier_release);
> +
>  int v4l2_async_register_subdev(struct v4l2_subdev *sd)
>  {
>   struct v4l2_async_notifier *notifier;
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c 
> b/drivers/media/v4l2-core/v4l2-fwnode.c
> index 706f9e7b90f1..f8c7a9bc6a58 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -19,6 +19,7 @@
>   */
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -26,6 +27,7 @@
>  #include 
>  #include 
>  
> +#include 
>  #include 
>  
>  enum v4l2_fwnode_bus_type {
> @@ -313,6 +315,140 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link)
>  }
>  EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);
>  
> +static int v4l2_async_notifier_realloc(struct v4l2_async_notifier *notifier,
> +unsigned int max_subdevs)
> +{
> + struct v4l2_async_subdev **subdevs;
> +
> + if (max_subdevs <= notifier->max_subdevs)
> + return 0;
> +
> + subdevs = kvmalloc_array(
> + max_subdevs, sizeof(*notifier->subdevs),
> + GFP_KERNEL | __GFP_ZERO);
> + if (!subdevs)
> + return -ENOMEM;
> +
> + if (notifier->subdevs) {
> + memcpy(subdevs, notifier->subdevs,
> +sizeof(*subdevs) * notifier->num_subdevs);
> +
> + kvfree(notifier->subdevs);
> + }
> +
> + notifier->subdevs = subdevs;
> + notifier->max_subdevs = max_subdevs;
> +
> + return 0;
> +}
> +
> +static int v4l2_async_notifier_fwnode_parse_endpoint(
> + struct device *dev, struct v4l2_async_notifier *notifier,
> + struct fwnode_handle *endpoint, unsigned int asd_struct_size,
> + int (*parse_endpoint)(struct device *dev,
> + struct v4l2_fwnode_endpoint *vep,
> + struct v4l2_async_subdev *asd))
> +{
> + struct v4l2_async_subdev *asd;
> + struct v4l2_fwnode_endpoint *vep;
> + int ret = 0;
> +
> + asd = kzalloc(asd_struct_size, GFP_KERNEL);
> + if (!asd)
> + return -ENOMEM;
> +
> + asd->match.fwnode.fwnode =
> + fwnode_graph_get_remote_port_parent(endpoint);
> + if (!asd->match.fwnode.fwnode) {
> + dev_warn(dev, "bad remote port parent\n");
> + ret = -EINVAL;
> + goto out_err;
> + }
> +
> + /* Ignore endpoints the parsing of which failed. */
> + vep = v4l2_fwnode_endpoint_alloc_parse(endpoint);
> + if (IS_ERR(vep)) {
> + ret = PTR_ERR(vep);
> + dev_warn(dev, "unable to parse V4L2 fwnode endpoint (%d)\n",
> +  ret);
> + goto out_err;
> + }
> +
> + ret = parse_endpoint ? parse_endpoint(dev, vep, asd) : 0;

How likely is it that parse_endpoint will be NULL? I ask because if it is
common, then you could put everything from v4l2_fwnode_endpoint_alloc_parse
until just before 'asd->match_type = V4L2_ASYNC_MATCH_FWNODE;' under
'if (parse_endpoint) {'.

The disadvantage is that you won't see parse errors, but on the other hand
nobody apparently needs it, so why bother. I'm not sure what is the right thing
to do here.

> + v4l2_fwnode_endpoint_free(vep);

Here vep is freed,

> + if (ret == -ENOTCONN) {
> + dev_dbg(dev, "ignoring endpoint %u,%u\n", vep->base.port,
> +

[PATCH v7 04/18] v4l: fwnode: Support generic parsing of graph endpoints in a device

[Sakari Ailus]

The current practice is that drivers iterate over their endpoints and
parse each endpoint separately. This is very similar in a number of
drivers, implement a generic function for the job. Driver specific matters
can be taken into account in the driver specific callback.

Signed-off-by: Sakari Ailus 
---
 drivers/media/v4l2-core/v4l2-async.c  |  16 
 drivers/media/v4l2-core/v4l2-fwnode.c | 136 ++
 include/media/v4l2-async.h|  24 +-
 include/media/v4l2-fwnode.h   |  53 +
 4 files changed, 227 insertions(+), 2 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-async.c 
b/drivers/media/v4l2-core/v4l2-async.c
index 851f128eba22..6740a241d4a1 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -22,6 +22,7 @@
 
 #include 
 #include 
+#include 
 #include 
 
 static bool match_i2c(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
@@ -278,6 +279,21 @@ void v4l2_async_notifier_unregister(struct 
v4l2_async_notifier *notifier)
 }
 EXPORT_SYMBOL(v4l2_async_notifier_unregister);
 
+void v4l2_async_notifier_release(struct v4l2_async_notifier *notifier)
+{
+   unsigned int i;
+
+   for (i = 0; i < notifier->num_subdevs; i++)
+   kfree(notifier->subdevs[i]);
+
+   notifier->max_subdevs = 0;
+   notifier->num_subdevs = 0;
+
+   kvfree(notifier->subdevs);
+   notifier->subdevs = NULL;
+}
+EXPORT_SYMBOL_GPL(v4l2_async_notifier_release);
+
 int v4l2_async_register_subdev(struct v4l2_subdev *sd)
 {
struct v4l2_async_notifier *notifier;
diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c 
b/drivers/media/v4l2-core/v4l2-fwnode.c
index 706f9e7b90f1..f8c7a9bc6a58 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c
@@ -19,6 +19,7 @@
  */
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -26,6 +27,7 @@
 #include 
 #include 
 
+#include 
 #include 
 
 enum v4l2_fwnode_bus_type {
@@ -313,6 +315,140 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link)
 }
 EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);
 
+static int v4l2_async_notifier_realloc(struct v4l2_async_notifier *notifier,
+  unsigned int max_subdevs)
+{
+   struct v4l2_async_subdev **subdevs;
+
+   if (max_subdevs <= notifier->max_subdevs)
+   return 0;
+
+   subdevs = kvmalloc_array(
+   max_subdevs, sizeof(*notifier->subdevs),
+   GFP_KERNEL | __GFP_ZERO);
+   if (!subdevs)
+   return -ENOMEM;
+
+   if (notifier->subdevs) {
+   memcpy(subdevs, notifier->subdevs,
+  sizeof(*subdevs) * notifier->num_subdevs);
+
+   kvfree(notifier->subdevs);
+   }
+
+   notifier->subdevs = subdevs;
+   notifier->max_subdevs = max_subdevs;
+
+   return 0;
+}
+
+static int v4l2_async_notifier_fwnode_parse_endpoint(
+   struct device *dev, struct v4l2_async_notifier *notifier,
+   struct fwnode_handle *endpoint, unsigned int asd_struct_size,
+   int (*parse_endpoint)(struct device *dev,
+   struct v4l2_fwnode_endpoint *vep,
+   struct v4l2_async_subdev *asd))
+{
+   struct v4l2_async_subdev *asd;
+   struct v4l2_fwnode_endpoint *vep;
+   int ret = 0;
+
+   asd = kzalloc(asd_struct_size, GFP_KERNEL);
+   if (!asd)
+   return -ENOMEM;
+
+   asd->match.fwnode.fwnode =
+   fwnode_graph_get_remote_port_parent(endpoint);
+   if (!asd->match.fwnode.fwnode) {
+   dev_warn(dev, "bad remote port parent\n");
+   ret = -EINVAL;
+   goto out_err;
+   }
+
+   /* Ignore endpoints the parsing of which failed. */
+   vep = v4l2_fwnode_endpoint_alloc_parse(endpoint);
+   if (IS_ERR(vep)) {
+   ret = PTR_ERR(vep);
+   dev_warn(dev, "unable to parse V4L2 fwnode endpoint (%d)\n",
+ret);
+   goto out_err;
+   }
+
+   ret = parse_endpoint ? parse_endpoint(dev, vep, asd) : 0;
+   v4l2_fwnode_endpoint_free(vep);
+   if (ret == -ENOTCONN) {
+   dev_dbg(dev, "ignoring endpoint %u,%u\n", vep->base.port,
+   vep->base.id);
+   kfree(asd);
+   return 0;
+   } else if (ret < 0) {
+   dev_warn(dev, "driver could not parse endpoint %u,%u (%d)\n",
+vep->base.port, vep->base.id, ret);
+   goto out_err;
+   }
+
+   asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
+   notifier->subdevs[notifier->num_subdevs] = asd;
+   notifier->num_subdevs++;
+
+   return 0;
+
+out_err:
+   fwnode_handle_put(asd->match.fwnode.fwnode);
+   kfree(asd);
+
+   return ret;
+}
+
+int v4l2_async_notifier_parse_fwnode_endpoints(
+   struct device *dev, struct