subject:"Re\: \[PATCH v3 2\/3\] v4l\: fwnode\: Support generic parsing of graph endpoints in a device"

Re: [PATCH v3 2/3] v4l: fwnode: Support generic parsing of graph endpoints in a device

2017-08-24 Thread Sakari Ailus

Hi Laurent,

On Wed, Aug 23, 2017 at 03:59:35PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Wednesday, 23 August 2017 12:01:24 EEST Sakari Ailus wrote:
> > On Tue, Aug 22, 2017 at 03:52:33PM +0300, Laurent Pinchart wrote:
> > > On Friday, 18 August 2017 14:23:16 EEST 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.
> > >> 
> > >> Convert the omap3isp as an example.
> > > 
> > > It would be nice to convert at least two drivers to show that the code can
> > > indeed be shared between multiple drivers. Even better, you could convert
> > > all drivers.
> > 
> > That's the intent in the long run. There's still no definite need to do
> > this in a single, big, hot patchset.
> 
> You don't need to convert them all in a single patch, but I'd still prefer 
> converting them all in a single patch series (and I'd split this patch in two 
> to make backporting easier). Otherwise we'll likely end up with multiple 
> competing implementations.

I don't think that presumption is founded. The fact that there is a driver
doing something on its own does not prevent add a helper function to the
framework which could be used to remove driver code. We have, as an
example, the pipeline power management code in v4l2-mc.c. It was introduced
with the omap3isp driver and is now used by a number of drivers.

This is also related preparation for supporting associations that will be
needed for lens and flash devices.

> 
> > >> Signed-off-by: Sakari Ailus 
> > >> ---
> > >> drivers/media/platform/omap3isp/isp.c | 116 ++--
> > >> drivers/media/v4l2-core/v4l2-fwnode.c | 125 +
> > >> include/media/v4l2-async.h|   4 +-
> > >> include/media/v4l2-fwnode.h   |   9 +++
> > >> 4 files changed, 173 insertions(+), 81 deletions(-)
> > > 
> > > [snip]
> > > 
> > >> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c
> > >> b/drivers/media/v4l2-core/v4l2-fwnode.c index 5cd2687310fe..cb0fc4b4e3bf
> > >> 100644
> > >> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > >> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > >> @@ -26,6 +26,7 @@
> > >>  #include 
> > >>  #include 
> > >> 
> > >> +#include 
> > >>  #include 
> > >>  
> > >>  enum v4l2_fwnode_bus_type {
> > >> @@ -383,6 +384,130 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link
> > >> *link) }
> > >> 
> > >>  EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);
> > >> 
> > >> +static int notifier_realloc(struct device *dev,
> > >> +struct v4l2_async_notifier *notifier,
> > >> +unsigned int max_subdevs)
> > > 
> > > It looks like you interpret the variable as an increment. You shouldn't
> > > call it max_subdevs in that case. I would however keep the name and pass
> > > the total number of subdevs instead of an increment, to mimic the realloc
> > > API.
> > 
> > Works for me.
> > 
> > >> +{
> > >> +struct v4l2_async_subdev **subdevs;
> > >> +unsigned int i;
> > >> +
> > >> +if (max_subdevs + notifier->num_subdevs <= 
> > >> notifier->max_subdevs)
> > >> +return 0;
> > >> +
> > >> +subdevs = devm_kcalloc(
> > >> +dev, max_subdevs + notifier->num_subdevs,
> > >> +sizeof(*notifier->subdevs), GFP_KERNEL);
> > > 
> > > We know that we'll have to move away from devm_* allocation to fix object
> > > lifetime management, so we could as well start now.
> > 
> > The memory is in practice allocated using devm_() interface in existing
> > drivers.
> 
> Yes, and that's bad :-)
> 
> > The fact that it's in a single location makes it much easier getting rid of
> > it.
> 
> Great, so let's get rid of it :-)
> 
> > I'd rather get rid of memory allocation here in the first place, to be
> > replaced by a linked list. But first the user of notifier->subdevs in
> > drivers need to go. The framework interface doesn't need to change as a
> > result.
> 
> How are you going to allocate and free the linked list entries ?

Thinking about it some more --- we can't actually use other objects (not
allocated here) to hold the list struct, so we'll keep the allocation.

