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

[Hans Verkuil]

On 09/08/17 13:15, 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 
> Reviewed-by: Jacek Anaszewski 
> Acked-by: Pavel Machek 

Looks good, but I have the same comment about using '= {};' to zero a struct and
the use of IS_ERR instead of IS_ERR_OR_NULL for the return value check of
v4l2_flash_init as in the 1/3 patch.

After updating that you can add my:

Acked-by: Hans Verkuil 

Regards,

Hans

> ---
>  drivers/leds/leds-aat1290.c|   4 +-
>  drivers/leds/leds-max77693.c   |   4 +-
>  drivers/media/v4l2-core/v4l2-flash-led-class.c | 113 
> +++--
>  drivers/staging/greybus/light.c|  23 +++--
>  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 = _sd_cfg->torch_intensity;
> + s = _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, _flash_ops,
> +   fled_cdev, _flash_ops,
> _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 = _sd_cfg->torch_intensity;
> + s = _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, _flash_ops,
> +   fled_cdev, _flash_ops,
> _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..4ceef217de83 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 = _cdev->led_cdev;
> + struct led_classdev *led_cdev = fled_cdev ? _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 = _cdev->led_cdev;
> + struct led_classdev *led_cdev = fled_cdev ? _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 = _init_data[INDICATOR_INTENSITY].config;
> 

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

[Rui Miguel Silva]

Hi,
On Wed, Aug 09, 2017 at 02:15:54PM +0300, 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 
> Reviewed-by: Jacek Anaszewski 
> Acked-by: Pavel Machek 
> ---
>  drivers/leds/leds-aat1290.c|   4 +-
>  drivers/leds/leds-max77693.c   |   4 +-
>  drivers/media/v4l2-core/v4l2-flash-led-class.c | 113 
> +++--
>  drivers/staging/greybus/light.c|  23 +++--

For greybus/light:
Reviewed-by: Rui Miguel Silva 

---
Cheers,
Rui

[PATCH v2 2/3] 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 
Reviewed-by: Jacek Anaszewski 
Acked-by: Pavel Machek 
---
 drivers/leds/leds-aat1290.c|   4 +-
 drivers/leds/leds-max77693.c   |   4 +-
 drivers/media/v4l2-core/v4l2-flash-led-class.c | 113 +++--
 drivers/staging/greybus/light.c|  23 +++--
 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 = _sd_cfg->torch_intensity;
+   s = _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, _flash_ops,
+ fled_cdev, _flash_ops,
  _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 = _sd_cfg->torch_intensity;
+   s = _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, _flash_ops,
+ fled_cdev, _flash_ops,
  _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..4ceef217de83 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 = _cdev->led_cdev;
+   struct led_classdev *led_cdev = fled_cdev ? _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 = _cdev->led_cdev;
+   struct led_classdev *led_cdev = fled_cdev ? _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 = _init_data[INDICATOR_INTENSITY].config;
+   __lfs_to_v4l2_ctrl_config(_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)))
+