| Message ID | 1417166286-27685-3-git-send-email-j.anaszewski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi! > +Flash LED handling under Linux > +============================== > + > +Some LED devices support two modes - torch and flash. The modes are > +supported by the LED class (see Documentation/leds/leds-class.txt) > +and LED Flash class respectively. > + > +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 to gain flash > +capabilities. > + > +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) > + - indicator_brightness - privacy LED brightness in microamperes (RW) > + - max_indicator_brightness - maximum privacy LED brightness in > + microamperes (RO) > + - flash_timeout - flash strobe duration in microseconds (RW) > + - max_flash_timeout - maximum available flash strobe duration (RO) > + - flash_strobe - flash strobe state (RW) > + - flash_sync_strobe - one flash device can control more than one > + sub-led; when this atrribute is set to 1 > + the flash led will be strobed synchronously > + with the other ones controlled by the same > + device (RW) This is not really clear. Does flash_timeout or flash_brightness need to be set, first? Do we really want to have separate indicator brightnesses in uA? Should we maybe reuse existing "brightness" parameter for torch and indication, maybe adding single (RO) indicator_brightness attribute? > + - flash_fault - bitmask of flash faults that may have occurred, > + possible flags are: > + * 0x01 - flash controller voltage to the flash LED has exceeded > + the limit specific to the flash controller > + * 0x02 - 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 > + * 0x04 - the flash controller has overheated > + * 0x08 - the short circuit protection of the flash controller > + has been triggered > + * 0x10 - current in the LED power supply has exceeded the limit > + specific to the flash controller > + * 0x40 - flash controller voltage to the flash LED has been > + below the minimum limit specific to the flash > + * 0x80 - 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 > + * 0x100 - the temperature of the LED has exceeded its allowed > + upper limit How are faults cleared? Should it be list of strings, instead of bitmask? We may want to add new fault modes in future... Pavel
Hi Pavel, Thanks for a review. On 11/29/2014 01:58 PM, Pavel Machek wrote: > Hi! > >> +Flash LED handling under Linux >> +============================== >> + >> +Some LED devices support two modes - torch and flash. The modes are >> +supported by the LED class (see Documentation/leds/leds-class.txt) >> +and LED Flash class respectively. >> + >> +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 to gain flash >> +capabilities. >> + >> +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) >> + - indicator_brightness - privacy LED brightness in microamperes (RW) >> + - max_indicator_brightness - maximum privacy LED brightness in >> + microamperes (RO) >> + - flash_timeout - flash strobe duration in microseconds (RW) >> + - max_flash_timeout - maximum available flash strobe duration (RO) >> + - flash_strobe - flash strobe state (RW) >> + - flash_sync_strobe - one flash device can control more than one >> + sub-led; when this atrribute is set to 1 >> + the flash led will be strobed synchronously >> + with the other ones controlled by the same >> + device (RW) > > This is not really clear. Does flash_timeout or flash_brightness need > to be set, first? I would go for inheriting the settings from the led that is strobed explicitly. Limits regarding current, the ones from device tree node, would have to however be preserved in my opinion. A consensus is needed here. > Do we really want to have separate indicator brightnesses in uA? > Should we maybe reuse existing "brightness" parameter for torch and > indication, maybe adding single (RO) indicator_brightness attribute? I forgot to remove the indicator related positions. It has been definitely removed from the LED subsystem related patches. >> + - flash_fault - bitmask of flash faults that may have occurred, >> + possible flags are: >> + * 0x01 - flash controller voltage to the flash LED has exceeded >> + the limit specific to the flash controller >> + * 0x02 - 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 >> + * 0x04 - the flash controller has overheated >> + * 0x08 - the short circuit protection of the flash controller >> + has been triggered >> + * 0x10 - current in the LED power supply has exceeded the limit >> + specific to the flash controller >> + * 0x40 - flash controller voltage to the flash LED has been >> + below the minimum limit specific to the flash >> + * 0x80 - 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 >> + * 0x100 - the temperature of the LED has exceeded its allowed >> + upper limit > > How are faults cleared? Should it be list of strings, instead of > bitmask? We may want to add new fault modes in future... Faults are cleared by reading the attribute. I will add this note. There can be more than one fault at a time. I think that the bitmask is a flexible solution. I don't see any troubles related to adding new fault modes in the future, do you? Best Regards, Jacek Anaszewski -- 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! > >How are faults cleared? Should it be list of strings, instead of > >bitmask? We may want to add new fault modes in future... > > Faults are cleared by reading the attribute. I will add this note. > There can be more than one fault at a time. I think that the bitmask > is a flexible solution. I don't see any troubles related to adding > new fault modes in the future, do you? I do not think that "read attribute to clear" is good idea. Normally, you'd want the error attribute world-readable, but you don't want non-root users to clear the errors. I am not sure if bitmask is good solution. I'd return space-separated strings like "overtemp". That way, there's good chance that other LED drivers would be able to use similar interface... Best regards, Pavel
Hi Pavel, On 12/01/2014 02:04 PM, Pavel Machek wrote: > Hi! > >>> How are faults cleared? Should it be list of strings, instead of >>> bitmask? We may want to add new fault modes in future... >> >> Faults are cleared by reading the attribute. I will add this note. >> There can be more than one fault at a time. I think that the bitmask >> is a flexible solution. I don't see any troubles related to adding >> new fault modes in the future, do you? > > I do not think that "read attribute to clear" is good idea. Normally, > you'd want the error attribute world-readable, but you don't want > non-root users to clear the errors. This is also V4L2_CID_FLASH_FAULT control semantics. Moreover many devices clear the errors upon reading register. I don't see anything wrong in the fact that an user can clear an error. If the user has a permission to use a device then it also should be allowed to clear the errors. > I am not sure if bitmask is good solution. I'd return space-separated > strings like "overtemp". That way, there's good chance that other LED > drivers would be able to use similar interface... The format of a sysfs attribute should be concise. The error codes are generic and map directly to the V4L2 Flash error codes. Best Regards, Jacek Anaszewski -- 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 Jacek and Pavel, On Mon, Dec 01, 2014 at 02:58:56PM +0100, Jacek Anaszewski wrote: > Hi Pavel, > > On 12/01/2014 02:04 PM, Pavel Machek wrote: > >Hi! > > > >>>How are faults cleared? Should it be list of strings, instead of > >>>bitmask? We may want to add new fault modes in future... > >> > >>Faults are cleared by reading the attribute. I will add this note. > >>There can be more than one fault at a time. I think that the bitmask > >>is a flexible solution. I don't see any troubles related to adding > >>new fault modes in the future, do you? > > > >I do not think that "read attribute to clear" is good idea. Normally, > >you'd want the error attribute world-readable, but you don't want > >non-root users to clear the errors. > > This is also V4L2_CID_FLASH_FAULT control semantics. > Moreover many devices clear the errors upon reading register. > I don't see anything wrong in the fact that an user can clear > an error. If the user has a permission to use a device then > it also should be allowed to clear the errors. I agree. Some of these such as the timeout are not hardware related problems at all, but others may be. I'd keep the same semantics as V4L2 already does. > >I am not sure if bitmask is good solution. I'd return space-separated > >strings like "overtemp". That way, there's good chance that other LED > >drivers would be able to use similar interface... > > The format of a sysfs attribute should be concise. > The error codes are generic and map directly to the V4L2 Flash > error codes. I'd guess a single application accesses either of the interfaces. From that point of view it doesn't matter what are the bit definitions in V4L2. The behaviour on sysfs could also be different, e.g. writing the attribute would clear the faults. This would need more functionality from drivers. The V4L2 behaviour mirrors the typical behaviour of flash controllers --- the chips mostly do not operate until the faults have been read, and the interface as well as the interface take no stance to that. So when the user reads the fault control value, the fault register on the chip is cleared as well.
On Mon, Dec 1, 2014 at 5:58 AM, Jacek Anaszewski <j.anaszewski@samsung.com> wrote: > Hi Pavel, > > On 12/01/2014 02:04 PM, Pavel Machek wrote: >> >> Hi! >> >>>> How are faults cleared? Should it be list of strings, instead of >>>> bitmask? We may want to add new fault modes in future... >>> >>> >>> Faults are cleared by reading the attribute. I will add this note. >>> There can be more than one fault at a time. I think that the bitmask >>> is a flexible solution. I don't see any troubles related to adding >>> new fault modes in the future, do you? >> >> >> I do not think that "read attribute to clear" is good idea. Normally, >> you'd want the error attribute world-readable, but you don't want >> non-root users to clear the errors. > > > This is also V4L2_CID_FLASH_FAULT control semantics. > Moreover many devices clear the errors upon reading register. > I don't see anything wrong in the fact that an user can clear > an error. If the user has a permission to use a device then > it also should be allowed to clear the errors. > >> I am not sure if bitmask is good solution. I'd return space-separated >> strings like "overtemp". That way, there's good chance that other LED >> drivers would be able to use similar interface... > > > The format of a sysfs attribute should be concise. > The error codes are generic and map directly to the V4L2 Flash > error codes. > Actually I'd like to see those flash fault code defined in LED subsystem. And V4L2 will just include LED flash header file to use it. Because flash fault code is not for V4L2 specific but it's a feature of LED flash devices. For clearing error code of flash devices, I think it depends on the hardware. If most of our LED flash is using reading to clear error code, we probably can make it simple as this now. But what if some other LED flash devices are using writing to clear error code? we should provide a API to that? -Bryan -- 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
> > The format of a sysfs attribute should be concise. > > The error codes are generic and map directly to the V4L2 Flash > > error codes. > > > > Actually I'd like to see those flash fault code defined in LED > subsystem. And V4L2 will just include LED flash header file to use it. > Because flash fault code is not for V4L2 specific but it's a feature > of LED flash devices. > > For clearing error code of flash devices, I think it depends on the > hardware. If most of our LED flash is using reading to clear error > code, we probably can make it simple as this now. But what if some > other LED flash devices are using writing to clear error code? we > should provide a API to that? Actually, we should provide API that makes sense, and that is easy to use by userspace. I believe "read" is called read because it does not change anything, and it should stay that way in /sysfs. You may want to talk to sysfs maintainers if you plan on doing another semantics. Pavel
On 12/06/2014 01:43 PM, Pavel Machek wrote: > >>> The format of a sysfs attribute should be concise. >>> The error codes are generic and map directly to the V4L2 Flash >>> error codes. >>> >> >> Actually I'd like to see those flash fault code defined in LED >> subsystem. And V4L2 will just include LED flash header file to use it. >> Because flash fault code is not for V4L2 specific but it's a feature >> of LED flash devices. >> >> For clearing error code of flash devices, I think it depends on the >> hardware. If most of our LED flash is using reading to clear error >> code, we probably can make it simple as this now. But what if some >> other LED flash devices are using writing to clear error code? we >> should provide a API to that? > > Actually, we should provide API that makes sense, and that is easy to > use by userspace. > > I believe "read" is called read because it does not change anything, > and it should stay that way in /sysfs. You may want to talk to sysfs > maintainers if you plan on doing another semantics. How would you proceed in case of devices which clear their fault register upon I2C readout (e.g. AS3645)? In this case read does have a side effect. For such devices attribute semantics would have to be different than for the devices which don't clear faults on readout. In case of devices which use writing to clear error code - I'd do that after reading flash_fault attribute, in the same callback. Best Regards, Jacek Anaszewski -- 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 Mon 2014-12-08 17:55:20, Jacek Anaszewski wrote: > On 12/06/2014 01:43 PM, Pavel Machek wrote: > > > >>>The format of a sysfs attribute should be concise. > >>>The error codes are generic and map directly to the V4L2 Flash > >>>error codes. > >>> > >> > >>Actually I'd like to see those flash fault code defined in LED > >>subsystem. And V4L2 will just include LED flash header file to use it. > >>Because flash fault code is not for V4L2 specific but it's a feature > >>of LED flash devices. > >> > >>For clearing error code of flash devices, I think it depends on the > >>hardware. If most of our LED flash is using reading to clear error > >>code, we probably can make it simple as this now. But what if some > >>other LED flash devices are using writing to clear error code? we > >>should provide a API to that? > > > >Actually, we should provide API that makes sense, and that is easy to > >use by userspace. > > > >I believe "read" is called read because it does not change anything, > >and it should stay that way in /sysfs. You may want to talk to sysfs > >maintainers if you plan on doing another semantics. > > How would you proceed in case of devices which clear their fault > register upon I2C readout (e.g. AS3645)? In this case read does have > a side effect. For such devices attribute semantics would have to be > different than for the devices which don't clear faults on readout. No, semantics should be same for all devices. If device clears fault register during I2C readout, kernel will simply gather faults in an variable, and clear them upon write to sysfs file. Best regards, Pavel
Hi Pavel, On 12/08/2014 09:18 PM, Pavel Machek wrote: > On Mon 2014-12-08 17:55:20, Jacek Anaszewski wrote: >> On 12/06/2014 01:43 PM, Pavel Machek wrote: >>> >>>>> The format of a sysfs attribute should be concise. >>>>> The error codes are generic and map directly to the V4L2 Flash >>>>> error codes. >>>>> >>>> >>>> Actually I'd like to see those flash fault code defined in LED >>>> subsystem. And V4L2 will just include LED flash header file to use it. >>>> Because flash fault code is not for V4L2 specific but it's a feature >>>> of LED flash devices. >>>> >>>> For clearing error code of flash devices, I think it depends on the >>>> hardware. If most of our LED flash is using reading to clear error >>>> code, we probably can make it simple as this now. But what if some >>>> other LED flash devices are using writing to clear error code? we >>>> should provide a API to that? >>> >>> Actually, we should provide API that makes sense, and that is easy to >>> use by userspace. >>> >>> I believe "read" is called read because it does not change anything, >>> and it should stay that way in /sysfs. You may want to talk to sysfs >>> maintainers if you plan on doing another semantics. >> >> How would you proceed in case of devices which clear their fault >> register upon I2C readout (e.g. AS3645)? In this case read does have >> a side effect. For such devices attribute semantics would have to be >> different than for the devices which don't clear faults on readout. > > No, semantics should be same for all devices. > > If device clears fault register during I2C readout, kernel will simply > gather faults in an variable, and clear them upon write to sysfs file. This approach would require implementing additional mechanisms on both sides: LED Flash class core and a LED Flash class driver. In the former the sysfs attribute write permissions would have to be decided in the runtime and in the latter caching mechanism would have to be implemented per driver. We would have to also consider how to approach the issue in case of sub-leds. The only reason for this overhead is trying to avoid side effects on reading sysfs attribute. After weighing the pros and cons, I am not sure if it is worthwhile. Best Regards, Jacek Anaszewski -- 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 Tue 2014-12-09 09:54:06, Jacek Anaszewski wrote: > Hi Pavel, > > On 12/08/2014 09:18 PM, Pavel Machek wrote: > >On Mon 2014-12-08 17:55:20, Jacek Anaszewski wrote: > >>On 12/06/2014 01:43 PM, Pavel Machek wrote: > >>> > >>>>>The format of a sysfs attribute should be concise. > >>>>>The error codes are generic and map directly to the V4L2 Flash > >>>>>error codes. > >>>>> > >>>> > >>>>Actually I'd like to see those flash fault code defined in LED > >>>>subsystem. And V4L2 will just include LED flash header file to use it. > >>>>Because flash fault code is not for V4L2 specific but it's a feature > >>>>of LED flash devices. > >>>> > >>>>For clearing error code of flash devices, I think it depends on the > >>>>hardware. If most of our LED flash is using reading to clear error > >>>>code, we probably can make it simple as this now. But what if some > >>>>other LED flash devices are using writing to clear error code? we > >>>>should provide a API to that? > >>> > >>>Actually, we should provide API that makes sense, and that is easy to > >>>use by userspace. > >>> > >>>I believe "read" is called read because it does not change anything, > >>>and it should stay that way in /sysfs. You may want to talk to sysfs > >>>maintainers if you plan on doing another semantics. > >> > >>How would you proceed in case of devices which clear their fault > >>register upon I2C readout (e.g. AS3645)? In this case read does have > >>a side effect. For such devices attribute semantics would have to be > >>different than for the devices which don't clear faults on readout. > > > >No, semantics should be same for all devices. > > > >If device clears fault register during I2C readout, kernel will simply > >gather faults in an variable, and clear them upon write to sysfs file. > > This approach would require implementing additional mechanisms on > both sides: LED Flash class core and a LED Flash class driver. > In the former the sysfs attribute write permissions would have > to be decided in the runtime and in the latter caching mechanism Write attributes at runtime? Why? We can emulate sane and consistent behaviour for all the controllers: read gives you list of faults, write clears it. We can do it for all the controllers. Only cost is few lines of code in the drivers where hardware clears faults at read. > would have to be implemented per driver. We would have to also > consider how to approach the issue in case of sub-leds. Actually.. sub-leds. That is one physical LED being connected to two current sources at the same time, right? > The only reason for this overhead is trying to avoid side effects > on reading sysfs attribute. After weighing the pros and cons, > I am not sure if it is worthwhile. I am pretty sure. Pavel
On 12/09/2014 04:50 PM, Pavel Machek wrote: > On Tue 2014-12-09 09:54:06, Jacek Anaszewski wrote: >> Hi Pavel, >> >> On 12/08/2014 09:18 PM, Pavel Machek wrote: >>> On Mon 2014-12-08 17:55:20, Jacek Anaszewski wrote: >>>> On 12/06/2014 01:43 PM, Pavel Machek wrote: >>>>> >>>>>>> The format of a sysfs attribute should be concise. >>>>>>> The error codes are generic and map directly to the V4L2 Flash >>>>>>> error codes. >>>>>>> >>>>>> >>>>>> Actually I'd like to see those flash fault code defined in LED >>>>>> subsystem. And V4L2 will just include LED flash header file to use it. >>>>>> Because flash fault code is not for V4L2 specific but it's a feature >>>>>> of LED flash devices. >>>>>> >>>>>> For clearing error code of flash devices, I think it depends on the >>>>>> hardware. If most of our LED flash is using reading to clear error >>>>>> code, we probably can make it simple as this now. But what if some >>>>>> other LED flash devices are using writing to clear error code? we >>>>>> should provide a API to that? >>>>> >>>>> Actually, we should provide API that makes sense, and that is easy to >>>>> use by userspace. >>>>> >>>>> I believe "read" is called read because it does not change anything, >>>>> and it should stay that way in /sysfs. You may want to talk to sysfs >>>>> maintainers if you plan on doing another semantics. >>>> >>>> How would you proceed in case of devices which clear their fault >>>> register upon I2C readout (e.g. AS3645)? In this case read does have >>>> a side effect. For such devices attribute semantics would have to be >>>> different than for the devices which don't clear faults on readout. >>> >>> No, semantics should be same for all devices. >>> >>> If device clears fault register during I2C readout, kernel will simply >>> gather faults in an variable, and clear them upon write to sysfs file. >> >> This approach would require implementing additional mechanisms on >> both sides: LED Flash class core and a LED Flash class driver. >> In the former the sysfs attribute write permissions would have >> to be decided in the runtime and in the latter caching mechanism > > Write attributes at runtime? Why? We can emulate sane and consistent > behaviour for all the controllers: read gives you list of faults, > write clears it. We can do it for all the controllers. I don't like the idea of listing the faults in the form of strings. I'd like to see the third opinion :) > Only cost is few lines of code in the drivers where hardware clears > faults at read. As above - the third opinion would be appreciated. >> would have to be implemented per driver. We would have to also >> consider how to approach the issue in case of sub-leds. > > Actually.. sub-leds. That is one physical LED being connected to two > current sources at the same time, right? There are possible designs with two separate LEDs.
Hi! > >>both sides: LED Flash class core and a LED Flash class driver. > >>In the former the sysfs attribute write permissions would have > >>to be decided in the runtime and in the latter caching mechanism > > > >Write attributes at runtime? Why? We can emulate sane and consistent > >behaviour for all the controllers: read gives you list of faults, > >write clears it. We can do it for all the controllers. > > I don't like the idea of listing the faults in the form of strings. > I'd like to see the third opinion :) Well, I see that you don't like to change existing code. But "I hacked it this way and I'm going to ask n-th opinion so that I don't have to touch it" is not a way to design interfaces. > >Only cost is few lines of code in the drivers where hardware clears > >faults at read. > > As above - the third opinion would be appreciated. Just email Greg if he likes /sys files with sideffects on read. Or just think. You never do grep in /sys? Pavel
Hi Pavel and Jacek, On Tue, Dec 09, 2014 at 04:50:33PM +0100, Pavel Machek wrote: > On Tue 2014-12-09 09:54:06, Jacek Anaszewski wrote: > > Hi Pavel, > > > > On 12/08/2014 09:18 PM, Pavel Machek wrote: > > >On Mon 2014-12-08 17:55:20, Jacek Anaszewski wrote: > > >>On 12/06/2014 01:43 PM, Pavel Machek wrote: > > >>> > > >>>>>The format of a sysfs attribute should be concise. > > >>>>>The error codes are generic and map directly to the V4L2 Flash > > >>>>>error codes. > > >>>>> > > >>>> > > >>>>Actually I'd like to see those flash fault code defined in LED > > >>>>subsystem. And V4L2 will just include LED flash header file to use it. > > >>>>Because flash fault code is not for V4L2 specific but it's a feature > > >>>>of LED flash devices. > > >>>> > > >>>>For clearing error code of flash devices, I think it depends on the > > >>>>hardware. If most of our LED flash is using reading to clear error > > >>>>code, we probably can make it simple as this now. But what if some > > >>>>other LED flash devices are using writing to clear error code? we > > >>>>should provide a API to that? > > >>> > > >>>Actually, we should provide API that makes sense, and that is easy to > > >>>use by userspace. > > >>> > > >>>I believe "read" is called read because it does not change anything, > > >>>and it should stay that way in /sysfs. You may want to talk to sysfs > > >>>maintainers if you plan on doing another semantics. > > >> > > >>How would you proceed in case of devices which clear their fault > > >>register upon I2C readout (e.g. AS3645)? In this case read does have > > >>a side effect. For such devices attribute semantics would have to be > > >>different than for the devices which don't clear faults on readout. > > > > > >No, semantics should be same for all devices. > > > > > >If device clears fault register during I2C readout, kernel will simply > > >gather faults in an variable, and clear them upon write to sysfs file. > > > > This approach would require implementing additional mechanisms on > > both sides: LED Flash class core and a LED Flash class driver. > > In the former the sysfs attribute write permissions would have > > to be decided in the runtime and in the latter caching mechanism > > Write attributes at runtime? Why? We can emulate sane and consistent > behaviour for all the controllers: read gives you list of faults, > write clears it. We can do it for all the controllers. > > Only cost is few lines of code in the drivers where hardware clears > faults at read. Please take the time to read this, and consider it. I'd say the cost is I2C register access, not so much a few lines added to the drivers. The functionality and behaviour between the flash controllers varies. They have different faults, presence of (some) faults may prevent strobing, some support reading the flash status and some don't. Some of the flash faults are mostly relevant in production testing, some can be used to find hardware issues during use (rare) and some are produced in common use (timeout, for instance). The V4L2 flash API defines that reading the faults clears them, but does not state whether presence of faults would prevent further use of the flash. This is flash controller chip specific. I think you *could* force a policy on the level of kernel API, for instance require that the user clears the faults before strobing again rather than relying on the chip requiring this instead. Most of the time there are no faults. When there are, they may appear at some point of time after the strobing, but how long? Probably roughly after the timeout period the flash should have faults available if there were any --- except if the strobe is external such as a sensor timed strobe. In that case the software running on the CPU has no knowledge when the flash is strobed nor when the faults should be read. So the requirement of checking the faults would probably have to be limited to software strobe only. The user would still have to be able to check the faults for externally strobed pulses. Would it be acceptable that the interface was different there? So, after the user has strobed, when the user should check the flash faults? After the timeout period has passed? Right before strobing again? If this was a requirement, it adds an additional I2C access to potentially the place which should absolutely have no extra delay --- the flash strobe time. This would be highly unwanted. The faults seldom happened in regular use, but more recent flash controllers have LED overtemperature or undervoltage faults, the latter of which isn't really a fault, but status information telling that the flash current will be limited. Reading the faults in this case is more important than it has used to be. Finally, should the LED flash class enforce such a policy, would the V4L2 flash API which is provided to the same devices be changed as well? I'm not against that if we have 1) can come up with a good policy that is understood to be meaningful for all thinkable flash controller implementations and 2) agreement the behaviour can be changed. Btw. I think I'm slightly leaning towards liking flash faults in form of strings better; that's what much of the sysfs interface already uses. V4L2 is quite a bit different from that; we have a bitmask control for faults with well defined meanings for the bits in the spec. The LED class API is much more usable from the command line, and using strings for flash faults is in line with that. I have no strict stance towards that however; hexadecimal numbers have advantages as well such as being slightly more practicable to check in a C program. The importance of good documentatation increases in that case though, and probably a header file with the bit definitions is needed as well.
Hi! > > > This approach would require implementing additional mechanisms on > > > both sides: LED Flash class core and a LED Flash class driver. > > > In the former the sysfs attribute write permissions would have > > > to be decided in the runtime and in the latter caching mechanism > > > > Write attributes at runtime? Why? We can emulate sane and consistent > > behaviour for all the controllers: read gives you list of faults, > > write clears it. We can do it for all the controllers. > > > > Only cost is few lines of code in the drivers where hardware clears > > faults at read. > > Please take the time to read this, and consider it. > > I'd say the cost is I2C register access, not so much a few lines added to > the drivers. The functionality and behaviour between the flash controllers > varies. They have different faults, presence of (some) faults may prevent > strobing, some support reading the flash status and some don't. > > Some of the flash faults are mostly relevant in production testing, some can > be used to find hardware issues during use (rare) and some are produced in > common use (timeout, for instance). > > The V4L2 flash API defines that reading the faults clears them, but does not > state whether presence of faults would prevent further use of the flash. > This is flash controller chip specific. Yeah, but we are discussing sysfs reads. V4L2 API can just behave differently. > I think you *could* force a policy on the level of kernel API, for instance > require that the user clears the faults before strobing again rather than > relying on the chip requiring this instead. Yes, we could do that. > Most of the time there are no faults. When there are, they may appear at > some point of time after the strobing, but how long? Probably roughly after > the timeout period the flash should have faults available if there were any > --- except if the strobe is external such as a sensor timed strobe. In that > case the software running on the CPU has no knowledge when the flash is > strobed nor when the faults should be read. So the requirement of checking > the faults would probably have to be limited to software strobe only. The > user would still have to be able to check the faults for externally strobed > pulses. Would it be acceptable that the interface was different > there? Should the user just read the faults before scheduling next strobe? > So, after the user has strobed, when the user should check the flash faults? > After the timeout period has passed? Right before strobing again? If this > was a requirement, it adds an additional I2C access to potentially the place > which should absolutely have no extra delay --- the flash strobe time. This > would be highly unwanted. I'd do it before strobing again. Not neccessarily "just" before strobing again (you claim it is slow ... is it really so slow it matters)? > Finally, should the LED flash class enforce such a policy, would the V4L2 > flash API which is provided to the same devices be changed as well? I'm not > against that if we have > > 1) can come up with a good policy that is understood to be > meaningful for all thinkable flash controller implementations and > > 2) agreement the behaviour can be changed. I am saying that reading from /sys should not have side effects. For V4L2, existing behaviour might be ok. Each driver should have two operations: read_faults() and clear_faults(). On devices where i2c read clears faults, operations will be: int my_faults read_faults() my_faults |= read_i2c_faults() return my_faults clear_faults() my_faults = 0 Best regards, Pavel
diff --git a/Documentation/leds/leds-class-flash.txt b/Documentation/leds/leds-class-flash.txt new file mode 100644 index 0000000..d68565c --- /dev/null +++ b/Documentation/leds/leds-class-flash.txt @@ -0,0 +1,48 @@ + +Flash LED handling under Linux +============================== + +Some LED devices support two modes - torch and flash. The modes are +supported by the LED class (see Documentation/leds/leds-class.txt) +and LED Flash class respectively. + +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 to gain flash +capabilities. + +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) + - indicator_brightness - privacy LED brightness in microamperes (RW) + - max_indicator_brightness - maximum privacy LED brightness in + microamperes (RO) + - flash_timeout - flash strobe duration in microseconds (RW) + - max_flash_timeout - maximum available flash strobe duration (RO) + - flash_strobe - flash strobe state (RW) + - flash_sync_strobe - one flash device can control more than one + sub-led; when this atrribute is set to 1 + the flash led will be strobed synchronously + with the other ones controlled by the same + device (RW) + - flash_fault - bitmask of flash faults that may have occurred, + possible flags are: + * 0x01 - flash controller voltage to the flash LED has exceeded + the limit specific to the flash controller + * 0x02 - 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 + * 0x04 - the flash controller has overheated + * 0x08 - the short circuit protection of the flash controller + has been triggered + * 0x10 - current in the LED power supply has exceeded the limit + specific to the flash controller + * 0x40 - flash controller voltage to the flash LED has been + below the minimum limit specific to the flash + * 0x80 - 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 + * 0x100 - the temperature of the LED has exceeded its allowed + upper limit