I think we'll need v4l2_async_notifier_release() as a result. I'll add that
to v2. This could be used to release resources related to a notifier which
is not yet registered.

> 
> > >> +if (!subdevs)
> > >> +return -ENOMEM;
> > >> +
> > >> +if (notifier->subdevs) {
> > >> +for (i = 0; i < notifier->num_subdevs; i++)
> > >> +subdevs[i] = notifier->subdevs[i];
> > > 
> > > Is there a reason to use a loop here instead of a memcpy() covering the
> > > whole array ?
> > 
> > You could do that, yes,

Re: [PATCH v3 2/3] v4l: fwnode: Support generic parsing of graph endpoints in a device

2017-08-23 Thread Laurent Pinchart

Hi Sakari,

On Wednesday, 23 August 2017 12:01:24 EEST Sakari Ailus wrote:
> On Tue, Aug 22, 2017 at 03:52:33PM +0300, Laurent Pinchart wrote:
> > On Friday, 18 August 2017 14:23:16 EEST 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.
> >> 
> >> Convert the omap3isp as an example.
> > 
> > It would be nice to convert at least two drivers to show that the code can
> > indeed be shared between multiple drivers. Even better, you could convert
> > all drivers.
> 
> That's the intent in the long run. There's still no definite need to do
> this in a single, big, hot patchset.

You don't need to convert them all in a single patch, but I'd still prefer 
converting them all in a single patch series (and I'd split this patch in two 
to make backporting easier). Otherwise we'll likely end up with multiple 
competing implementations.

> >> Signed-off-by: Sakari Ailus 
> >> ---
> >> drivers/media/platform/omap3isp/isp.c | 116 ++--
> >> drivers/media/v4l2-core/v4l2-fwnode.c | 125 +
> >> include/media/v4l2-async.h|   4 +-
> >> include/media/v4l2-fwnode.h   |   9 +++
> >> 4 files changed, 173 insertions(+), 81 deletions(-)
> > 
> > [snip]
> > 
> >> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c
> >> b/drivers/media/v4l2-core/v4l2-fwnode.c index 5cd2687310fe..cb0fc4b4e3bf
> >> 100644
> >> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> >> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> >> @@ -26,6 +26,7 @@
> >>  #include 
> >>  #include 
> >> 
> >> +#include 
> >>  #include 
> >>  
> >>  enum v4l2_fwnode_bus_type {
> >> @@ -383,6 +384,130 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link
> >> *link) }
> >> 
> >>  EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);
> >> 
> >> +static int notifier_realloc(struct device *dev,
> >> +  struct v4l2_async_notifier *notifier,
> >> +  unsigned int max_subdevs)
> > 
> > It looks like you interpret the variable as an increment. You shouldn't
> > call it max_subdevs in that case. I would however keep the name and pass
> > the total number of subdevs instead of an increment, to mimic the realloc
> > API.
> 
> Works for me.
> 
> >> +{
> >> +  struct v4l2_async_subdev **subdevs;
> >> +  unsigned int i;
> >> +
> >> +  if (max_subdevs + notifier->num_subdevs <= notifier->max_subdevs)
> >> +  return 0;
> >> +
> >> +  subdevs = devm_kcalloc(
> >> +  dev, max_subdevs + notifier->num_subdevs,
> >> +  sizeof(*notifier->subdevs), GFP_KERNEL);
> > 
> > We know that we'll have to move away from devm_* allocation to fix object
> > lifetime management, so we could as well start now.
> 
> The memory is in practice allocated using devm_() interface in existing
> drivers.

Yes, and that's bad :-)

> The fact that it's in a single location makes it much easier getting rid of
> it.

Great, so let's get rid of it :-)

> I'd rather get rid of memory allocation here in the first place, to be
> replaced by a linked list. But first the user of notifier->subdevs in
> drivers need to go. The framework interface doesn't need to change as a
> result.

How are you going to allocate and free the linked list entries ?

> >> +  if (!subdevs)
> >> +  return -ENOMEM;
> >> +
> >> +  if (notifier->subdevs) {
> >> +  for (i = 0; i < notifier->num_subdevs; i++)
> >> +  subdevs[i] = notifier->subdevs[i];
> > 
> > Is there a reason to use a loop here instead of a memcpy() covering the
> > whole array ?
> 
> You could do that, yes, although I'd think this looks nicer. Performance is
> hardly a concern here.

It's certainly not a performance issue, I would find the code easier to read.

> >> +  devm_kfree(dev, notifier->subdevs);
> >> +  }
> >> +
> >> +  notifier->subdevs = subdevs;
> >> +  notifier->max_subdevs = max_subdevs + notifier->num_subdevs;
> >> +
> >> +  return 0;
> >> +}
> >> +
> >> +static int __v4l2_fwnode_endpoint_parse(
> >> +  struct device *dev, struct v4l2_async_notifier *notifier,
> >> +  struct fwnode_handle *endpoint, struct v4l2_async_subdev *asd,
> >> +  int (*parse_single)(struct device *dev,
> >> +  struct v4l2_fwnode_endpoint *vep,
> >> +  struct v4l2_async_subdev *asd))
> >> +{
> >> +  struct v4l2_fwnode_endpoint *vep;
> >> +  int ret;
> >> +
> >> +  /* Ignore endpoints the parsing of which failed. */
> > 
> > Silently ignoring invalid DT sounds bad, I'd rather catch errors and
> > return with an error code to make sure that DT gets fixed.
> 
> That would mean that if a single node is bad, none of the correct ones can
> be used either. I'm not sure everyone would be happy about it.

We should be tolerant

Re: [PATCH v3 2/3] v4l: fwnode: Support generic parsing of graph endpoints in a device

2017-08-23 Thread Sakari Ailus

Hi Laurent,

Thanks for the critique.

On Tue, Aug 22, 2017 at 03:52:33PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Friday, 18 August 2017 14:23:16 EEST 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.
> > 
> > Convert the omap3isp as an example.
> 
> It would be nice to convert at least two drivers to show that the code can 
> indeed be shared between multiple drivers. Even better, you could convert all 
> drivers.

That's the intent in the long run. There's still no definite need to do
this in a single, big, hot patchset.

>  
> > Signed-off-by: Sakari Ailus 
> > ---
> > drivers/media/platform/omap3isp/isp.c | 116 ++-
> > drivers/media/v4l2-core/v4l2-fwnode.c | 125 
> > include/media/v4l2-async.h|   4 +-
> > include/media/v4l2-fwnode.h   |   9 +++
> > 4 files changed, 173 insertions(+), 81 deletions(-)
> 
> [snip]
> 
> > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c
> > b/drivers/media/v4l2-core/v4l2-fwnode.c index 5cd2687310fe..cb0fc4b4e3bf
> > 100644
> > --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > @@ -26,6 +26,7 @@
> >  #include 
> >  #include 
> > 
> > +#include 
> >  #include 
> > 
> >  enum v4l2_fwnode_bus_type {
> > @@ -383,6 +384,130 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link
> > *link) }
> >  EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);
> > 
> > +static int notifier_realloc(struct device *dev,
> > +   struct v4l2_async_notifier *notifier,
> > +   unsigned int max_subdevs)
> 
> It looks like you interpret the variable as an increment. You shouldn't call 
> it max_subdevs in that case. I would however keep the name and pass the total 
> number of subdevs instead of an increment, to mimic the realloc API.

