[PATCH 0/2] Create sub-device per LED

[PATCH 0/2] Create sub-device per LED

[Sakari Ailus]

Hi folks,

The original design decision in the V4L2 flash API allows controlling a two
LEDs (an indicator and a flash) through a single sub-device. This covered
virtually all flash driver chips back then but this no longer holds as
there are many LED driver chips with multiple flash LED outputs. This
necessitates creating as many sub-devices as there are flash LEDs.

Additionally the flash LEDs can be associated to different sensors. This is
not unconceivable although not very probable.

This patchset splits the indicator and flash LEDs exposed by the V4L2 flash
class framework into multiple sub-devices. This way the driver creates the
sub-devices individually for each LED which will also facilitate fwnode
matching (e.g. will you refer to LED or a LED driver chip with a phandle?).

I'll post that set soonish.

These go on top of the other flash patches here:

<URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=for-4.14-2>

Sakari Ailus (2):
  staging: greybus: light: Don't leak memory for no gain
  v4l2-flash-led-class: Create separate sub-devices for indicators

 drivers/leds/leds-aat1290.c                    |   4 +-
 drivers/leds/leds-max77693.c                   |   4 +-
 drivers/media/v4l2-core/v4l2-flash-led-class.c | 112 +++++++++++++++----------
 drivers/staging/greybus/light.c                |  33 +++++---
 include/media/v4l2-flash-led-class.h           |  41 ++++++---
 5 files changed, 119 insertions(+), 75 deletions(-)

-- 
2.11.0

[PATCH 1/2] staging: greybus: light: Don't leak memory for no gain

[Sakari Ailus]

Memory for struct v4l2_flash_config is allocated in
gb_lights_light_v4l2_register() for no gain and yet the allocated memory is
leaked; the struct isn't used outside the function. Fix this.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/staging/greybus/light.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/greybus/light.c b/drivers/staging/greybus/light.c
index 129ceed39829..b25c117ec41a 100644
--- a/drivers/staging/greybus/light.c
+++ b/drivers/staging/greybus/light.c
@@ -534,25 +534,21 @@ static int gb_lights_light_v4l2_register(struct gb_light *light)
 {
 	struct gb_connection *connection = get_conn_from_light(light);
 	struct device *dev = &connection->bundle->dev;
-	struct v4l2_flash_config *sd_cfg;
+	struct v4l2_flash_config sd_cfg = { 0 };
 	struct led_classdev_flash *fled;
 	struct led_classdev *iled = NULL;
 	struct gb_channel *channel_torch, *channel_ind, *channel_flash;
 	int ret = 0;
 
-	sd_cfg = kcalloc(1, sizeof(*sd_cfg), GFP_KERNEL);
-	if (!sd_cfg)
-		return -ENOMEM;
-
 	channel_torch = get_channel_from_mode(light, GB_CHANNEL_MODE_TORCH);
 	if (channel_torch)
 		__gb_lights_channel_v4l2_config(&channel_torch->intensity_uA,
-						&sd_cfg->torch_intensity);
+						&sd_cfg.torch_intensity);
 
 	channel_ind = get_channel_from_mode(light, GB_CHANNEL_MODE_INDICATOR);
 	if (channel_ind) {
 		__gb_lights_channel_v4l2_config(&channel_ind->intensity_uA,
-						&sd_cfg->indicator_intensity);
+						&sd_cfg.indicator_intensity);
 		iled = &channel_ind->fled.led_cdev;
 	}
 
@@ -561,17 +557,17 @@ static int gb_lights_light_v4l2_register(struct gb_light *light)
 
 	fled = &channel_flash->fled;
 
-	snprintf(sd_cfg->dev_name, sizeof(sd_cfg->dev_name), "%s", light->name);
+	snprintf(sd_cfg.dev_name, sizeof(sd_cfg.dev_name), "%s", light->name);
 
 	/* Set the possible values to faults, in our case all faults */
-	sd_cfg->flash_faults = LED_FAULT_OVER_VOLTAGE | LED_FAULT_TIMEOUT |
+	sd_cfg.flash_faults = LED_FAULT_OVER_VOLTAGE | LED_FAULT_TIMEOUT |
 		LED_FAULT_OVER_TEMPERATURE | LED_FAULT_SHORT_CIRCUIT |
 		LED_FAULT_OVER_CURRENT | LED_FAULT_INDICATOR |
 		LED_FAULT_UNDER_VOLTAGE | LED_FAULT_INPUT_VOLTAGE |
 		LED_FAULT_LED_OVER_TEMPERATURE;
 
 	light->v4l2_flash = v4l2_flash_init(dev, NULL, fled, iled,
-					    &v4l2_flash_ops, sd_cfg);
+					    &v4l2_flash_ops, &sd_cfg);
 	if (IS_ERR_OR_NULL(light->v4l2_flash)) {
 		ret = PTR_ERR(light->v4l2_flash);
 		goto out_free;
@@ -580,7 +576,6 @@ static int gb_lights_light_v4l2_register(struct gb_light *light)
 	return ret;
 
 out_free:
-	kfree(sd_cfg);
 	return ret;
 }
 
-- 
2.11.0

Re: [PATCH 1/2] staging: greybus: light: Don't leak memory for no gain

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1828 bytes --]

Hi!

> Memory for struct v4l2_flash_config is allocated in
> gb_lights_light_v4l2_register() for no gain and yet the allocated memory is
> leaked; the struct isn't used outside the function. Fix this.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>


> diff --git a/drivers/staging/greybus/light.c b/drivers/staging/greybus/light.c
> index 129ceed39829..b25c117ec41a 100644
> --- a/drivers/staging/greybus/light.c
> +++ b/drivers/staging/greybus/light.c
> @@ -534,25 +534,21 @@ static int gb_lights_light_v4l2_register(struct gb_light *light)
>  {
>  	struct gb_connection *connection = get_conn_from_light(light);
>  	struct device *dev = &connection->bundle->dev;
> -	struct v4l2_flash_config *sd_cfg;
> +	struct v4l2_flash_config sd_cfg = { 0 };
>  	struct led_classdev_flash *fled;
>  	struct led_classdev *iled = NULL;
>  	struct gb_channel *channel_torch, *channel_ind, *channel_flash;
>  	int ret = 0;
....
>  	light->v4l2_flash = v4l2_flash_init(dev, NULL, fled, iled,
> -					    &v4l2_flash_ops, sd_cfg);
> +					    &v4l2_flash_ops, &sd_cfg);
>  	if (IS_ERR_OR_NULL(light->v4l2_flash)) {
>  		ret = PTR_ERR(light->v4l2_flash);
>  		goto out_free;

Acked-by: Pavel Machek <pavel@ucw.cz>

struct v4l2_flash *v4l2_flash_init(
        struct device *dev, struct fwnode_handle *fwn,
	       struct led_classdev_flash *fled_cdev,
	       	      struct led_classdev_flash *iled_cdev,
		      	     const struct v4l2_flash_ops *ops,
			     	   struct v4l2_flash_config *config)
				   
This function saves "ops" argument, but is careful to copy from
"config" argument. Perhaps that's worth a comment?

Thanks,
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[PATCH 1/1] v4l2-flash-led-class: Document v4l2_flash_init() references

[Sakari Ailus]

The v4l2_flash_init() keeps a reference to the ops struct but not to the
config struct (nor anything it contains). Document this.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 include/media/v4l2-flash-led-class.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/media/v4l2-flash-led-class.h b/include/media/v4l2-flash-led-class.h
index c3f39992f3fa..6f4825b6a352 100644
--- a/include/media/v4l2-flash-led-class.h
+++ b/include/media/v4l2-flash-led-class.h
@@ -112,6 +112,9 @@ static inline struct v4l2_flash *v4l2_ctrl_to_v4l2_flash(struct v4l2_ctrl *c)
  * @config:	initialization data for V4L2 Flash sub-device
  *
  * Create V4L2 Flash sub-device wrapping given LED subsystem device.
+ * The ops pointer is stored by the V4L2 flash framework. No
+ * references are held to config nor its contents once this function
+ * has returned.
  *
  * Returns: A valid pointer, or, when an error occurs, the return
  * value is encoded using ERR_PTR(). Use IS_ERR() to check and
@@ -130,6 +133,9 @@ struct v4l2_flash *v4l2_flash_init(
  * @config:	initialization data for V4L2 Flash sub-device
  *
  * Create V4L2 Flash sub-device wrapping given LED subsystem device.
+ * The ops pointer is stored by the V4L2 flash framework. No
+ * references are held to config nor its contents once this function
+ * has returned.
  *
  * Returns: A valid pointer, or, when an error occurs, the return
  * value is encoded using ERR_PTR(). Use IS_ERR() to check and
-- 
2.11.0

Re: [PATCH 1/1] v4l2-flash-led-class: Document v4l2_flash_init() references

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 465 bytes --]

On Thu 2017-07-20 01:40:31, Sakari Ailus wrote:
> The v4l2_flash_init() keeps a reference to the ops struct but not to the
> config struct (nor anything it contains). Document this.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Acked-by: Pavel Machek <pavel@ucw.cz>

Thanks!
							Pavel
							
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 1/2] staging: greybus: light: Don't leak memory for no gain

[Johan Hovold]

[ +CC: Rui and Greg ]

