[PATCH v2] Documentation: leds: Add description of LED Flash class extension

[PATCH v2] Documentation: leds: Add description of LED Flash class extension

[Jacek Anaszewski]

The documentation being added contains overall description of the
LED Flash Class and the related sysfs attributes.

Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
Cc: Bryan Wu <cooloney@gmail.com>
Cc: Richard Purdie <rpurdie@rpsys.net>
---
 Documentation/leds/leds-class-flash.txt |   63 +++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)
 create mode 100644 Documentation/leds/leds-class-flash.txt

diff --git a/Documentation/leds/leds-class-flash.txt b/Documentation/leds/leds-class-flash.txt
new file mode 100644
index 0000000..16c6187
--- /dev/null
+++ b/Documentation/leds/leds-class-flash.txt
@@ -0,0 +1,63 @@
+
+Flash LED handling under Linux
+==============================
+
+Some LED devices support two modes - torch and flash. In the LED subsystem
+those modes are supported by LED class (see Documentation/leds/leds-class.txt)
+and LED Flash class respectively. The torch mode related features are enabled
+by default and the flash ones only if a driver declares it by setting
+LED_DEV_CAP_FLASH flag.
+
+In order to enable support for flash LEDs CONFIG_LEDS_CLASS_FLASH symbol
+must be defined in the kernel config. A flash LED driver must register
+in the LED subsystem with led_classdev_flash_register function to gain flash
+related capabilities.
+
+There are flash LED devices which can control more than one LED and allow for
+strobing the sub-LEDs synchronously. A LED will be strobed synchronously with
+the one whose identifier is written to the flash_sync_strobe sysfs attribute.
+The list of available sub-LED identifiers can be read from the available_sync_leds
+sysfs attribute. In order to enable the related settings the driver must set
+LED_DEV_CAP_SYNC_STROBE flag.
+
+Following sysfs attributes are exposed for controlling flash LED devices:
+
+	- flash_brightness - flash LED brightness in microamperes (RW)
+	- max_flash_brightness - maximum available flash LED brightness (RO)
+	- flash_timeout - flash strobe duration in microseconds (RW)
+	- max_flash_timeout - maximum available flash strobe duration (RO)
+	- flash_strobe - flash strobe state (RW)
+	- available_sync_leds - list of sub-LEDs available for flash strobe
+				synchronization (RO)
+	- flash_sync_strobe - identifier of the sub-LED to synchronize the flash
+			      strobe with; 0 stands for no synchronization (RW)
+	- flash_fault - list of flash faults that may have occurred:
+		* led-over-voltage - flash controller voltage to the flash LED
+			has exceededthe limit specific to the flash controller
+		* flash-timeout-exceeded - the flash strobe was still on when
+			the timeout set by the user has expired; not all flash
+			controllers may set this in all such conditions
+		* controller-over-temperature - the flash controller has
+			overheated
+		* controller-short-circuit - the short circuit protection
+			of the flash controller has been triggered
+		* led-power-supply-over-current - current in the LED power
+			supply has exceeded the limit specific to the flash
+			controller
+		* indicator-led-fault - the flash controller has detected
+			a short or open circuit condition on the indicator LED
+		* led-under-voltage - flash controller voltage to the flash
+			LED has been below the minimum limit specific to
+			the flash
+		* controller-under-voltage - the input voltage of the flash
+			controller is below the limit under which strobing the
+			flash at full current will not be possible;
+			the condition persists until this flag is no longer set
+		* led-over-temperature - the temperature of the LED has exceeded
+			its allowed upper limit
+
+		Flash faults should be read in the strobe_set callback, right
+		after the instruction initiating the strobe. If a flash LED
+		device provides an instruction for clearing the faults, then,
+		in case there were any faults read, it should be also executed
+		in the callback.
-- 
1.7.9.5

Re: [PATCH v2] Documentation: leds: Add description of LED Flash class extension

[Bryan Wu]

On Thu, Feb 5, 2015 at 12:42 AM, Jacek Anaszewski
<j.anaszewski@samsung.com> wrote:
> The documentation being added contains overall description of the
> LED Flash Class and the related sysfs attributes.
>
> Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
> Cc: Bryan Wu <cooloney@gmail.com>
> Cc: Richard Purdie <rpurdie@rpsys.net>
> ---

Jacek, I lost the discussion here. Did you change something since V1 patch?

Pavel, Is this version OK for you?

-Bryan