Works for me.

> 
> > +{
> > +   struct v4l2_async_subdev **subdevs;
> > +   unsigned int i;
> > +
> > +   if (max_subdevs + notifier->num_subdevs <= notifier->max_subdevs)
> > +   return 0;
> > +
> > +   subdevs = devm_kcalloc(
> > +   dev, max_subdevs + notifier->num_subdevs,
> > +   sizeof(*notifier->subdevs), GFP_KERNEL);
> 
> We know that we'll have to move away from devm_* allocation to fix object 
> lifetime management, so we could as well start now.

The memory is in practice allocated using devm_() interface in existing
drivers. The fact that it's in a single location makes it much easier
getting rid of it.

I'd rather get rid of memory allocation here in the first place, to be
replaced by a linked list. But first the user of notifier->subdevs in
drivers need to go. The framework interface doesn't need to change as a
result.

> 
> > +   if (!subdevs)
> > +   return -ENOMEM;
> > +
> > +   if (notifier->subdevs) {
> > +   for (i = 0; i < notifier->num_subdevs; i++)
> > +   subdevs[i] = notifier->subdevs[i];
> 
> Is there a reason to use a loop here instead of a memcpy() covering the whole 
> array ?

You could do that, yes, although I'd think this looks nicer. Performance is
hardly a concern here.

> 
> > +   devm_kfree(dev, notifier->subdevs);
> > +   }
> > +
> > +   notifier->subdevs = subdevs;
> > +   notifier->max_subdevs = max_subdevs + notifier->num_subdevs;
> > +
> > +   return 0;
> > +}
> > +
> > +static int __v4l2_fwnode_endpoint_parse(
> > +   struct device *dev, struct v4l2_async_notifier *notifier,
> > +   struct fwnode_handle *endpoint, struct v4l2_async_subdev *asd,
> > +   int (*parse_single)(struct device *dev,
> > +   struct v4l2_fwnode_endpoint *vep,
> > +   struct v4l2_async_subdev *asd))
> > +{
> > +   struct v4l2_fwnode_endpoint *vep;
> > +   int ret;
> > +
> > +   /* Ignore endpoints the parsing of which failed. */
> 
> Silently ignoring invalid DT sounds bad, I'd rather catch errors and return 
> with an error code to make sure that DT gets fixed.

That would mean that if a single node is bad, none of the correct ones can
be used either. I'm not sure everyone would be happy about it.

> 
> > +   vep = v4l2_fwnode_endpoint_alloc_parse(endpoint);
> > +   if (IS_ERR(vep))
> > +   return 0;
> > +
> > +   notifier->subdevs[notifier->num_subdevs] = asd;
> > +
> > +   ret = parse_single(dev, vep, asd);
> > +   v4l2_fwnode_endpoint_free(vep);
> > +   if (ret)
> > +   return ret;
> > +
> > +   asd->match.fwnode.fwnode =
> > +   fwnode_graph_get_remote_port_parent(endpoint);
> > +   if (!asd->match.fwnode.fwnode) {
> > +   dev_warn(dev, "bad remote port parent\n");
> > +   return -EINVAL;
> > +   }
> > +
> > +   asd->match_type =

Re: [PATCH v3 2/3] v4l: fwnode: Support generic parsing of graph endpoints in a device

2017-08-22 Thread Laurent Pinchart

Hi Sakari,

Thank you for the patch.

On Friday, 18 August 2017 14:23:16 EEST 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.
> 
> Convert the omap3isp as an example.

It would be nice to convert at least two drivers to show that the code can 
indeed be shared between multiple drivers. Even better, you could convert all 
drivers.
 
> Signed-off-by: Sakari Ailus 
> ---
> drivers/media/platform/omap3isp/isp.c | 116 ++-
> drivers/media/v4l2-core/v4l2-fwnode.c | 125 
> include/media/v4l2-async.h|   4 +-
> include/media/v4l2-fwnode.h   |   9 +++
> 4 files changed, 173 insertions(+), 81 deletions(-)

[snip]

> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c
> b/drivers/media/v4l2-core/v4l2-fwnode.c index 5cd2687310fe..cb0fc4b4e3bf
> 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -26,6 +26,7 @@
>  #include 
>  #include 
> 
> +#include 
>  #include 
> 
>  enum v4l2_fwnode_bus_type {
> @@ -383,6 +384,130 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link
> *link) }
>  EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);
> 
> +static int notifier_realloc(struct device *dev,
> + struct v4l2_async_notifier *notifier,
> + unsigned int max_subdevs)