On Tue, Jul 18, 2017 at 09:41:06PM +0300, Sakari Ailus wrote:
> Memory for struct v4l2_flash_config is allocated in
> gb_lights_light_v4l2_register() for no gain and yet the allocated memory is
> leaked; the struct isn't used outside the function. Fix this.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/staging/greybus/light.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/staging/greybus/light.c b/drivers/staging/greybus/light.c
> index 129ceed39829..b25c117ec41a 100644
> --- a/drivers/staging/greybus/light.c
> +++ b/drivers/staging/greybus/light.c
> @@ -534,25 +534,21 @@ static int gb_lights_light_v4l2_register(struct gb_light *light)
>  {
>  	struct gb_connection *connection = get_conn_from_light(light);
>  	struct device *dev = &connection->bundle->dev;
> -	struct v4l2_flash_config *sd_cfg;
> +	struct v4l2_flash_config sd_cfg = { 0 };
>  	struct led_classdev_flash *fled;
>  	struct led_classdev *iled = NULL;
>  	struct gb_channel *channel_torch, *channel_ind, *channel_flash;
>  	int ret = 0;
>  
> -	sd_cfg = kcalloc(1, sizeof(*sd_cfg), GFP_KERNEL);
> -	if (!sd_cfg)
> -		return -ENOMEM;
> -
>  	channel_torch = get_channel_from_mode(light, GB_CHANNEL_MODE_TORCH);
>  	if (channel_torch)
>  		__gb_lights_channel_v4l2_config(&channel_torch->intensity_uA,
> -						&sd_cfg->torch_intensity);
> +						&sd_cfg.torch_intensity);
>  
>  	channel_ind = get_channel_from_mode(light, GB_CHANNEL_MODE_INDICATOR);
>  	if (channel_ind) {
>  		__gb_lights_channel_v4l2_config(&channel_ind->intensity_uA,
> -						&sd_cfg->indicator_intensity);
> +						&sd_cfg.indicator_intensity);
>  		iled = &channel_ind->fled.led_cdev;
>  	}
>  
> @@ -561,17 +557,17 @@ static int gb_lights_light_v4l2_register(struct gb_light *light)
>  
>  	fled = &channel_flash->fled;
>  
> -	snprintf(sd_cfg->dev_name, sizeof(sd_cfg->dev_name), "%s", light->name);
> +	snprintf(sd_cfg.dev_name, sizeof(sd_cfg.dev_name), "%s", light->name);
>  
>  	/* Set the possible values to faults, in our case all faults */
> -	sd_cfg->flash_faults = LED_FAULT_OVER_VOLTAGE | LED_FAULT_TIMEOUT |
> +	sd_cfg.flash_faults = LED_FAULT_OVER_VOLTAGE | LED_FAULT_TIMEOUT |
>  		LED_FAULT_OVER_TEMPERATURE | LED_FAULT_SHORT_CIRCUIT |
>  		LED_FAULT_OVER_CURRENT | LED_FAULT_INDICATOR |
>  		LED_FAULT_UNDER_VOLTAGE | LED_FAULT_INPUT_VOLTAGE |
>  		LED_FAULT_LED_OVER_TEMPERATURE;
>  
>  	light->v4l2_flash = v4l2_flash_init(dev, NULL, fled, iled,
> -					    &v4l2_flash_ops, sd_cfg);
> +					    &v4l2_flash_ops, &sd_cfg);
>  	if (IS_ERR_OR_NULL(light->v4l2_flash)) {
>  		ret = PTR_ERR(light->v4l2_flash);
>  		goto out_free;
> @@ -580,7 +576,6 @@ static int gb_lights_light_v4l2_register(struct gb_light *light)
>  	return ret;
>  
>  out_free:
> -	kfree(sd_cfg);

This looks a bit lazy, even if I just noticed that you repurpose this
error label (without renaming it) in you second patch.


>  	return ret;
>  }

And while it's fine to take this through linux-media, it would still be
good to keep the maintainers on CC.

Thanks,
Johan

Re: [PATCH 1/2] staging: greybus: light: Don't leak memory for no gain

[Rui Miguel Silva]

Hi,
On Tue, Jul 25, 2017 at 02:30:31PM +0200, Johan Hovold wrote:
> [ +CC: Rui and Greg ]

Thanks Johan. I only got this because of you.

> 
> On Tue, Jul 18, 2017 at 09:41:06PM +0300, Sakari Ailus wrote:
> > Memory for struct v4l2_flash_config is allocated in
> > gb_lights_light_v4l2_register() for no gain and yet the allocated memory is
> > leaked; the struct isn't used outside the function. Fix this.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  drivers/staging/greybus/light.c | 17 ++++++-----------
> >  1 file changed, 6 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/staging/greybus/light.c b/drivers/staging/greybus/light.c
> > index 129ceed39829..b25c117ec41a 100644
> > --- a/drivers/staging/greybus/light.c
> > +++ b/drivers/staging/greybus/light.c
> > @@ -534,25 +534,21 @@ static int gb_lights_light_v4l2_register(struct gb_light *light)
> >  {
> >  	struct gb_connection *connection = get_conn_from_light(light);
> >  	struct device *dev = &connection->bundle->dev;
> > -	struct v4l2_flash_config *sd_cfg;
> > +	struct v4l2_flash_config sd_cfg = { 0 };
> >  	struct led_classdev_flash *fled;
> >  	struct led_classdev *iled = NULL;
> >  	struct gb_channel *channel_torch, *channel_ind, *channel_flash;
> >  	int ret = 0;
> >  
> > -	sd_cfg = kcalloc(1, sizeof(*sd_cfg), GFP_KERNEL);
> > -	if (!sd_cfg)
> > -		return -ENOMEM;
> > -
> >  	channel_torch = get_channel_from_mode(light, GB_CHANNEL_MODE_TORCH);
> >  	if (channel_torch)
> >  		__gb_lights_channel_v4l2_config(&channel_torch->intensity_uA,
> > -						&sd_cfg->torch_intensity);
> > +						&sd_cfg.torch_intensity);
> >  
> >  	channel_ind = get_channel_from_mode(light, GB_CHANNEL_MODE_INDICATOR);
> >  	if (channel_ind) {
> >  		__gb_lights_channel_v4l2_config(&channel_ind->intensity_uA,
> > -						&sd_cfg->indicator_intensity);
> > +						&sd_cfg.indicator_intensity);
> >  		iled = &channel_ind->fled.led_cdev;
> >  	}
> >  
> > @@ -561,17 +557,17 @@ static int gb_lights_light_v4l2_register(struct gb_light *light)
> >  
> >  	fled = &channel_flash->fled;
> >  
> > -	snprintf(sd_cfg->dev_name, sizeof(sd_cfg->dev_name), "%s", light->name);
> > +	snprintf(sd_cfg.dev_name, sizeof(sd_cfg.dev_name), "%s", light->name);
> >  
> >  	/* Set the possible values to faults, in our case all faults */
> > -	sd_cfg->flash_faults = LED_FAULT_OVER_VOLTAGE | LED_FAULT_TIMEOUT |
> > +	sd_cfg.flash_faults = LED_FAULT_OVER_VOLTAGE | LED_FAULT_TIMEOUT |
> >  		LED_FAULT_OVER_TEMPERATURE | LED_FAULT_SHORT_CIRCUIT |
> >  		LED_FAULT_OVER_CURRENT | LED_FAULT_INDICATOR |
> >  		LED_FAULT_UNDER_VOLTAGE | LED_FAULT_INPUT_VOLTAGE |
> >  		LED_FAULT_LED_OVER_TEMPERATURE;
> >  
> >  	light->v4l2_flash = v4l2_flash_init(dev, NULL, fled, iled,
> > -					    &v4l2_flash_ops, sd_cfg);
> > +					    &v4l2_flash_ops, &sd_cfg);
> >  	if (IS_ERR_OR_NULL(light->v4l2_flash)) {
> >  		ret = PTR_ERR(light->v4l2_flash);
> >  		goto out_free;
> > @@ -580,7 +576,6 @@ static int gb_lights_light_v4l2_register(struct gb_light *light)
> >  	return ret;
> >  
> >  out_free:
> > -	kfree(sd_cfg);
> 
> This looks a bit lazy, even if I just noticed that you repurpose this
> error label (without renaming it) in you second patch.
> 
> 
> >  	return ret;
> >  }
> 
> And while it's fine to take this through linux-media, it would still be
> good to keep the maintainers on CC.

Sakari, if you could resend the all series to the right lists and
maintainers for proper review that would be great.

I did not get 0/2 and 2/2 patches.

---
Cheers,
	Rui

Re: [PATCH 1/2] staging: greybus: light: Don't leak memory for no gain

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 4303 bytes --]

Hi!

> On Tue, Jul 25, 2017 at 02:30:31PM +0200, Johan Hovold wrote:
> > [ +CC: Rui and Greg ]
> 
> Thanks Johan. I only got this because of you.

> > >  	return ret;
> > >  }
> > 
> > And while it's fine to take this through linux-media, it would still be
> > good to keep the maintainers on CC.
> 
> Sakari, if you could resend the all series to the right lists and
> maintainers for proper review that would be great.
> 
> I did not get 0/2 and 2/2 patches.

0/2 and 2/2 were unrelated to the memory leak, IIRC. Let me google it
for you...

