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@xxxxxxxxxxx> > >>>>>>>>Acked-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx> > >>>>>>>>Cc: Bryan Wu <cooloney@xxxxxxxxx> > >>>>>>>>Cc: Richard Purdie <rpurdie@xxxxxxxxx> > >>>>>>> > >>>>>>>NAK-ed-by: Pavel Machek > >>>>>>> > >>>>>>>>+What: /sys/class/leds/<led>/available_sync_leds > >>>>>>>>+Date: February 2015 > >>>>>>>>+KernelVersion: 3.20 > >>>>>>>>+Contact: Jacek Anaszewski <j.anaszewski@xxxxxxxxxxx> > >>>>>>>>+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. -- Kind regards, Sakari Ailus e-mail: sakari.ailus@xxxxxx XMPP: sailus@xxxxxxxxxxxxxx -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