It looks like you interpret the variable as an increment. You shouldn't call 
it max_subdevs in that case. I would however keep the name and pass the total 
number of subdevs instead of an increment, to mimic the realloc API.

> +{
> + struct v4l2_async_subdev **subdevs;
> + unsigned int i;
> +
> + if (max_subdevs + notifier->num_subdevs <= notifier->max_subdevs)
> + return 0;
> +
> + subdevs = devm_kcalloc(
> + dev, max_subdevs + notifier->num_subdevs,
> + sizeof(*notifier->subdevs), GFP_KERNEL);

We know that we'll have to move away from devm_* allocation to fix object 
lifetime management, so we could as well start now.

> + if (!subdevs)
> + return -ENOMEM;
> +
> + if (notifier->subdevs) {
> + for (i = 0; i < notifier->num_subdevs; i++)
> + subdevs[i] = notifier->subdevs[i];

Is there a reason to use a loop here instead of a memcpy() covering the whole 
array ?

> + devm_kfree(dev, notifier->subdevs);
> + }
> +
> + notifier->subdevs = subdevs;
> + notifier->max_subdevs = max_subdevs + notifier->num_subdevs;
> +
> + return 0;
> +}
> +
> +static int __v4l2_fwnode_endpoint_parse(
> + struct device *dev, struct v4l2_async_notifier *notifier,
> + struct fwnode_handle *endpoint, struct v4l2_async_subdev *asd,
> + int (*parse_single)(struct device *dev,
> + struct v4l2_fwnode_endpoint *vep,
> + struct v4l2_async_subdev *asd))
> +{
> + struct v4l2_fwnode_endpoint *vep;
> + int ret;
> +
> + /* Ignore endpoints the parsing of which failed. */

Silently ignoring invalid DT sounds bad, I'd rather catch errors and return 
with an error code to make sure that DT gets fixed.

> + vep = v4l2_fwnode_endpoint_alloc_parse(endpoint);
> + if (IS_ERR(vep))
> + return 0;
> +
> + notifier->subdevs[notifier->num_subdevs] = asd;
> +
> + ret = parse_single(dev, vep, asd);
> + v4l2_fwnode_endpoint_free(vep);
> + if (ret)
> + return ret;
> +
> + asd->match.fwnode.fwnode =
> + fwnode_graph_get_remote_port_parent(endpoint);
> + if (!asd->match.fwnode.fwnode) {
> + dev_warn(dev, "bad remote port parent\n");
> + return -EINVAL;
> + }
> +
> + asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
> + notifier->num_subdevs++;
> +
> + return 0;
> +}
> +
> +/**
> + * v4l2_fwnode_endpoint_parse - Parse V4L2 fwnode endpoints in a device
> node

This doesn't match the function name.

> + * @dev: local struct device

Based on the documentation only and without a priori knowledge of the API, 
local struct device is very vague.

> + * @notifier: async notifier related to @dev

Ditto. You need more documentation, especially given that this is the first 
function in the core that fills a notifier from DT. You might also want to 
reflect that fact in the function name.

> + * @asd_struct_size: size of the driver's async sub-device struct,
> including
> + *sizeof(struct v4l2_async_subdev)
> + * @parse_single: driver's callback function called on each V4L2 fwnode
> endpoint

The parse_single return values should be documented.

> + * Parse all V4L2 fwnode endpoints related to the device.
> + *
> + * Note that this function is intended for drivers to

Re: [PATCH v3 2/3] v4l: fwnode: Support generic parsing of graph endpoints in a device

Re: [PATCH v3 2/3] v4l: fwnode: Support generic parsing of graph endpoints in a device

Re: [PATCH v3 2/3] v4l: fwnode: Support generic parsing of graph endpoints in a device

Re: [PATCH v3 2/3] v4l: fwnode: Support generic parsing of graph endpoints in a device

4 matches

Site Navigation

Mail list logo

Footer information