https://www.mail-archive.com/linux-media@vger.kernel.org/msg115840.html

This is memory leak and the driver is in staging. Acked-by or fixing
it yourself would be appropriate response, asking for resending of the
series... not quite so.

Best regards,

									Pavel

> > On Tue, Jul 18, 2017 at 09:41:06PM +0300, Sakari Ailus wrote:
> > > Memory for struct v4l2_flash_config is allocated in
> > > gb_lights_light_v4l2_register() for no gain and yet the allocated memory is
> > > leaked; the struct isn't used outside the function. Fix this.
> > > 
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > >  drivers/staging/greybus/light.c | 17 ++++++-----------
> > >  1 file changed, 6 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/drivers/staging/greybus/light.c b/drivers/staging/greybus/light.c
> > > index 129ceed39829..b25c117ec41a 100644
> > > --- a/drivers/staging/greybus/light.c
> > > +++ b/drivers/staging/greybus/light.c
> > > @@ -534,25 +534,21 @@ static int gb_lights_light_v4l2_register(struct gb_light *light)
> > >  {
> > >  	struct gb_connection *connection = get_conn_from_light(light);
> > >  	struct device *dev = &connection->bundle->dev;
> > > -	struct v4l2_flash_config *sd_cfg;
> > > +	struct v4l2_flash_config sd_cfg = { 0 };
> > >  	struct led_classdev_flash *fled;
> > >  	struct led_classdev *iled = NULL;
> > >  	struct gb_channel *channel_torch, *channel_ind, *channel_flash;
> > >  	int ret = 0;
> > >  
> > > -	sd_cfg = kcalloc(1, sizeof(*sd_cfg), GFP_KERNEL);
> > > -	if (!sd_cfg)
> > > -		return -ENOMEM;
> > > -
> > >  	channel_torch = get_channel_from_mode(light, GB_CHANNEL_MODE_TORCH);
> > >  	if (channel_torch)
> > >  		__gb_lights_channel_v4l2_config(&channel_torch->intensity_uA,
> > > -						&sd_cfg->torch_intensity);
> > > +						&sd_cfg.torch_intensity);
> > >  
> > >  	channel_ind = get_channel_from_mode(light, GB_CHANNEL_MODE_INDICATOR);
> > >  	if (channel_ind) {
> > >  		__gb_lights_channel_v4l2_config(&channel_ind->intensity_uA,
> > > -						&sd_cfg->indicator_intensity);
> > > +						&sd_cfg.indicator_intensity);
> > >  		iled = &channel_ind->fled.led_cdev;
> > >  	}
> > >  
> > > @@ -561,17 +557,17 @@ static int gb_lights_light_v4l2_register(struct gb_light *light)
> > >  
> > >  	fled = &channel_flash->fled;
> > >  
> > > -	snprintf(sd_cfg->dev_name, sizeof(sd_cfg->dev_name), "%s", light->name);
> > > +	snprintf(sd_cfg.dev_name, sizeof(sd_cfg.dev_name), "%s", light->name);
> > >  
> > >  	/* Set the possible values to faults, in our case all faults */
> > > -	sd_cfg->flash_faults = LED_FAULT_OVER_VOLTAGE | LED_FAULT_TIMEOUT |
> > > +	sd_cfg.flash_faults = LED_FAULT_OVER_VOLTAGE | LED_FAULT_TIMEOUT |
> > >  		LED_FAULT_OVER_TEMPERATURE | LED_FAULT_SHORT_CIRCUIT |
> > >  		LED_FAULT_OVER_CURRENT | LED_FAULT_INDICATOR |
> > >  		LED_FAULT_UNDER_VOLTAGE | LED_FAULT_INPUT_VOLTAGE |
> > >  		LED_FAULT_LED_OVER_TEMPERATURE;
> > >  
> > >  	light->v4l2_flash = v4l2_flash_init(dev, NULL, fled, iled,
> > > -					    &v4l2_flash_ops, sd_cfg);
> > > +					    &v4l2_flash_ops, &sd_cfg);
> > >  	if (IS_ERR_OR_NULL(light->v4l2_flash)) {
> > >  		ret = PTR_ERR(light->v4l2_flash);
> > >  		goto out_free;
> > > @@ -580,7 +576,6 @@ static int gb_lights_light_v4l2_register(struct gb_light *light)
> > >  	return ret;
> > >  
> > >  out_free:
> > > -	kfree(sd_cfg);
> > 
> > This looks a bit lazy, even if I just noticed that you repurpose this
> > error label (without renaming it) in you second patch.
> > 
> > 

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 1/2] staging: greybus: light: Don't leak memory for no gain

[Rui Miguel Silva]

Hi,
On Wed, Jul 26, 2017 at 08:32:08PM +0200, Pavel Machek wrote:
> Hi!
> 
> > On Tue, Jul 25, 2017 at 02:30:31PM +0200, Johan Hovold wrote:
> > > [ +CC: Rui and Greg ]
> > 
> > Thanks Johan. I only got this because of you.
> 
> > > >  	return ret;
> > > >  }
> > > 
> > > And while it's fine to take this through linux-media, it would still be
> > > good to keep the maintainers on CC.
> > 
> > Sakari, if you could resend the all series to the right lists and
> > maintainers for proper review that would be great.
> > 
> > I did not get 0/2 and 2/2 patches.
> 
> 0/2 and 2/2 were unrelated to the memory leak, IIRC. Let me google it
> for you...
> 
> https://www.mail-archive.com/linux-media@vger.kernel.org/msg115840.html
> 
> This is memory leak and the driver is in staging. Acked-by

Yeah, I will not Ack a patch that never hit my inbox, sorry.

>or fixing > it yourself would be appropriate response, asking
> for resending of the series... not quite so.

Sure, if Sakari do not send a new version already taking in
account Johan comments, which I agree. I will fix it myself when
possible.

---
Cheers,
	Rui

[PATCH 2/2] v4l2-flash-led-class: Create separate sub-devices for indicators

[Sakari Ailus]

The V4L2 flash interface allows controlling multiple LEDs through a single
sub-devices if, and only if, these LEDs are of different types. This
approach scales badly for flash controllers that drive multiple flash LEDs
or for LED specific associations. Essentially, the original assumption of a
LED driver chip that drives a single flash LED and an indicator LED is no
longer valid.

Address the matter by registering one sub-device per LED.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/leds/leds-aat1290.c                    |   4 +-
 drivers/leds/leds-max77693.c                   |   4 +-
 drivers/media/v4l2-core/v4l2-flash-led-class.c | 112 +++++++++++++++----------
 drivers/staging/greybus/light.c                |  24 ++++--
 include/media/v4l2-flash-led-class.h           |  41 ++++++---
 5 files changed, 117 insertions(+), 68 deletions(-)