>  Documentation/leds/leds-class-flash.txt |   63 +++++++++++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
>  create mode 100644 Documentation/leds/leds-class-flash.txt
>
> diff --git a/Documentation/leds/leds-class-flash.txt b/Documentation/leds/leds-class-flash.txt
> new file mode 100644
> index 0000000..16c6187
> --- /dev/null
> +++ b/Documentation/leds/leds-class-flash.txt
> @@ -0,0 +1,63 @@
> +
> +Flash LED handling under Linux
> +==============================
> +
> +Some LED devices support two modes - torch and flash. In the LED subsystem
> +those modes are supported by LED class (see Documentation/leds/leds-class.txt)
> +and LED Flash class respectively. The torch mode related features are enabled
> +by default and the flash ones only if a driver declares it by setting
> +LED_DEV_CAP_FLASH flag.
> +
> +In order to enable support for flash LEDs CONFIG_LEDS_CLASS_FLASH symbol
> +must be defined in the kernel config. A flash LED driver must register
> +in the LED subsystem with led_classdev_flash_register function to gain flash
> +related capabilities.
> +
> +There are flash LED devices which can control more than one LED and allow for
> +strobing the sub-LEDs synchronously. A LED will be strobed synchronously with
> +the one whose identifier is written to the flash_sync_strobe sysfs attribute.
> +The list of available sub-LED identifiers can be read from the available_sync_leds
> +sysfs attribute. In order to enable the related settings the driver must set
> +LED_DEV_CAP_SYNC_STROBE flag.
> +
> +Following sysfs attributes are exposed for controlling flash LED devices:
> +
> +       - flash_brightness - flash LED brightness in microamperes (RW)
> +       - max_flash_brightness - maximum available flash LED brightness (RO)
> +       - flash_timeout - flash strobe duration in microseconds (RW)
> +       - max_flash_timeout - maximum available flash strobe duration (RO)
> +       - flash_strobe - flash strobe state (RW)
> +       - available_sync_leds - list of sub-LEDs available for flash strobe
> +                               synchronization (RO)
> +       - flash_sync_strobe - identifier of the sub-LED to synchronize the flash
> +                             strobe with; 0 stands for no synchronization (RW)
> +       - flash_fault - list of flash faults that may have occurred:
> +               * led-over-voltage - flash controller voltage to the flash LED
> +                       has exceededthe limit specific to the flash controller
> +               * flash-timeout-exceeded - the flash strobe was still on when
> +                       the timeout set by the user has expired; not all flash
> +                       controllers may set this in all such conditions
> +               * controller-over-temperature - the flash controller has
> +                       overheated
> +               * controller-short-circuit - the short circuit protection
> +                       of the flash controller has been triggered
> +               * led-power-supply-over-current - current in the LED power
> +                       supply has exceeded the limit specific to the flash
> +                       controller
> +               * indicator-led-fault - the flash controller has detected
> +                       a short or open circuit condition on the indicator LED
> +               * led-under-voltage - flash controller voltage to the flash
> +                       LED has been below the minimum limit specific to
> +                       the flash
> +               * controller-under-voltage - the input voltage of the flash
> +                       controller is below the limit under which strobing the
> +                       flash at full current will not be possible;
> +                       the condition persists until this flag is no longer set
> +               * led-over-temperature - the temperature of the LED has exceeded
> +                       its allowed upper limit
> +
> +               Flash faults should be read in the strobe_set callback, right
> +               after the instruction initiating the strobe. If a flash LED
> +               device provides an instruction for clearing the faults, then,
> +               in case there were any faults read, it should be also executed
> +               in the callback.
> --
> 1.7.9.5
>

Re: [PATCH v2] Documentation: leds: Add description of LED Flash class extension

[Jacek Anaszewski]

On 02/06/2015 07:48 PM, Bryan Wu wrote:
> On Thu, Feb 5, 2015 at 12:42 AM, Jacek Anaszewski
> <j.anaszewski@samsung.com> wrote:
>> The documentation being added contains overall description of the
>> LED Flash Class and the related sysfs attributes.
>>
>> Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
>> Cc: Bryan Wu <cooloney@gmail.com>
>> Cc: Richard Purdie <rpurdie@rpsys.net>
>> ---
>
> Jacek, I lost the discussion here. Did you change something since V1 patch?
>
> Pavel, Is this version OK for you?

I changed the instruction on how to proceed with flash faults handling
in LED Flash class drivers (the part after listing possible flash
faults, as it is exposed below. There hasn't been any opinion on this
expressed yet, though.


>> +               * led-over-temperature - the temperature of the LED has exceeded
>> +                       its allowed upper limit
>> +
>> +               Flash faults should be read in the strobe_set callback, right
>> +               after the instruction initiating the strobe. If a flash LED
>> +               device provides an instruction for clearing the faults, then,
>> +               in case there were any faults read, it should be also executed
>> +               in the callback.
>> --
>> 1.7.9.5
>>
>


-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH v2] Documentation: leds: Add description of LED Flash class extension

