Re: 0.led_name 2.other.led.name in /sysfs Re: [PATCH/RFC v11 01/20] leds: flash: document sysfs interface

[Jacek Anaszewski <j.anaszewski@samsung.com>]

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.

-- 
Best Regards,
Jacek Anaszewski