Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1230798 > unrolled thread

Re: [PATCH 3/5] leds: Rename brightness_set_sync op to brightness_set_blocking

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 3/5] leds: Rename brightness_set_sync op to  brightness_set_blocking Andrew Lunn <andrew@lunn.ch> - 2015-09-22 21:10 +0200
    Re: [PATCH 3/5] leds: Rename brightness_set_sync op to  brightness_set_blocking Jacek Anaszewski <j.anaszewski@samsung.com> - 2015-09-23 10:40 +0200
      Re: [PATCH 3/5] leds: Rename brightness_set_sync op to  brightness_set_blocking Jacek Anaszewski <j.anaszewski@samsung.com> - 2015-09-23 11:40 +0200

#1230798 — Re: [PATCH 3/5] leds: Rename brightness_set_sync op to brightness_set_blocking

On Wed, Sep 16, 2015 at 12:47:42PM +0200, Jacek Anaszewski wrote:
> The initial purpose of brightness_set_sync op, introduced along with
> the LED flash class extension, was to add a means for setting torch LED
> brightness as soon as possible, which couldn't have been guaranteed by
> brightness_set op. This patch renames the op to brightness_set_blocking,
> which describes its purpose in a more generic way, and is beneficial
> in view of the prospective changes in the core related to using
> LED core's set_brightness_work for setting brightness for LED class
> drivers that can sleep or use delays while setting brightness.

...

> -	/*
> -	 * Set LED brightness level immediately - it can block the caller for
> -	 * the time required for accessing a LED device register.
> -	 */
> -	int		(*brightness_set_sync)(struct led_classdev *led_cdev,
> -					enum led_brightness brightness);
> +	/* Can sleep or use delays */
> +	int (*brightness_set_blocking)(struct led_classdev *led_cdev,

I'm no expert when it comes to flash photography with digital
cameras. But to me the old comment seems better. Doesn't the caller
want to know the flash is now giving out light? The new comment gives
no indication of this, where as the old one does?

   Andrew


>  	/* Get LED brightness level */
>  	enum led_brightness (*brightness_get)(struct led_classdev *led_cdev);
>  
> -- 
> 1.7.9.5
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1231230

On 09/22/2015 08:54 PM, Andrew Lunn wrote:
> On Wed, Sep 16, 2015 at 12:47:42PM +0200, Jacek Anaszewski wrote:
>> The initial purpose of brightness_set_sync op, introduced along with
>> the LED flash class extension, was to add a means for setting torch LED
>> brightness as soon as possible, which couldn't have been guaranteed by
>> brightness_set op. This patch renames the op to brightness_set_blocking,
>> which describes its purpose in a more generic way, and is beneficial
>> in view of the prospective changes in the core related to using
>> LED core's set_brightness_work for setting brightness for LED class
>> drivers that can sleep or use delays while setting brightness.
>
> ...
>
>> -	/*
>> -	 * Set LED brightness level immediately - it can block the caller for
>> -	 * the time required for accessing a LED device register.
>> -	 */
>> -	int		(*brightness_set_sync)(struct led_classdev *led_cdev,
>> -					enum led_brightness brightness);
>> +	/* Can sleep or use delays */
>> +	int (*brightness_set_blocking)(struct led_classdev *led_cdev,
>
> I'm no expert when it comes to flash photography with digital
> cameras.

This op is now not specific to flash LEDs. The last sentence in the
commit message explains this, but now I see that it is too long.
Let's change it to:

"This patch renames the op to brightness_set_blocking,
which describes its purpose in a more generic way. It is beneficial
in view of the prospective changes in the LED core, aiming at removing
the need for using work queues in LED class drivers that can sleep
or use delays while setting brightness."

> But to me the old comment seems better.

I changed it to highlight the essence of how it differs from
brightness_set.

> Doesn't the caller
> want to know the flash is now giving out light?

We have strobe_get op for this, but it is in led-class-flash extension.
This is irrelevant here.

> The new comment gives
> no indication of this, where as the old one does?



-- 
Best Regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1231286

On 09/23/2015 10:36 AM, Jacek Anaszewski wrote:
> On 09/22/2015 08:54 PM, Andrew Lunn wrote:
>> On Wed, Sep 16, 2015 at 12:47:42PM +0200, Jacek Anaszewski wrote:
>>> The initial purpose of brightness_set_sync op, introduced along with
>>> the LED flash class extension, was to add a means for setting torch LED
>>> brightness as soon as possible, which couldn't have been guaranteed by
>>> brightness_set op. This patch renames the op to brightness_set_blocking,
>>> which describes its purpose in a more generic way, and is beneficial
>>> in view of the prospective changes in the core related to using
>>> LED core's set_brightness_work for setting brightness for LED class
>>> drivers that can sleep or use delays while setting brightness.
>>
>> ...
>>
>>> -    /*
>>> -     * Set LED brightness level immediately - it can block the
>>> caller for
>>> -     * the time required for accessing a LED device register.
>>> -     */
>>> -    int        (*brightness_set_sync)(struct led_classdev *led_cdev,
>>> -                    enum led_brightness brightness);
>>> +    /* Can sleep or use delays */
>>> +    int (*brightness_set_blocking)(struct led_classdev *led_cdev,
>>
>> I'm no expert when it comes to flash photography with digital
>> cameras.
>
> This op is now not specific to flash LEDs. The last sentence in the
> commit message explains this, but now I see that it is too long.
> Let's change it to:
>
> "This patch renames the op to brightness_set_blocking,
> which describes its purpose in a more generic way. It is beneficial
> in view of the prospective changes in the LED core, aiming at removing
> the need for using work queues in LED class drivers that can sleep
> or use delays while setting brightness."
>
>> But to me the old comment seems better.
>
> I changed it to highlight the essence of how it differs from
> brightness_set.
>
>> Doesn't the caller
>> want to know the flash is now giving out light?
>
> We have strobe_get op for this, but it is in led-class-flash extension.
> This is irrelevant here.

I focused myself on flash mode of flash LED, but you asked
probably about brightness in torch mode, which can be obtained
with brightness_get op, if implemented by the driver.


-- 
Best Regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web