[Pavel Machek]

On Thu 2015-02-05 09:42:53, Jacek Anaszewski wrote:
> The documentation being added contains overall description of the
> LED Flash Class and the related sysfs attributes.
> 
> Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
> Cc: Bryan Wu <cooloney@gmail.com>
> Cc: Richard Purdie <rpurdie@rpsys.net>
> ---
>  Documentation/leds/leds-class-flash.txt |   63 +++++++++++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
>  create mode 100644 Documentation/leds/leds-class-flash.txt
> 
> diff --git a/Documentation/leds/leds-class-flash.txt b/Documentation/leds/leds-class-flash.txt
> new file mode 100644
> index 0000000..16c6187
> --- /dev/null
> +++ b/Documentation/leds/leds-class-flash.txt
> @@ -0,0 +1,63 @@
> +
> +Flash LED handling under Linux
> +==============================
> +
> +Some LED devices support two modes - torch and flash. In the LED subsystem
> +those modes are supported by LED class (see Documentation/leds/leds-class.txt)
> +and LED Flash class respectively. The torch mode related features are enabled
> +by default and the flash ones only if a driver declares it by setting
> +LED_DEV_CAP_FLASH flag.
> +
> +In order to enable support for flash LEDs CONFIG_LEDS_CLASS_FLASH symbol
> +must be defined in the kernel config. A flash LED driver must register
> +in the LED subsystem with led_classdev_flash_register function to gain flash
> +related capabilities.
> +
> +There are flash LED devices which can control more than one LED and allow for
> +strobing the sub-LEDs synchronously. A LED will be strobed synchronously with
> +the one whose identifier is written to the flash_sync_strobe sysfs attribute.
> +The list of available sub-LED identifiers can be read from the available_sync_leds
> +sysfs attribute. In order to enable the related settings the driver must set
> +LED_DEV_CAP_SYNC_STROBE flag.
> +
> +Following sysfs attributes are exposed for controlling flash LED devices:
> +
> +	- flash_brightness - flash LED brightness in microamperes (RW)
> +	- max_flash_brightness - maximum available flash LED brightness (RO)

in microamperes

> +	- flash_timeout - flash strobe duration in microseconds (RW)
> +	- max_flash_timeout - maximum available flash strobe duration
> (RO)

in microseconds

> +	- flash_strobe - flash strobe state (RW)

This is integer..?

> +	- available_sync_leds - list of sub-LEDs available for flash strobe
> +				synchronization (RO)

"space separated"?

So this will say something like "0 3 5"

> +	- flash_sync_strobe - identifier of the sub-LED to synchronize the flash
> +			      strobe with; 0 stands for no synchronization (RW)

...and it means that I can put 0, 3 or 5 into this file?

> +	- flash_fault - list of flash faults that may have occurred:

"space separated"?

> +		* led-over-voltage - flash controller voltage to the flash LED
> +			has exceededthe limit specific to the flash controller
> +		* flash-timeout-exceeded - the flash strobe was still on when
> +			the timeout set by the user has expired; not all flash
> +			controllers may set this in all such conditions
> +		* controller-over-temperature - the flash controller has
> +			overheated
> +		* controller-short-circuit - the short circuit protection
> +			of the flash controller has been triggered
> +		* led-power-supply-over-current - current in the LED power
> +			supply has exceeded the limit specific to the flash
> +			controller
> +		* indicator-led-fault - the flash controller has detected
> +			a short or open circuit condition on the indicator LED
> +		* led-under-voltage - flash controller voltage to the flash
> +			LED has been below the minimum limit specific to
> +			the flash
> +		* controller-under-voltage - the input voltage of the flash
> +			controller is below the limit under which strobing the
> +			flash at full current will not be possible;
> +			the condition persists until this flag is no longer set
> +		* led-over-temperature - the temperature of the LED has exceeded
> +			its allowed upper limit
> +
> +		Flash faults should be read in the strobe_set callback, right
> +		after the instruction initiating the strobe. If a flash LED

Ok, so this is part sysfs documentation, part kernel class
documentation... it is a bit confusing.

Best regards,
									Pavel

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

Re: [PATCH v2] Documentation: leds: Add description of LED Flash class extension

[Jacek Anaszewski]

Hi Pavel,

Thanks for the review.

