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

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel