Re: [PATCH v7 15/18] v4l2-fwnode: Add convenience function for parsing generic references

[Hans Verkuil]

On 09/04/2017 06:34 PM, Sakari Ailus wrote:
>>>  MODULE_LICENSE("GPL");
>>>  MODULE_AUTHOR("Sakari Ailus ");
>>>  MODULE_AUTHOR("Sylwester Nawrocki ");
>>> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
>>> index 6d125f26ec84..52f528162818 100644
>>> --- a/include/media/v4l2-fwnode.h
>>> +++ b/include/media/v4l2-fwnode.h
>>> @@ -254,4 +254,32 @@ int v4l2_async_notifier_parse_fwnode_endpoints(
>>>   struct v4l2_fwnode_endpoint *vep,
>>>   struct v4l2_async_subdev *asd));
>>>  
>>> +/**
>>> + * v4l2_fwnode_reference_parse - parse references for async sub-devices
>>> + * @dev: the device node the properties of which are parsed for references
>>
>> the device node whose properties are...
> 
> Both mean the same thing.

Yes, but I think your sentence is a bit awkward. Just my opinion, though.

Regards,

Hans

Re: [PATCH v7 15/18] v4l2-fwnode: Add convenience function for parsing generic references

[Sakari Ailus]

Hi Hans,

On Mon, Sep 04, 2017 at 04:44:48PM +0200, Hans Verkuil wrote:
> On 09/03/2017 07:49 PM, Sakari Ailus wrote:
> > Add function v4l2_fwnode_reference_count() for counting external
> > references and v4l2_fwnode_reference_parse() for parsing them as async
> > sub-devices.
> > 
> > This can be done on e.g. flash or lens async sub-devices that are not part
> > of but are associated with a sensor.
> > 
> > struct v4l2_async_notifier.max_subdevs field is added to contain the
> > maximum number of sub-devices in a notifier to reflect the memory
> > allocated for the subdevs array.
> > 
> > Signed-off-by: Sakari Ailus 
> > ---
> >  drivers/media/v4l2-core/v4l2-fwnode.c | 81 
> > +++
> >  include/media/v4l2-fwnode.h   | 28 
> >  2 files changed, 109 insertions(+)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c 
> > b/drivers/media/v4l2-core/v4l2-fwnode.c
> > index f8c7a9bc6a58..24f8020caf28 100644
> > --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > @@ -449,6 +449,87 @@ int v4l2_async_notifier_parse_fwnode_endpoints(
> >  }
> >  EXPORT_SYMBOL_GPL(v4l2_async_notifier_parse_fwnode_endpoints);
> >  
> > +static void v4l2_fwnode_print_args(struct fwnode_reference_args *args)
> > +{
> > +   unsigned int i;
> > +
> > +   for (i = 0; i < args->nargs; i++) {
> > +   pr_cont(" %u", args->args[i]);
> > +   if (i + 1 < args->nargs)
> > +   pr_cont(",");
> > +   }
> > +}
> > +
> > +int v4l2_fwnode_reference_parse(
> > +   struct device *dev, struct v4l2_async_notifier *notifier,
> > +   const char *prop, const char *nargs_prop, unsigned int nargs,
> > +   size_t asd_struct_size,
> > +   int (*parse_single)(struct device *dev,
> > +   struct fwnode_reference_args *args,
> > +   struct v4l2_async_subdev *asd))
> > +{
> > +   struct fwnode_reference_args args;
> > +   unsigned int index = 0;
> > +   int ret = -ENOENT;
> > +
> > +   if (asd_struct_size < sizeof(struct v4l2_async_subdev))
> > +   return -EINVAL;
> > +
> > +   for (; !fwnode_property_get_reference_args(
> > +dev_fwnode(dev), prop, nargs_prop, nargs,
> > +index, ); index++)
> > +   fwnode_handle_put(args.fwnode);
> > +
> > +   ret = v4l2_async_notifier_realloc(notifier,
> > + notifier->num_subdevs + index);
> > +   if (ret)
> > +   return -ENOMEM;
> > +
> > +   for (ret = -ENOENT, index = 0; !fwnode_property_get_reference_args(
> > +dev_fwnode(dev), prop, nargs_prop, nargs,
> > +index, ); index++) {
> > +   struct v4l2_async_subdev *asd;
> > +
> > +   if (WARN_ON(notifier->num_subdevs >= notifier->max_subdevs))
> > +   break;
> 
> As mentioned elsewhere: I think this should return an error.

I'll use -EINVAL and put the fwnode as well, i.e. goto error.

> 
> > +
> > +   asd = kzalloc(asd_struct_size, GFP_KERNEL);
> > +   if (!asd) {
> > +   ret = -ENOMEM;
> > +   goto error;
> > +   }
> > +
> > +   ret = parse_single ? parse_single(dev, , asd) : 0;
> > +   if (ret == -ENOTCONN) {
> > +   dev_dbg(dev,
> > +   "ignoring reference prop \"%s\", nargs_prop 
> > \"%s\", nargs %u, index %u",
> > +   prop, nargs_prop, nargs, index);
> > +   v4l2_fwnode_print_args();
> > +   pr_cont("\n");
> 
> asd isn't freed.

Will fix both.

> 
> > +   continue;
> > +   } else if (ret < 0) {
> > +   dev_warn(dev,
> > +"driver could not parse reference prop \"%s\", 
> > nargs_prop \"%s\", nargs %u, index %u",
> > +prop, nargs_prop, nargs, index);
> > +   v4l2_fwnode_print_args();
> > +   pr_cont("\n");
> 
> Ditto.
> 
> > +   goto error;
> > +   }
> > +
> > +   notifier->subdevs[notifier->num_subdevs] = asd;
> > +   asd->match.fwnode.fwnode = args.fwnode;
> > +   asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
> > +   notifier->num_subdevs++;
> > +   }
> > +
> > +   return 0;
> > +
> > +error:
> > +   fwnode_handle_put(args.fwnode);
> > +   return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(v4l2_fwnode_reference_parse);
> > +
> >  MODULE_LICENSE("GPL");
> >  MODULE_AUTHOR("Sakari Ailus ");
> >  MODULE_AUTHOR("Sylwester Nawrocki ");
> > diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> > index 6d125f26ec84..52f528162818 100644
> > --- a/include/media/v4l2-fwnode.h
> > +++ b/include/media/v4l2-fwnode.h
> > @@ -254,4 +254,32 @@ int v4l2_async_notifier_parse_fwnode_endpoints(
> >   struct 

Re: [PATCH v7 15/18] v4l2-fwnode: Add convenience function for parsing generic references

[Hans Verkuil]

On 09/03/2017 07:49 PM, Sakari Ailus wrote:
> Add function v4l2_fwnode_reference_count() for counting external
> references and v4l2_fwnode_reference_parse() for parsing them as async
> sub-devices.
> 
> This can be done on e.g. flash or lens async sub-devices that are not part
> of but are associated with a sensor.
> 
> struct v4l2_async_notifier.max_subdevs field is added to contain the
> maximum number of sub-devices in a notifier to reflect the memory
> allocated for the subdevs array.
> 
> Signed-off-by: Sakari Ailus 
> ---
>  drivers/media/v4l2-core/v4l2-fwnode.c | 81 
> +++
>  include/media/v4l2-fwnode.h   | 28 
>  2 files changed, 109 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c 
> b/drivers/media/v4l2-core/v4l2-fwnode.c
> index f8c7a9bc6a58..24f8020caf28 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -449,6 +449,87 @@ int v4l2_async_notifier_parse_fwnode_endpoints(
>  }
>  EXPORT_SYMBOL_GPL(v4l2_async_notifier_parse_fwnode_endpoints);
>  
> +static void v4l2_fwnode_print_args(struct fwnode_reference_args *args)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < args->nargs; i++) {
> + pr_cont(" %u", args->args[i]);
> + if (i + 1 < args->nargs)
> + pr_cont(",");
> + }
> +}
> +
> +int v4l2_fwnode_reference_parse(
> + struct device *dev, struct v4l2_async_notifier *notifier,
> + const char *prop, const char *nargs_prop, unsigned int nargs,
> + size_t asd_struct_size,
> + int (*parse_single)(struct device *dev,
> + struct fwnode_reference_args *args,
> + struct v4l2_async_subdev *asd))
> +{
> + struct fwnode_reference_args args;
> + unsigned int index = 0;
> + int ret = -ENOENT;
> +
> + if (asd_struct_size < sizeof(struct v4l2_async_subdev))
> + return -EINVAL;
> +
> + for (; !fwnode_property_get_reference_args(
> +  dev_fwnode(dev), prop, nargs_prop, nargs,
> +  index, ); index++)
> + fwnode_handle_put(args.fwnode);
> +
> + ret = v4l2_async_notifier_realloc(notifier,
> +   notifier->num_subdevs + index);
> + if (ret)
> + return -ENOMEM;
> +
> + for (ret = -ENOENT, index = 0; !fwnode_property_get_reference_args(
> +  dev_fwnode(dev), prop, nargs_prop, nargs,
> +  index, ); index++) {
> + struct v4l2_async_subdev *asd;
> +
> + if (WARN_ON(notifier->num_subdevs >= notifier->max_subdevs))
> + break;

As mentioned elsewhere: I think this should return an error.

> +
> + asd = kzalloc(asd_struct_size, GFP_KERNEL);
> + if (!asd) {
> + ret = -ENOMEM;
> + goto error;
> + }
> +
> + ret = parse_single ? parse_single(dev, , asd) : 0;
> + if (ret == -ENOTCONN) {
> + dev_dbg(dev,
> + "ignoring reference prop \"%s\", nargs_prop 
> \"%s\", nargs %u, index %u",
> + prop, nargs_prop, nargs, index);
> + v4l2_fwnode_print_args();
> + pr_cont("\n");

asd isn't freed.

> + continue;
> + } else if (ret < 0) {
> + dev_warn(dev,
> +  "driver could not parse reference prop \"%s\", 
> nargs_prop \"%s\", nargs %u, index %u",
> +  prop, nargs_prop, nargs, index);
> + v4l2_fwnode_print_args();
> + pr_cont("\n");

Ditto.

> + goto error;
> + }
> +
> + notifier->subdevs[notifier->num_subdevs] = asd;
> + asd->match.fwnode.fwnode = args.fwnode;
> + asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
> + notifier->num_subdevs++;
> + }
> +
> + return 0;
> +
> +error:
> + fwnode_handle_put(args.fwnode);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_fwnode_reference_parse);
> +
>  MODULE_LICENSE("GPL");
>  MODULE_AUTHOR("Sakari Ailus ");
>  MODULE_AUTHOR("Sylwester Nawrocki ");
> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> index 6d125f26ec84..52f528162818 100644
> --- a/include/media/v4l2-fwnode.h
> +++ b/include/media/v4l2-fwnode.h
> @@ -254,4 +254,32 @@ int v4l2_async_notifier_parse_fwnode_endpoints(
> struct v4l2_fwnode_endpoint *vep,
> struct v4l2_async_subdev *asd));
>  
> +/**
> + * v4l2_fwnode_reference_parse - parse references for async sub-devices
> + * @dev: the device node the properties of which are parsed for references

the device node whose properties are...

[PATCH v7 15/18] v4l2-fwnode: Add convenience function for parsing generic references

[Sakari Ailus]

Add function v4l2_fwnode_reference_count() for counting external
references and v4l2_fwnode_reference_parse() for parsing them as async
sub-devices.

This can be done on e.g. flash or lens async sub-devices that are not part
of but are associated with a sensor.

struct v4l2_async_notifier.max_subdevs field is added to contain the
maximum number of sub-devices in a notifier to reflect the memory
allocated for the subdevs array.

Signed-off-by: Sakari Ailus 
---
 drivers/media/v4l2-core/v4l2-fwnode.c | 81 +++
 include/media/v4l2-fwnode.h   | 28 
 2 files changed, 109 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c 
b/drivers/media/v4l2-core/v4l2-fwnode.c
index f8c7a9bc6a58..24f8020caf28 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c
@@ -449,6 +449,87 @@ int v4l2_async_notifier_parse_fwnode_endpoints(
 }
 EXPORT_SYMBOL_GPL(v4l2_async_notifier_parse_fwnode_endpoints);
 
+static void v4l2_fwnode_print_args(struct fwnode_reference_args *args)
+{
+   unsigned int i;
+
+   for (i = 0; i < args->nargs; i++) {
+   pr_cont(" %u", args->args[i]);
+   if (i + 1 < args->nargs)
+   pr_cont(",");
+   }
+}
+
+int v4l2_fwnode_reference_parse(
+   struct device *dev, struct v4l2_async_notifier *notifier,
+   const char *prop, const char *nargs_prop, unsigned int nargs,
+   size_t asd_struct_size,
+   int (*parse_single)(struct device *dev,
+   struct fwnode_reference_args *args,
+   struct v4l2_async_subdev *asd))
+{
+   struct fwnode_reference_args args;
+   unsigned int index = 0;
+   int ret = -ENOENT;
+
+   if (asd_struct_size < sizeof(struct v4l2_async_subdev))
+   return -EINVAL;
+
+   for (; !fwnode_property_get_reference_args(
+dev_fwnode(dev), prop, nargs_prop, nargs,
+index, ); index++)
+   fwnode_handle_put(args.fwnode);
+
+   ret = v4l2_async_notifier_realloc(notifier,
+ notifier->num_subdevs + index);
+   if (ret)
+   return -ENOMEM;
+
+   for (ret = -ENOENT, index = 0; !fwnode_property_get_reference_args(
+dev_fwnode(dev), prop, nargs_prop, nargs,
+index, ); index++) {
+   struct v4l2_async_subdev *asd;
+
+   if (WARN_ON(notifier->num_subdevs >= notifier->max_subdevs))
+   break;
+
+   asd = kzalloc(asd_struct_size, GFP_KERNEL);
+   if (!asd) {
+   ret = -ENOMEM;
+   goto error;
+   }
+
+   ret = parse_single ? parse_single(dev, , asd) : 0;
+   if (ret == -ENOTCONN) {
+   dev_dbg(dev,
+   "ignoring reference prop \"%s\", nargs_prop 
\"%s\", nargs %u, index %u",
+   prop, nargs_prop, nargs, index);
+   v4l2_fwnode_print_args();
+   pr_cont("\n");
+   continue;
+   } else if (ret < 0) {
+   dev_warn(dev,
+"driver could not parse reference prop \"%s\", 
nargs_prop \"%s\", nargs %u, index %u",
+prop, nargs_prop, nargs, index);
+   v4l2_fwnode_print_args();
+   pr_cont("\n");
+   goto error;
+   }
+
+   notifier->subdevs[notifier->num_subdevs] = asd;
+   asd->match.fwnode.fwnode = args.fwnode;
+   asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
+   notifier->num_subdevs++;
+   }
+
+   return 0;
+
+error:
+   fwnode_handle_put(args.fwnode);
+   return ret;
+}
+EXPORT_SYMBOL_GPL(v4l2_fwnode_reference_parse);
+
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Sakari Ailus ");
 MODULE_AUTHOR("Sylwester Nawrocki ");
diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
index 6d125f26ec84..52f528162818 100644
--- a/include/media/v4l2-fwnode.h
+++ b/include/media/v4l2-fwnode.h
@@ -254,4 +254,32 @@ int v4l2_async_notifier_parse_fwnode_endpoints(
  struct v4l2_fwnode_endpoint *vep,
  struct v4l2_async_subdev *asd));
 
+/**
+ * v4l2_fwnode_reference_parse - parse references for async sub-devices
+ * @dev: the device node the properties of which are parsed for references
+ * @notifier: the async notifier where the async subdevs will be added
+ * @prop: the name of the property
+ * @nargs_prop: the name of the property in the remote node that specifies the
+ * number of integer arguments (may be NULL, in that case nargs
+ * will be used).
+ *