On 02/11/2015 10:57 PM, Pavel Machek wrote:
> On Thu 2015-02-05 09:42:53, Jacek Anaszewski wrote:
>> The documentation being added contains overall description of the
>> LED Flash Class and the related sysfs attributes.
>>
>> Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
>> Cc: Bryan Wu <cooloney@gmail.com>
>> Cc: Richard Purdie <rpurdie@rpsys.net>
>> ---
>>   Documentation/leds/leds-class-flash.txt |   63 +++++++++++++++++++++++++++++++
>>   1 file changed, 63 insertions(+)
>>   create mode 100644 Documentation/leds/leds-class-flash.txt
>>
>> diff --git a/Documentation/leds/leds-class-flash.txt b/Documentation/leds/leds-class-flash.txt
>> new file mode 100644
>> index 0000000..16c6187
>> --- /dev/null
>> +++ b/Documentation/leds/leds-class-flash.txt
>> @@ -0,0 +1,63 @@
>> +
>> +Flash LED handling under Linux
>> +==============================
>> +
>> +Some LED devices support two modes - torch and flash. In the LED subsystem
>> +those modes are supported by LED class (see Documentation/leds/leds-class.txt)
>> +and LED Flash class respectively. The torch mode related features are enabled
>> +by default and the flash ones only if a driver declares it by setting
>> +LED_DEV_CAP_FLASH flag.
>> +
>> +In order to enable support for flash LEDs CONFIG_LEDS_CLASS_FLASH symbol
>> +must be defined in the kernel config. A flash LED driver must register
>> +in the LED subsystem with led_classdev_flash_register function to gain flash
>> +related capabilities.
>> +
>> +There are flash LED devices which can control more than one LED and allow for
>> +strobing the sub-LEDs synchronously. A LED will be strobed synchronously with
>> +the one whose identifier is written to the flash_sync_strobe sysfs attribute.
>> +The list of available sub-LED identifiers can be read from the available_sync_leds
>> +sysfs attribute. In order to enable the related settings the driver must set
>> +LED_DEV_CAP_SYNC_STROBE flag.
>> +
>> +Following sysfs attributes are exposed for controlling flash LED devices:
>> +
>> +	- flash_brightness - flash LED brightness in microamperes (RW)
>> +	- max_flash_brightness - maximum available flash LED brightness (RO)
>
> in microamperes
>
>> +	- flash_timeout - flash strobe duration in microseconds (RW)
>> +	- max_flash_timeout - maximum available flash strobe duration
>> (RO)
>
> in microseconds
>
>> +	- flash_strobe - flash strobe state (RW)
>
> This is integer..?

I can be 0 or 1. Let's make it more precise:

- flash_strobe - flash strobe state (RW):
			semantics on write:
				0: turn the flash LED off
				1: strobe the flash LED
			semantics on read:
				0: flash LED is off
				1: flash LED is strobing

>> +	- available_sync_leds - list of sub-LEDs available for flash strobe
>> +				synchronization (RO)
>
> "space separated"?

- available_sync_leds - space separated list of sub-LEDs available for
			flash strobe synchronization; each sub-LED is
			described in the form of chunks:
			[led_id: led_name]


> So this will say something like "0 3 5"

Rather e.g.: [0: none] [1: max77693-led1] [2: max77693-led2]

>> +	- flash_sync_strobe - identifier of the sub-LED to synchronize the flash
>> +			      strobe with; 0 stands for no synchronization (RW)
>
> ...and it means that I can put 0, 3 or 5 into this file?

Yes.

>> +	- flash_fault - list of flash faults that may have occurred:
>
> "space separated"?

Right.

>> +		* led-over-voltage - flash controller voltage to the flash LED
>> +			has exceededthe limit specific to the flash controller
>> +		* flash-timeout-exceeded - the flash strobe was still on when
>> +			the timeout set by the user has expired; not all flash
>> +			controllers may set this in all such conditions
>> +		* controller-over-temperature - the flash controller has
>> +			overheated
>> +		* controller-short-circuit - the short circuit protection
>> +			of the flash controller has been triggered
>> +		* led-power-supply-over-current - current in the LED power
>> +			supply has exceeded the limit specific to the flash
>> +			controller
>> +		* indicator-led-fault - the flash controller has detected
>> +			a short or open circuit condition on the indicator LED
>> +		* led-under-voltage - flash controller voltage to the flash
>> +			LED has been below the minimum limit specific to
>> +			the flash
>> +		* controller-under-voltage - the input voltage of the flash
>> +			controller is below the limit under which strobing the
>> +			flash at full current will not be possible;
>> +			the condition persists until this flag is no longer set
>> +		* led-over-temperature - the temperature of the LED has exceeded
>> +			its allowed upper limit
>> +
>> +		Flash faults should be read in the strobe_set callback, right
>> +		after the instruction initiating the strobe. If a flash LED
>
> Ok, so this is part sysfs documentation, part kernel class
> documentation... it is a bit confusing.

As it documents the class itself it is convenient that all information
is available in one place. LED class documentation
Documentation/leds/leds-class.txt also contains information about
sysfs attributes semantics.