diff --git a/drivers/leds/leds-aat1290.c b/drivers/leds/leds-aat1290.c
index a21e19297745..424898e6c69d 100644
--- a/drivers/leds/leds-aat1290.c
+++ b/drivers/leds/leds-aat1290.c
@@ -432,7 +432,7 @@ static void aat1290_init_v4l2_flash_config(struct aat1290_led *led,
 	strlcpy(v4l2_sd_cfg->dev_name, led_cdev->name,
 		sizeof(v4l2_sd_cfg->dev_name));
 
-	s = &v4l2_sd_cfg->torch_intensity;
+	s = &v4l2_sd_cfg->intensity;
 	s->min = led->mm_current_scale[0];
 	s->max = led_cfg->max_mm_current;
 	s->step = 1;
@@ -504,7 +504,7 @@ static int aat1290_led_probe(struct platform_device *pdev)
 
 	/* Create V4L2 Flash subdev. */
 	led->v4l2_flash = v4l2_flash_init(dev, of_fwnode_handle(sub_node),
-					  fled_cdev, NULL, &v4l2_flash_ops,
+					  fled_cdev, &v4l2_flash_ops,
 					  &v4l2_sd_cfg);
 	if (IS_ERR(led->v4l2_flash)) {
 		ret = PTR_ERR(led->v4l2_flash);
diff --git a/drivers/leds/leds-max77693.c b/drivers/leds/leds-max77693.c
index 2d3062d53325..adf0f191f794 100644
--- a/drivers/leds/leds-max77693.c
+++ b/drivers/leds/leds-max77693.c
@@ -856,7 +856,7 @@ static void max77693_init_v4l2_flash_config(struct max77693_sub_led *sub_led,
 		 "%s %d-%04x", sub_led->fled_cdev.led_cdev.name,
 		 i2c_adapter_id(i2c->adapter), i2c->addr);
 
-	s = &v4l2_sd_cfg->torch_intensity;
+	s = &v4l2_sd_cfg->intensity;
 	s->min = TORCH_IOUT_MIN;
 	s->max = sub_led->fled_cdev.led_cdev.max_brightness * TORCH_IOUT_STEP;
 	s->step = TORCH_IOUT_STEP;
@@ -931,7 +931,7 @@ static int max77693_register_led(struct max77693_sub_led *sub_led,
 
 	/* Register in the V4L2 subsystem. */
 	sub_led->v4l2_flash = v4l2_flash_init(dev, of_fwnode_handle(sub_node),
-					      fled_cdev, NULL, &v4l2_flash_ops,
+					      fled_cdev, &v4l2_flash_ops,
 					      &v4l2_sd_cfg);
 	if (IS_ERR(sub_led->v4l2_flash)) {
 		ret = PTR_ERR(sub_led->v4l2_flash);
diff --git a/drivers/media/v4l2-core/v4l2-flash-led-class.c b/drivers/media/v4l2-core/v4l2-flash-led-class.c
index aabc85dbb8b5..b017bdb4aaab 100644
--- a/drivers/media/v4l2-core/v4l2-flash-led-class.c
+++ b/drivers/media/v4l2-core/v4l2-flash-led-class.c
@@ -197,7 +197,7 @@ static int v4l2_flash_s_ctrl(struct v4l2_ctrl *c)
 {
 	struct v4l2_flash *v4l2_flash = v4l2_ctrl_to_v4l2_flash(c);
 	struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
-	struct led_classdev *led_cdev = &fled_cdev->led_cdev;
+	struct led_classdev *led_cdev = fled_cdev ? &fled_cdev->led_cdev : NULL;
 	struct v4l2_ctrl **ctrls = v4l2_flash->ctrls;
 	bool external_strobe;
 	int ret = 0;
@@ -299,10 +299,26 @@ static void __fill_ctrl_init_data(struct v4l2_flash *v4l2_flash,
 			  struct v4l2_flash_ctrl_data *ctrl_init_data)
 {
 	struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
-	struct led_classdev *led_cdev = &fled_cdev->led_cdev;
+	struct led_classdev *led_cdev = fled_cdev ? &fled_cdev->led_cdev : NULL;
 	struct v4l2_ctrl_config *ctrl_cfg;
 	u32 mask;
 
+	/* Init INDICATOR_INTENSITY ctrl data */
+	if (v4l2_flash->iled_cdev) {
+		ctrl_init_data[INDICATOR_INTENSITY].cid =
+					V4L2_CID_FLASH_INDICATOR_INTENSITY;
+		ctrl_cfg = &ctrl_init_data[INDICATOR_INTENSITY].config;
+		__lfs_to_v4l2_ctrl_config(&flash_cfg->intensity,
+					  ctrl_cfg);
+		ctrl_cfg->id = V4L2_CID_FLASH_INDICATOR_INTENSITY;
+		ctrl_cfg->min = 0;
+		ctrl_cfg->flags = V4L2_CTRL_FLAG_VOLATILE |
+				  V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
+	}
+
+	if (!led_cdev || WARN_ON(!(led_cdev->flags & LED_DEV_CAP_FLASH)))
+		return;
+
 	/* Init FLASH_FAULT ctrl data */
 	if (flash_cfg->flash_faults) {
 		ctrl_init_data[FLASH_FAULT].cid = V4L2_CID_FLASH_FAULT;
@@ -330,27 +346,11 @@ static void __fill_ctrl_init_data(struct v4l2_flash *v4l2_flash,
 	/* Init TORCH_INTENSITY ctrl data */
 	ctrl_init_data[TORCH_INTENSITY].cid = V4L2_CID_FLASH_TORCH_INTENSITY;
 	ctrl_cfg = &ctrl_init_data[TORCH_INTENSITY].config;
-	__lfs_to_v4l2_ctrl_config(&flash_cfg->torch_intensity, ctrl_cfg);
+	__lfs_to_v4l2_ctrl_config(&flash_cfg->intensity, ctrl_cfg);
 	ctrl_cfg->id = V4L2_CID_FLASH_TORCH_INTENSITY;
 	ctrl_cfg->flags = V4L2_CTRL_FLAG_VOLATILE |
 			  V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
 
-	/* Init INDICATOR_INTENSITY ctrl data */
-	if (v4l2_flash->iled_cdev) {
-		ctrl_init_data[INDICATOR_INTENSITY].cid =
-					V4L2_CID_FLASH_INDICATOR_INTENSITY;
-		ctrl_cfg = &ctrl_init_data[INDICATOR_INTENSITY].config;
-		__lfs_to_v4l2_ctrl_config(&flash_cfg->indicator_intensity,
-					  ctrl_cfg);
-		ctrl_cfg->id = V4L2_CID_FLASH_INDICATOR_INTENSITY;
-		ctrl_cfg->min = 0;
-		ctrl_cfg->flags = V4L2_CTRL_FLAG_VOLATILE |
-				  V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
-	}
-
-	if (!(led_cdev->flags & LED_DEV_CAP_FLASH))
-		return;
-
 	/* Init FLASH_STROBE ctrl data */
 	ctrl_init_data[FLASH_STROBE].cid = V4L2_CID_FLASH_STROBE;
 	ctrl_cfg = &ctrl_init_data[FLASH_STROBE].config;
@@ -485,7 +485,8 @@ static int __sync_device_with_v4l2_controls(struct v4l2_flash *v4l2_flash)
 	struct v4l2_ctrl **ctrls = v4l2_flash->ctrls;
 	int ret = 0;
 
-	v4l2_flash_set_led_brightness(v4l2_flash, ctrls[TORCH_INTENSITY]);
+	if (ctrls[TORCH_INTENSITY])
+		v4l2_flash_set_led_brightness(v4l2_flash, ctrls[TORCH_INTENSITY]);
 
 	if (ctrls[INDICATOR_INTENSITY])
 		v4l2_flash_set_led_brightness(v4l2_flash,
@@ -527,19 +528,21 @@ static int v4l2_flash_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
 {
 	struct v4l2_flash *v4l2_flash = v4l2_subdev_to_v4l2_flash(sd);
 	struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
-	struct led_classdev *led_cdev = &fled_cdev->led_cdev;
+	struct led_classdev *led_cdev = fled_cdev ? &fled_cdev->led_cdev : NULL;
 	struct led_classdev *led_cdev_ind = v4l2_flash->iled_cdev;
 	int ret = 0;
 
 	if (!v4l2_fh_is_singular(&fh->vfh))
 		return 0;
 
-	mutex_lock(&led_cdev->led_access);
+	if (led_cdev) {
+		mutex_lock(&led_cdev->led_access);
 
-	led_sysfs_disable(led_cdev);
-	led_trigger_remove(led_cdev);
+		led_sysfs_disable(led_cdev);
+		led_trigger_remove(led_cdev);
 
-	mutex_unlock(&led_cdev->led_access);
+		mutex_unlock(&led_cdev->led_access);
+	}
 
 	if (led_cdev_ind) {
 		mutex_lock(&led_cdev_ind->led_access);
@@ -556,9 +559,11 @@ static int v4l2_flash_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
 
 	return 0;
 out_sync_device:
-	mutex_lock(&led_cdev->led_access);
-	led_sysfs_enable(led_cdev);
-	mutex_unlock(&led_cdev->led_access);
+	if (led_cdev) {
+		mutex_lock(&led_cdev->led_access);
+		led_sysfs_enable(led_cdev);
+		mutex_unlock(&led_cdev->led_access);
+	}
 
 	if (led_cdev_ind) {
 		mutex_lock(&led_cdev_ind->led_access);
@@ -573,21 +578,24 @@ static int v4l2_flash_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
 {
 	struct v4l2_flash *v4l2_flash = v4l2_subdev_to_v4l2_flash(sd);
 	struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
-	struct led_classdev *led_cdev = &fled_cdev->led_cdev;
+	struct led_classdev *led_cdev = fled_cdev ? &fled_cdev->led_cdev : NULL;
 	struct led_classdev *led_cdev_ind = v4l2_flash->iled_cdev;
 	int ret = 0;
 
 	if (!v4l2_fh_is_singular(&fh->vfh))
 		return 0;
 
-	mutex_lock(&led_cdev->led_access);
+	if (led_cdev) {
+		mutex_lock(&led_cdev->led_access);
 
-	if (v4l2_flash->ctrls[STROBE_SOURCE])
-		ret = v4l2_ctrl_s_ctrl(v4l2_flash->ctrls[STROBE_SOURCE],
+		if (v4l2_flash->ctrls[STROBE_SOURCE])
+			ret = v4l2_ctrl_s_ctrl(
+				v4l2_flash->ctrls[STROBE_SOURCE],
 				V4L2_FLASH_STROBE_SOURCE_SOFTWARE);
-	led_sysfs_enable(led_cdev);
+		led_sysfs_enable(led_cdev);
 
-	mutex_unlock(&led_cdev->led_access);
+		mutex_unlock(&led_cdev->led_access);
+	}
 
 	if (led_cdev_ind) {
 		mutex_lock(&led_cdev_ind->led_access);
@@ -605,25 +613,19 @@ static const struct v4l2_subdev_internal_ops v4l2_flash_subdev_internal_ops = {
 
 static const struct v4l2_subdev_ops v4l2_flash_subdev_ops;
 
-struct v4l2_flash *v4l2_flash_init(
+static struct v4l2_flash *__v4l2_flash_init(
 	struct device *dev, struct fwnode_handle *fwn,
-	struct led_classdev_flash *fled_cdev,
-	struct led_classdev *iled_cdev,
-	const struct v4l2_flash_ops *ops,
-	struct v4l2_flash_config *config)
+	struct led_classdev_flash *fled_cdev, struct led_classdev *iled_cdev,
+	const struct v4l2_flash_ops *ops, struct v4l2_flash_config *config)
 {
 	struct v4l2_flash *v4l2_flash;
-	struct led_classdev *led_cdev;
 	struct v4l2_subdev *sd;
 	int ret;
 
-	if (!fled_cdev || !config)
+	if (!config)
 		return ERR_PTR(-EINVAL);
 
-	led_cdev = &fled_cdev->led_cdev;
-
-	v4l2_flash = devm_kzalloc(led_cdev->dev, sizeof(*v4l2_flash),
-					GFP_KERNEL);
+	v4l2_flash = devm_kzalloc(dev, sizeof(*v4l2_flash), GFP_KERNEL);
 	if (!v4l2_flash)
 		return ERR_PTR(-ENOMEM);
 
@@ -632,7 +634,7 @@ struct v4l2_flash *v4l2_flash_init(
 	v4l2_flash->iled_cdev = iled_cdev;
 	v4l2_flash->ops = ops;
 	sd->dev = dev;
-	sd->fwnode = fwn ? fwn : dev_fwnode(led_cdev->dev);
+	sd->fwnode = fwn ? fwn : dev_fwnode(dev);
 	v4l2_subdev_init(sd, &v4l2_flash_subdev_ops);
 	sd->internal_ops = &v4l2_flash_subdev_internal_ops;
 	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
@@ -664,8 +666,26 @@ struct v4l2_flash *v4l2_flash_init(
 
 	return ERR_PTR(ret);
 }
+
+struct v4l2_flash *v4l2_flash_init(
+	struct device *dev, struct fwnode_handle *fwn,
+	struct led_classdev_flash *fled_cdev,
+	const struct v4l2_flash_ops *ops,
+	struct v4l2_flash_config *config)
+{
+	return __v4l2_flash_init(dev, fwn, fled_cdev, NULL, ops, config);
+}
 EXPORT_SYMBOL_GPL(v4l2_flash_init);
 
+struct v4l2_flash *v4l2_flash_indicator_init(
+	struct device *dev, struct fwnode_handle *fwn,
+	struct led_classdev *iled_cdev,
+	struct v4l2_flash_config *config)
+{
+	return __v4l2_flash_init(dev, fwn, NULL, iled_cdev, NULL, config);
+}
+EXPORT_SYMBOL_GPL(v4l2_flash_indicator_init);
+
 void v4l2_flash_release(struct v4l2_flash *v4l2_flash)
 {
 	struct v4l2_subdev *sd;
diff --git a/drivers/staging/greybus/light.c b/drivers/staging/greybus/light.c
index b25c117ec41a..17de450ee7e5 100644
--- a/drivers/staging/greybus/light.c
+++ b/drivers/staging/greybus/light.c
@@ -58,6 +58,7 @@ struct gb_light {
 	bool			ready;
 #if IS_REACHABLE(CONFIG_V4L2_FLASH_LED_CLASS)
 	struct v4l2_flash	*v4l2_flash;
+	struct v4l2_flash	*v4l2_flash_ind;
 #endif
 };
 
@@ -534,7 +535,7 @@ static int gb_lights_light_v4l2_register(struct gb_light *light)
 {
 	struct gb_connection *connection = get_conn_from_light(light);
 	struct device *dev = &connection->bundle->dev;
-	struct v4l2_flash_config sd_cfg = { 0 };
+	struct v4l2_flash_config sd_cfg = { 0 }, sd_cfg_ind = { 0 };
 	struct led_classdev_flash *fled;
 	struct led_classdev *iled = NULL;
 	struct gb_channel *channel_torch, *channel_ind, *channel_flash;
@@ -543,12 +544,12 @@ static int gb_lights_light_v4l2_register(struct gb_light *light)
 	channel_torch = get_channel_from_mode(light, GB_CHANNEL_MODE_TORCH);
 	if (channel_torch)
 		__gb_lights_channel_v4l2_config(&channel_torch->intensity_uA,
-						&sd_cfg.torch_intensity);
+						&sd_cfg.intensity);
 
 	channel_ind = get_channel_from_mode(light, GB_CHANNEL_MODE_INDICATOR);
 	if (channel_ind) {
 		__gb_lights_channel_v4l2_config(&channel_ind->intensity_uA,
-						&sd_cfg.indicator_intensity);
+						&sd_cfg_ind.intensity);
 		iled = &channel_ind->fled.led_cdev;
 	}
 
@@ -558,6 +559,8 @@ static int gb_lights_light_v4l2_register(struct gb_light *light)
 	fled = &channel_flash->fled;
 
 	snprintf(sd_cfg.dev_name, sizeof(sd_cfg.dev_name), "%s", light->name);
+	snprintf(sd_cfg_ind.dev_name, sizeof(sd_cfg_ind.dev_name),
+		 "%s indicator", light->name);
 
 	/* Set the possible values to faults, in our case all faults */
 	sd_cfg.flash_faults = LED_FAULT_OVER_VOLTAGE | LED_FAULT_TIMEOUT |
@@ -566,21 +569,32 @@ static int gb_lights_light_v4l2_register(struct gb_light *light)
 		LED_FAULT_UNDER_VOLTAGE | LED_FAULT_INPUT_VOLTAGE |
 		LED_FAULT_LED_OVER_TEMPERATURE;
 
-	light->v4l2_flash = v4l2_flash_init(dev, NULL, fled, iled,
-					    &v4l2_flash_ops, &sd_cfg);
+	light->v4l2_flash = v4l2_flash_init(dev, NULL, fled, &v4l2_flash_ops,
+					    &sd_cfg);
 	if (IS_ERR_OR_NULL(light->v4l2_flash)) {
 		ret = PTR_ERR(light->v4l2_flash);
 		goto out_free;
 	}
 
+	if (channel_ind) {
+		light->v4l2_flash_ind =
+			v4l2_flash_indicator_init(dev, NULL, iled, &sd_cfg_ind);
+		if (IS_ERR_OR_NULL(light->v4l2_flash_ind)) {
+			ret = PTR_ERR(light->v4l2_flash_ind);
+			goto out_free;
+		}
+	}
+
 	return ret;
 
 out_free:
+	v4l2_flash_release(light->v4l2_flash);
 	return ret;
 }
 
 static void gb_lights_light_v4l2_unregister(struct gb_light *light)
 {
+	v4l2_flash_release(light->v4l2_flash_ind);
 	v4l2_flash_release(light->v4l2_flash);
 }
 #else
diff --git a/include/media/v4l2-flash-led-class.h b/include/media/v4l2-flash-led-class.h
index 54e31a805a88..c3f39992f3fa 100644
--- a/include/media/v4l2-flash-led-class.h
+++ b/include/media/v4l2-flash-led-class.h
@@ -56,8 +56,7 @@ struct v4l2_flash_ops {
  * struct v4l2_flash_config - V4L2 Flash sub-device initialization data
  * @dev_name:			the name of the media entity,
  *				unique in the system
- * @torch_intensity:		constraints for the LED in torch mode
- * @indicator_intensity:	constraints for the indicator LED
+ * @intensity:			non-flash strobe constraints for the LED
  * @flash_faults:		bitmask of flash faults that the LED flash class
  *				device can report; corresponding LED_FAULT* bit
  *				definitions are available in the header file
@@ -66,8 +65,7 @@ struct v4l2_flash_ops {
  */
 struct v4l2_flash_config {
 	char dev_name[32];
-	struct led_flash_setting torch_intensity;
-	struct led_flash_setting indicator_intensity;
+	struct led_flash_setting intensity;
 	u32 flash_faults;
 	unsigned int has_external_strobe:1;
 };
@@ -110,8 +108,6 @@ static inline struct v4l2_flash *v4l2_ctrl_to_v4l2_flash(struct v4l2_ctrl *c)
  * @dev:	flash device, e.g. an I2C device
  * @fwn:	fwnode_handle of the LED, may be NULL if the same as device's
  * @fled_cdev:	LED flash class device to wrap
- * @iled_cdev:	LED flash class device representing indicator LED associated
- *		with fled_cdev, may be NULL
  * @ops:	V4L2 Flash device ops
  * @config:	initialization data for V4L2 Flash sub-device
  *
@@ -124,9 +120,24 @@ static inline struct v4l2_flash *v4l2_ctrl_to_v4l2_flash(struct v4l2_ctrl *c)
 struct v4l2_flash *v4l2_flash_init(
 	struct device *dev, struct fwnode_handle *fwn,
 	struct led_classdev_flash *fled_cdev,
-	struct led_classdev *iled_cdev,
-	const struct v4l2_flash_ops *ops,
-	struct v4l2_flash_config *config);
+	const struct v4l2_flash_ops *ops, struct v4l2_flash_config *config);
+
+/**
+ * v4l2_flash_indicator_init - initialize V4L2 indicator sub-device
+ * @dev:	flash device, e.g. an I2C device
+ * @fwn:	fwnode_handle of the LED, may be NULL if the same as device's
+ * @iled_cdev:	LED flash class device representing the indicator LED
+ * @config:	initialization data for V4L2 Flash sub-device
+ *
+ * Create V4L2 Flash sub-device wrapping given LED subsystem device.
+ *
+ * Returns: A valid pointer, or, when an error occurs, the return
+ * 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_indicator_init(
+	struct device *dev, struct fwnode_handle *fwn,
+	struct led_classdev *iled_cdev, struct v4l2_flash_config *config);
 
 /**
  * v4l2_flash_release - release V4L2 Flash sub-device
@@ -139,10 +150,14 @@ void v4l2_flash_release(struct v4l2_flash *v4l2_flash);
 #else
 static inline struct v4l2_flash *v4l2_flash_init(
 	struct device *dev, struct fwnode_handle *fwn,
-	struct led_classdev_flash *fled_cdev,
-	struct led_classdev *iled_cdev,
-	const struct v4l2_flash_ops *ops,
-	struct v4l2_flash_config *config)
+	struct led_classdev_flash *fled_cdev, struct v4l2_flash_config *config)
+{
+	return NULL;
+}
+
+static inline struct v4l2_flash *v4l2_flash_indicator_init(
+	struct device *dev, struct fwnode_handle *fwn,
+	struct led_classdev *iled_cdev, struct v4l2_flash_config *config)
 {
 	return NULL;
 }
-- 
2.11.0

Re: [PATCH 2/2] v4l2-flash-led-class: Create separate sub-devices for indicators

[Jacek Anaszewski]

Hi Sakari,

Thanks for the patch.

On 07/18/2017 08:41 PM, Sakari Ailus wrote:
> The V4L2 flash interface allows controlling multiple LEDs through a single
> sub-devices if, and only if, these LEDs are of different types. This
> approach scales badly for flash controllers that drive multiple flash LEDs
> or for LED specific associations. Essentially, the original assumption of a
> LED driver chip that drives a single flash LED and an indicator LED is no
> longer valid.
> 
> Address the matter by registering one sub-device per LED.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/leds/leds-aat1290.c                    |   4 +-
>  drivers/leds/leds-max77693.c                   |   4 +-
>  drivers/media/v4l2-core/v4l2-flash-led-class.c | 112 +++++++++++++++----------
>  drivers/staging/greybus/light.c                |  24 ++++--
>  include/media/v4l2-flash-led-class.h           |  41 ++++++---
>  5 files changed, 117 insertions(+), 68 deletions(-)
> 
> diff --git a/drivers/leds/leds-aat1290.c b/drivers/leds/leds-aat1290.c
> index a21e19297745..424898e6c69d 100644
> --- a/drivers/leds/leds-aat1290.c
> +++ b/drivers/leds/leds-aat1290.c
> @@ -432,7 +432,7 @@ static void aat1290_init_v4l2_flash_config(struct aat1290_led *led,
>  	strlcpy(v4l2_sd_cfg->dev_name, led_cdev->name,
>  		sizeof(v4l2_sd_cfg->dev_name));
>  
> -	s = &v4l2_sd_cfg->torch_intensity;
> +	s = &v4l2_sd_cfg->intensity;
>  	s->min = led->mm_current_scale[0];
>  	s->max = led_cfg->max_mm_current;
>  	s->step = 1;
> @@ -504,7 +504,7 @@ static int aat1290_led_probe(struct platform_device *pdev)
>  
>  	/* Create V4L2 Flash subdev. */
>  	led->v4l2_flash = v4l2_flash_init(dev, of_fwnode_handle(sub_node),
> -					  fled_cdev, NULL, &v4l2_flash_ops,
> +					  fled_cdev, &v4l2_flash_ops,
>  					  &v4l2_sd_cfg);
>  	if (IS_ERR(led->v4l2_flash)) {
>  		ret = PTR_ERR(led->v4l2_flash);
> diff --git a/drivers/leds/leds-max77693.c b/drivers/leds/leds-max77693.c
> index 2d3062d53325..adf0f191f794 100644
> --- a/drivers/leds/leds-max77693.c
> +++ b/drivers/leds/leds-max77693.c
> @@ -856,7 +856,7 @@ static void max77693_init_v4l2_flash_config(struct max77693_sub_led *sub_led,
>  		 "%s %d-%04x", sub_led->fled_cdev.led_cdev.name,
>  		 i2c_adapter_id(i2c->adapter), i2c->addr);
>  
> -	s = &v4l2_sd_cfg->torch_intensity;
> +	s = &v4l2_sd_cfg->intensity;
>  	s->min = TORCH_IOUT_MIN;
>  	s->max = sub_led->fled_cdev.led_cdev.max_brightness * TORCH_IOUT_STEP;
>  	s->step = TORCH_IOUT_STEP;
> @@ -931,7 +931,7 @@ static int max77693_register_led(struct max77693_sub_led *sub_led,
>  
>  	/* Register in the V4L2 subsystem. */
>  	sub_led->v4l2_flash = v4l2_flash_init(dev, of_fwnode_handle(sub_node),
> -					      fled_cdev, NULL, &v4l2_flash_ops,
> +					      fled_cdev, &v4l2_flash_ops,
>  					      &v4l2_sd_cfg);
>  	if (IS_ERR(sub_led->v4l2_flash)) {
>  		ret = PTR_ERR(sub_led->v4l2_flash);
> diff --git a/drivers/media/v4l2-core/v4l2-flash-led-class.c b/drivers/media/v4l2-core/v4l2-flash-led-class.c
> index aabc85dbb8b5..b017bdb4aaab 100644
> --- a/drivers/media/v4l2-core/v4l2-flash-led-class.c
> +++ b/drivers/media/v4l2-core/v4l2-flash-led-class.c
> @@ -197,7 +197,7 @@ static int v4l2_flash_s_ctrl(struct v4l2_ctrl *c)
>  {
>  	struct v4l2_flash *v4l2_flash = v4l2_ctrl_to_v4l2_flash(c);
>  	struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
> -	struct led_classdev *led_cdev = &fled_cdev->led_cdev;
> +	struct led_classdev *led_cdev = fled_cdev ? &fled_cdev->led_cdev : NULL;
>  	struct v4l2_ctrl **ctrls = v4l2_flash->ctrls;
>  	bool external_strobe;
>  	int ret = 0;
> @@ -299,10 +299,26 @@ static void __fill_ctrl_init_data(struct v4l2_flash *v4l2_flash,
>  			  struct v4l2_flash_ctrl_data *ctrl_init_data)
>  {
>  	struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
> -	struct led_classdev *led_cdev = &fled_cdev->led_cdev;
> +	struct led_classdev *led_cdev = fled_cdev ? &fled_cdev->led_cdev : NULL;
>  	struct v4l2_ctrl_config *ctrl_cfg;
>  	u32 mask;
>  
> +	/* Init INDICATOR_INTENSITY ctrl data */
> +	if (v4l2_flash->iled_cdev) {
> +		ctrl_init_data[INDICATOR_INTENSITY].cid =
> +					V4L2_CID_FLASH_INDICATOR_INTENSITY;
> +		ctrl_cfg = &ctrl_init_data[INDICATOR_INTENSITY].config;
> +		__lfs_to_v4l2_ctrl_config(&flash_cfg->intensity,
> +					  ctrl_cfg);
> +		ctrl_cfg->id = V4L2_CID_FLASH_INDICATOR_INTENSITY;
> +		ctrl_cfg->min = 0;
> +		ctrl_cfg->flags = V4L2_CTRL_FLAG_VOLATILE |
> +				  V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
> +	}
> +
> +	if (!led_cdev || WARN_ON(!(led_cdev->flags & LED_DEV_CAP_FLASH)))
> +		return;
> +
>  	/* Init FLASH_FAULT ctrl data */
>  	if (flash_cfg->flash_faults) {
>  		ctrl_init_data[FLASH_FAULT].cid = V4L2_CID_FLASH_FAULT;
> @@ -330,27 +346,11 @@ static void __fill_ctrl_init_data(struct v4l2_flash *v4l2_flash,
>  	/* Init TORCH_INTENSITY ctrl data */
>  	ctrl_init_data[TORCH_INTENSITY].cid = V4L2_CID_FLASH_TORCH_INTENSITY;
>  	ctrl_cfg = &ctrl_init_data[TORCH_INTENSITY].config;
> -	__lfs_to_v4l2_ctrl_config(&flash_cfg->torch_intensity, ctrl_cfg);
> +	__lfs_to_v4l2_ctrl_config(&flash_cfg->intensity, ctrl_cfg);
>  	ctrl_cfg->id = V4L2_CID_FLASH_TORCH_INTENSITY;
>  	ctrl_cfg->flags = V4L2_CTRL_FLAG_VOLATILE |
>  			  V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
>  
> -	/* Init INDICATOR_INTENSITY ctrl data */
> -	if (v4l2_flash->iled_cdev) {
> -		ctrl_init_data[INDICATOR_INTENSITY].cid =
> -					V4L2_CID_FLASH_INDICATOR_INTENSITY;
> -		ctrl_cfg = &ctrl_init_data[INDICATOR_INTENSITY].config;
> -		__lfs_to_v4l2_ctrl_config(&flash_cfg->indicator_intensity,
> -					  ctrl_cfg);
> -		ctrl_cfg->id = V4L2_CID_FLASH_INDICATOR_INTENSITY;
> -		ctrl_cfg->min = 0;
> -		ctrl_cfg->flags = V4L2_CTRL_FLAG_VOLATILE |
> -				  V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
> -	}
> -
> -	if (!(led_cdev->flags & LED_DEV_CAP_FLASH))
> -		return;
> -
>  	/* Init FLASH_STROBE ctrl data */
>  	ctrl_init_data[FLASH_STROBE].cid = V4L2_CID_FLASH_STROBE;
>  	ctrl_cfg = &ctrl_init_data[FLASH_STROBE].config;
> @@ -485,7 +485,8 @@ static int __sync_device_with_v4l2_controls(struct v4l2_flash *v4l2_flash)
>  	struct v4l2_ctrl **ctrls = v4l2_flash->ctrls;
>  	int ret = 0;
>  
> -	v4l2_flash_set_led_brightness(v4l2_flash, ctrls[TORCH_INTENSITY]);
> +	if (ctrls[TORCH_INTENSITY])
> +		v4l2_flash_set_led_brightness(v4l2_flash, ctrls[TORCH_INTENSITY]);
>  
>  	if (ctrls[INDICATOR_INTENSITY])
>  		v4l2_flash_set_led_brightness(v4l2_flash,
> @@ -527,19 +528,21 @@ static int v4l2_flash_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
>  {
>  	struct v4l2_flash *v4l2_flash = v4l2_subdev_to_v4l2_flash(sd);
>  	struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
> -	struct led_classdev *led_cdev = &fled_cdev->led_cdev;
> +	struct led_classdev *led_cdev = fled_cdev ? &fled_cdev->led_cdev : NULL;
>  	struct led_classdev *led_cdev_ind = v4l2_flash->iled_cdev;
>  	int ret = 0;
>  
>  	if (!v4l2_fh_is_singular(&fh->vfh))
>  		return 0;
>  
> -	mutex_lock(&led_cdev->led_access);
> +	if (led_cdev) {
> +		mutex_lock(&led_cdev->led_access);
>  
> -	led_sysfs_disable(led_cdev);
> -	led_trigger_remove(led_cdev);
> +		led_sysfs_disable(led_cdev);
> +		led_trigger_remove(led_cdev);
>  
> -	mutex_unlock(&led_cdev->led_access);
> +		mutex_unlock(&led_cdev->led_access);
> +	}
>  
>  	if (led_cdev_ind) {
>  		mutex_lock(&led_cdev_ind->led_access);
> @@ -556,9 +559,11 @@ static int v4l2_flash_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
>  
>  	return 0;
>  out_sync_device:
> -	mutex_lock(&led_cdev->led_access);
> -	led_sysfs_enable(led_cdev);
> -	mutex_unlock(&led_cdev->led_access);
> +	if (led_cdev) {
> +		mutex_lock(&led_cdev->led_access);
> +		led_sysfs_enable(led_cdev);
> +		mutex_unlock(&led_cdev->led_access);
> +	}
>  
>  	if (led_cdev_ind) {
>  		mutex_lock(&led_cdev_ind->led_access);
> @@ -573,21 +578,24 @@ static int v4l2_flash_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
>  {
>  	struct v4l2_flash *v4l2_flash = v4l2_subdev_to_v4l2_flash(sd);
>  	struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
> -	struct led_classdev *led_cdev = &fled_cdev->led_cdev;
> +	struct led_classdev *led_cdev = fled_cdev ? &fled_cdev->led_cdev : NULL;
>  	struct led_classdev *led_cdev_ind = v4l2_flash->iled_cdev;
>  	int ret = 0;
>  
>  	if (!v4l2_fh_is_singular(&fh->vfh))
>  		return 0;
>  
> -	mutex_lock(&led_cdev->led_access);
> +	if (led_cdev) {
> +		mutex_lock(&led_cdev->led_access);
>  
> -	if (v4l2_flash->ctrls[STROBE_SOURCE])
> -		ret = v4l2_ctrl_s_ctrl(v4l2_flash->ctrls[STROBE_SOURCE],
> +		if (v4l2_flash->ctrls[STROBE_SOURCE])
> +			ret = v4l2_ctrl_s_ctrl(
> +				v4l2_flash->ctrls[STROBE_SOURCE],
>  				V4L2_FLASH_STROBE_SOURCE_SOFTWARE);
> -	led_sysfs_enable(led_cdev);
> +		led_sysfs_enable(led_cdev);
>  
> -	mutex_unlock(&led_cdev->led_access);
> +		mutex_unlock(&led_cdev->led_access);
> +	}
>  
>  	if (led_cdev_ind) {
>  		mutex_lock(&led_cdev_ind->led_access);
> @@ -605,25 +613,19 @@ static const struct v4l2_subdev_internal_ops v4l2_flash_subdev_internal_ops = {
>  
>  static const struct v4l2_subdev_ops v4l2_flash_subdev_ops;
>  
> -struct v4l2_flash *v4l2_flash_init(
> +static struct v4l2_flash *__v4l2_flash_init(
>  	struct device *dev, struct fwnode_handle *fwn,
> -	struct led_classdev_flash *fled_cdev,
> -	struct led_classdev *iled_cdev,
> -	const struct v4l2_flash_ops *ops,
> -	struct v4l2_flash_config *config)
> +	struct led_classdev_flash *fled_cdev, struct led_classdev *iled_cdev,
> +	const struct v4l2_flash_ops *ops, struct v4l2_flash_config *config)
>  {
>  	struct v4l2_flash *v4l2_flash;
> -	struct led_classdev *led_cdev;
>  	struct v4l2_subdev *sd;
>  	int ret;
>  
> -	if (!fled_cdev || !config)
> +	if (!config)
>  		return ERR_PTR(-EINVAL);
>  
> -	led_cdev = &fled_cdev->led_cdev;
> -
> -	v4l2_flash = devm_kzalloc(led_cdev->dev, sizeof(*v4l2_flash),
> -					GFP_KERNEL);
> +	v4l2_flash = devm_kzalloc(dev, sizeof(*v4l2_flash), GFP_KERNEL);
>  	if (!v4l2_flash)
>  		return ERR_PTR(-ENOMEM);
>  
> @@ -632,7 +634,7 @@ struct v4l2_flash *v4l2_flash_init(
>  	v4l2_flash->iled_cdev = iled_cdev;
>  	v4l2_flash->ops = ops;
>  	sd->dev = dev;
> -	sd->fwnode = fwn ? fwn : dev_fwnode(led_cdev->dev);
> +	sd->fwnode = fwn ? fwn : dev_fwnode(dev);
>  	v4l2_subdev_init(sd, &v4l2_flash_subdev_ops);
>  	sd->internal_ops = &v4l2_flash_subdev_internal_ops;
>  	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> @@ -664,8 +666,26 @@ struct v4l2_flash *v4l2_flash_init(
>  
>  	return ERR_PTR(ret);
>  }
> +
> +struct v4l2_flash *v4l2_flash_init(
> +	struct device *dev, struct fwnode_handle *fwn,
> +	struct led_classdev_flash *fled_cdev,
> +	const struct v4l2_flash_ops *ops,
> +	struct v4l2_flash_config *config)
> +{
> +	return __v4l2_flash_init(dev, fwn, fled_cdev, NULL, ops, config);
> +}
>  EXPORT_SYMBOL_GPL(v4l2_flash_init);
>  
> +struct v4l2_flash *v4l2_flash_indicator_init(
> +	struct device *dev, struct fwnode_handle *fwn,
> +	struct led_classdev *iled_cdev,
> +	struct v4l2_flash_config *config)
> +{
> +	return __v4l2_flash_init(dev, fwn, NULL, iled_cdev, NULL, config);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_flash_indicator_init);
> +
>  void v4l2_flash_release(struct v4l2_flash *v4l2_flash)
>  {
>  	struct v4l2_subdev *sd;
> diff --git a/drivers/staging/greybus/light.c b/drivers/staging/greybus/light.c
> index b25c117ec41a..17de450ee7e5 100644
> --- a/drivers/staging/greybus/light.c
> +++ b/drivers/staging/greybus/light.c
> @@ -58,6 +58,7 @@ struct gb_light {
>  	bool			ready;
>  #if IS_REACHABLE(CONFIG_V4L2_FLASH_LED_CLASS)
>  	struct v4l2_flash	*v4l2_flash;
> +	struct v4l2_flash	*v4l2_flash_ind;
>  #endif
>  };
>  
> @@ -534,7 +535,7 @@ static int gb_lights_light_v4l2_register(struct gb_light *light)
>  {
>  	struct gb_connection *connection = get_conn_from_light(light);
>  	struct device *dev = &connection->bundle->dev;
> -	struct v4l2_flash_config sd_cfg = { 0 };
> +	struct v4l2_flash_config sd_cfg = { 0 }, sd_cfg_ind = { 0 };
>  	struct led_classdev_flash *fled;
>  	struct led_classdev *iled = NULL;
>  	struct gb_channel *channel_torch, *channel_ind, *channel_flash;
> @@ -543,12 +544,12 @@ static int gb_lights_light_v4l2_register(struct gb_light *light)
>  	channel_torch = get_channel_from_mode(light, GB_CHANNEL_MODE_TORCH);
>  	if (channel_torch)
>  		__gb_lights_channel_v4l2_config(&channel_torch->intensity_uA,
> -						&sd_cfg.torch_intensity);
> +						&sd_cfg.intensity);
>  
>  	channel_ind = get_channel_from_mode(light, GB_CHANNEL_MODE_INDICATOR);
>  	if (channel_ind) {
>  		__gb_lights_channel_v4l2_config(&channel_ind->intensity_uA,
> -						&sd_cfg.indicator_intensity);
> +						&sd_cfg_ind.intensity);
>  		iled = &channel_ind->fled.led_cdev;
>  	}
>  
> @@ -558,6 +559,8 @@ static int gb_lights_light_v4l2_register(struct gb_light *light)
>  	fled = &channel_flash->fled;
>  
>  	snprintf(sd_cfg.dev_name, sizeof(sd_cfg.dev_name), "%s", light->name);
> +	snprintf(sd_cfg_ind.dev_name, sizeof(sd_cfg_ind.dev_name),
> +		 "%s indicator", light->name);
>  
>  	/* Set the possible values to faults, in our case all faults */
>  	sd_cfg.flash_faults = LED_FAULT_OVER_VOLTAGE | LED_FAULT_TIMEOUT |
> @@ -566,21 +569,32 @@ static int gb_lights_light_v4l2_register(struct gb_light *light)
>  		LED_FAULT_UNDER_VOLTAGE | LED_FAULT_INPUT_VOLTAGE |
>  		LED_FAULT_LED_OVER_TEMPERATURE;
>  
> -	light->v4l2_flash = v4l2_flash_init(dev, NULL, fled, iled,
> -					    &v4l2_flash_ops, &sd_cfg);
> +	light->v4l2_flash = v4l2_flash_init(dev, NULL, fled, &v4l2_flash_ops,
> +					    &sd_cfg);
>  	if (IS_ERR_OR_NULL(light->v4l2_flash)) {
>  		ret = PTR_ERR(light->v4l2_flash);
>  		goto out_free;
>  	}
>  
> +	if (channel_ind) {
> +		light->v4l2_flash_ind =
> +			v4l2_flash_indicator_init(dev, NULL, iled, &sd_cfg_ind);
> +		if (IS_ERR_OR_NULL(light->v4l2_flash_ind)) {
> +			ret = PTR_ERR(light->v4l2_flash_ind);
> +			goto out_free;
> +		}
> +	}
> +
>  	return ret;
>  
>  out_free:
> +	v4l2_flash_release(light->v4l2_flash);
>  	return ret;
>  }
>  
>  static void gb_lights_light_v4l2_unregister(struct gb_light *light)
>  {
> +	v4l2_flash_release(light->v4l2_flash_ind);
>  	v4l2_flash_release(light->v4l2_flash);
>  }
>  #else
> diff --git a/include/media/v4l2-flash-led-class.h b/include/media/v4l2-flash-led-class.h
> index 54e31a805a88..c3f39992f3fa 100644
> --- a/include/media/v4l2-flash-led-class.h
> +++ b/include/media/v4l2-flash-led-class.h
> @@ -56,8 +56,7 @@ struct v4l2_flash_ops {
>   * struct v4l2_flash_config - V4L2 Flash sub-device initialization data
>   * @dev_name:			the name of the media entity,
>   *				unique in the system
> - * @torch_intensity:		constraints for the LED in torch mode
> - * @indicator_intensity:	constraints for the indicator LED
> + * @intensity:			non-flash strobe constraints for the LED
>   * @flash_faults:		bitmask of flash faults that the LED flash class
>   *				device can report; corresponding LED_FAULT* bit
>   *				definitions are available in the header file
> @@ -66,8 +65,7 @@ struct v4l2_flash_ops {
>   */
>  struct v4l2_flash_config {
>  	char dev_name[32];
> -	struct led_flash_setting torch_intensity;
> -	struct led_flash_setting indicator_intensity;
> +	struct led_flash_setting intensity;
>  	u32 flash_faults;
>  	unsigned int has_external_strobe:1;
>  };
> @@ -110,8 +108,6 @@ static inline struct v4l2_flash *v4l2_ctrl_to_v4l2_flash(struct v4l2_ctrl *c)
>   * @dev:	flash device, e.g. an I2C device
>   * @fwn:	fwnode_handle of the LED, may be NULL if the same as device's
>   * @fled_cdev:	LED flash class device to wrap
> - * @iled_cdev:	LED flash class device representing indicator LED associated
> - *		with fled_cdev, may be NULL
>   * @ops:	V4L2 Flash device ops
>   * @config:	initialization data for V4L2 Flash sub-device
>   *
> @@ -124,9 +120,24 @@ static inline struct v4l2_flash *v4l2_ctrl_to_v4l2_flash(struct v4l2_ctrl *c)
>  struct v4l2_flash *v4l2_flash_init(
>  	struct device *dev, struct fwnode_handle *fwn,
>  	struct led_classdev_flash *fled_cdev,
> -	struct led_classdev *iled_cdev,
> -	const struct v4l2_flash_ops *ops,
> -	struct v4l2_flash_config *config);
> +	const struct v4l2_flash_ops *ops, struct v4l2_flash_config *config);
> +
> +/**
> + * v4l2_flash_indicator_init - initialize V4L2 indicator sub-device
> + * @dev:	flash device, e.g. an I2C device
> + * @fwn:	fwnode_handle of the LED, may be NULL if the same as device's
> + * @iled_cdev:	LED flash class device representing the indicator LED
> + * @config:	initialization data for V4L2 Flash sub-device
> + *
> + * Create V4L2 Flash sub-device wrapping given LED subsystem device.
> + *
> + * Returns: A valid pointer, or, when an error occurs, the return
> + * 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_indicator_init(
> +	struct device *dev, struct fwnode_handle *fwn,
> +	struct led_classdev *iled_cdev, struct v4l2_flash_config *config);
>  
>  /**
>   * v4l2_flash_release - release V4L2 Flash sub-device
> @@ -139,10 +150,14 @@ void v4l2_flash_release(struct v4l2_flash *v4l2_flash);
>  #else
>  static inline struct v4l2_flash *v4l2_flash_init(
>  	struct device *dev, struct fwnode_handle *fwn,
> -	struct led_classdev_flash *fled_cdev,
> -	struct led_classdev *iled_cdev,
> -	const struct v4l2_flash_ops *ops,
> -	struct v4l2_flash_config *config)
> +	struct led_classdev_flash *fled_cdev, struct v4l2_flash_config *config)
> +{
> +	return NULL;
> +}
> +
> +static inline struct v4l2_flash *v4l2_flash_indicator_init(
> +	struct device *dev, struct fwnode_handle *fwn,
> +	struct led_classdev *iled_cdev, struct v4l2_flash_config *config)
>  {
>  	return NULL;
>  }
> 

Reviewed-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH 2/2] v4l2-flash-led-class: Create separate sub-devices for indicators

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 864 bytes --]

On Tue 2017-07-18 21:41:07, Sakari Ailus wrote:
> The V4L2 flash interface allows controlling multiple LEDs through a single
> sub-devices if, and only if, these LEDs are of different types. This
> approach scales badly for flash controllers that drive multiple flash LEDs
> or for LED specific associations. Essentially, the original assumption of a
> LED driver chip that drives a single flash LED and an indicator LED is no
> longer valid.
> 
> Address the matter by registering one sub-device per LED.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Acked-by: Pavel Machek <pavel@ucw.cz>

Does anything need to be done with drivers/media/i2c/adp1653.c ?

Best regards,
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 2/2] v4l2-flash-led-class: Create separate sub-devices for indicators

[Sakari Ailus]

On Wed, Jul 19, 2017 at 02:02:46PM +0200, Pavel Machek wrote:
> On Tue 2017-07-18 21:41:07, Sakari Ailus wrote:
> > The V4L2 flash interface allows controlling multiple LEDs through a single
> > sub-devices if, and only if, these LEDs are of different types. This
> > approach scales badly for flash controllers that drive multiple flash LEDs
> > or for LED specific associations. Essentially, the original assumption of a
> > LED driver chip that drives a single flash LED and an indicator LED is no
> > longer valid.
> > 
> > Address the matter by registering one sub-device per LED.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> Acked-by: Pavel Machek <pavel@ucw.cz>

Thanks!

> 
> Does anything need to be done with drivers/media/i2c/adp1653.c ?

Well, it does expose the two LEDs through the same sub-device. I don't
think that'd really be an issue. The drivers/media/i2c/as3645a.c does the
same, I think it's fine to keep that.

Effectively only new drivers will have the new behaviour (apart from the
greybus staging driver).

-- 
Regards,

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

Re: [PATCH 2/2] v4l2-flash-led-class: Create separate sub-devices for indicators

[Sakari Ailus]

On Wed, Jul 19, 2017 at 02:02:46PM +0200, Pavel Machek wrote:
> On Tue 2017-07-18 21:41:07, Sakari Ailus wrote:
> > The V4L2 flash interface allows controlling multiple LEDs through a single
> > sub-devices if, and only if, these LEDs are of different types. This
> > approach scales badly for flash controllers that drive multiple flash LEDs
> > or for LED specific associations. Essentially, the original assumption of a
> > LED driver chip that drives a single flash LED and an indicator LED is no
> > longer valid.
> > 
> > Address the matter by registering one sub-device per LED.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> Acked-by: Pavel Machek <pavel@ucw.cz>

Thanks!

> Does anything need to be done with drivers/media/i2c/adp1653.c ?

No, it's stand-alone and does not use the V4L2 flash LED class
framework-let.

-- 
Sakari Ailus
sakari.ailus@linux.intel.com