[PATCH 0/5] V4L2 flash API wrapper improvements

[PATCH 0/5] V4L2 flash API wrapper improvements

[Sakari Ailus]

Hi Jacek, Mauro, others,

The first patch in this set adds the of_node pointer to struct v4l2_subdev
in order to match an async sub-device based on an explicit OF node instead
of the device's OF node, where the former is typically a child of the
latter.

Mauro: would you be ok with this patch going through the LED tree as well?
The rest of the patches in the set depend on this one. There are currently
no conflicts with media-tree master branch.

The rest of the patches (2--5) are intended to be merged with Jacek's
patches here:

<URL:http://www.spinics.net/lists/linux-media/msg88999.html>

Jacek: if you're ok with the patches, could you merge them with appropriate
patches in your set, please? The patches have not been tested since I don't
have the hardware.

-- 
Kind regards,
Sakari

[PATCH 1/5] v4l: async: Add a pointer to of_node to struct v4l2_subdev, match it

[Sakari Ailus]

V4L2 async sub-devices are currently matched (OF case) based on the struct
device_node pointer in struct device. LED devices may have more than one
LED, and in that case the OF node to match is not directly the device's
node, but a LED's node.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
 drivers/media/v4l2-core/v4l2-async.c  |   34 +++++++++++++++++++++------------
 drivers/media/v4l2-core/v4l2-device.c |    3 +++
 include/media/v4l2-subdev.h           |    2 ++
 3 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 85a6a34..bcdd140 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -22,10 +22,10 @@
 #include <media/v4l2-device.h>
 #include <media/v4l2-subdev.h>
 