-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH v2] Documentation: leds: Add description of LED Flash class extension

[Pavel Machek]

Hi!

> I can be 0 or 1. Let's make it more precise:
> 
> - flash_strobe - flash strobe state (RW):
> 			semantics on write:
> 				0: turn the flash LED off
> 				1: strobe the flash LED
> 			semantics on read:
> 				0: flash LED is off
> 				1: flash LED is strobing

Thanks.

> >>+	- available_sync_leds - list of sub-LEDs available for flash strobe
> >>+				synchronization (RO)
> >
> >"space separated"?
> 
> - available_sync_leds - space separated list of sub-LEDs available for
> 			flash strobe synchronization; each sub-LED is
> 			described in the form of chunks:
> 			[led_id: led_name]
> 
> 
> >So this will say something like "0 3 5"
> 
> Rather e.g.: [0: none] [1: max77693-led1] [2: max77693-led2]

No no, sorry, you can't do that. Sysfs is supposed to be one value per
file, and this is stretching it. (It would be also difficult to parse;
for example, you can reasonably have ":" in led name, and perhaps even
" " or "]"....
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v2] Documentation: leds: Add description of LED Flash class extension

[Jacek Anaszewski]

On 02/12/2015 10:07 AM, Pavel Machek wrote:
> Hi!
>
>> I can be 0 or 1. Let's make it more precise:
>>
>> - flash_strobe - flash strobe state (RW):
>> 			semantics on write:
>> 				0: turn the flash LED off
>> 				1: strobe the flash LED
>> 			semantics on read:
>> 				0: flash LED is off
>> 				1: flash LED is strobing
>
> Thanks.
>
>>>> +	- available_sync_leds - list of sub-LEDs available for flash strobe
>>>> +				synchronization (RO)
>>>
>>> "space separated"?
>>
>> - available_sync_leds - space separated list of sub-LEDs available for
>> 			flash strobe synchronization; each sub-LED is
>> 			described in the form of chunks:
>> 			[led_id: led_name]
>>
>>
>>> So this will say something like "0 3 5"
>>
>> Rather e.g.: [0: none] [1: max77693-led1] [2: max77693-led2]
>
> No no, sorry, you can't do that. Sysfs is supposed to be one value per
> file, and this is stretching it. (It would be also difficult to parse;
> for example, you can reasonably have ":" in led name, and perhaps even
> " " or "]"....

You acked LED Flash class patch, didn't you? :)

There are many attributes documented in the list fashion, e.g.:
available_frequencies in the Documentation/ABI/testing/sysfs-class-devfreq.

LED flash class attributes should be also probably added to the
Documentation/ABI/testing/sysfs-class-led, or a new file
sysfs-class-flash-led should be created.

If we changed this a bit it would be easily parsed with AWK:

echo "0 none;1 max77693-led1;2 max77693-led2" | awk -F';' '{ for (i=1; 
i<=NF; i++) print $i}' | awk '{print $1": "$2}'


-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH v2] Documentation: leds: Add description of LED Flash class extension

[Pavel Machek]

On Thu 2015-02-12 11:10:38, Jacek Anaszewski wrote:
> On 02/12/2015 10:07 AM, Pavel Machek wrote:
> >Hi!
> >
> >>I can be 0 or 1. Let's make it more precise:
> >>
> >>- flash_strobe - flash strobe state (RW):
> >>			semantics on write:
> >>				0: turn the flash LED off
> >>				1: strobe the flash LED
> >>			semantics on read:
> >>				0: flash LED is off
> >>				1: flash LED is strobing
> >
> >Thanks.
> >
> >>>>+	- available_sync_leds - list of sub-LEDs available for flash strobe
> >>>>+				synchronization (RO)
> >>>
> >>>"space separated"?
> >>
> >>- available_sync_leds - space separated list of sub-LEDs available for
> >>			flash strobe synchronization; each sub-LED is
> >>			described in the form of chunks:
> >>			[led_id: led_name]
> >>
> >>
> >>>So this will say something like "0 3 5"
> >>
> >>Rather e.g.: [0: none] [1: max77693-led1] [2: max77693-led2]
> >
> >No no, sorry, you can't do that. Sysfs is supposed to be one value per
> >file, and this is stretching it. (It would be also difficult to parse;
> >for example, you can reasonably have ":" in led name, and perhaps even
> >" " or "]"....
> 
> You acked LED Flash class patch, didn't you? :)
> 
> There are many attributes documented in the list fashion, e.g.:
> available_frequencies in the Documentation/ABI/testing/sysfs-class-devfreq.

No, sorry, you can't do this. We have this parsing nightmare in /proc,
before, and we don't want it again.

> If we changed this a bit it would be easily parsed with AWK:
> 
> echo "0 none;1 max77693-led1;2 max77693-led2" | awk -F';' '{ for (i=1;
> i<=NF; i++) print $i}' | awk '{print $1": "$2}'

