| Message ID | 1424276441-3969-2-git-send-email-j.anaszewski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed 2015-02-18 17:20:22, Jacek Anaszewski wrote: > Add a documentation of LED Flash class specific 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> NAK-ed-by: Pavel Machek > +What: /sys/class/leds/<led>/available_sync_leds > +Date: February 2015 > +KernelVersion: 3.20 > +Contact: Jacek Anaszewski <j.anaszewski@samsung.com> > +Description: read/write > + Space separated list of LEDs available for flash strobe > + synchronization, displayed in the format: > + > + led1_id.led1_name led2_id.led2_name led3_id.led3_name etc. Multiple values per file, with all the problems we had in /proc. I assume led_id is an integer? What prevents space or dot in led name? Pavel
On 02/18/2015 11:47 PM, Pavel Machek wrote: > > On Wed 2015-02-18 17:20:22, Jacek Anaszewski wrote: >> Add a documentation of LED Flash class specific 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> > > NAK-ed-by: Pavel Machek > >> +What: /sys/class/leds/<led>/available_sync_leds >> +Date: February 2015 >> +KernelVersion: 3.20 >> +Contact: Jacek Anaszewski <j.anaszewski@samsung.com> >> +Description: read/write Here it should be 'read only', to be fixed. >> + Space separated list of LEDs available for flash strobe >> + synchronization, displayed in the format: >> + >> + led1_id.led1_name led2_id.led2_name led3_id.led3_name etc. > > Multiple values per file, with all the problems we had in /proc. I > assume led_id is an integer? Yes. > What prevents space or dot in led name? Space can be forbidden by defining naming convention. The name comes from the DT binding 'label' property and I don't see any problem in forbidding space in it. A dot in the name does not introduce parsing problems - simply the first dot after digits separates led id from led name.
On Wed, Feb 18, 2015 at 11:47:47PM +0100, Pavel Machek wrote: > > On Wed 2015-02-18 17:20:22, Jacek Anaszewski wrote: > > Add a documentation of LED Flash class specific 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> > > NAK-ed-by: Pavel Machek > > > +What: /sys/class/leds/<led>/available_sync_leds > > +Date: February 2015 > > +KernelVersion: 3.20 > > +Contact: Jacek Anaszewski <j.anaszewski@samsung.com> > > +Description: read/write > > + Space separated list of LEDs available for flash strobe > > + synchronization, displayed in the format: > > + > > + led1_id.led1_name led2_id.led2_name led3_id.led3_name etc. > > Multiple values per file, with all the problems we had in /proc. I > assume led_id is an integer? What prevents space or dot in led name? Very good point. How about using a newline instead? That'd be a little bit easier to parse, too.
On Thu, Feb 19, 2015 at 11:02:04AM +0200, Sakari Ailus wrote: > On Wed, Feb 18, 2015 at 11:47:47PM +0100, Pavel Machek wrote: > > > > On Wed 2015-02-18 17:20:22, Jacek Anaszewski wrote: > > > Add a documentation of LED Flash class specific 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> > > > > NAK-ed-by: Pavel Machek > > > > > +What: /sys/class/leds/<led>/available_sync_leds > > > +Date: February 2015 > > > +KernelVersion: 3.20 > > > +Contact: Jacek Anaszewski <j.anaszewski@samsung.com> > > > +Description: read/write > > > + Space separated list of LEDs available for flash strobe > > > + synchronization, displayed in the format: > > > + > > > + led1_id.led1_name led2_id.led2_name led3_id.led3_name etc. > > > > Multiple values per file, with all the problems we had in /proc. I > > assume led_id is an integer? What prevents space or dot in led name? > > Very good point. How about using a newline instead? That'd be a little bit > easier to parse, too. No, please make it one value per-file, which is what sysfs requires. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/19/2015 10:40 PM, Greg KH wrote: > On Thu, Feb 19, 2015 at 11:02:04AM +0200, Sakari Ailus wrote: >> On Wed, Feb 18, 2015 at 11:47:47PM +0100, Pavel Machek wrote: >>> >>> On Wed 2015-02-18 17:20:22, Jacek Anaszewski wrote: >>>> Add a documentation of LED Flash class specific 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> >>> >>> NAK-ed-by: Pavel Machek >>> >>>> +What: /sys/class/leds/<led>/available_sync_leds >>>> +Date: February 2015 >>>> +KernelVersion: 3.20 >>>> +Contact: Jacek Anaszewski <j.anaszewski@samsung.com> >>>> +Description: read/write >>>> + Space separated list of LEDs available for flash strobe >>>> + synchronization, displayed in the format: >>>> + >>>> + led1_id.led1_name led2_id.led2_name led3_id.led3_name etc. >>> >>> Multiple values per file, with all the problems we had in /proc. I >>> assume led_id is an integer? What prevents space or dot in led name? >> >> Very good point. How about using a newline instead? That'd be a little bit >> easier to parse, too. > > No, please make it one value per-file, which is what sysfs requires. The purpose of this attribute is only to provide an information about the range of valid identifiers that can be written to the flash_sync_strobe attribute. Wouldn't splitting this to many attributes be an unnecessary inflation of sysfs files? Apart from it, we have also flash_faults attribute, that currently provides a space separated list of flash faults that have occurred. If we are to stick tightly to the one-value-per-file rule, then how we should approach flash_faults case? Should the separate file be dynamically created for each reported fault?
Hi! > >>>>+What: /sys/class/leds/<led>/available_sync_leds > >>>>+Date: February 2015 > >>>>+KernelVersion: 3.20 > >>>>+Contact: Jacek Anaszewski <j.anaszewski@samsung.com> > >>>>+Description: read/write > >>>>+ Space separated list of LEDs available for flash strobe > >>>>+ synchronization, displayed in the format: > >>>>+ > >>>>+ led1_id.led1_name led2_id.led2_name led3_id.led3_name etc. > >>> > >>>Multiple values per file, with all the problems we had in /proc. I > >>>assume led_id is an integer? What prevents space or dot in led name? > >> > >>Very good point. How about using a newline instead? That'd be a little bit > >>easier to parse, too. > > > >No, please make it one value per-file, which is what sysfs requires. > > The purpose of this attribute is only to provide an information about > the range of valid identifiers that can be written to the > flash_sync_strobe attribute. Wouldn't splitting this to many attributes > be an unnecessary inflation of sysfs files? No, it would not. It is required so that we don't end up with broken parsers. > Apart from it, we have also flash_faults attribute, that currently > provides a space separated list of flash faults that have occurred. > If we are to stick tightly to the one-value-per-file rule, then how > we should approach flash_faults case? Should the separate file be > dynamically created for each reported fault? I think you can get away with flash_faults attribute (since the strings are hardcoded). Dynamically created files would be extremely ugly interface, but you could also have files such as "overvoltage_fault" containing either 0 or 1 ... Pavel
Hi Pavel, On 02/20/2015 09:16 AM, Pavel Machek wrote: > Hi! > >>>>>> +What: /sys/class/leds/<led>/available_sync_leds >>>>>> +Date: February 2015 >>>>>> +KernelVersion: 3.20 >>>>>> +Contact: Jacek Anaszewski <j.anaszewski@samsung.com> >>>>>> +Description: read/write >>>>>> + Space separated list of LEDs available for flash strobe >>>>>> + synchronization, displayed in the format: >>>>>> + >>>>>> + led1_id.led1_name led2_id.led2_name led3_id.led3_name etc. >>>>> >>>>> Multiple values per file, with all the problems we had in /proc. I >>>>> assume led_id is an integer? What prevents space or dot in led name? >>>> >>>> Very good point. How about using a newline instead? That'd be a little bit >>>> easier to parse, too. >>> >>> No, please make it one value per-file, which is what sysfs requires. >> >> The purpose of this attribute is only to provide an information about >> the range of valid identifiers that can be written to the >> flash_sync_strobe attribute. Wouldn't splitting this to many attributes >> be an unnecessary inflation of sysfs files? > > No, it would not. It is required so that we don't end up with broken > parsers. Let's discuss the acceptable approach then. I propose a directory named synchronized_strobe and containing the files as you proposed in one of the previous messages: led_id.active and led_id.name. The attribute flash_sync_strobe would be redundant then and should be removed. Use cases for two LEDs: - max77693-led1 - max77693-led2 #cd synchronized_strobe #ls #0.active 0.name 1.active 1.name #cat 0.name #max77693-led1 #cat 0.active #0 #cat 1.name #max77693-led2 #cat 1.active #0 #echo 1 > 0.active #cat 0.active #1 #echo 1 > 1.active #cat 0.active #0 #cat 1.active #1 >> Apart from it, we have also flash_faults attribute, that currently >> provides a space separated list of flash faults that have occurred. >> If we are to stick tightly to the one-value-per-file rule, then how >> we should approach flash_faults case? Should the separate file be >> dynamically created for each reported fault? > > I think you can get away with flash_faults attribute (since the > strings are hardcoded). If so, the attribute will be left as is. > Dynamically created files would be extremely ugly interface, but you > could also have files such as "overvoltage_fault" containing either 0 > or 1 ...
On Fri, Feb 20, 2015 at 08:56:11AM +0100, Jacek Anaszewski wrote: > On 02/19/2015 10:40 PM, Greg KH wrote: > >On Thu, Feb 19, 2015 at 11:02:04AM +0200, Sakari Ailus wrote: > >>On Wed, Feb 18, 2015 at 11:47:47PM +0100, Pavel Machek wrote: > >>> > >>>On Wed 2015-02-18 17:20:22, Jacek Anaszewski wrote: > >>>>Add a documentation of LED Flash class specific 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> > >>> > >>>NAK-ed-by: Pavel Machek > >>> > >>>>+What: /sys/class/leds/<led>/available_sync_leds > >>>>+Date: February 2015 > >>>>+KernelVersion: 3.20 > >>>>+Contact: Jacek Anaszewski <j.anaszewski@samsung.com> > >>>>+Description: read/write > >>>>+ Space separated list of LEDs available for flash strobe > >>>>+ synchronization, displayed in the format: > >>>>+ > >>>>+ led1_id.led1_name led2_id.led2_name led3_id.led3_name etc. > >>> > >>>Multiple values per file, with all the problems we had in /proc. I > >>>assume led_id is an integer? What prevents space or dot in led name? > >> > >>Very good point. How about using a newline instead? That'd be a little bit > >>easier to parse, too. > > > >No, please make it one value per-file, which is what sysfs requires. > > The purpose of this attribute is only to provide an information about > the range of valid identifiers that can be written to the > flash_sync_strobe attribute. Wouldn't splitting this to many attributes > be an unnecessary inflation of sysfs files? Ok a list of allowed values to write is acceptable, as long as it is not hard to parse and always is space separated. > Apart from it, we have also flash_faults attribute, that currently > provides a space separated list of flash faults that have occurred. That's crazy, what's to keep it from growing and growing to be larger than is allowed to be read? > If we are to stick tightly to the one-value-per-file rule, then how > we should approach flash_faults case? Should the separate file be > dynamically created for each reported fault? I think you need to use something other than sysfs here, sorry. uevents for your faults? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/20/2015 04:36 PM, Greg KH wrote: > On Fri, Feb 20, 2015 at 08:56:11AM +0100, Jacek Anaszewski wrote: >> On 02/19/2015 10:40 PM, Greg KH wrote: >>> On Thu, Feb 19, 2015 at 11:02:04AM +0200, Sakari Ailus wrote: >>>> On Wed, Feb 18, 2015 at 11:47:47PM +0100, Pavel Machek wrote: >>>>> >>>>> On Wed 2015-02-18 17:20:22, Jacek Anaszewski wrote: >>>>>> Add a documentation of LED Flash class specific 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> >>>>> >>>>> NAK-ed-by: Pavel Machek >>>>> >>>>>> +What: /sys/class/leds/<led>/available_sync_leds >>>>>> +Date: February 2015 >>>>>> +KernelVersion: 3.20 >>>>>> +Contact: Jacek Anaszewski <j.anaszewski@samsung.com> >>>>>> +Description: read/write >>>>>> + Space separated list of LEDs available for flash strobe >>>>>> + synchronization, displayed in the format: >>>>>> + >>>>>> + led1_id.led1_name led2_id.led2_name led3_id.led3_name etc. >>>>> >>>>> Multiple values per file, with all the problems we had in /proc. I >>>>> assume led_id is an integer? What prevents space or dot in led name? >>>> >>>> Very good point. How about using a newline instead? That'd be a little bit >>>> easier to parse, too. >>> >>> No, please make it one value per-file, which is what sysfs requires. >> >> The purpose of this attribute is only to provide an information about >> the range of valid identifiers that can be written to the >> flash_sync_strobe attribute. Wouldn't splitting this to many attributes >> be an unnecessary inflation of sysfs files? > > Ok a list of allowed values to write is acceptable, as long as it is not > hard to parse and always is space separated. Is a new line character also acceptable as a delimiter? >> Apart from it, we have also flash_faults attribute, that currently >> provides a space separated list of flash faults that have occurred. > > That's crazy, what's to keep it from growing and growing to be larger > than is allowed to be read? The number of possible faults is fixed to 9 currently. They are presented in the form of strings no longer currently than 40 characters. There can be maximum 9 faults reported at a time, this is not a kind of a log. This will allow to define roughly 100 types of faults, having that PAGE_SIZE is 4096. I think this is far more than it is conceivable for the simple LED flash device. >> If we are to stick tightly to the one-value-per-file rule, then how >> we should approach flash_faults case? Should the separate file be >> dynamically created for each reported fault? > > I think you need to use something other than sysfs here, sorry. > > uevents for your faults? > > thanks, > > greg k-h >
On Fri 2015-02-20 07:36:16, Greg KH wrote: > On Fri, Feb 20, 2015 at 08:56:11AM +0100, Jacek Anaszewski wrote: > > On 02/19/2015 10:40 PM, Greg KH wrote: > > >On Thu, Feb 19, 2015 at 11:02:04AM +0200, Sakari Ailus wrote: > > >>On Wed, Feb 18, 2015 at 11:47:47PM +0100, Pavel Machek wrote: > > >>> > > >>>On Wed 2015-02-18 17:20:22, Jacek Anaszewski wrote: > > >>>>Add a documentation of LED Flash class specific 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> > > >>> > > >>>NAK-ed-by: Pavel Machek > > >>> > > >>>>+What: /sys/class/leds/<led>/available_sync_leds > > >>>>+Date: February 2015 > > >>>>+KernelVersion: 3.20 > > >>>>+Contact: Jacek Anaszewski <j.anaszewski@samsung.com> > > >>>>+Description: read/write > > >>>>+ Space separated list of LEDs available for flash strobe > > >>>>+ synchronization, displayed in the format: > > >>>>+ > > >>>>+ led1_id.led1_name led2_id.led2_name led3_id.led3_name etc. > > >>> > > >>>Multiple values per file, with all the problems we had in /proc. I > > >>>assume led_id is an integer? What prevents space or dot in led name? > > >> > > >>Very good point. How about using a newline instead? That'd be a little bit > > >>easier to parse, too. > > > > > >No, please make it one value per-file, which is what sysfs requires. > > > > The purpose of this attribute is only to provide an information about > > the range of valid identifiers that can be written to the > > flash_sync_strobe attribute. Wouldn't splitting this to many attributes > > be an unnecessary inflation of sysfs files? > > Ok a list of allowed values to write is acceptable, as long as it is not > hard to parse and always is space separated. Well, this one is list of LED numbers and LED names. > > Apart from it, we have also flash_faults attribute, that currently > > provides a space separated list of flash faults that have occurred. > > That's crazy, what's to keep it from growing and growing to be larger > than is allowed to be read? Umm. Actually, this one is less crazy, I'd say. List of faults is fixed, and you can have them all-at-once, at most, which is way below 4K limit. Pavel
Hi Pavel and Greg, On Fri, Feb 20, 2015 at 09:57:38PM +0100, Pavel Machek wrote: > On Fri 2015-02-20 07:36:16, Greg KH wrote: > > On Fri, Feb 20, 2015 at 08:56:11AM +0100, Jacek Anaszewski wrote: > > > On 02/19/2015 10:40 PM, Greg KH wrote: > > > >On Thu, Feb 19, 2015 at 11:02:04AM +0200, Sakari Ailus wrote: > > > >>On Wed, Feb 18, 2015 at 11:47:47PM +0100, Pavel Machek wrote: > > > >>> > > > >>>On Wed 2015-02-18 17:20:22, Jacek Anaszewski wrote: > > > >>>>Add a documentation of LED Flash class specific 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> > > > >>> > > > >>>NAK-ed-by: Pavel Machek > > > >>> > > > >>>>+What: /sys/class/leds/<led>/available_sync_leds > > > >>>>+Date: February 2015 > > > >>>>+KernelVersion: 3.20 > > > >>>>+Contact: Jacek Anaszewski <j.anaszewski@samsung.com> > > > >>>>+Description: read/write > > > >>>>+ Space separated list of LEDs available for flash strobe > > > >>>>+ synchronization, displayed in the format: > > > >>>>+ > > > >>>>+ led1_id.led1_name led2_id.led2_name led3_id.led3_name etc. > > > >>> > > > >>>Multiple values per file, with all the problems we had in /proc. I > > > >>>assume led_id is an integer? What prevents space or dot in led name? > > > >> > > > >>Very good point. How about using a newline instead? That'd be a little bit > > > >>easier to parse, too. > > > > > > > >No, please make it one value per-file, which is what sysfs requires. > > > > > > The purpose of this attribute is only to provide an information about > > > the range of valid identifiers that can be written to the > > > flash_sync_strobe attribute. Wouldn't splitting this to many attributes > > > be an unnecessary inflation of sysfs files? > > > > Ok a list of allowed values to write is acceptable, as long as it is not > > hard to parse and always is space separated. > > Well, this one is list of LED numbers and LED names. It'd be nice if these names would match the V4L2 sub-device names. We don't have any rules for them other than they must be unique, and there's the established practice that an I2C address follows the component name. We're about to discuss the matter on Monday on #v4l (11:00 Finnish time), but I don't think we can generally guarantee any of the names won't have spaces. Separate files, then? > > > Apart from it, we have also flash_faults attribute, that currently > > > provides a space separated list of flash faults that have occurred. > > > > That's crazy, what's to keep it from growing and growing to be larger > > than is allowed to be read? > > Umm. Actually, this one is less crazy, I'd say. List of faults is > fixed, and you can have them all-at-once, at most, which is way below > 4K limit. We'd first run out of V4L2 bitmask control bits --- there are 32 of them. I'm frankly not really worried about that either.
On Sat, Feb 21, 2015 at 12:57:33PM +0200, Sakari Ailus wrote: > Hi Pavel and Greg, > > On Fri, Feb 20, 2015 at 09:57:38PM +0100, Pavel Machek wrote: > > On Fri 2015-02-20 07:36:16, Greg KH wrote: > > > On Fri, Feb 20, 2015 at 08:56:11AM +0100, Jacek Anaszewski wrote: > > > > On 02/19/2015 10:40 PM, Greg KH wrote: > > > > >On Thu, Feb 19, 2015 at 11:02:04AM +0200, Sakari Ailus wrote: > > > > >>On Wed, Feb 18, 2015 at 11:47:47PM +0100, Pavel Machek wrote: > > > > >>> > > > > >>>On Wed 2015-02-18 17:20:22, Jacek Anaszewski wrote: > > > > >>>>Add a documentation of LED Flash class specific 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> > > > > >>> > > > > >>>NAK-ed-by: Pavel Machek > > > > >>> > > > > >>>>+What: /sys/class/leds/<led>/available_sync_leds > > > > >>>>+Date: February 2015 > > > > >>>>+KernelVersion: 3.20 > > > > >>>>+Contact: Jacek Anaszewski <j.anaszewski@samsung.com> > > > > >>>>+Description: read/write > > > > >>>>+ Space separated list of LEDs available for flash strobe > > > > >>>>+ synchronization, displayed in the format: > > > > >>>>+ > > > > >>>>+ led1_id.led1_name led2_id.led2_name led3_id.led3_name etc. > > > > >>> > > > > >>>Multiple values per file, with all the problems we had in /proc. I > > > > >>>assume led_id is an integer? What prevents space or dot in led name? > > > > >> > > > > >>Very good point. How about using a newline instead? That'd be a little bit > > > > >>easier to parse, too. > > > > > > > > > >No, please make it one value per-file, which is what sysfs requires. > > > > > > > > The purpose of this attribute is only to provide an information about > > > > the range of valid identifiers that can be written to the > > > > flash_sync_strobe attribute. Wouldn't splitting this to many attributes > > > > be an unnecessary inflation of sysfs files? > > > > > > Ok a list of allowed values to write is acceptable, as long as it is not > > > hard to parse and always is space separated. > > > > Well, this one is list of LED numbers and LED names. > > It'd be nice if these names would match the V4L2 sub-device names. We don't > have any rules for them other than they must be unique, and there's the > established practice that an I2C address follows the component name. We're > about to discuss the matter on Monday on #v4l (11:00 Finnish time), but I > don't think we can generally guarantee any of the names won't have spaces. > > Separate files, then? Yes please. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 20, 2015 at 05:15:10PM +0100, Jacek Anaszewski wrote: > On 02/20/2015 04:36 PM, Greg KH wrote: > >On Fri, Feb 20, 2015 at 08:56:11AM +0100, Jacek Anaszewski wrote: > >>On 02/19/2015 10:40 PM, Greg KH wrote: > >>>On Thu, Feb 19, 2015 at 11:02:04AM +0200, Sakari Ailus wrote: > >>>>On Wed, Feb 18, 2015 at 11:47:47PM +0100, Pavel Machek wrote: > >>>>> > >>>>>On Wed 2015-02-18 17:20:22, Jacek Anaszewski wrote: > >>>>>>Add a documentation of LED Flash class specific 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> > >>>>> > >>>>>NAK-ed-by: Pavel Machek > >>>>> > >>>>>>+What: /sys/class/leds/<led>/available_sync_leds > >>>>>>+Date: February 2015 > >>>>>>+KernelVersion: 3.20 > >>>>>>+Contact: Jacek Anaszewski <j.anaszewski@samsung.com> > >>>>>>+Description: read/write > >>>>>>+ Space separated list of LEDs available for flash strobe > >>>>>>+ synchronization, displayed in the format: > >>>>>>+ > >>>>>>+ led1_id.led1_name led2_id.led2_name led3_id.led3_name etc. > >>>>> > >>>>>Multiple values per file, with all the problems we had in /proc. I > >>>>>assume led_id is an integer? What prevents space or dot in led name? > >>>> > >>>>Very good point. How about using a newline instead? That'd be a little bit > >>>>easier to parse, too. > >>> > >>>No, please make it one value per-file, which is what sysfs requires. > >> > >>The purpose of this attribute is only to provide an information about > >>the range of valid identifiers that can be written to the > >>flash_sync_strobe attribute. Wouldn't splitting this to many attributes > >>be an unnecessary inflation of sysfs files? > > > >Ok a list of allowed values to write is acceptable, as long as it is not > >hard to parse and always is space separated. > > Is a new line character also acceptable as a delimiter? No. Again, sysfs files should not need to be "parsed", they are one-value-per-file for a good reason. If you want to do something else, wonderful, but don't use sysfs. It's looking like this whole interface should not be using sysfs as it doesn't fit there at all. sorry, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Sakari, On 02/21/2015 11:57 AM, Sakari Ailus wrote: > Hi Pavel and Greg, > > On Fri, Feb 20, 2015 at 09:57:38PM +0100, Pavel Machek wrote: >> On Fri 2015-02-20 07:36:16, Greg KH wrote: >>> On Fri, Feb 20, 2015 at 08:56:11AM +0100, Jacek Anaszewski wrote: >>>> On 02/19/2015 10:40 PM, Greg KH wrote: >>>>> On Thu, Feb 19, 2015 at 11:02:04AM +0200, Sakari Ailus wrote: >>>>>> On Wed, Feb 18, 2015 at 11:47:47PM +0100, Pavel Machek wrote: >>>>>>> >>>>>>> On Wed 2015-02-18 17:20:22, Jacek Anaszewski wrote: >>>>>>>> Add a documentation of LED Flash class specific 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> >>>>>>> >>>>>>> NAK-ed-by: Pavel Machek >>>>>>> >>>>>>>> +What: /sys/class/leds/<led>/available_sync_leds >>>>>>>> +Date: February 2015 >>>>>>>> +KernelVersion: 3.20 >>>>>>>> +Contact: Jacek Anaszewski <j.anaszewski@samsung.com> >>>>>>>> +Description: read/write >>>>>>>> + Space separated list of LEDs available for flash strobe >>>>>>>> + synchronization, displayed in the format: >>>>>>>> + >>>>>>>> + led1_id.led1_name led2_id.led2_name led3_id.led3_name etc. >>>>>>> >>>>>>> Multiple values per file, with all the problems we had in /proc. I >>>>>>> assume led_id is an integer? What prevents space or dot in led name? >>>>>> >>>>>> Very good point. How about using a newline instead? That'd be a little bit >>>>>> easier to parse, too. >>>>> >>>>> No, please make it one value per-file, which is what sysfs requires. >>>> >>>> The purpose of this attribute is only to provide an information about >>>> the range of valid identifiers that can be written to the >>>> flash_sync_strobe attribute. Wouldn't splitting this to many attributes >>>> be an unnecessary inflation of sysfs files? >>> >>> Ok a list of allowed values to write is acceptable, as long as it is not >>> hard to parse and always is space separated. >> >> Well, this one is list of LED numbers and LED names. > > It'd be nice if these names would match the V4L2 sub-device names. We don't From the discussion on IRC it turned out that one of components of the V4L2 sub-device name will be a media controller identifier. This implies that if support for V4L2 Flash devices will be turned off in the kernel config the LED name will have to differ from the case when the support is on. I think that this is undesired. > have any rules for them other than they must be unique, and there's the > established practice that an I2C address follows the component name. We're > about to discuss the matter on Monday on #v4l (11:00 Finnish time), but I > don't think we can generally guarantee any of the names won't have spaces. > Separate files, then? I tried to split this to separate files but it turned out to be awkward. Since the number of LEDs to synchronize can vary from device to device, the number of the related sysfs attributes cannot be fixed. As far as I know allocating the sysfs attributes dynamically is unsafe, and thus the maximum allowed number of synchronized LEDs would have to be agreed on for the whole led-class-flash and the relevant number of similar struct attribute instances and related callbacks would have to be created statically for every LED Flash class device, no matter if a device would need them. Of course the relevant sysfs group could be initialized only with the needed number of sync leds attributes, but still this is less than optimal design. It looks like this interface indeed doesn't fit for sysfs. I am leaning towards removing the support for synchronized flash LEDs from the LED subsystem entirely and leave it only to V4L2.
Hi Jacek, On Fri, Feb 27, 2015 at 03:34:22PM +0100, Jacek Anaszewski wrote: > Hi Sakari, > > On 02/21/2015 11:57 AM, Sakari Ailus wrote: > >Hi Pavel and Greg, > > > >On Fri, Feb 20, 2015 at 09:57:38PM +0100, Pavel Machek wrote: > >>On Fri 2015-02-20 07:36:16, Greg KH wrote: > >>>On Fri, Feb 20, 2015 at 08:56:11AM +0100, Jacek Anaszewski wrote: > >>>>On 02/19/2015 10:40 PM, Greg KH wrote: > >>>>>On Thu, Feb 19, 2015 at 11:02:04AM +0200, Sakari Ailus wrote: > >>>>>>On Wed, Feb 18, 2015 at 11:47:47PM +0100, Pavel Machek wrote: > >>>>>>> > >>>>>>>On Wed 2015-02-18 17:20:22, Jacek Anaszewski wrote: > >>>>>>>>Add a documentation of LED Flash class specific 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> > >>>>>>> > >>>>>>>NAK-ed-by: Pavel Machek > >>>>>>> > >>>>>>>>+What: /sys/class/leds/<led>/available_sync_leds > >>>>>>>>+Date: February 2015 > >>>>>>>>+KernelVersion: 3.20 > >>>>>>>>+Contact: Jacek Anaszewski <j.anaszewski@samsung.com> > >>>>>>>>+Description: read/write > >>>>>>>>+ Space separated list of LEDs available for flash strobe > >>>>>>>>+ synchronization, displayed in the format: > >>>>>>>>+ > >>>>>>>>+ led1_id.led1_name led2_id.led2_name led3_id.led3_name etc. > >>>>>>> > >>>>>>>Multiple values per file, with all the problems we had in /proc. I > >>>>>>>assume led_id is an integer? What prevents space or dot in led name? > >>>>>> > >>>>>>Very good point. How about using a newline instead? That'd be a little bit > >>>>>>easier to parse, too. > >>>>> > >>>>>No, please make it one value per-file, which is what sysfs requires. > >>>> > >>>>The purpose of this attribute is only to provide an information about > >>>>the range of valid identifiers that can be written to the > >>>>flash_sync_strobe attribute. Wouldn't splitting this to many attributes > >>>>be an unnecessary inflation of sysfs files? > >>> > >>>Ok a list of allowed values to write is acceptable, as long as it is not > >>>hard to parse and always is space separated. > >> > >>Well, this one is list of LED numbers and LED names. > > > >It'd be nice if these names would match the V4L2 sub-device names. We don't > > From the discussion on IRC it turned out that one of components of the > V4L2 sub-device name will be a media controller identifier. > > This implies that if support for V4L2 Flash devices will be turned off > in the kernel config the LED name will have to differ from the case > when the support is on. I think that this is undesired. Well... the media entity names need to be unique in the Media controller device. In the future we may have just a single Media controller device in the system, possibly depending on the driver so that some drivers can make use of that while some will have one on their own, mostly older drivers that is. I think what Laurent proposed to refer to an ID was the hardware device, so that in the future the hardware device / media entity name would be unique. That'd be a much more manageable and easier to verify for correctness than a global name that is defined by a driver. Older drivers wouldn't be affected. Old user space might not work with new drivers without taking the hwdev field into account. So the hwdev (name or ID) would be part of the struct media_entity_desc, but *not* a part of the name field in the struct. Cc Laurent and Hans. > >have any rules for them other than they must be unique, and there's the > >established practice that an I2C address follows the component name. We're > >about to discuss the matter on Monday on #v4l (11:00 Finnish time), but I > >don't think we can generally guarantee any of the names won't have spaces. > > >Separate files, then? > > I tried to split this to separate files but it turned out to be awkward. > Since the number of LEDs to synchronize can vary from device to device, > the number of the related sysfs attributes cannot be fixed. > > As far as I know allocating the sysfs attributes dynamically is unsafe, How so? I think most implementations use static variables because that's all they need. > and thus the maximum allowed number of synchronized LEDs would have to > be agreed on for the whole led-class-flash and the relevant number of > similar struct attribute instances and related callbacks would have to > be created statically for every LED Flash class device, no matter if > a device would need them. This could be handled in the framework instead. > Of course the relevant sysfs group could be initialized only with > the needed number of sync leds attributes, but still this is less > than optimal design. > > It looks like this interface indeed doesn't fit for sysfs. > > I am leaning towards removing the support for synchronized flash LEDs > from the LED subsystem entirely and leave it only to V4L2. Perfectly fine for me as well, I guess the synchronised strobe has mostly use on V4L2. It could always be added later on if needed.
On 03/02/2015 01:54 PM, Sakari Ailus wrote: > Hi Jacek, > > On Fri, Feb 27, 2015 at 03:34:22PM +0100, Jacek Anaszewski wrote: >> Hi Sakari, >> >> On 02/21/2015 11:57 AM, Sakari Ailus wrote: >>> Hi Pavel and Greg, >>> >>> On Fri, Feb 20, 2015 at 09:57:38PM +0100, Pavel Machek wrote: >>>> On Fri 2015-02-20 07:36:16, Greg KH wrote: >>>>> On Fri, Feb 20, 2015 at 08:56:11AM +0100, Jacek Anaszewski wrote: >>>>>> On 02/19/2015 10:40 PM, Greg KH wrote: >>>>>>> On Thu, Feb 19, 2015 at 11:02:04AM +0200, Sakari Ailus wrote: >>>>>>>> On Wed, Feb 18, 2015 at 11:47:47PM +0100, Pavel Machek wrote: >>>>>>>>> >>>>>>>>> On Wed 2015-02-18 17:20:22, Jacek Anaszewski wrote: >>>>>>>>>> Add a documentation of LED Flash class specific 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> >>>>>>>>> >>>>>>>>> NAK-ed-by: Pavel Machek >>>>>>>>> >>>>>>>>>> +What: /sys/class/leds/<led>/available_sync_leds >>>>>>>>>> +Date: February 2015 >>>>>>>>>> +KernelVersion: 3.20 >>>>>>>>>> +Contact: Jacek Anaszewski <j.anaszewski@samsung.com> >>>>>>>>>> +Description: read/write >>>>>>>>>> + Space separated list of LEDs available for flash strobe >>>>>>>>>> + synchronization, displayed in the format: >>>>>>>>>> + >>>>>>>>>> + led1_id.led1_name led2_id.led2_name led3_id.led3_name etc. >>>>>>>>> >>>>>>>>> Multiple values per file, with all the problems we had in /proc. I >>>>>>>>> assume led_id is an integer? What prevents space or dot in led name? >>>>>>>> >>>>>>>> Very good point. How about using a newline instead? That'd be a little bit >>>>>>>> easier to parse, too. >>>>>>> >>>>>>> No, please make it one value per-file, which is what sysfs requires. >>>>>> >>>>>> The purpose of this attribute is only to provide an information about >>>>>> the range of valid identifiers that can be written to the >>>>>> flash_sync_strobe attribute. Wouldn't splitting this to many attributes >>>>>> be an unnecessary inflation of sysfs files? >>>>> >>>>> Ok a list of allowed values to write is acceptable, as long as it is not >>>>> hard to parse and always is space separated. >>>> >>>> Well, this one is list of LED numbers and LED names. >>> >>> It'd be nice if these names would match the V4L2 sub-device names. We don't >> >> From the discussion on IRC it turned out that one of components of the >> V4L2 sub-device name will be a media controller identifier. >> >> This implies that if support for V4L2 Flash devices will be turned off >> in the kernel config the LED name will have to differ from the case >> when the support is on. I think that this is undesired. > > Well... the media entity names need to be unique in the Media controller > device. In the future we may have just a single Media controller device in > the system, possibly depending on the driver so that some drivers can make > use of that while some will have one on their own, mostly older drivers that > is. > > I think what Laurent proposed to refer to an ID was the hardware device, so > that in the future the hardware device / media entity name would be unique. > That'd be a much more manageable and easier to verify for correctness than a > global name that is defined by a driver. > > Older drivers wouldn't be affected. Old user space might not work with new > drivers without taking the hwdev field into account. > > So the hwdev (name or ID) would be part of the struct media_entity_desc, but > *not* a part of the name field in the struct. The origin of this discussion was your statement: >>> It'd be nice if these names would match the V4L2 sub-device names. >>> We don't have any rules for them other than they must be unique, >>> and there's the established practice that an I2C address follows >>> the component name. Has the naming scheme been already agreed? > Cc Laurent and Hans. > >>> have any rules for them other than they must be unique, and there's the >>> established practice that an I2C address follows the component name. We're >>> about to discuss the matter on Monday on #v4l (11:00 Finnish time), but I >>> don't think we can generally guarantee any of the names won't have spaces. >> >>> Separate files, then? >> >> I tried to split this to separate files but it turned out to be awkward. >> Since the number of LEDs to synchronize can vary from device to device, >> the number of the related sysfs attributes cannot be fixed. >> >> As far as I know allocating the sysfs attributes dynamically is unsafe, > > How so? I think most implementations use static variables because that's all > they need. I was thinking about the need for freeing the memory allocated for attributes on remove and races with udev. >> and thus the maximum allowed number of synchronized LEDs would have to >> be agreed on for the whole led-class-flash and the relevant number of >> similar struct attribute instances and related callbacks would have to >> be created statically for every LED Flash class device, no matter if >> a device would need them. > > This could be handled in the framework instead. > >> Of course the relevant sysfs group could be initialized only with >> the needed number of sync leds attributes, but still this is less >> than optimal design. >> >> It looks like this interface indeed doesn't fit for sysfs. >> >> I am leaning towards removing the support for synchronized flash LEDs >> from the LED subsystem entirely and leave it only to V4L2. > > Perfectly fine for me as well, I guess the synchronised strobe has mostly > use on V4L2. It could always be added later on if needed. I think that as I have it implemented and tested for LED subsystem it will cost not too much to just move it to v4l2-flash sub-device.
Hi! > > Of course the relevant sysfs group could be initialized only with > > the needed number of sync leds attributes, but still this is less > > than optimal design. > > > > It looks like this interface indeed doesn't fit for sysfs. > > > > I am leaning towards removing the support for synchronized flash LEDs > > from the LED subsystem entirely and leave it only to V4L2. > > Perfectly fine for me as well, I guess the synchronised strobe has mostly > use on V4L2. It could always be added later on if needed. Makes sense... Pavel
diff --git a/Documentation/ABI/testing/sysfs-class-led-flash b/Documentation/ABI/testing/sysfs-class-led-flash new file mode 100644 index 0000000..c941d21 --- /dev/null +++ b/Documentation/ABI/testing/sysfs-class-led-flash @@ -0,0 +1,104 @@ +What: /sys/class/leds/<led>/flash_brightness +Date: February 2015 +KernelVersion: 3.20 +Contact: Jacek Anaszewski <j.anaszewski@samsung.com> +Description: read/write + Set the brightness of this LED in the flash strobe mode, in + microamperes. The file is created only for the flash LED devices + that support setting flash brightness. + + The value is between 0 and + /sys/class/leds/<led>/max_flash_brightness. + +What: /sys/class/leds/<led>/max_flash_brightness +Date: February 2015 +KernelVersion: 3.20 +Contact: Jacek Anaszewski <j.anaszewski@samsung.com> +Description: read only + Maximum brightness level for this LED in the flash strobe mode, + in microamperes. + +What: /sys/class/leds/<led>/flash_timeout +Date: February 2015 +KernelVersion: 3.20 +Contact: Jacek Anaszewski <j.anaszewski@samsung.com> +Description: read/write + Hardware timeout for flash, in microseconds. The flash strobe + is stopped after this period of time has passed from the start + of the strobe. The file is created only for the flash LED + devices that support setting flash timeout. + +What: /sys/class/leds/<led>/max_flash_timeout +Date: February 2015 +KernelVersion: 3.20 +Contact: Jacek Anaszewski <j.anaszewski@samsung.com> +Description: read only + Maximum flash timeout for this LED, in microseconds. + +What: /sys/class/leds/<led>/flash_strobe +Date: February 2015 +KernelVersion: 3.20 +Contact: Jacek Anaszewski <j.anaszewski@samsung.com> +Description: read/write + Flash strobe state. When written with 1 it triggers flash strobe + and when written with 0 it turns the flash off. + + On read 1 means that flash is currently strobing and 0 means + that flash is off. + +What: /sys/class/leds/<led>/flash_sync_strobe +Date: February 2015 +KernelVersion: 3.20 +Contact: Jacek Anaszewski <j.anaszewski@samsung.com> +Description: read/write + Identifier of the LED to synchronize the flash strobe with. + 0 stands for no synchronization. Usually the LEDs available for + flash strobing are driven by the same flash LED device. The LEDs + available for flash strobe synchronization can be obtained by + reading the /sys/class/leds/<led>/available_sync_leds attribute. + + On read the currently selected LED is displayed in the format: + led_id.led_name + +What: /sys/class/leds/<led>/available_sync_leds +Date: February 2015 +KernelVersion: 3.20 +Contact: Jacek Anaszewski <j.anaszewski@samsung.com> +Description: read/write + Space separated list of LEDs available for flash strobe + synchronization, displayed in the format: + + led1_id.led1_name led2_id.led2_name led3_id.led3_name etc. + +What: /sys/class/leds/<led>/flash_fault +Date: February 2015 +KernelVersion: 3.20 +Contact: Jacek Anaszewski <j.anaszewski@samsung.com> +Description: read only + Space separated list of flash faults that may have occurred. + Flash faults are re-read after strobing the flash. Possible + flash faults: + + * led-over-voltage - flash controller voltage to the flash LED + has exceeded the 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