-static bool match_i2c(struct device *dev, struct v4l2_async_subdev *asd)
+static bool match_i2c(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
 {
 #if IS_ENABLED(CONFIG_I2C)
-	struct i2c_client *client = i2c_verify_client(dev);
+	struct i2c_client *client = i2c_verify_client(sd->dev);
 	return client &&
 		asd->match.i2c.adapter_id == client->adapter->nr &&
 		asd->match.i2c.address == client->addr;
@@ -34,14 +34,27 @@ static bool match_i2c(struct device *dev, struct v4l2_async_subdev *asd)
 #endif
 }
 
-static bool match_devname(struct device *dev, struct v4l2_async_subdev *asd)
+static bool match_devname(struct v4l2_subdev *sd,
+			  struct v4l2_async_subdev *asd)
 {
-	return !strcmp(asd->match.device_name.name, dev_name(dev));
+	return !strcmp(asd->match.device_name.name, dev_name(sd->dev));
 }
 
-static bool match_of(struct device *dev, struct v4l2_async_subdev *asd)
+static bool match_of(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
 {
-	return dev->of_node == asd->match.of.node;
+	struct device_node *of_node =
+		sd->of_node ? sd->of_node : sd->dev->of_node;
+
+	return of_node == asd->match.of.node;
+}
+
+static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
+{
+	if (!asd->match.custom.match)
+		/* Match always */
+		return true;
+
+	return asd->match.custom.match(sd->dev, asd);
 }
 
 static LIST_HEAD(subdev_list);
@@ -51,17 +64,14 @@ static DEFINE_MUTEX(list_lock);
 static struct v4l2_async_subdev *v4l2_async_belongs(struct v4l2_async_notifier *notifier,
 						    struct v4l2_subdev *sd)
 {
+	bool (*match)(struct v4l2_subdev *, struct v4l2_async_subdev *);
 	struct v4l2_async_subdev *asd;
-	bool (*match)(struct device *, struct v4l2_async_subdev *);
 
 	list_for_each_entry(asd, &notifier->waiting, list) {
 		/* bus_type has been verified valid before */
 		switch (asd->match_type) {
 		case V4L2_ASYNC_MATCH_CUSTOM:
-			match = asd->match.custom.match;
-			if (!match)
-				/* Match always */
-				return asd;
+			match = match_custom;
 			break;
 		case V4L2_ASYNC_MATCH_DEVNAME:
 			match = match_devname;
@@ -79,7 +89,7 @@ static struct v4l2_async_subdev *v4l2_async_belongs(struct v4l2_async_notifier *
 		}
 
 		/* match cannot be NULL here */
-		if (match(sd->dev, asd))
+		if (match(sd, asd))
 			return asd;
 	}
 
diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
index 5b0a30b..a741c6c 100644
--- a/drivers/media/v4l2-core/v4l2-device.c
+++ b/drivers/media/v4l2-core/v4l2-device.c
@@ -157,6 +157,9 @@ int v4l2_device_register_subdev(struct v4l2_device *v4l2_dev,
 	/* Warn if we apparently re-register a subdev */
 	WARN_ON(sd->v4l2_dev != NULL);
 
+	if (!sd->of_node && sd->dev)
+		sd->of_node = sd->dev->of_node;
+
 	/*
 	 * The reason to acquire the module here is to avoid unloading
 	 * a module of sub-device which is registered to a media
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 8f5da73..5c51987 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -603,6 +603,8 @@ struct v4l2_subdev {
 	struct video_device *devnode;
 	/* pointer to the physical device, if any */
 	struct device *dev;
+	/* A device_node of the sub-device, iff not dev->of_node. */
+	struct device_node *of_node;
 	/* Links this subdev to a global subdev_list or @notifier->done list. */
 	struct list_head async_list;
 	/* Pointer to respective struct v4l2_async_subdev. */
-- 
1.7.10.4

Re: [PATCH 1/5] v4l: async: Add a pointer to of_node to struct v4l2_subdev, match it

[Jacek Anaszewski]

On 05/20/2015 01:04 AM, Sakari Ailus wrote:
> V4L2 async sub-devices are currently matched (OF case) based on the struct
> device_node pointer in struct device. LED devices may have more than one
> LED, and in that case the OF node to match is not directly the device's
> node, but a LED's node.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> ---
>   drivers/media/v4l2-core/v4l2-async.c  |   34 +++++++++++++++++++++------------
>   drivers/media/v4l2-core/v4l2-device.c |    3 +++
>   include/media/v4l2-subdev.h           |    2 ++
>   3 files changed, 27 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 85a6a34..bcdd140 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -22,10 +22,10 @@
>   #include <media/v4l2-device.h>
>   #include <media/v4l2-subdev.h>
>
> -static bool match_i2c(struct device *dev, struct v4l2_async_subdev *asd)
> +static bool match_i2c(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
>   {
>   #if IS_ENABLED(CONFIG_I2C)
> -	struct i2c_client *client = i2c_verify_client(dev);
> +	struct i2c_client *client = i2c_verify_client(sd->dev);
>   	return client &&
>   		asd->match.i2c.adapter_id == client->adapter->nr &&
>   		asd->match.i2c.address == client->addr;
> @@ -34,14 +34,27 @@ static bool match_i2c(struct device *dev, struct v4l2_async_subdev *asd)
>   #endif
>   }
>
> -static bool match_devname(struct device *dev, struct v4l2_async_subdev *asd)
> +static bool match_devname(struct v4l2_subdev *sd,
> +			  struct v4l2_async_subdev *asd)
>   {
> -	return !strcmp(asd->match.device_name.name, dev_name(dev));
> +	return !strcmp(asd->match.device_name.name, dev_name(sd->dev));
>   }
>
> -static bool match_of(struct device *dev, struct v4l2_async_subdev *asd)
> +static bool match_of(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
>   {
> -	return dev->of_node == asd->match.of.node;
> +	struct device_node *of_node =
> +		sd->of_node ? sd->of_node : sd->dev->of_node;
> +
> +	return of_node == asd->match.of.node;
> +}
> +
> +static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
> +{
> +	if (!asd->match.custom.match)
> +		/* Match always */
> +		return true;
> +
> +	return asd->match.custom.match(sd->dev, asd);
>   }
>
>   static LIST_HEAD(subdev_list);
> @@ -51,17 +64,14 @@ static DEFINE_MUTEX(list_lock);
>   static struct v4l2_async_subdev *v4l2_async_belongs(struct v4l2_async_notifier *notifier,
>   						    struct v4l2_subdev *sd)
>   {
> +	bool (*match)(struct v4l2_subdev *, struct v4l2_async_subdev *);
>   	struct v4l2_async_subdev *asd;
> -	bool (*match)(struct device *, struct v4l2_async_subdev *);
>
>   	list_for_each_entry(asd, &notifier->waiting, list) {
>   		/* bus_type has been verified valid before */
>   		switch (asd->match_type) {
>   		case V4L2_ASYNC_MATCH_CUSTOM:
> -			match = asd->match.custom.match;
> -			if (!match)
> -				/* Match always */
> -				return asd;
> +			match = match_custom;
>   			break;
>   		case V4L2_ASYNC_MATCH_DEVNAME:
>   			match = match_devname;
> @@ -79,7 +89,7 @@ static struct v4l2_async_subdev *v4l2_async_belongs(struct v4l2_async_notifier *
>   		}
>
>   		/* match cannot be NULL here */
> -		if (match(sd->dev, asd))
> +		if (match(sd, asd))
>   			return asd;
>   	}
>
> diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
> index 5b0a30b..a741c6c 100644
> --- a/drivers/media/v4l2-core/v4l2-device.c
> +++ b/drivers/media/v4l2-core/v4l2-device.c
> @@ -157,6 +157,9 @@ int v4l2_device_register_subdev(struct v4l2_device *v4l2_dev,
>   	/* Warn if we apparently re-register a subdev */
>   	WARN_ON(sd->v4l2_dev != NULL);
>
> +	if (!sd->of_node && sd->dev)
> +		sd->of_node = sd->dev->of_node;
> +
>   	/*
>   	 * The reason to acquire the module here is to avoid unloading
>   	 * a module of sub-device which is registered to a media
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 8f5da73..5c51987 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -603,6 +603,8 @@ struct v4l2_subdev {
>   	struct video_device *devnode;
>   	/* pointer to the physical device, if any */
>   	struct device *dev;
> +	/* A device_node of the sub-device, iff not dev->of_node. */
> +	struct device_node *of_node;
>   	/* Links this subdev to a global subdev_list or @notifier->done list. */
>   	struct list_head async_list;
>   	/* Pointer to respective struct v4l2_async_subdev. */
>

I've tested it with V4L2_ASYNC_MATCH_OF matching type.
For this you can add my:

Tested-by: Jacek Anaszewski <j.anaszewski@samsung.com>

-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH 1/5] v4l: async: Add a pointer to of_node to struct v4l2_subdev, match it

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Wednesday 20 May 2015 02:04:01 Sakari Ailus wrote:
> V4L2 async sub-devices are currently matched (OF case) based on the struct
> device_node pointer in struct device. LED devices may have more than one
> LED, and in that case the OF node to match is not directly the device's
> node, but a LED's node.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> ---
>  drivers/media/v4l2-core/v4l2-async.c  |   34 +++++++++++++++++++-----------
>  drivers/media/v4l2-core/v4l2-device.c |    3 +++
>  include/media/v4l2-subdev.h           |    2 ++
>  3 files changed, 27 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c
> b/drivers/media/v4l2-core/v4l2-async.c index 85a6a34..bcdd140 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -22,10 +22,10 @@
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-subdev.h>
> 
> -static bool match_i2c(struct device *dev, struct v4l2_async_subdev *asd)
> +static bool match_i2c(struct v4l2_subdev *sd, struct v4l2_async_subdev
> *asd) {
>  #if IS_ENABLED(CONFIG_I2C)
> -	struct i2c_client *client = i2c_verify_client(dev);
> +	struct i2c_client *client = i2c_verify_client(sd->dev);
>  	return client &&
>  		asd->match.i2c.adapter_id == client->adapter->nr &&
>  		asd->match.i2c.address == client->addr;
> @@ -34,14 +34,27 @@ static bool match_i2c(struct device *dev, struct
> v4l2_async_subdev *asd) #endif
>  }
> 
> -static bool match_devname(struct device *dev, struct v4l2_async_subdev
> *asd) +static bool match_devname(struct v4l2_subdev *sd,
> +			  struct v4l2_async_subdev *asd)
>  {
> -	return !strcmp(asd->match.device_name.name, dev_name(dev));
> +	return !strcmp(asd->match.device_name.name, dev_name(sd->dev));
>  }
> 
> -static bool match_of(struct device *dev, struct v4l2_async_subdev *asd)
> +static bool match_of(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
> {
> -	return dev->of_node == asd->match.of.node;
> +	struct device_node *of_node =
> +		sd->of_node ? sd->of_node : sd->dev->of_node;
> +
> +	return of_node == asd->match.of.node;
> +}
> +
> +static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev
> *asd) +{
> +	if (!asd->match.custom.match)
> +		/* Match always */
> +		return true;
> +
> +	return asd->match.custom.match(sd->dev, asd);
>  }
> 
>  static LIST_HEAD(subdev_list);
> @@ -51,17 +64,14 @@ static DEFINE_MUTEX(list_lock);
>  static struct v4l2_async_subdev *v4l2_async_belongs(struct
> v4l2_async_notifier *notifier, struct v4l2_subdev *sd)
>  {
> +	bool (*match)(struct v4l2_subdev *, struct v4l2_async_subdev *);
>  	struct v4l2_async_subdev *asd;
> -	bool (*match)(struct device *, struct v4l2_async_subdev *);
> 
>  	list_for_each_entry(asd, &notifier->waiting, list) {
>  		/* bus_type has been verified valid before */
>  		switch (asd->match_type) {
>  		case V4L2_ASYNC_MATCH_CUSTOM:
> -			match = asd->match.custom.match;
> -			if (!match)
> -				/* Match always */
> -				return asd;
> +			match = match_custom;
>  			break;
>  		case V4L2_ASYNC_MATCH_DEVNAME:
>  			match = match_devname;
> @@ -79,7 +89,7 @@ static struct v4l2_async_subdev *v4l2_async_belongs(struct
> v4l2_async_notifier * }
> 
>  		/* match cannot be NULL here */
> -		if (match(sd->dev, asd))
> +		if (match(sd, asd))
>  			return asd;
>  	}
> 
> diff --git a/drivers/media/v4l2-core/v4l2-device.c
> b/drivers/media/v4l2-core/v4l2-device.c index 5b0a30b..a741c6c 100644
> --- a/drivers/media/v4l2-core/v4l2-device.c
> +++ b/drivers/media/v4l2-core/v4l2-device.c
> @@ -157,6 +157,9 @@ int v4l2_device_register_subdev(struct v4l2_device
> *v4l2_dev, /* Warn if we apparently re-register a subdev */
>  	WARN_ON(sd->v4l2_dev != NULL);
> 
> +	if (!sd->of_node && sd->dev)
> +		sd->of_node = sd->dev->of_node;
> +
>  	/*
>  	 * The reason to acquire the module here is to avoid unloading
>  	 * a module of sub-device which is registered to a media
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 8f5da73..5c51987 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -603,6 +603,8 @@ struct v4l2_subdev {
>  	struct video_device *devnode;
>  	/* pointer to the physical device, if any */
>  	struct device *dev;
> +	/* A device_node of the sub-device, iff not dev->of_node. */

"iff" ?

I think we need to define usage of this field more precisely. If the subdev 
driver doesn't initialize it it will be set in v4l2_device_register_subdev(), 
meaning that there's part of the lifetime of the subdev during which the field 
will be NULL even though the subdev's device has a DT node. Could that be a 
problem, aren't there cases when we would need to access sd->of_node before 
v4l2_device_register_subdev(), in particular when using V4l2-async ?

> +	struct device_node *of_node;
>  	/* Links this subdev to a global subdev_list or @notifier->done list. */
>  	struct list_head async_list;
>  	/* Pointer to respective struct v4l2_async_subdev. */

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/5] v4l: async: Add a pointer to of_node to struct v4l2_subdev, match it

[Sakari Ailus]

Hi Laurent,

Thanks for the comments.

On Sat, May 23, 2015 at 09:47:28PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Wednesday 20 May 2015 02:04:01 Sakari Ailus wrote:
> > V4L2 async sub-devices are currently matched (OF case) based on the struct
> > device_node pointer in struct device. LED devices may have more than one
> > LED, and in that case the OF node to match is not directly the device's
> > node, but a LED's node.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> > ---
> >  drivers/media/v4l2-core/v4l2-async.c  |   34 +++++++++++++++++++-----------
> >  drivers/media/v4l2-core/v4l2-device.c |    3 +++
> >  include/media/v4l2-subdev.h           |    2 ++
> >  3 files changed, 27 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c
> > b/drivers/media/v4l2-core/v4l2-async.c index 85a6a34..bcdd140 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -22,10 +22,10 @@
> >  #include <media/v4l2-device.h>
> >  #include <media/v4l2-subdev.h>
> > 
> > -static bool match_i2c(struct device *dev, struct v4l2_async_subdev *asd)
> > +static bool match_i2c(struct v4l2_subdev *sd, struct v4l2_async_subdev
> > *asd) {
> >  #if IS_ENABLED(CONFIG_I2C)
> > -	struct i2c_client *client = i2c_verify_client(dev);
> > +	struct i2c_client *client = i2c_verify_client(sd->dev);
> >  	return client &&
> >  		asd->match.i2c.adapter_id == client->adapter->nr &&
> >  		asd->match.i2c.address == client->addr;
> > @@ -34,14 +34,27 @@ static bool match_i2c(struct device *dev, struct
> > v4l2_async_subdev *asd) #endif
> >  }
> > 
> > -static bool match_devname(struct device *dev, struct v4l2_async_subdev
> > *asd) +static bool match_devname(struct v4l2_subdev *sd,
> > +			  struct v4l2_async_subdev *asd)
> >  {
> > -	return !strcmp(asd->match.device_name.name, dev_name(dev));
> > +	return !strcmp(asd->match.device_name.name, dev_name(sd->dev));
> >  }
> > 
> > -static bool match_of(struct device *dev, struct v4l2_async_subdev *asd)
> > +static bool match_of(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
> > {
> > -	return dev->of_node == asd->match.of.node;
> > +	struct device_node *of_node =
> > +		sd->of_node ? sd->of_node : sd->dev->of_node;
> > +
> > +	return of_node == asd->match.of.node;
> > +}
> > +
> > +static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev
> > *asd) +{
> > +	if (!asd->match.custom.match)
> > +		/* Match always */
> > +		return true;
> > +
> > +	return asd->match.custom.match(sd->dev, asd);
> >  }
> > 
> >  static LIST_HEAD(subdev_list);
> > @@ -51,17 +64,14 @@ static DEFINE_MUTEX(list_lock);
> >  static struct v4l2_async_subdev *v4l2_async_belongs(struct
> > v4l2_async_notifier *notifier, struct v4l2_subdev *sd)
> >  {
> > +	bool (*match)(struct v4l2_subdev *, struct v4l2_async_subdev *);
> >  	struct v4l2_async_subdev *asd;
> > -	bool (*match)(struct device *, struct v4l2_async_subdev *);
> > 
> >  	list_for_each_entry(asd, &notifier->waiting, list) {
> >  		/* bus_type has been verified valid before */
> >  		switch (asd->match_type) {
> >  		case V4L2_ASYNC_MATCH_CUSTOM:
> > -			match = asd->match.custom.match;
> > -			if (!match)
> > -				/* Match always */
> > -				return asd;
> > +			match = match_custom;
> >  			break;
> >  		case V4L2_ASYNC_MATCH_DEVNAME:
> >  			match = match_devname;
> > @@ -79,7 +89,7 @@ static struct v4l2_async_subdev *v4l2_async_belongs(struct
> > v4l2_async_notifier * }
> > 
> >  		/* match cannot be NULL here */
> > -		if (match(sd->dev, asd))
> > +		if (match(sd, asd))
> >  			return asd;
> >  	}
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-device.c
> > b/drivers/media/v4l2-core/v4l2-device.c index 5b0a30b..a741c6c 100644
> > --- a/drivers/media/v4l2-core/v4l2-device.c
> > +++ b/drivers/media/v4l2-core/v4l2-device.c
> > @@ -157,6 +157,9 @@ int v4l2_device_register_subdev(struct v4l2_device
> > *v4l2_dev, /* Warn if we apparently re-register a subdev */
> >  	WARN_ON(sd->v4l2_dev != NULL);
> > 
> > +	if (!sd->of_node && sd->dev)
> > +		sd->of_node = sd->dev->of_node;
> > +
> >  	/*
> >  	 * The reason to acquire the module here is to avoid unloading
> >  	 * a module of sub-device which is registered to a media
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index 8f5da73..5c51987 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -603,6 +603,8 @@ struct v4l2_subdev {
> >  	struct video_device *devnode;
> >  	/* pointer to the physical device, if any */
> >  	struct device *dev;
> > +	/* A device_node of the sub-device, iff not dev->of_node. */
> 
> "iff" ?
> 
> I think we need to define usage of this field more precisely. If the subdev 
> driver doesn't initialize it it will be set in v4l2_device_register_subdev(), 
> meaning that there's part of the lifetime of the subdev during which the field 
> will be NULL even though the subdev's device has a DT node. Could that be a 
> problem, aren't there cases when we would need to access sd->of_node before 
> v4l2_device_register_subdev(), in particular when using V4l2-async ?

Thinking about this again, I'll move the check for sd->of_node and the
assignment to v4l2_async_register_subdev() instead. This way the field will
always be available. The extra check in match_of() isn't needed in that
case.

The comment would be:

	/* A device_node of the subdev, usually the same as dev->of_node. */

The alternative would be to change all the drivers using V4L2 async (and OF)
but there are quite a few of them.

What do you think?

> 
> > +	struct device_node *of_node;
> >  	/* Links this subdev to a global subdev_list or @notifier->done list. */
> >  	struct list_head async_list;
> >  	/* Pointer to respective struct v4l2_async_subdev. */

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

[PATCH v1.1 1/5] v4l: async: Add a pointer to of_node to struct v4l2_subdev, match it

[Sakari Ailus]

V4L2 async sub-devices are currently matched (OF case) based on the struct
device_node pointer in struct device. LED devices may have more than one
LED, and in that case the OF node to match is not directly the device's
node, but a LED's node.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
since v1:

- Move conditional setting of struct v4l2_subdev.of_node from
  v4l2_device_register_subdev() to v4l2_async_register_subdev.

- Remove the check for NULL struct v4l2_subdev.of_node from match_of() as
  it's no longer needed.

- Unconditionally state in the struct v4l2_subdev.of_node field comment that
  the field contains (a pointer to) the sub-device's of_node.

 drivers/media/v4l2-core/v4l2-async.c | 34 ++++++++++++++++++++++------------
 include/media/v4l2-subdev.h          |  2 ++
 2 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 85a6a34..b0badac 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -22,10 +22,10 @@
 #include <media/v4l2-device.h>
 #include <media/v4l2-subdev.h>
 
-static bool match_i2c(struct device *dev, struct v4l2_async_subdev *asd)
+static bool match_i2c(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
 {
 #if IS_ENABLED(CONFIG_I2C)
-	struct i2c_client *client = i2c_verify_client(dev);
+	struct i2c_client *client = i2c_verify_client(sd->dev);
 	return client &&
 		asd->match.i2c.adapter_id == client->adapter->nr &&
 		asd->match.i2c.address == client->addr;
@@ -34,14 +34,24 @@ static bool match_i2c(struct device *dev, struct v4l2_async_subdev *asd)
 #endif
 }
 
-static bool match_devname(struct device *dev, struct v4l2_async_subdev *asd)
+static bool match_devname(struct v4l2_subdev *sd,
+			  struct v4l2_async_subdev *asd)
 {
-	return !strcmp(asd->match.device_name.name, dev_name(dev));
+	return !strcmp(asd->match.device_name.name, dev_name(sd->dev));
 }
 
-static bool match_of(struct device *dev, struct v4l2_async_subdev *asd)
+static bool match_of(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
 {
-	return dev->of_node == asd->match.of.node;
+	return sd->of_node == asd->match.of.node;
+}
+
+static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
+{
+	if (!asd->match.custom.match)
+		/* Match always */
+		return true;
+
+	return asd->match.custom.match(sd->dev, asd);
 }
 
 static LIST_HEAD(subdev_list);
@@ -51,17 +61,14 @@ static DEFINE_MUTEX(list_lock);
 static struct v4l2_async_subdev *v4l2_async_belongs(struct v4l2_async_notifier *notifier,
 						    struct v4l2_subdev *sd)
 {
+	bool (*match)(struct v4l2_subdev *, struct v4l2_async_subdev *);
 	struct v4l2_async_subdev *asd;
-	bool (*match)(struct device *, struct v4l2_async_subdev *);
 
 	list_for_each_entry(asd, &notifier->waiting, list) {
 		/* bus_type has been verified valid before */
 		switch (asd->match_type) {
 		case V4L2_ASYNC_MATCH_CUSTOM:
-			match = asd->match.custom.match;
-			if (!match)
-				/* Match always */
-				return asd;
+			match = match_custom;
 			break;
 		case V4L2_ASYNC_MATCH_DEVNAME:
 			match = match_devname;
@@ -79,7 +86,7 @@ static struct v4l2_async_subdev *v4l2_async_belongs(struct v4l2_async_notifier *
 		}
 
 		/* match cannot be NULL here */
-		if (match(sd->dev, asd))
+		if (match(sd, asd))
 			return asd;
 	}
 
@@ -266,6 +273,9 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd)
 {
 	struct v4l2_async_notifier *notifier;
 
+	if (!sd->of_node && sd->dev)
+		sd->of_node = sd->dev->of_node;
+
 	mutex_lock(&list_lock);
 
 	INIT_LIST_HEAD(&sd->async_list);
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 8f5da73..8a17c24 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -603,6 +603,8 @@ struct v4l2_subdev {
 	struct video_device *devnode;
 	/* pointer to the physical device, if any */
 	struct device *dev;
+	/* A device_node of the subdev, usually the same as dev->of_node. */
+	struct device_node *of_node;
 	/* Links this subdev to a global subdev_list or @notifier->done list. */
 	struct list_head async_list;
 	/* Pointer to respective struct v4l2_async_subdev. */
-- 
2.1.4

Re: [PATCH v1.1 1/5] v4l: async: Add a pointer to of_node to struct v4l2_subdev, match it

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch. Please see below for one small comment.

On Monday 01 June 2015 01:24:39 Sakari Ailus wrote:
> V4L2 async sub-devices are currently matched (OF case) based on the struct
> device_node pointer in struct device. LED devices may have more than one
> LED, and in that case the OF node to match is not directly the device's
> node, but a LED's node.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> ---
> since v1:
> 
> - Move conditional setting of struct v4l2_subdev.of_node from
>   v4l2_device_register_subdev() to v4l2_async_register_subdev.
> 
> - Remove the check for NULL struct v4l2_subdev.of_node from match_of() as
>   it's no longer needed.
> 
> - Unconditionally state in the struct v4l2_subdev.of_node field comment that
> the field contains (a pointer to) the sub-device's of_node.
> 
>  drivers/media/v4l2-core/v4l2-async.c | 34 +++++++++++++++++++++------------
>  include/media/v4l2-subdev.h          |  2 ++
>  2 files changed, 24 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c
> b/drivers/media/v4l2-core/v4l2-async.c index 85a6a34..b0badac 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c

[snip]

> @@ -266,6 +273,9 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd)
>  {
>  	struct v4l2_async_notifier *notifier;
> 
> +	if (!sd->of_node && sd->dev)
> +		sd->of_node = sd->dev->of_node;
> +

I think we don't need to take a reference to of_node here, as we assume 
there's a reference to dev through the whole life of the subdev, and dev 
should have a reference to of_node, but could you double-check ?

If that's indeed not a problem,

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

(and maybe a small comment in the source code would be useful)

>  	mutex_lock(&list_lock);
> 
>  	INIT_LIST_HEAD(&sd->async_list);
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 8f5da73..8a17c24 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -603,6 +603,8 @@ struct v4l2_subdev {
>  	struct video_device *devnode;
>  	/* pointer to the physical device, if any */
>  	struct device *dev;
> +	/* A device_node of the subdev, usually the same as dev->of_node. */
> +	struct device_node *of_node;
>  	/* Links this subdev to a global subdev_list or @notifier->done list. */
>  	struct list_head async_list;
>  	/* Pointer to respective struct v4l2_async_subdev. */

-- 
Regards,

Laurent Pinchart

Re: [PATCH v1.1 1/5] v4l: async: Add a pointer to of_node to struct v4l2_subdev, match it

[Sakari Ailus]

Hi Laurent,

On Tue, Jun 02, 2015 at 05:47:12AM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch. Please see below for one small comment.
> 
> On Monday 01 June 2015 01:24:39 Sakari Ailus wrote:
> > V4L2 async sub-devices are currently matched (OF case) based on the struct
> > device_node pointer in struct device. LED devices may have more than one
> > LED, and in that case the OF node to match is not directly the device's
> > node, but a LED's node.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> > ---
> > since v1:
> > 
> > - Move conditional setting of struct v4l2_subdev.of_node from
> >   v4l2_device_register_subdev() to v4l2_async_register_subdev.
> > 
> > - Remove the check for NULL struct v4l2_subdev.of_node from match_of() as
> >   it's no longer needed.
> > 
> > - Unconditionally state in the struct v4l2_subdev.of_node field comment that
> > the field contains (a pointer to) the sub-device's of_node.
> > 
> >  drivers/media/v4l2-core/v4l2-async.c | 34 +++++++++++++++++++++------------
> >  include/media/v4l2-subdev.h          |  2 ++
> >  2 files changed, 24 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c
> > b/drivers/media/v4l2-core/v4l2-async.c index 85a6a34..b0badac 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> 
> [snip]
> 
> > @@ -266,6 +273,9 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd)
> >  {
> >  	struct v4l2_async_notifier *notifier;
> > 
> > +	if (!sd->of_node && sd->dev)
> > +		sd->of_node = sd->dev->of_node;
> > +
> 
> I think we don't need to take a reference to of_node here, as we assume 
> there's a reference to dev through the whole life of the subdev, and dev 
> should have a reference to of_node, but could you double-check ?
> 
> If that's indeed not a problem,
> 
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Good question.

I don't think a reference would be needed also because we are only
interested in the pointer value itself, the memory the pointer is pointing
to is never accessed.

As far as I can tell, there's no reference through the device.

I can add a comment on this if you like. Perhaps it'd be a good idea
actually, as you usually do take a reference in such cases.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

[PATCH v1.2 1/5] v4l: async: Add a pointer to of_node to struct v4l2_subdev, match it

[Sakari Ailus]

V4L2 async sub-devices are currently matched (OF case) based on the struct
device_node pointer in struct device. LED devices may have more than one
LED, and in that case the OF node to match is not directly the device's
node, but a LED's node.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
since v1.1:

- Add a comment on reference in v4l2_async_register_subdev().

 drivers/media/v4l2-core/v4l2-async.c | 38 ++++++++++++++++++++++++------------
 include/media/v4l2-subdev.h          |  2 ++
 2 files changed, 28 insertions(+), 12 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 85a6a34..436f79e 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -22,10 +22,10 @@
 #include <media/v4l2-device.h>
 #include <media/v4l2-subdev.h>
 
-static bool match_i2c(struct device *dev, struct v4l2_async_subdev *asd)
+static bool match_i2c(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
 {
 #if IS_ENABLED(CONFIG_I2C)
-	struct i2c_client *client = i2c_verify_client(dev);
+	struct i2c_client *client = i2c_verify_client(sd->dev);
 	return client &&
 		asd->match.i2c.adapter_id == client->adapter->nr &&
 		asd->match.i2c.address == client->addr;
@@ -34,14 +34,24 @@ static bool match_i2c(struct device *dev, struct v4l2_async_subdev *asd)
 #endif
 }
 
-static bool match_devname(struct device *dev, struct v4l2_async_subdev *asd)
+static bool match_devname(struct v4l2_subdev *sd,
+			  struct v4l2_async_subdev *asd)
 {
-	return !strcmp(asd->match.device_name.name, dev_name(dev));
+	return !strcmp(asd->match.device_name.name, dev_name(sd->dev));
 }
 
-static bool match_of(struct device *dev, struct v4l2_async_subdev *asd)
+static bool match_of(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
 {
-	return dev->of_node == asd->match.of.node;
+	return sd->of_node == asd->match.of.node;
+}
+
+static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
+{
+	if (!asd->match.custom.match)
+		/* Match always */
+		return true;
+
+	return asd->match.custom.match(sd->dev, asd);
 }
 
 static LIST_HEAD(subdev_list);
@@ -51,17 +61,14 @@ static DEFINE_MUTEX(list_lock);
 static struct v4l2_async_subdev *v4l2_async_belongs(struct v4l2_async_notifier *notifier,
 						    struct v4l2_subdev *sd)
 {
+	bool (*match)(struct v4l2_subdev *, struct v4l2_async_subdev *);
 	struct v4l2_async_subdev *asd;
-	bool (*match)(struct device *, struct v4l2_async_subdev *);
 
 	list_for_each_entry(asd, &notifier->waiting, list) {
 		/* bus_type has been verified valid before */
 		switch (asd->match_type) {
 		case V4L2_ASYNC_MATCH_CUSTOM:
-			match = asd->match.custom.match;
-			if (!match)
-				/* Match always */
-				return asd;
+			match = match_custom;
 			break;
 		case V4L2_ASYNC_MATCH_DEVNAME:
 			match = match_devname;
@@ -79,7 +86,7 @@ static struct v4l2_async_subdev *v4l2_async_belongs(struct v4l2_async_notifier *
 		}
 
 		/* match cannot be NULL here */
-		if (match(sd->dev, asd))
+		if (match(sd, asd))
 			return asd;
 	}
 
@@ -266,6 +273,13 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd)
 {
 	struct v4l2_async_notifier *notifier;
 
+	/*
+	 * No reference taken. We're just interested in the pointer
+	 * value, not what's in the struct itself.
+	 */
+	if (!sd->of_node && sd->dev)
+		sd->of_node = sd->dev->of_node;
+
 	mutex_lock(&list_lock);
 
 	INIT_LIST_HEAD(&sd->async_list);
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 8f5da73..8a17c24 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -603,6 +603,8 @@ struct v4l2_subdev {
 	struct video_device *devnode;
 	/* pointer to the physical device, if any */
 	struct device *dev;
+	/* A device_node of the subdev, usually the same as dev->of_node. */
+	struct device_node *of_node;
 	/* Links this subdev to a global subdev_list or @notifier->done list. */
 	struct list_head async_list;
 	/* Pointer to respective struct v4l2_async_subdev. */
-- 
2.1.4

[PATCH v1.3 1/5] v4l: async: Add a pointer to of_node to struct v4l2_subdev, match it

[Sakari Ailus]

V4L2 async sub-devices are currently matched (OF case) based on the struct
device_node pointer in struct device. LED devices may have more than one
LED, and in that case the OF node to match is not directly the device's
node, but a LED's node.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
since v1.2:

- "A" -> "The" in the of_node field comment in struct v4l2_subdev.

- A better reason for not taking a reference to the of_node is that a
  reference is already there for struct device, pointed to by the dev field
  of struct v4l2_subdev. The async sub-device never exists without a device.

 drivers/media/v4l2-core/v4l2-async.c | 39 +++++++++++++++++++++++++-----------
 include/media/v4l2-subdev.h          |  2 ++
 2 files changed, 29 insertions(+), 12 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 85a6a34..5bada20 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -22,10 +22,10 @@
 #include <media/v4l2-device.h>
 #include <media/v4l2-subdev.h>
 
-static bool match_i2c(struct device *dev, struct v4l2_async_subdev *asd)
+static bool match_i2c(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
 {
 #if IS_ENABLED(CONFIG_I2C)
-	struct i2c_client *client = i2c_verify_client(dev);
+	struct i2c_client *client = i2c_verify_client(sd->dev);
 	return client &&
 		asd->match.i2c.adapter_id == client->adapter->nr &&
 		asd->match.i2c.address == client->addr;
@@ -34,14 +34,24 @@ static bool match_i2c(struct device *dev, struct v4l2_async_subdev *asd)
 #endif
 }
 
-static bool match_devname(struct device *dev, struct v4l2_async_subdev *asd)
+static bool match_devname(struct v4l2_subdev *sd,
+			  struct v4l2_async_subdev *asd)
 {
-	return !strcmp(asd->match.device_name.name, dev_name(dev));
+	return !strcmp(asd->match.device_name.name, dev_name(sd->dev));
 }
 
-static bool match_of(struct device *dev, struct v4l2_async_subdev *asd)
+static bool match_of(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
 {
-	return dev->of_node == asd->match.of.node;
+	return sd->of_node == asd->match.of.node;
+}
+
+static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
+{
+	if (!asd->match.custom.match)
+		/* Match always */
+		return true;
+
+	return asd->match.custom.match(sd->dev, asd);
 }
 
 static LIST_HEAD(subdev_list);
@@ -51,17 +61,14 @@ static DEFINE_MUTEX(list_lock);
 static struct v4l2_async_subdev *v4l2_async_belongs(struct v4l2_async_notifier *notifier,
 						    struct v4l2_subdev *sd)
 {
+	bool (*match)(struct v4l2_subdev *, struct v4l2_async_subdev *);
 	struct v4l2_async_subdev *asd;
-	bool (*match)(struct device *, struct v4l2_async_subdev *);
 
 	list_for_each_entry(asd, &notifier->waiting, list) {
 		/* bus_type has been verified valid before */
 		switch (asd->match_type) {
 		case V4L2_ASYNC_MATCH_CUSTOM:
-			match = asd->match.custom.match;
-			if (!match)
-				/* Match always */
-				return asd;
+			match = match_custom;
 			break;
 		case V4L2_ASYNC_MATCH_DEVNAME:
 			match = match_devname;
@@ -79,7 +86,7 @@ static struct v4l2_async_subdev *v4l2_async_belongs(struct v4l2_async_notifier *
 		}
 
 		/* match cannot be NULL here */
-		if (match(sd->dev, asd))
+		if (match(sd, asd))
 			return asd;
 	}
 
@@ -266,6 +273,14 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd)
 {
 	struct v4l2_async_notifier *notifier;
 
+	/*
+	 * No reference taken. The reference is held by the device
+	 * (struct v4l2_subdev.dev), and async sub-device does not
+	 * exist independently of the device at any point of time.
+	 */
+	if (!sd->of_node && sd->dev)
+		sd->of_node = sd->dev->of_node;
+
 	mutex_lock(&list_lock);
 
 	INIT_LIST_HEAD(&sd->async_list);
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 8f5da73..cdd534b 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -603,6 +603,8 @@ struct v4l2_subdev {
 	struct video_device *devnode;
 	/* pointer to the physical device, if any */
 	struct device *dev;
+	/* The device_node of the subdev, usually the same as dev->of_node. */
+	struct device_node *of_node;
 	/* Links this subdev to a global subdev_list or @notifier->done list. */
 	struct list_head async_list;
 	/* Pointer to respective struct v4l2_async_subdev. */
-- 
2.1.4

Re: [PATCH v1.3 1/5] v4l: async: Add a pointer to of_node to struct v4l2_subdev, match it

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Thursday 11 June 2015 22:18:01 Sakari Ailus wrote:
> V4L2 async sub-devices are currently matched (OF case) based on the struct
> device_node pointer in struct device. LED devices may have more than one
> LED, and in that case the OF node to match is not directly the device's
> node, but a LED's node.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> since v1.2:
> 
> - "A" -> "The" in the of_node field comment in struct v4l2_subdev.
> 
> - A better reason for not taking a reference to the of_node is that a
>   reference is already there for struct device, pointed to by the dev field
>   of struct v4l2_subdev. The async sub-device never exists without a device.
> 
>  drivers/media/v4l2-core/v4l2-async.c | 39 ++++++++++++++++++++++-----------
>  include/media/v4l2-subdev.h          |  2 ++
>  2 files changed, 29 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c
> b/drivers/media/v4l2-core/v4l2-async.c index 85a6a34..5bada20 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -22,10 +22,10 @@
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-subdev.h>
> 
> -static bool match_i2c(struct device *dev, struct v4l2_async_subdev *asd)
> +static bool match_i2c(struct v4l2_subdev *sd, struct v4l2_async_subdev
> *asd) {
>  #if IS_ENABLED(CONFIG_I2C)
> -	struct i2c_client *client = i2c_verify_client(dev);
> +	struct i2c_client *client = i2c_verify_client(sd->dev);
>  	return client &&
>  		asd->match.i2c.adapter_id == client->adapter->nr &&
>  		asd->match.i2c.address == client->addr;
> @@ -34,14 +34,24 @@ static bool match_i2c(struct device *dev, struct
> v4l2_async_subdev *asd) #endif
>  }
> 
> -static bool match_devname(struct device *dev, struct v4l2_async_subdev
> *asd) +static bool match_devname(struct v4l2_subdev *sd,
> +			  struct v4l2_async_subdev *asd)
>  {
> -	return !strcmp(asd->match.device_name.name, dev_name(dev));
> +	return !strcmp(asd->match.device_name.name, dev_name(sd->dev));
>  }
> 
> -static bool match_of(struct device *dev, struct v4l2_async_subdev *asd)
> +static bool match_of(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
> {
> -	return dev->of_node == asd->match.of.node;
> +	return sd->of_node == asd->match.of.node;
> +}
> +
> +static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev
> *asd) +{
> +	if (!asd->match.custom.match)
> +		/* Match always */
> +		return true;
> +
> +	return asd->match.custom.match(sd->dev, asd);
>  }
> 
>  static LIST_HEAD(subdev_list);
> @@ -51,17 +61,14 @@ static DEFINE_MUTEX(list_lock);
>  static struct v4l2_async_subdev *v4l2_async_belongs(struct
> v4l2_async_notifier *notifier, struct v4l2_subdev *sd)
>  {
> +	bool (*match)(struct v4l2_subdev *, struct v4l2_async_subdev *);
>  	struct v4l2_async_subdev *asd;
> -	bool (*match)(struct device *, struct v4l2_async_subdev *);
> 
>  	list_for_each_entry(asd, &notifier->waiting, list) {
>  		/* bus_type has been verified valid before */
>  		switch (asd->match_type) {
>  		case V4L2_ASYNC_MATCH_CUSTOM:
> -			match = asd->match.custom.match;
> -			if (!match)
> -				/* Match always */
> -				return asd;
> +			match = match_custom;
>  			break;
>  		case V4L2_ASYNC_MATCH_DEVNAME:
>  			match = match_devname;
> @@ -79,7 +86,7 @@ static struct v4l2_async_subdev *v4l2_async_belongs(struct
> v4l2_async_notifier * }
> 
>  		/* match cannot be NULL here */
> -		if (match(sd->dev, asd))
> +		if (match(sd, asd))
>  			return asd;
>  	}
> 
> @@ -266,6 +273,14 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd)
>  {
>  	struct v4l2_async_notifier *notifier;
> 
> +	/*
> +	 * No reference taken. The reference is held by the device
> +	 * (struct v4l2_subdev.dev), and async sub-device does not
> +	 * exist independently of the device at any point of time.
> +	 */
> +	if (!sd->of_node && sd->dev)
> +		sd->of_node = sd->dev->of_node;
> +
>  	mutex_lock(&list_lock);
> 
>  	INIT_LIST_HEAD(&sd->async_list);
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 8f5da73..cdd534b 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -603,6 +603,8 @@ struct v4l2_subdev {
>  	struct video_device *devnode;
>  	/* pointer to the physical device, if any */
>  	struct device *dev;
> +	/* The device_node of the subdev, usually the same as dev->of_node. */
> +	struct device_node *of_node;
>  	/* Links this subdev to a global subdev_list or @notifier->done list. */
>  	struct list_head async_list;
>  	/* Pointer to respective struct v4l2_async_subdev. */

-- 
Regards,

Laurent Pinchart

Re: [PATCH v1.3 1/5] v4l: async: Add a pointer to of_node to struct v4l2_subdev, match it

[Sakari Ailus]

On Thu, Jun 11, 2015 at 10:27:30PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Thursday 11 June 2015 22:18:01 Sakari Ailus wrote:
> > V4L2 async sub-devices are currently matched (OF case) based on the struct
> > device_node pointer in struct device. LED devices may have more than one
> > LED, and in that case the OF node to match is not directly the device's
> > node, but a LED's node.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> 
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Bryan, could you apply the patch to your tree? It's required by Jacek's
patchset you attempted to apply a few days back.

Thanks!

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH v1.3 1/5] v4l: async: Add a pointer to of_node to struct v4l2_subdev, match it

[Bryan Wu]

On Thu, Jun 11, 2015 at 11:41 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> On Thu, Jun 11, 2015 at 10:27:30PM +0300, Laurent Pinchart wrote:
>> Hi Sakari,
>>
>> Thank you for the patch.
>>
>> On Thursday 11 June 2015 22:18:01 Sakari Ailus wrote:
>> > V4L2 async sub-devices are currently matched (OF case) based on the struct
>> > device_node pointer in struct device. LED devices may have more than one
>> > LED, and in that case the OF node to match is not directly the device's
>> > node, but a LED's node.
>> >
>> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
>>
>> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> Bryan, could you apply the patch to your tree? It's required by Jacek's
> patchset you attempted to apply a few days back.
>
> Thanks!

Sure, I will merge that through my tree.

Thanks,
-Bryan

[PATCH 2/5] v4l: flash: Make v4l2_flash_init() and v4l2_flash_release() functions always

[Sakari Ailus]

If CONFIG_V4L2_FLASH_LED_CLASS wasn't defined, v4l2_flash_init() and
v4l2_flash_release() were empty macros. This will lead to compiler warnings
in form of unused variables if the variables are not used for other
purposes.

Instead, implement v4l2_flash_init() and v4l2_flash_release() as static
inline functions.

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
 include/media/v4l2-flash.h |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/include/media/v4l2-flash.h b/include/media/v4l2-flash.h
index 945fa08..67a2cbf 100644
--- a/include/media/v4l2-flash.h
+++ b/include/media/v4l2-flash.h
@@ -138,8 +138,16 @@ struct v4l2_flash *v4l2_flash_init(struct led_classdev_flash *fled_cdev,
 void v4l2_flash_release(struct v4l2_flash *v4l2_flash);
 
 #else
-#define v4l2_flash_init(fled_cdev, ops, config) (NULL)
-#define v4l2_flash_release(v4l2_flash)
+static inline struct v4l2_flash *v4l2_flash_init(
+	struct led_classdev_flash *fled_cdev, const struct v4l2_flash_ops *ops,
+	struct v4l2_flash_config *config)
+{
+	return NULL;
+}
+
+static inline void v4l2_flash_release(struct v4l2_flash *v4l2_flash)
+{
+}
 #endif /* CONFIG_V4L2_FLASH_LED_CLASS */
 
 #endif /* _V4L2_FLASH_H */
-- 
1.7.10.4

Re: [PATCH 2/5] v4l: flash: Make v4l2_flash_init() and v4l2_flash_release() functions always

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Wednesday 20 May 2015 02:04:02 Sakari Ailus wrote:
> If CONFIG_V4L2_FLASH_LED_CLASS wasn't defined, v4l2_flash_init() and
> v4l2_flash_release() were empty macros. This will lead to compiler warnings
> in form of unused variables if the variables are not used for other
> purposes.
> 
> Instead, implement v4l2_flash_init() and v4l2_flash_release() as static
> inline functions.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  include/media/v4l2-flash.h |   12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/include/media/v4l2-flash.h b/include/media/v4l2-flash.h
> index 945fa08..67a2cbf 100644
> --- a/include/media/v4l2-flash.h
> +++ b/include/media/v4l2-flash.h
> @@ -138,8 +138,16 @@ struct v4l2_flash *v4l2_flash_init(struct
> led_classdev_flash *fled_cdev, void v4l2_flash_release(struct v4l2_flash
> *v4l2_flash);
> 
>  #else
> -#define v4l2_flash_init(fled_cdev, ops, config) (NULL)
> -#define v4l2_flash_release(v4l2_flash)
> +static inline struct v4l2_flash *v4l2_flash_init(
> +	struct led_classdev_flash *fled_cdev, const struct v4l2_flash_ops *ops,
> +	struct v4l2_flash_config *config)
> +{
> +	return NULL;
> +}
> +
> +static inline void v4l2_flash_release(struct v4l2_flash *v4l2_flash)
> +{
> +}
>  #endif /* CONFIG_V4L2_FLASH_LED_CLASS */
> 
>  #endif /* _V4L2_FLASH_H */

-- 
Regards,

Laurent Pinchart

[PATCH 3/5] v4l: flash: Pass struct device and device_node to v4l2_flash_init()

[Sakari Ailus]

The V4L2 sub-device node's dev will thus refer to the physical device, not
the LED flash device node. Also matching against device_node is possible in
cases where the LED flash controller drives multiple LEDs.

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
 drivers/media/v4l2-core/v4l2-flash.c |   10 ++++++----
 include/media/v4l2-flash.h           |   10 +++++++---
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-flash.c b/drivers/media/v4l2-core/v4l2-flash.c
index bed2036..d182adc 100644
--- a/drivers/media/v4l2-core/v4l2-flash.c
+++ b/drivers/media/v4l2-core/v4l2-flash.c
@@ -542,9 +542,10 @@ static const struct v4l2_subdev_ops v4l2_flash_subdev_ops = {
 	.core = &v4l2_flash_core_ops,
 };
 
-struct v4l2_flash *v4l2_flash_init(struct led_classdev_flash *fled_cdev,
-				   const struct v4l2_flash_ops *ops,
-				   struct v4l2_flash_config *config)
+struct v4l2_flash *v4l2_flash_init(
+	struct device *dev, struct device_node *of_node,
+	struct led_classdev_flash *fled_cdev, const struct v4l2_flash_ops *ops,
+	struct v4l2_flash_config *config)
 {
 	struct v4l2_flash *v4l2_flash;
 	struct led_classdev *led_cdev = &fled_cdev->led_cdev;
@@ -562,7 +563,8 @@ struct v4l2_flash *v4l2_flash_init(struct led_classdev_flash *fled_cdev,
 	sd = &v4l2_flash->sd;
 	v4l2_flash->fled_cdev = fled_cdev;
 	v4l2_flash->ops = ops;
-	sd->dev = led_cdev->dev;
+	sd->dev = dev;
+	sd->of_node = of_node;
 	v4l2_subdev_init(sd, &v4l2_flash_subdev_ops);
 	sd->internal_ops = &v4l2_flash_subdev_internal_ops;
 	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
diff --git a/include/media/v4l2-flash.h b/include/media/v4l2-flash.h
index 67a2cbf..31e421d 100644
--- a/include/media/v4l2-flash.h
+++ b/include/media/v4l2-flash.h
@@ -115,6 +115,8 @@ static inline struct v4l2_flash *v4l2_ctrl_to_v4l2_flash(struct v4l2_ctrl *c)
 #if IS_ENABLED(CONFIG_V4L2_FLASH_LED_CLASS)
 /**
  * v4l2_flash_init - initialize V4L2 flash led sub-device
+ * @dev:	flash device, e.g. an I2C device
+ * @of_node:	of_node of the LED, may be NULL if the same as device's
  * @fled_cdev:	the LED Flash class device to wrap
  * @flash_ops:	V4L2 Flash device ops
  * @config:	initialization data for V4L2 Flash sub-device
@@ -125,9 +127,10 @@ static inline struct v4l2_flash *v4l2_ctrl_to_v4l2_flash(struct v4l2_ctrl *c)
  * value is encoded using ERR_PTR(). Use IS_ERR() to check and
  * PTR_ERR() to obtain the numeric return value.
  */
-struct v4l2_flash *v4l2_flash_init(struct led_classdev_flash *fled_cdev,
-				   const struct v4l2_flash_ops *ops,
-				   struct v4l2_flash_config *config);
+struct v4l2_flash *v4l2_flash_init(
+	struct device *dev, struct device_node *of_node,
+	struct led_classdev_flash *fled_cdev, const struct v4l2_flash_ops *ops,
+	struct v4l2_flash_config *config);
 
 /**
  * v4l2_flash_release - release V4L2 Flash sub-device
@@ -139,6 +142,7 @@ void v4l2_flash_release(struct v4l2_flash *v4l2_flash);
 
 #else
 static inline struct v4l2_flash *v4l2_flash_init(
+	struct device *dev, struct device_node *of_node,
 	struct led_classdev_flash *fled_cdev, const struct v4l2_flash_ops *ops,
 	struct v4l2_flash_config *config)
 {
-- 
1.7.10.4

[PATCH 4/5] leds: aat1290: Pass dev and dev->of_node to v4l2_flash_init()

[Sakari Ailus]

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
 drivers/leds/leds-aat1290.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/leds/leds-aat1290.c b/drivers/leds/leds-aat1290.c
index c656a2d..71bf6bb 100644
--- a/drivers/leds/leds-aat1290.c
+++ b/drivers/leds/leds-aat1290.c
@@ -524,9 +524,8 @@ static int aat1290_led_probe(struct platform_device *pdev)
 	led_cdev->dev->of_node = sub_node;
 
 	/* Create V4L2 Flash subdev. */
-	led->v4l2_flash = v4l2_flash_init(fled_cdev,
-					  &v4l2_flash_ops,
-					  &v4l2_sd_cfg);
+	led->v4l2_flash = v4l2_flash_init(dev, NULL, fled_cdev,
+					  &v4l2_flash_ops, &v4l2_sd_cfg);
 	if (IS_ERR(led->v4l2_flash)) {
 		ret = PTR_ERR(led->v4l2_flash);
 		goto error_v4l2_flash_init;
-- 
1.7.10.4

Re: [PATCH 4/5] leds: aat1290: Pass dev and dev->of_node to v4l2_flash_init()

[Jacek Anaszewski]

Hi Sakari,

On 05/20/2015 01:04 AM, Sakari Ailus wrote:
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> ---
>   drivers/leds/leds-aat1290.c |    5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/leds/leds-aat1290.c b/drivers/leds/leds-aat1290.c
> index c656a2d..71bf6bb 100644
> --- a/drivers/leds/leds-aat1290.c
> +++ b/drivers/leds/leds-aat1290.c
> @@ -524,9 +524,8 @@ static int aat1290_led_probe(struct platform_device *pdev)
>   	led_cdev->dev->of_node = sub_node;
>
>   	/* Create V4L2 Flash subdev. */
> -	led->v4l2_flash = v4l2_flash_init(fled_cdev,
> -					  &v4l2_flash_ops,
> -					  &v4l2_sd_cfg);
> +	led->v4l2_flash = v4l2_flash_init(dev, NULL, fled_cdev,
> +					  &v4l2_flash_ops, &v4l2_sd_cfg);

Here the first argument should be led_cdev->dev, not dev, which is
&pdev->dev, whereas led_cdev->dev is returned by
device_create_with_groups (it takes dev as a parent) called from 
led_classdev_register.

>   	if (IS_ERR(led->v4l2_flash)) {
>   		ret = PTR_ERR(led->v4l2_flash);
>   		goto error_v4l2_flash_init;
>


-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH 4/5] leds: aat1290: Pass dev and dev->of_node to v4l2_flash_init()

[Sakari Ailus]

Hi Jacek,

On Wed, May 20, 2015 at 11:47:26AM +0200, Jacek Anaszewski wrote:
> Hi Sakari,
> 
> On 05/20/2015 01:04 AM, Sakari Ailus wrote:
> >Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> >---
> >  drivers/leds/leds-aat1290.c |    5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >
> >diff --git a/drivers/leds/leds-aat1290.c b/drivers/leds/leds-aat1290.c
> >index c656a2d..71bf6bb 100644
> >--- a/drivers/leds/leds-aat1290.c
> >+++ b/drivers/leds/leds-aat1290.c
> >@@ -524,9 +524,8 @@ static int aat1290_led_probe(struct platform_device *pdev)
> >  	led_cdev->dev->of_node = sub_node;
> >
> >  	/* Create V4L2 Flash subdev. */
> >-	led->v4l2_flash = v4l2_flash_init(fled_cdev,
> >-					  &v4l2_flash_ops,
> >-					  &v4l2_sd_cfg);
> >+	led->v4l2_flash = v4l2_flash_init(dev, NULL, fled_cdev,
> >+					  &v4l2_flash_ops, &v4l2_sd_cfg);
> 
> Here the first argument should be led_cdev->dev, not dev, which is
> &pdev->dev, whereas led_cdev->dev is returned by
> device_create_with_groups (it takes dev as a parent) called from
> led_classdev_register.

Should it? The underlying hardware is still the I2C device, not the LED
class device node.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH 4/5] leds: aat1290: Pass dev and dev->of_node to v4l2_flash_init()

[Jacek Anaszewski]

On 05/20/2015 11:47 AM, Jacek Anaszewski wrote:
> Hi Sakari,
>
> On 05/20/2015 01:04 AM, Sakari Ailus wrote:
>> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
>> ---
>>   drivers/leds/leds-aat1290.c |    5 ++---
>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/leds/leds-aat1290.c b/drivers/leds/leds-aat1290.c
>> index c656a2d..71bf6bb 100644
>> --- a/drivers/leds/leds-aat1290.c
>> +++ b/drivers/leds/leds-aat1290.c
>> @@ -524,9 +524,8 @@ static int aat1290_led_probe(struct
>> platform_device *pdev)
>>       led_cdev->dev->of_node = sub_node;
>>
>>       /* Create V4L2 Flash subdev. */
>> -    led->v4l2_flash = v4l2_flash_init(fled_cdev,
>> -                      &v4l2_flash_ops,
>> -                      &v4l2_sd_cfg);
>> +    led->v4l2_flash = v4l2_flash_init(dev, NULL, fled_cdev,
>> +                      &v4l2_flash_ops, &v4l2_sd_cfg);
>
> Here the first argument should be led_cdev->dev, not dev, which is
> &pdev->dev, whereas led_cdev->dev is returned by
> device_create_with_groups (it takes dev as a parent) called from
> led_classdev_register.

The reason for this is the fact that pdev->dev has its of_node
field initialized, which makes v4l2_async trying to match
subdev by parent node of a LED device, not by sub-LED related
DT node.

 From your patches it looks like you want to guarantee that
v4l2_subdev's dev field will point to the struct device
related to the LED controller, not to the sub-LED.

If so, then the second argument of v4l2_flash_init mustn't
be NULL.

I am however wondering if in case of LEDs the struct v4l2_subdev's dev
field couldn't be made pointing to sub-LEDs related struct device,
which would allow for avoiding the modifications in v4l2-async.

of_node field of struct device returned by device_create_with_groups
is NULL, and basically the remaining fields are copied from the parent.
In my approach I was just assigning to it the sub-LED related of_node.

>>       if (IS_ERR(led->v4l2_flash)) {
>>           ret = PTR_ERR(led->v4l2_flash);
>>           goto error_v4l2_flash_init;
>>
>
>


-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH 4/5] leds: aat1290: Pass dev and dev->of_node to v4l2_flash_init()

[Sakari Ailus]

Hi Jacek,

On Wed, May 20, 2015 at 12:37:03PM +0200, Jacek Anaszewski wrote:
> On 05/20/2015 11:47 AM, Jacek Anaszewski wrote:
> >Hi Sakari,
> >
> >On 05/20/2015 01:04 AM, Sakari Ailus wrote:
> >>Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> >>---
> >>  drivers/leds/leds-aat1290.c |    5 ++---
> >>  1 file changed, 2 insertions(+), 3 deletions(-)
> >>
> >>diff --git a/drivers/leds/leds-aat1290.c b/drivers/leds/leds-aat1290.c
> >>index c656a2d..71bf6bb 100644
> >>--- a/drivers/leds/leds-aat1290.c
> >>+++ b/drivers/leds/leds-aat1290.c
> >>@@ -524,9 +524,8 @@ static int aat1290_led_probe(struct
> >>platform_device *pdev)
> >>      led_cdev->dev->of_node = sub_node;
> >>
> >>      /* Create V4L2 Flash subdev. */
> >>-    led->v4l2_flash = v4l2_flash_init(fled_cdev,
> >>-                      &v4l2_flash_ops,
> >>-                      &v4l2_sd_cfg);
> >>+    led->v4l2_flash = v4l2_flash_init(dev, NULL, fled_cdev,
> >>+                      &v4l2_flash_ops, &v4l2_sd_cfg);
> >
> >Here the first argument should be led_cdev->dev, not dev, which is
> >&pdev->dev, whereas led_cdev->dev is returned by
> >device_create_with_groups (it takes dev as a parent) called from
> >led_classdev_register.
> 
> The reason for this is the fact that pdev->dev has its of_node
> field initialized, which makes v4l2_async trying to match
> subdev by parent node of a LED device, not by sub-LED related
> DT node.

If v4l2_subdev->of_node is set, then it won't be replaced with one from
struct device. I.e. you need to provide of_node pointer only if it's
different from dev->of_node.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH 4/5] leds: aat1290: Pass dev and dev->of_node to v4l2_flash_init()

[Jacek Anaszewski]

On 05/20/2015 02:27 PM, Sakari Ailus wrote:
> Hi Jacek,
>
> On Wed, May 20, 2015 at 12:37:03PM +0200, Jacek Anaszewski wrote:
>> On 05/20/2015 11:47 AM, Jacek Anaszewski wrote:
>>> Hi Sakari,
>>>
>>> On 05/20/2015 01:04 AM, Sakari Ailus wrote:
>>>> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
>>>> ---
>>>>   drivers/leds/leds-aat1290.c |    5 ++---
>>>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/leds/leds-aat1290.c b/drivers/leds/leds-aat1290.c
>>>> index c656a2d..71bf6bb 100644
>>>> --- a/drivers/leds/leds-aat1290.c
>>>> +++ b/drivers/leds/leds-aat1290.c
>>>> @@ -524,9 +524,8 @@ static int aat1290_led_probe(struct
>>>> platform_device *pdev)
>>>>       led_cdev->dev->of_node = sub_node;
>>>>
>>>>       /* Create V4L2 Flash subdev. */
>>>> -    led->v4l2_flash = v4l2_flash_init(fled_cdev,
>>>> -                      &v4l2_flash_ops,
>>>> -                      &v4l2_sd_cfg);
>>>> +    led->v4l2_flash = v4l2_flash_init(dev, NULL, fled_cdev,
>>>> +                      &v4l2_flash_ops, &v4l2_sd_cfg);
>>>
>>> Here the first argument should be led_cdev->dev, not dev, which is
>>> &pdev->dev, whereas led_cdev->dev is returned by
>>> device_create_with_groups (it takes dev as a parent) called from
>>> led_classdev_register.
>>
>> The reason for this is the fact that pdev->dev has its of_node
>> field initialized, which makes v4l2_async trying to match
>> subdev by parent node of a LED device, not by sub-LED related
>> DT node.
>
> If v4l2_subdev->of_node is set, then it won't be replaced with one from
> struct device. I.e. you need to provide of_node pointer only if it's
> different from dev->of_node.
>

It will always be different since dev->of_node pointer is related
to the main DT node of LED device, whereas each LED connected to it
must be expressed in the form of sub-node, as
Documentation/devicetree/bindings/leds/common.txt DT states.

-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH 4/5] leds: aat1290: Pass dev and dev->of_node to v4l2_flash_init()

[Sakari Ailus]

Hi Jacek,

On Wed, May 20, 2015 at 03:47:25PM +0200, Jacek Anaszewski wrote:
...
> >>>>--- a/drivers/leds/leds-aat1290.c
> >>>>+++ b/drivers/leds/leds-aat1290.c
> >>>>@@ -524,9 +524,8 @@ static int aat1290_led_probe(struct
> >>>>platform_device *pdev)
> >>>>      led_cdev->dev->of_node = sub_node;
> >>>>
> >>>>      /* Create V4L2 Flash subdev. */
> >>>>-    led->v4l2_flash = v4l2_flash_init(fled_cdev,
> >>>>-                      &v4l2_flash_ops,
> >>>>-                      &v4l2_sd_cfg);
> >>>>+    led->v4l2_flash = v4l2_flash_init(dev, NULL, fled_cdev,
> >>>>+                      &v4l2_flash_ops, &v4l2_sd_cfg);
> >>>
> >>>Here the first argument should be led_cdev->dev, not dev, which is
> >>>&pdev->dev, whereas led_cdev->dev is returned by
> >>>device_create_with_groups (it takes dev as a parent) called from
> >>>led_classdev_register.
> >>
> >>The reason for this is the fact that pdev->dev has its of_node
> >>field initialized, which makes v4l2_async trying to match
> >>subdev by parent node of a LED device, not by sub-LED related
> >>DT node.
> >
> >If v4l2_subdev->of_node is set, then it won't be replaced with one from
> >struct device. I.e. you need to provide of_node pointer only if it's
> >different from dev->of_node.
> >
> 
> It will always be different since dev->of_node pointer is related
> to the main DT node of LED device, whereas each LED connected to it
> must be expressed in the form of sub-node, as
> Documentation/devicetree/bindings/leds/common.txt DT states.

You can still refer to the device's root device_node using a phandle.

Say, if you have a LED flash controller with an indicator. It's intended to
be used together with the flash LED, and the existing as3645a driver exposes
it through the same sub-device. I think that'd make sense with LED class
driver as well (i.e. you'd have two LED class devices but a single
sub-device). Small changes to the wrapper would be needed.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH 4/5] leds: aat1290: Pass dev and dev->of_node to v4l2_flash_init()

[Jacek Anaszewski]

Hi Sakari,

On 05/20/2015 04:31 PM, Sakari Ailus wrote:
> Hi Jacek,
>
> On Wed, May 20, 2015 at 03:47:25PM +0200, Jacek Anaszewski wrote:
> ...
>>>>>> --- a/drivers/leds/leds-aat1290.c
>>>>>> +++ b/drivers/leds/leds-aat1290.c
>>>>>> @@ -524,9 +524,8 @@ static int aat1290_led_probe(struct
>>>>>> platform_device *pdev)
>>>>>>       led_cdev->dev->of_node = sub_node;
>>>>>>
>>>>>>       /* Create V4L2 Flash subdev. */
>>>>>> -    led->v4l2_flash = v4l2_flash_init(fled_cdev,
>>>>>> -                      &v4l2_flash_ops,
>>>>>> -                      &v4l2_sd_cfg);
>>>>>> +    led->v4l2_flash = v4l2_flash_init(dev, NULL, fled_cdev,
>>>>>> +                      &v4l2_flash_ops, &v4l2_sd_cfg);
>>>>>
>>>>> Here the first argument should be led_cdev->dev, not dev, which is
>>>>> &pdev->dev, whereas led_cdev->dev is returned by
>>>>> device_create_with_groups (it takes dev as a parent) called from
>>>>> led_classdev_register.
>>>>
>>>> The reason for this is the fact that pdev->dev has its of_node
>>>> field initialized, which makes v4l2_async trying to match
>>>> subdev by parent node of a LED device, not by sub-LED related
>>>> DT node.
>>>
>>> If v4l2_subdev->of_node is set, then it won't be replaced with one from
>>> struct device. I.e. you need to provide of_node pointer only if it's
>>> different from dev->of_node.
>>>
>>
>> It will always be different since dev->of_node pointer is related
>> to the main DT node of LED device, whereas each LED connected to it
>> must be expressed in the form of sub-node, as
>> Documentation/devicetree/bindings/leds/common.txt DT states.
>
> You can still refer to the device's root device_node using a phandle.

Why should I need to refer to the device's root node?

What I meant here was that DT documentation enforces that even if
there is a single LED connected to the device it has to be expressed
as a sub-node anyway. Each LED will have to be matched by the phandle
to the sub-node representing it. This implies that v4l2_subdev->of_node
(related to sub-LED DT node) will be always different from dev->of_node
(related to LED controller DT node).

> Say, if you have a LED flash controller with an indicator. It's intended to
> be used together with the flash LED, and the existing as3645a driver exposes
> it through the same sub-device. I think that'd make sense with LED class
> driver as well (i.e. you'd have two LED class devices but a single
> sub-device). Small changes to the wrapper would be needed.
>

How the sub-device name should look like then? We would have to
concatenate somehow both LED class device names?

-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH 4/5] leds: aat1290: Pass dev and dev->of_node to v4l2_flash_init()

[Sakari Ailus]

Hi Jacek,

On Thu, May 21, 2015 at 10:54:07AM +0200, Jacek Anaszewski wrote:
> Hi Sakari,
> 
> On 05/20/2015 04:31 PM, Sakari Ailus wrote:
> >Hi Jacek,
> >
> >On Wed, May 20, 2015 at 03:47:25PM +0200, Jacek Anaszewski wrote:
> >...
> >>>>>>--- a/drivers/leds/leds-aat1290.c
> >>>>>>+++ b/drivers/leds/leds-aat1290.c
> >>>>>>@@ -524,9 +524,8 @@ static int aat1290_led_probe(struct
> >>>>>>platform_device *pdev)
> >>>>>>      led_cdev->dev->of_node = sub_node;
> >>>>>>
> >>>>>>      /* Create V4L2 Flash subdev. */
> >>>>>>-    led->v4l2_flash = v4l2_flash_init(fled_cdev,
> >>>>>>-                      &v4l2_flash_ops,
> >>>>>>-                      &v4l2_sd_cfg);
> >>>>>>+    led->v4l2_flash = v4l2_flash_init(dev, NULL, fled_cdev,
> >>>>>>+                      &v4l2_flash_ops, &v4l2_sd_cfg);
> >>>>>
> >>>>>Here the first argument should be led_cdev->dev, not dev, which is
> >>>>>&pdev->dev, whereas led_cdev->dev is returned by
> >>>>>device_create_with_groups (it takes dev as a parent) called from
> >>>>>led_classdev_register.
> >>>>
> >>>>The reason for this is the fact that pdev->dev has its of_node
> >>>>field initialized, which makes v4l2_async trying to match
> >>>>subdev by parent node of a LED device, not by sub-LED related
> >>>>DT node.
> >>>
> >>>If v4l2_subdev->of_node is set, then it won't be replaced with one from
> >>>struct device. I.e. you need to provide of_node pointer only if it's
> >>>different from dev->of_node.
> >>>
> >>
> >>It will always be different since dev->of_node pointer is related
> >>to the main DT node of LED device, whereas each LED connected to it
> >>must be expressed in the form of sub-node, as
> >>Documentation/devicetree/bindings/leds/common.txt DT states.
> >
> >You can still refer to the device's root device_node using a phandle.
> 
> Why should I need to refer to the device's root node?
> 
> What I meant here was that DT documentation enforces that even if
> there is a single LED connected to the device it has to be expressed
> as a sub-node anyway. Each LED will have to be matched by the phandle
> to the sub-node representing it. This implies that v4l2_subdev->of_node
> (related to sub-LED DT node) will be always different from dev->of_node
> (related to LED controller DT node).

>From driver point of view this makes no difference; it's just easier to
parse if you don't refer to the LEDs separately. I think this is a bit
special case; nowadays many LED flash controllers drive two LEDs.

> 
> >Say, if you have a LED flash controller with an indicator. It's intended to
> >be used together with the flash LED, and the existing as3645a driver exposes
> >it through the same sub-device. I think that'd make sense with LED class
> >driver as well (i.e. you'd have two LED class devices but a single
> >sub-device). Small changes to the wrapper would be needed.
> >
> 
> How the sub-device name should look like then? We would have to
> concatenate somehow both LED class device names?

It'd be different, i.e. there would be no flash or indicator in the name.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH 4/5] leds: aat1290: Pass dev and dev->of_node to v4l2_flash_init()

[Jacek Anaszewski]

Hi Sakari

On 05/21/2015 12:06 PM, Sakari Ailus wrote:
[...]
>>> On Wed, May 20, 2015 at 03:47:25PM +0200, Jacek Anaszewski wrote:
>>> ...
>>>>>>>> --- a/drivers/leds/leds-aat1290.c
>>>>>>>> +++ b/drivers/leds/leds-aat1290.c
>>>>>>>> @@ -524,9 +524,8 @@ static int aat1290_led_probe(struct
>>>>>>>> platform_device *pdev)
>>>>>>>>       led_cdev->dev->of_node = sub_node;
>>>>>>>>
>>>>>>>>       /* Create V4L2 Flash subdev. */
>>>>>>>> -    led->v4l2_flash = v4l2_flash_init(fled_cdev,
>>>>>>>> -                      &v4l2_flash_ops,
>>>>>>>> -                      &v4l2_sd_cfg);
>>>>>>>> +    led->v4l2_flash = v4l2_flash_init(dev, NULL, fled_cdev,
>>>>>>>> +                      &v4l2_flash_ops, &v4l2_sd_cfg);
>>>>>>>
>>>>>>> Here the first argument should be led_cdev->dev, not dev, which is
>>>>>>> &pdev->dev, whereas led_cdev->dev is returned by
>>>>>>> device_create_with_groups (it takes dev as a parent) called from
>>>>>>> led_classdev_register.
>>>>>>
>>>>>> The reason for this is the fact that pdev->dev has its of_node
>>>>>> field initialized, which makes v4l2_async trying to match
>>>>>> subdev by parent node of a LED device, not by sub-LED related
>>>>>> DT node.
>>>>>
>>>>> If v4l2_subdev->of_node is set, then it won't be replaced with one from
>>>>> struct device. I.e. you need to provide of_node pointer only if it's
>>>>> different from dev->of_node.
>>>>>
>>>>
>>>> It will always be different since dev->of_node pointer is related
>>>> to the main DT node of LED device, whereas each LED connected to it
>>>> must be expressed in the form of sub-node, as
>>>> Documentation/devicetree/bindings/leds/common.txt DT states.
>>>
>>> You can still refer to the device's root device_node using a phandle.
>>
>> Why should I need to refer to the device's root node?
>>
>> What I meant here was that DT documentation enforces that even if
>> there is a single LED connected to the device it has to be expressed
>> as a sub-node anyway. Each LED will have to be matched by the phandle
>> to the sub-node representing it. This implies that v4l2_subdev->of_node
>> (related to sub-LED DT node) will be always different from dev->of_node
>> (related to LED controller DT node).
>
>>From driver point of view this makes no difference; it's just easier to
> parse if you don't refer to the LEDs separately. I think this is a bit
> special case; nowadays many LED flash controllers drive two LEDs.

As I understand, your stance is as follows:
- second argument to v4l2_flash_init needn't always be initialized
   because some LEDs could be referred to by the phandle to the parent
   node (e.g. flash LED and indicator under common sub-device)

If this is true, than how we would handle the situation where
there is a flash LED controller with two separate flash LEDs
and one of them is associated with indicator LED?

>>
>>> Say, if you have a LED flash controller with an indicator. It's intended to
>>> be used together with the flash LED, and the existing as3645a driver exposes
>>> it through the same sub-device. I think that'd make sense with LED class
>>> driver as well (i.e. you'd have two LED class devices but a single
>>> sub-device). Small changes to the wrapper would be needed.
>>>
>>
>> How the sub-device name should look like then? We would have to
>> concatenate somehow both LED class device names?
>
> It'd be different, i.e. there would be no flash or indicator in the name.
>

Currently there is no such a requirement too. As we discussed it few
months ago v4l2-flash sub-device name should be composed:
- for I2C devices "<LED class dev name> <i2c_adapter_id>-<i2c_addr>"
- for GPIO driven devices: <LED class dev name>

-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH 4/5] leds: aat1290: Pass dev and dev->of_node to v4l2_flash_init()

[Sakari Ailus]

On Thu, May 21, 2015 at 02:13:12PM +0200, Jacek Anaszewski wrote:
> Hi Sakari
> 
> On 05/21/2015 12:06 PM, Sakari Ailus wrote:
> [...]
> >>>On Wed, May 20, 2015 at 03:47:25PM +0200, Jacek Anaszewski wrote:
> >>>...
> >>>>>>>>--- a/drivers/leds/leds-aat1290.c
> >>>>>>>>+++ b/drivers/leds/leds-aat1290.c
> >>>>>>>>@@ -524,9 +524,8 @@ static int aat1290_led_probe(struct
> >>>>>>>>platform_device *pdev)
> >>>>>>>>      led_cdev->dev->of_node = sub_node;
> >>>>>>>>
> >>>>>>>>      /* Create V4L2 Flash subdev. */
> >>>>>>>>-    led->v4l2_flash = v4l2_flash_init(fled_cdev,
> >>>>>>>>-                      &v4l2_flash_ops,
> >>>>>>>>-                      &v4l2_sd_cfg);
> >>>>>>>>+    led->v4l2_flash = v4l2_flash_init(dev, NULL, fled_cdev,
> >>>>>>>>+                      &v4l2_flash_ops, &v4l2_sd_cfg);
> >>>>>>>
> >>>>>>>Here the first argument should be led_cdev->dev, not dev, which is
> >>>>>>>&pdev->dev, whereas led_cdev->dev is returned by
> >>>>>>>device_create_with_groups (it takes dev as a parent) called from
> >>>>>>>led_classdev_register.
> >>>>>>
> >>>>>>The reason for this is the fact that pdev->dev has its of_node
> >>>>>>field initialized, which makes v4l2_async trying to match
> >>>>>>subdev by parent node of a LED device, not by sub-LED related
> >>>>>>DT node.
> >>>>>
> >>>>>If v4l2_subdev->of_node is set, then it won't be replaced with one from
> >>>>>struct device. I.e. you need to provide of_node pointer only if it's
> >>>>>different from dev->of_node.
> >>>>>
> >>>>
> >>>>It will always be different since dev->of_node pointer is related
> >>>>to the main DT node of LED device, whereas each LED connected to it
> >>>>must be expressed in the form of sub-node, as
> >>>>Documentation/devicetree/bindings/leds/common.txt DT states.
> >>>
> >>>You can still refer to the device's root device_node using a phandle.
> >>
> >>Why should I need to refer to the device's root node?
> >>
> >>What I meant here was that DT documentation enforces that even if
> >>there is a single LED connected to the device it has to be expressed
> >>as a sub-node anyway. Each LED will have to be matched by the phandle
> >>to the sub-node representing it. This implies that v4l2_subdev->of_node
> >>(related to sub-LED DT node) will be always different from dev->of_node
> >>(related to LED controller DT node).
> >
> >>From driver point of view this makes no difference; it's just easier to
> >parse if you don't refer to the LEDs separately. I think this is a bit
> >special case; nowadays many LED flash controllers drive two LEDs.
> 
> As I understand, your stance is as follows:
> - second argument to v4l2_flash_init needn't always be initialized
>   because some LEDs could be referred to by the phandle to the parent
>   node (e.g. flash LED and indicator under common sub-device)
> 
> If this is true, than how we would handle the situation where
> there is a flash LED controller with two separate flash LEDs
> and one of them is associated with indicator LED?

I referred to cases where there's exactly one flash LED and one indicator
LED. 

The as3645a driver already does expose them through the same sub-device, as
does the adp1653 driver. I wouldn't attempt to change this, and I think it
makes sense for other drivers that have one of each LEDs.

> >>>Say, if you have a LED flash controller with an indicator. It's intended to
> >>>be used together with the flash LED, and the existing as3645a driver exposes
> >>>it through the same sub-device. I think that'd make sense with LED class
> >>>driver as well (i.e. you'd have two LED class devices but a single
> >>>sub-device). Small changes to the wrapper would be needed.
> >>>
> >>
> >>How the sub-device name should look like then? We would have to
> >>concatenate somehow both LED class device names?
> >
> >It'd be different, i.e. there would be no flash or indicator in the name.
> >
> 
> Currently there is no such a requirement too. As we discussed it few
> months ago v4l2-flash sub-device name should be composed:
> - for I2C devices "<LED class dev name> <i2c_adapter_id>-<i2c_addr>"
> - for GPIO driven devices: <LED class dev name>

The indicator and the flash may not have the same name, so it wouldn't be a
bad idea to include that to the name of the LED device. I2C devices the
names are always different anyway due to the bus address. It's up to the
driver to set the name correctly.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

[PATCH 5/5] leds: max77693: Pass dev and dev->of_node to v4l2_flash_init()

[Sakari Ailus]

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
 drivers/leds/leds-max77693.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/leds/leds-max77693.c b/drivers/leds/leds-max77693.c
index fecd0ed..bd12744 100644
--- a/drivers/leds/leds-max77693.c
+++ b/drivers/leds/leds-max77693.c
@@ -961,11 +961,9 @@ static int max77693_register_led(struct max77693_sub_led *sub_led,
 
 	max77693_init_v4l2_flash_config(sub_led, led_cfg, &v4l2_sd_cfg);
 
-	fled_cdev->led_cdev.dev->of_node = sub_node;
-
 	/* Register in the V4L2 subsystem. */
-	sub_led->v4l2_flash = v4l2_flash_init(fled_cdev, &v4l2_flash_ops,
-						&v4l2_sd_cfg);
+	sub_led->v4l2_flash = v4l2_flash_init(dev, sub_node, fled_cdev,
+					      &v4l2_flash_ops, &v4l2_sd_cfg);
 	if (IS_ERR(sub_led->v4l2_flash)) {
 		ret = PTR_ERR(sub_led->v4l2_flash);
 		goto err_v4l2_flash_init;
-- 
1.7.10.4