sysfs is one entry per file. If someone screwed it up in devfreq, it
is not reason to screw it up here. (Space separated lists of integers
might be acceptable. Fixed strings with one marked by []s happen,
too. Maps between ints and names are not.)
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v2] Documentation: leds: Add description of LED Flash class extension

[Jacek Anaszewski]

On 02/12/2015 11:54 AM, Pavel Machek wrote:
> On Thu 2015-02-12 11:10:38, Jacek Anaszewski wrote:
>> On 02/12/2015 10:07 AM, Pavel Machek wrote:
>>> Hi!
>>>
>>>> I can be 0 or 1. Let's make it more precise:
>>>>
>>>> - flash_strobe - flash strobe state (RW):
>>>> 			semantics on write:
>>>> 				0: turn the flash LED off
>>>> 				1: strobe the flash LED
>>>> 			semantics on read:
>>>> 				0: flash LED is off
>>>> 				1: flash LED is strobing
>>>
>>> Thanks.
>>>
>>>>>> +	- available_sync_leds - list of sub-LEDs available for flash strobe
>>>>>> +				synchronization (RO)
>>>>>
>>>>> "space separated"?
>>>>
>>>> - available_sync_leds - space separated list of sub-LEDs available for
>>>> 			flash strobe synchronization; each sub-LED is
>>>> 			described in the form of chunks:
>>>> 			[led_id: led_name]
>>>>
>>>>
>>>>> So this will say something like "0 3 5"
>>>>
>>>> Rather e.g.: [0: none] [1: max77693-led1] [2: max77693-led2]
>>>
>>> No no, sorry, you can't do that. Sysfs is supposed to be one value per
>>> file, and this is stretching it. (It would be also difficult to parse;
>>> for example, you can reasonably have ":" in led name, and perhaps even
>>> " " or "]"....
>>
>> You acked LED Flash class patch, didn't you? :)
>>
>> There are many attributes documented in the list fashion, e.g.:
>> available_frequencies in the Documentation/ABI/testing/sysfs-class-devfreq.
>
> No, sorry, you can't do this. We have this parsing nightmare in /proc,
> before, and we don't want it again.
>
>> If we changed this a bit it would be easily parsed with AWK:
>>
>> echo "0 none;1 max77693-led1;2 max77693-led2" | awk -F';' '{ for (i=1;
>> i<=NF; i++) print $i}' | awk '{print $1": "$2}'
>
> sysfs is one entry per file. If someone screwed it up in devfreq, it
> is not reason to screw it up here. (Space separated lists of integers
> might be acceptable. Fixed strings with one marked by []s happen,
> too. Maps between ints and names are not.)

"Fixed strings with one marked by []s happen" - I don't quite
get this.
Could you propose the acceptable format then?
The numbers alone are inconvenient to use, we need a human
readable description next to them. Or some another way
to obtain the name.

-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH v2] Documentation: leds: Add description of LED Flash class extension

[Pavel Machek]

Hi!

> >>You acked LED Flash class patch, didn't you? :)
> >>
> >>There are many attributes documented in the list fashion, e.g.:
> >>available_frequencies in the Documentation/ABI/testing/sysfs-class-devfreq.
> >
> >No, sorry, you can't do this. We have this parsing nightmare in /proc,
> >before, and we don't want it again.
> >
> >>If we changed this a bit it would be easily parsed with AWK:
> >>
> >>echo "0 none;1 max77693-led1;2 max77693-led2" | awk -F';' '{ for (i=1;
> >>i<=NF; i++) print $i}' | awk '{print $1": "$2}'
> >
> >sysfs is one entry per file. If someone screwed it up in devfreq, it
> >is not reason to screw it up here. (Space separated lists of integers
> >might be acceptable. Fixed strings with one marked by []s happen,
> >too. Maps between ints and names are not.)
> 
> "Fixed strings with one marked by []s happen" - I don't quite
> get this.

root@duo:~# cat /sys/power/state
freeze mem disk
root@duo:~# cat /sys/power/disk
[platform] shutdown reboot suspend
root@duo:~#

> Could you propose the acceptable format then?
> The numbers alone are inconvenient to use, we need a human
> readable description next to them. Or some another way
> to obtain the name.

Files like "0.active", "0.name", "1.active", "1.name"?

Subdirectories 0/1 with "name" and "active" files?
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v2] Documentation: leds: Add description of LED Flash class extension

[Jacek Anaszewski]

Hi Pavel,

On 02/12/2015 06:44 PM, Pavel Machek wrote:
> Hi!
>
>>>> You acked LED Flash class patch, didn't you? :)
>>>>
>>>> There are many attributes documented in the list fashion, e.g.:
>>>> available_frequencies in the Documentation/ABI/testing/sysfs-class-devfreq.
>>>
>>> No, sorry, you can't do this. We have this parsing nightmare in /proc,
>>> before, and we don't want it again.
>>>
>>>> If we changed this a bit it would be easily parsed with AWK:
>>>>
>>>> echo "0 none;1 max77693-led1;2 max77693-led2" | awk -F';' '{ for (i=1;
>>>> i<=NF; i++) print $i}' | awk '{print $1": "$2}'
>>>
>>> sysfs is one entry per file. If someone screwed it up in devfreq, it
>>> is not reason to screw it up here. (Space separated lists of integers
>>> might be acceptable. Fixed strings with one marked by []s happen,
>>> too. Maps between ints and names are not.)
>>
>> "Fixed strings with one marked by []s happen" - I don't quite
>> get this.
>
> root@duo:~# cat /sys/power/state
> freeze mem disk
> root@duo:~# cat /sys/power/disk
> [platform] shutdown reboot suspend
> root@duo:~#
>
>> Could you propose the acceptable format then?
>> The numbers alone are inconvenient to use, we need a human
>> readable description next to them. Or some another way
>> to obtain the name.
>
> Files like "0.active", "0.name", "1.active", "1.name"?
>
> Subdirectories 0/1 with "name" and "active" files?

We have flash_sync_strobe attribute which allows to read currently
chosen sync_led. I would simplify this:

Let's assume there are two LED Flash class deviecs:

- max77693-flash1
- max77693-flash2

Proposed design described by use cases:

#cd /sys/class/leds/max77693-flash1
#cat available_flash_leds
#[0.none] 1.max77693-flash2
#cat flash_sync_strobe
#0.none
#echo 1 > flash_sync_strobe
#cat flash_sync_strobe
#1.max77693-flash2
#cat available_sync_leds
#0.none [1.max77693-flash2]


-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH v2] Documentation: leds: Add description of LED Flash class extension

[Sakari Ailus]

Hi Jacek,

On Mon, Feb 16, 2015 at 12:18:48PM +0100, Jacek Anaszewski wrote:
> Hi Pavel,
> 
> On 02/12/2015 06:44 PM, Pavel Machek wrote:
> >Hi!
> >
> >>>>You acked LED Flash class patch, didn't you? :)
> >>>>
> >>>>There are many attributes documented in the list fashion, e.g.:
> >>>>available_frequencies in the Documentation/ABI/testing/sysfs-class-devfreq.
> >>>
> >>>No, sorry, you can't do this. We have this parsing nightmare in /proc,
> >>>before, and we don't want it again.
> >>>
> >>>>If we changed this a bit it would be easily parsed with AWK:
> >>>>
> >>>>echo "0 none;1 max77693-led1;2 max77693-led2" | awk -F';' '{ for (i=1;
> >>>>i<=NF; i++) print $i}' | awk '{print $1": "$2}'
> >>>
> >>>sysfs is one entry per file. If someone screwed it up in devfreq, it
> >>>is not reason to screw it up here. (Space separated lists of integers
> >>>might be acceptable. Fixed strings with one marked by []s happen,
> >>>too. Maps between ints and names are not.)
> >>
> >>"Fixed strings with one marked by []s happen" - I don't quite
> >>get this.
> >
> >root@duo:~# cat /sys/power/state
> >freeze mem disk
> >root@duo:~# cat /sys/power/disk
> >[platform] shutdown reboot suspend
> >root@duo:~#
> >
> >>Could you propose the acceptable format then?
> >>The numbers alone are inconvenient to use, we need a human
> >>readable description next to them. Or some another way
> >>to obtain the name.
> >
> >Files like "0.active", "0.name", "1.active", "1.name"?
> >
> >Subdirectories 0/1 with "name" and "active" files?
> 
> We have flash_sync_strobe attribute which allows to read currently
> chosen sync_led. I would simplify this:
> 
> Let's assume there are two LED Flash class deviecs:
> 
> - max77693-flash1
> - max77693-flash2
> 
> Proposed design described by use cases:
> 
> #cd /sys/class/leds/max77693-flash1
> #cat available_flash_leds
> #[0.none] 1.max77693-flash2
> #cat flash_sync_strobe
> #0.none
> #echo 1 > flash_sync_strobe
> #cat flash_sync_strobe
> #1.max77693-flash2
> #cat available_sync_leds
> #0.none [1.max77693-flash2]

I'd either drop the brackets from the selected LED, or the flash_sync_strobe
attribute in order to avoid redundancy in the interface. I slightly prefer
the former since it's easier to use, but I'm fine with the latter as well.

How about the number and the dot, where do they come from?

-- 
Kind regards,

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

Re: [PATCH v2] Documentation: leds: Add description of LED Flash class extension

[Jacek Anaszewski]

Hi Sakari,

On 02/16/2015 02:06 PM, Sakari Ailus wrote:
> Hi Jacek,
>
> On Mon, Feb 16, 2015 at 12:18:48PM +0100, Jacek Anaszewski wrote:
>> Hi Pavel,
>>
>> On 02/12/2015 06:44 PM, Pavel Machek wrote:
>>> Hi!
>>>
>>>>>> You acked LED Flash class patch, didn't you? :)
>>>>>>
>>>>>> There are many attributes documented in the list fashion, e.g.:
>>>>>> available_frequencies in the Documentation/ABI/testing/sysfs-class-devfreq.
>>>>>
>>>>> No, sorry, you can't do this. We have this parsing nightmare in /proc,
>>>>> before, and we don't want it again.
>>>>>
>>>>>> If we changed this a bit it would be easily parsed with AWK:
>>>>>>
>>>>>> echo "0 none;1 max77693-led1;2 max77693-led2" | awk -F';' '{ for (i=1;
>>>>>> i<=NF; i++) print $i}' | awk '{print $1": "$2}'
>>>>>
>>>>> sysfs is one entry per file. If someone screwed it up in devfreq, it
>>>>> is not reason to screw it up here. (Space separated lists of integers
>>>>> might be acceptable. Fixed strings with one marked by []s happen,
>>>>> too. Maps between ints and names are not.)
>>>>
>>>> "Fixed strings with one marked by []s happen" - I don't quite
>>>> get this.
>>>
>>> root@duo:~# cat /sys/power/state
>>> freeze mem disk
>>> root@duo:~# cat /sys/power/disk
>>> [platform] shutdown reboot suspend
>>> root@duo:~#
>>>
>>>> Could you propose the acceptable format then?
>>>> The numbers alone are inconvenient to use, we need a human
>>>> readable description next to them. Or some another way
>>>> to obtain the name.
>>>
>>> Files like "0.active", "0.name", "1.active", "1.name"?
>>>
>>> Subdirectories 0/1 with "name" and "active" files?
>>
>> We have flash_sync_strobe attribute which allows to read currently
>> chosen sync_led. I would simplify this:
>>
>> Let's assume there are two LED Flash class deviecs:
>>
>> - max77693-flash1
>> - max77693-flash2
>>
>> Proposed design described by use cases:
>>
>> #cd /sys/class/leds/max77693-flash1
>> #cat available_flash_leds
>> #[0.none] 1.max77693-flash2
>> #cat flash_sync_strobe
>> #0.none
>> #echo 1 > flash_sync_strobe
>> #cat flash_sync_strobe
>> #1.max77693-flash2
>> #cat available_sync_leds
>> #0.none [1.max77693-flash2]
>
> I'd either drop the brackets from the selected LED, or the flash_sync_strobe
> attribute in order to avoid redundancy in the interface. I slightly prefer
> the former since it's easier to use, but I'm fine with the latter as well.

I'm OK with droping the brackets.

> How about the number and the dot, where do they come from?

The number is the identifier of the flash LED in the sync_leds
array, the property of struct led_classdev_flash, incremented by 1.
It is concatenated, along with the dot, by the LED Flash class core
in the related sysfs attribute callback.

-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH v2] Documentation: leds: Add description of LED Flash class extension

[Pavel Machek]

Hi!

> >root@duo:~# cat /sys/power/state
> >freeze mem disk
> >root@duo:~# cat /sys/power/disk
> >[platform] shutdown reboot suspend
> >root@duo:~#
> >
> >>Could you propose the acceptable format then?
> >>The numbers alone are inconvenient to use, we need a human
> >>readable description next to them. Or some another way
> >>to obtain the name.
> >
> >Files like "0.active", "0.name", "1.active", "1.name"?
> >
> >Subdirectories 0/1 with "name" and "active" files?
> 
> We have flash_sync_strobe attribute which allows to read currently
> chosen sync_led. I would simplify this:
> 
> Let's assume there are two LED Flash class deviecs:
> 
> - max77693-flash1
> - max77693-flash2
> 
> Proposed design described by use cases:
> 
> #cd /sys/class/leds/max77693-flash1
> #cat available_flash_leds
> #[0.none] 1.max77693-flash2

Yeah. Should not have showed you /sys/power/state, right?

state gets away with this, because it uses few fixed strings. No funny
characters there, and it is "closely guarded". You don't have enough
control to do this... plus you have put the numbers there.

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