* [PATCH 1/2] Documentation: leds: Add description of LED Flash class extension @ 2015-01-27 8:07 Jacek Anaszewski 2015-01-27 8:07 ` [PATCH 2/2] DT: leds: Add led-sources property Jacek Anaszewski 2015-01-27 22:37 ` Reading /sys with side effects (was Re: [PATCH 1/2] Documentation: leds: Add description of LED Flash class extension) Pavel Machek 0 siblings, 2 replies; 19+ messages in thread From: Jacek Anaszewski @ 2015-01-27 8:07 UTC (permalink / raw) To: linux-leds, devicetree Cc: kyungmin.park, b.zolnierkie, pavel, cooloney, rpurdie, sakari.ailus, s.nawrocki, Jacek Anaszewski The documentation being added contains overall description of the LED Flash Class and the related sysfs attributes. Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com> Acked-by: Kyungmin Park <kyungmin.park@samsung.com> Cc: Bryan Wu <cooloney@gmail.com> Cc: Richard Purdie <rpurdie@rpsys.net> --- Documentation/leds/leds-class-flash.txt | 59 +++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) create mode 100644 Documentation/leds/leds-class-flash.txt diff --git a/Documentation/leds/leds-class-flash.txt b/Documentation/leds/leds-class-flash.txt new file mode 100644 index 0000000..297401a --- /dev/null +++ b/Documentation/leds/leds-class-flash.txt @@ -0,0 +1,59 @@ + +Flash LED handling under Linux +============================== + +Some LED devices support two modes - torch and flash. In the LED subsystem +those modes are supported by LED class (see Documentation/leds/leds-class.txt) +and LED Flash class respectively. The torch mode related features are enabled +by default and the flash ones only if a driver declares it by setting +LED_DEV_CAP_FLASH flag. + +In order to enable support for flash LEDs CONFIG_LEDS_CLASS_FLASH symbol +must be defined in the kernel config. A flash LED driver must register +in the LED subsystem with led_classdev_flash_register function to gain flash +related capabilities. + +There are flash LED devices which can control more than one LED and allow for +strobing the sub-LEDs synchronously. A LED will be strobed synchronously with +the one whose identifier is written to the flash_sync_strobe sysfs attribute. +The list of available sub-LED identifiers can be read from the available_sync_leds +sysfs attribute. In order to enable the related settings the driver must set +LED_DEV_CAP_SYNC_STROBE flag. + +Following sysfs attributes are exposed for controlling flash LED devices: + + - flash_brightness - flash LED brightness in microamperes (RW) + - max_flash_brightness - maximum available flash LED brightness (RO) + - flash_timeout - flash strobe duration in microseconds (RW) + - max_flash_timeout - maximum available flash strobe duration (RO) + - flash_strobe - flash strobe state (RW) + - available_sync_leds - list of sub-LEDs available for flash strobe + synchronization (RO) + - flash_sync_strobe - identifier of the sub-LED to synchronize the flash + strobe with; 0 stands for no synchronization (RW) + - flash_fault - list of flash faults that may have occurred: + * led-over-voltage - flash controller voltage to the flash LED + has exceededthe limit specific to the flash controller + * flash-timeout-exceeded - the flash strobe was still on when + the timeout set by the user has expired; not all flash + controllers may set this in all such conditions + * controller-over-temperature - the flash controller has + overheated + * controller-short-circuit - the short circuit protection + of the flash controller has been triggered + * led-power-supply-over-current - current in the LED power + supply has exceeded the limit specific to the flash + controller + * indicator-led-fault - the flash controller has detected + a short or open circuit condition on the indicator LED + * led-under-voltage - flash controller voltage to the flash + LED has been below the minimum limit specific to + the flash + * controller-under-voltage - the input voltage of the flash + controller is below the limit under which strobing the + flash at full current will not be possible. The condition + persists until this flag is no longer set + * led-over-temperature - the temperature of the LED has exceeded + its allowed upper limit + + Flash faults are cleared, if possible, by reading the attribute. -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 2/2] DT: leds: Add led-sources property 2015-01-27 8:07 [PATCH 1/2] Documentation: leds: Add description of LED Flash class extension Jacek Anaszewski @ 2015-01-27 8:07 ` Jacek Anaszewski 2015-01-29 14:55 ` Jacek Anaszewski 2015-01-27 22:37 ` Reading /sys with side effects (was Re: [PATCH 1/2] Documentation: leds: Add description of LED Flash class extension) Pavel Machek 1 sibling, 1 reply; 19+ messages in thread From: Jacek Anaszewski @ 2015-01-27 8:07 UTC (permalink / raw) To: linux-leds, devicetree Cc: kyungmin.park, b.zolnierkie, pavel, cooloney, rpurdie, sakari.ailus, s.nawrocki, Jacek Anaszewski, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala Add a property for defining device outputs the LED represented by the DT child node is connected to. 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> Cc: Rob Herring <robh+dt@kernel.org> Cc: Pawel Moll <pawel.moll@arm.com> Cc: Mark Rutland <mark.rutland@arm.com> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk> Cc: Kumar Gala <galak@codeaurora.org> Cc: devicetree@vger.kernel.org --- Documentation/devicetree/bindings/leds/common.txt | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt index a2c3f7a..34811c5 100644 --- a/Documentation/devicetree/bindings/leds/common.txt +++ b/Documentation/devicetree/bindings/leds/common.txt @@ -1,6 +1,19 @@ Common leds properties. +LED and flash LED devices provide the same basic functionality as current +regulators, but extended with LED and flash LED specific features like +blinking patterns, flash timeout, flash faults and external flash strobe mode. + +Many LED devices expose more than one current output that can be connected +to one or more discrete LED component. Since the arrangement of connections +can influence the way of the LED device initialization, the LED components +have to be tightly coupled with the LED device binding. They are represented +by child nodes of the parent LED device binding. + Optional properties for child nodes: +- led-sources : List of device current outputs the LED is connected to. The + outputs are identified by the numbers that must be defined + in the LED device binding documentation. - label : The label for this LED. If omitted, the label is taken from the node name (excluding the unit address). @@ -33,7 +46,8 @@ system-status { camera-flash { label = "Flash"; + led-sources = <0>, <1>; max-microamp = <50000>; flash-max-microamp = <320000>; flash-timeout-us = <500000>; -} +}; -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] DT: leds: Add led-sources property 2015-01-27 8:07 ` [PATCH 2/2] DT: leds: Add led-sources property Jacek Anaszewski @ 2015-01-29 14:55 ` Jacek Anaszewski 2015-01-29 20:28 ` Pavel Machek 2015-01-29 21:03 ` Rob Herring 0 siblings, 2 replies; 19+ messages in thread From: Jacek Anaszewski @ 2015-01-29 14:55 UTC (permalink / raw) To: Rob Herring Cc: Jacek Anaszewski, linux-leds, devicetree, kyungmin.park, b.zolnierkie, pavel, cooloney, rpurdie, sakari.ailus, s.nawrocki, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala Hi Rob, Have we achieved consensus concerning this patch? If so, can you give your ack? On 01/27/2015 09:07 AM, Jacek Anaszewski wrote: > Add a property for defining device outputs the LED > represented by the DT child node is connected to. > > 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> > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Pawel Moll <pawel.moll@arm.com> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: Ian Campbell <ijc+devicetree@hellion.org.uk> > Cc: Kumar Gala <galak@codeaurora.org> > Cc: devicetree@vger.kernel.org > --- > Documentation/devicetree/bindings/leds/common.txt | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt > index a2c3f7a..34811c5 100644 > --- a/Documentation/devicetree/bindings/leds/common.txt > +++ b/Documentation/devicetree/bindings/leds/common.txt > @@ -1,6 +1,19 @@ > Common leds properties. > > +LED and flash LED devices provide the same basic functionality as current > +regulators, but extended with LED and flash LED specific features like > +blinking patterns, flash timeout, flash faults and external flash strobe mode. > + > +Many LED devices expose more than one current output that can be connected > +to one or more discrete LED component. Since the arrangement of connections > +can influence the way of the LED device initialization, the LED components > +have to be tightly coupled with the LED device binding. They are represented > +by child nodes of the parent LED device binding. > + > Optional properties for child nodes: > +- led-sources : List of device current outputs the LED is connected to. The > + outputs are identified by the numbers that must be defined > + in the LED device binding documentation. > - label : The label for this LED. If omitted, the label is > taken from the node name (excluding the unit address). > > @@ -33,7 +46,8 @@ system-status { > > camera-flash { > label = "Flash"; > + led-sources = <0>, <1>; > max-microamp = <50000>; > flash-max-microamp = <320000>; > flash-timeout-us = <500000>; > -} > +}; > -- Best Regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] DT: leds: Add led-sources property 2015-01-29 14:55 ` Jacek Anaszewski @ 2015-01-29 20:28 ` Pavel Machek 2015-01-29 21:03 ` Rob Herring 1 sibling, 0 replies; 19+ messages in thread From: Pavel Machek @ 2015-01-29 20:28 UTC (permalink / raw) To: Jacek Anaszewski Cc: Rob Herring, linux-leds, devicetree, kyungmin.park, b.zolnierkie, cooloney, rpurdie, sakari.ailus, s.nawrocki, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala On Thu 2015-01-29 15:55:52, Jacek Anaszewski wrote: > Hi Rob, > > Have we achieved consensus concerning this patch? > If so, can you give your ack? I'm not Rob, but: Acked-by: Pavel Machek <pavel@ucw.cz> > On 01/27/2015 09:07 AM, Jacek Anaszewski wrote: > >Add a property for defining device outputs the LED > >represented by the DT child node is connected to. > > > >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> > >Cc: Rob Herring <robh+dt@kernel.org> > >Cc: Pawel Moll <pawel.moll@arm.com> > >Cc: Mark Rutland <mark.rutland@arm.com> > >Cc: Ian Campbell <ijc+devicetree@hellion.org.uk> > >Cc: Kumar Gala <galak@codeaurora.org> > >Cc: devicetree@vger.kernel.org > >--- > > Documentation/devicetree/bindings/leds/common.txt | 16 +++++++++++++++- > > 1 file changed, 15 insertions(+), 1 deletion(-) > > > >diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt > >index a2c3f7a..34811c5 100644 > >--- a/Documentation/devicetree/bindings/leds/common.txt > >+++ b/Documentation/devicetree/bindings/leds/common.txt > >@@ -1,6 +1,19 @@ > > Common leds properties. > > > >+LED and flash LED devices provide the same basic functionality as current > >+regulators, but extended with LED and flash LED specific features like > >+blinking patterns, flash timeout, flash faults and external flash strobe mode. > >+ > >+Many LED devices expose more than one current output that can be connected > >+to one or more discrete LED component. Since the arrangement of connections > >+can influence the way of the LED device initialization, the LED components > >+have to be tightly coupled with the LED device binding. They are represented > >+by child nodes of the parent LED device binding. > >+ > > Optional properties for child nodes: > >+- led-sources : List of device current outputs the LED is connected to. The > >+ outputs are identified by the numbers that must be defined > >+ in the LED device binding documentation. > > - label : The label for this LED. If omitted, the label is > > taken from the node name (excluding the unit address). > > > >@@ -33,7 +46,8 @@ system-status { > > > > camera-flash { > > label = "Flash"; > >+ led-sources = <0>, <1>; > > max-microamp = <50000>; > > flash-max-microamp = <320000>; > > flash-timeout-us = <500000>; > >-} > >+}; > > > > -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] DT: leds: Add led-sources property 2015-01-29 14:55 ` Jacek Anaszewski 2015-01-29 20:28 ` Pavel Machek @ 2015-01-29 21:03 ` Rob Herring 2015-01-29 22:14 ` Bryan Wu 1 sibling, 1 reply; 19+ messages in thread From: Rob Herring @ 2015-01-29 21:03 UTC (permalink / raw) To: Jacek Anaszewski Cc: Rob Herring, linux-leds, devicetree, Kyungmin Park, Bartlomiej Zolnierkiewicz, Pavel Machek, Bryan Wu, Richard Purdie, sakari.ailus, Sylwester Nawrocki, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala On Thu, Jan 29, 2015 at 8:55 AM, Jacek Anaszewski <j.anaszewski@samsung.com> wrote: > Hi Rob, > > Have we achieved consensus concerning this patch? > If so, can you give your ack? I think so. Acked-by: Rob Herring <robh@kernel.org> > > > On 01/27/2015 09:07 AM, Jacek Anaszewski wrote: >> >> Add a property for defining device outputs the LED >> represented by the DT child node is connected to. >> >> 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> >> Cc: Rob Herring <robh+dt@kernel.org> >> Cc: Pawel Moll <pawel.moll@arm.com> >> Cc: Mark Rutland <mark.rutland@arm.com> >> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk> >> Cc: Kumar Gala <galak@codeaurora.org> >> Cc: devicetree@vger.kernel.org >> --- >> Documentation/devicetree/bindings/leds/common.txt | 16 >> +++++++++++++++- >> 1 file changed, 15 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/devicetree/bindings/leds/common.txt >> b/Documentation/devicetree/bindings/leds/common.txt >> index a2c3f7a..34811c5 100644 >> --- a/Documentation/devicetree/bindings/leds/common.txt >> +++ b/Documentation/devicetree/bindings/leds/common.txt >> @@ -1,6 +1,19 @@ >> Common leds properties. >> >> +LED and flash LED devices provide the same basic functionality as current >> +regulators, but extended with LED and flash LED specific features like >> +blinking patterns, flash timeout, flash faults and external flash strobe >> mode. >> + >> +Many LED devices expose more than one current output that can be >> connected >> +to one or more discrete LED component. Since the arrangement of >> connections >> +can influence the way of the LED device initialization, the LED >> components >> +have to be tightly coupled with the LED device binding. They are >> represented >> +by child nodes of the parent LED device binding. >> + >> Optional properties for child nodes: >> +- led-sources : List of device current outputs the LED is connected to. >> The >> + outputs are identified by the numbers that must be defined >> + in the LED device binding documentation. >> - label : The label for this LED. If omitted, the label is >> taken from the node name (excluding the unit address). >> >> @@ -33,7 +46,8 @@ system-status { >> >> camera-flash { >> label = "Flash"; >> + led-sources = <0>, <1>; >> max-microamp = <50000>; >> flash-max-microamp = <320000>; >> flash-timeout-us = <500000>; >> -} >> +}; >> > > > -- > Best Regards, > Jacek Anaszewski ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] DT: leds: Add led-sources property 2015-01-29 21:03 ` Rob Herring @ 2015-01-29 22:14 ` Bryan Wu [not found] ` <CAK5ve-LKBfjRzxK_b0ByEF5uWHfAaBGS2OPznOiYNg9+FJP5Gg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 0 siblings, 1 reply; 19+ messages in thread From: Bryan Wu @ 2015-01-29 22:14 UTC (permalink / raw) To: Rob Herring Cc: Jacek Anaszewski, Rob Herring, Linux LED Subsystem, devicetree, Kyungmin Park, Bartlomiej Zolnierkiewicz, Pavel Machek, Richard Purdie, Sakari Ailus, Sylwester Nawrocki, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala On Thu, Jan 29, 2015 at 1:03 PM, Rob Herring <robherring2@gmail.com> wrote: > On Thu, Jan 29, 2015 at 8:55 AM, Jacek Anaszewski > <j.anaszewski@samsung.com> wrote: >> Hi Rob, >> >> Have we achieved consensus concerning this patch? >> If so, can you give your ack? > > I think so. > > Acked-by: Rob Herring <robh@kernel.org> > >> OK, I can merge this patch into my tree with you guys Ack. Jacek, does this patch depend on the first one which you are still discussing with Pavel? Can I merge this one firstly. -Bryan >> >> On 01/27/2015 09:07 AM, Jacek Anaszewski wrote: >>> >>> Add a property for defining device outputs the LED >>> represented by the DT child node is connected to. >>> >>> 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> >>> Cc: Rob Herring <robh+dt@kernel.org> >>> Cc: Pawel Moll <pawel.moll@arm.com> >>> Cc: Mark Rutland <mark.rutland@arm.com> >>> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk> >>> Cc: Kumar Gala <galak@codeaurora.org> >>> Cc: devicetree@vger.kernel.org >>> --- >>> Documentation/devicetree/bindings/leds/common.txt | 16 >>> +++++++++++++++- >>> 1 file changed, 15 insertions(+), 1 deletion(-) >>> >>> diff --git a/Documentation/devicetree/bindings/leds/common.txt >>> b/Documentation/devicetree/bindings/leds/common.txt >>> index a2c3f7a..34811c5 100644 >>> --- a/Documentation/devicetree/bindings/leds/common.txt >>> +++ b/Documentation/devicetree/bindings/leds/common.txt >>> @@ -1,6 +1,19 @@ >>> Common leds properties. >>> >>> +LED and flash LED devices provide the same basic functionality as current >>> +regulators, but extended with LED and flash LED specific features like >>> +blinking patterns, flash timeout, flash faults and external flash strobe >>> mode. >>> + >>> +Many LED devices expose more than one current output that can be >>> connected >>> +to one or more discrete LED component. Since the arrangement of >>> connections >>> +can influence the way of the LED device initialization, the LED >>> components >>> +have to be tightly coupled with the LED device binding. They are >>> represented >>> +by child nodes of the parent LED device binding. >>> + >>> Optional properties for child nodes: >>> +- led-sources : List of device current outputs the LED is connected to. >>> The >>> + outputs are identified by the numbers that must be defined >>> + in the LED device binding documentation. >>> - label : The label for this LED. If omitted, the label is >>> taken from the node name (excluding the unit address). >>> >>> @@ -33,7 +46,8 @@ system-status { >>> >>> camera-flash { >>> label = "Flash"; >>> + led-sources = <0>, <1>; >>> max-microamp = <50000>; >>> flash-max-microamp = <320000>; >>> flash-timeout-us = <500000>; >>> -} >>> +}; >>> >> >> >> -- >> Best Regards, >> Jacek Anaszewski ^ permalink raw reply [flat|nested] 19+ messages in thread
[parent not found: <CAK5ve-LKBfjRzxK_b0ByEF5uWHfAaBGS2OPznOiYNg9+FJP5Gg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH 2/2] DT: leds: Add led-sources property [not found] ` <CAK5ve-LKBfjRzxK_b0ByEF5uWHfAaBGS2OPznOiYNg9+FJP5Gg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2015-01-29 22:30 ` Pavel Machek 2015-01-29 22:59 ` Bryan Wu 0 siblings, 1 reply; 19+ messages in thread From: Pavel Machek @ 2015-01-29 22:30 UTC (permalink / raw) To: Bryan Wu Cc: Rob Herring, Jacek Anaszewski, Rob Herring, Linux LED Subsystem, devicetree-u79uwXL29TY76Z2rM5mHXA, Kyungmin Park, Bartlomiej Zolnierkiewicz, Richard Purdie, Sakari Ailus, Sylwester Nawrocki, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala On Thu 2015-01-29 14:14:54, Bryan Wu wrote: > On Thu, Jan 29, 2015 at 1:03 PM, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote: > > On Thu, Jan 29, 2015 at 8:55 AM, Jacek Anaszewski > > <j.anaszewski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote: > >> Hi Rob, > >> > >> Have we achieved consensus concerning this patch? > >> If so, can you give your ack? > > > > I think so. > > > > Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> > > > >> > > OK, I can merge this patch into my tree with you guys Ack. > Jacek, does this patch depend on the first one which you are still > discussing with Pavel? Can I merge this one firstly. Yes, you can.. they are independend. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] DT: leds: Add led-sources property 2015-01-29 22:30 ` Pavel Machek @ 2015-01-29 22:59 ` Bryan Wu 0 siblings, 0 replies; 19+ messages in thread From: Bryan Wu @ 2015-01-29 22:59 UTC (permalink / raw) To: Pavel Machek Cc: Rob Herring, Jacek Anaszewski, Rob Herring, Linux LED Subsystem, devicetree, Kyungmin Park, Bartlomiej Zolnierkiewicz, Richard Purdie, Sakari Ailus, Sylwester Nawrocki, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala On Thu, Jan 29, 2015 at 2:30 PM, Pavel Machek <pavel@ucw.cz> wrote: > On Thu 2015-01-29 14:14:54, Bryan Wu wrote: >> On Thu, Jan 29, 2015 at 1:03 PM, Rob Herring <robherring2@gmail.com> wrote: >> > On Thu, Jan 29, 2015 at 8:55 AM, Jacek Anaszewski >> > <j.anaszewski@samsung.com> wrote: >> >> Hi Rob, >> >> >> >> Have we achieved consensus concerning this patch? >> >> If so, can you give your ack? >> > >> > I think so. >> > >> > Acked-by: Rob Herring <robh@kernel.org> >> > >> >> >> >> OK, I can merge this patch into my tree with you guys Ack. >> Jacek, does this patch depend on the first one which you are still >> discussing with Pavel? Can I merge this one firstly. > > Yes, you can.. they are independend. > Pavel Applied, thanks. -Bryan ^ permalink raw reply [flat|nested] 19+ messages in thread
* Reading /sys with side effects (was Re: [PATCH 1/2] Documentation: leds: Add description of LED Flash class extension) 2015-01-27 8:07 [PATCH 1/2] Documentation: leds: Add description of LED Flash class extension Jacek Anaszewski 2015-01-27 8:07 ` [PATCH 2/2] DT: leds: Add led-sources property Jacek Anaszewski @ 2015-01-27 22:37 ` Pavel Machek 2015-01-28 8:43 ` Jacek Anaszewski 1 sibling, 1 reply; 19+ messages in thread From: Pavel Machek @ 2015-01-27 22:37 UTC (permalink / raw) To: Jacek Anaszewski, Greg KH, kernel list Cc: linux-leds, devicetree, kyungmin.park, b.zolnierkie, cooloney, rpurdie, sakari.ailus, s.nawrocki Hi! > + - flash_fault - list of flash faults that may have occurred: > + * led-over-voltage - flash controller voltage to the flash LED > + has exceededthe limit specific to the flash controller > + * flash-timeout-exceeded - the flash strobe was still on when > + the timeout set by the user has expired; not all flash > + controllers may set this in all such conditions > + * controller-over-temperature - the flash controller has > + overheated > + * controller-short-circuit - the short circuit protection > + of the flash controller has been triggered > + * led-power-supply-over-current - current in the LED power > + supply has exceeded the limit specific to the flash > + controller > + * indicator-led-fault - the flash controller has detected > + a short or open circuit condition on the indicator LED > + * led-under-voltage - flash controller voltage to the flash > + LED has been below the minimum limit specific to > + the flash > + * controller-under-voltage - the input voltage of the flash > + controller is below the limit under which strobing the > + flash at full current will not be possible. The condition > + persists until this flag is no longer set > + * led-over-temperature - the temperature of the LED has exceeded > + its allowed upper limit > + > + Flash faults are cleared, if possible, by reading the attribute. That's bad. Now you can no longer present flash_fault file as readable to non-root users, and grep -ri foo /sys will interfere with your camera application. Bad interface, just fix it. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: Reading /sys with side effects (was Re: [PATCH 1/2] Documentation: leds: Add description of LED Flash class extension) 2015-01-27 22:37 ` Reading /sys with side effects (was Re: [PATCH 1/2] Documentation: leds: Add description of LED Flash class extension) Pavel Machek @ 2015-01-28 8:43 ` Jacek Anaszewski [not found] ` <54C8A130.8000807-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> 0 siblings, 1 reply; 19+ messages in thread From: Jacek Anaszewski @ 2015-01-28 8:43 UTC (permalink / raw) To: Pavel Machek Cc: Greg KH, kernel list, linux-leds, devicetree, kyungmin.park, b.zolnierkie, cooloney, rpurdie, sakari.ailus, s.nawrocki Hi Pavel, On 01/27/2015 11:37 PM, Pavel Machek wrote: > Hi! > >> + - flash_fault - list of flash faults that may have occurred: >> + * led-over-voltage - flash controller voltage to the flash LED >> + has exceededthe limit specific to the flash controller >> + * flash-timeout-exceeded - the flash strobe was still on when >> + the timeout set by the user has expired; not all flash >> + controllers may set this in all such conditions >> + * controller-over-temperature - the flash controller has >> + overheated >> + * controller-short-circuit - the short circuit protection >> + of the flash controller has been triggered >> + * led-power-supply-over-current - current in the LED power >> + supply has exceeded the limit specific to the flash >> + controller >> + * indicator-led-fault - the flash controller has detected >> + a short or open circuit condition on the indicator LED >> + * led-under-voltage - flash controller voltage to the flash >> + LED has been below the minimum limit specific to >> + the flash >> + * controller-under-voltage - the input voltage of the flash >> + controller is below the limit under which strobing the >> + flash at full current will not be possible. The condition >> + persists until this flag is no longer set >> + * led-over-temperature - the temperature of the LED has exceeded >> + its allowed upper limit >> + >> + Flash faults are cleared, if possible, by reading the attribute. > > That's bad. Now you can no longer present flash_fault file as readable > to non-root users, and grep -ri foo /sys will interfere with your > camera application. > > Bad interface, just fix it. In my opinion it isn't crucial for the user to be aware of the fact that some non-persistent fault happened right after strobing the flash (e.g. over temperature). I cannot see anything harmful in the situation when someone does grep on /sys and clears non-persistent fault on a flash LED device. Also, not all devices may be able to report the faults that happened earlier but are not valid at the time of I2C readout. In that case the user will never now that the fault has ever occurred, unless they read the flash_fault attribute at the proper moment. In this case we cannot enforce consistent policy for all devices. Please describe the use case when clearing the fault on read can be harmful, if you have any. Moreover, I don't see your reply to Sakari's message [1], where he considers the problem from several perspectives. [1] http://www.spinics.net/lists/linux-leds/msg02653.html -- Best Regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 19+ messages in thread
[parent not found: <54C8A130.8000807-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>]
* Re: Reading /sys with side effects (was Re: [PATCH 1/2] Documentation: leds: Add description of LED Flash class extension) 2015-01-28 8:43 ` Jacek Anaszewski @ 2015-01-29 21:14 ` Pavel Machek 0 siblings, 0 replies; 19+ messages in thread From: Pavel Machek @ 2015-01-29 21:14 UTC (permalink / raw) To: Jacek Anaszewski Cc: Greg KH, kernel list, linux-leds-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA, kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ, b.zolnierkie-Sze3O3UU22JBDgjK7y7TUQ, cooloney-Re5JQEeQqe8AvxtiuMwx3w, rpurdie-Fm38FmjxZ/leoWH0uzbU5w, sakari.ailus-X3B1VOXEql0, s.nawrocki-Sze3O3UU22JBDgjK7y7TUQ Hi! > >>+ - flash_fault - list of flash faults that may have occurred: > >>+ * led-over-voltage - flash controller voltage to the flash LED > >>+ has exceededthe limit specific to the flash controller > >>+ * flash-timeout-exceeded - the flash strobe was still on when > >>+ the timeout set by the user has expired; not all flash > >>+ controllers may set this in all such conditions > >>+ * controller-over-temperature - the flash controller has > >>+ overheated > >>+ * controller-short-circuit - the short circuit protection > >>+ of the flash controller has been triggered > >>+ * led-power-supply-over-current - current in the LED power > >>+ supply has exceeded the limit specific to the flash > >>+ controller > >>+ * indicator-led-fault - the flash controller has detected > >>+ a short or open circuit condition on the indicator LED > >>+ * led-under-voltage - flash controller voltage to the flash > >>+ LED has been below the minimum limit specific to > >>+ the flash > >>+ * controller-under-voltage - the input voltage of the flash > >>+ controller is below the limit under which strobing the > >>+ flash at full current will not be possible. The condition > >>+ persists until this flag is no longer set > >>+ * led-over-temperature - the temperature of the LED has exceeded > >>+ its allowed upper limit > >>+ > >>+ Flash faults are cleared, if possible, by reading the attribute. > > > >That's bad. Now you can no longer present flash_fault file as readable > >to non-root users, and grep -ri foo /sys will interfere with your > >camera application. > > > >Bad interface, just fix it. > > In my opinion it isn't crucial for the user to be aware of the > fact that some non-persistent fault happened right after strobing the > flash (e.g. over temperature). > > I cannot see anything harmful in the situation when someone does grep > on /sys and clears non-persistent fault on a flash LED device. So why export the faults at all? I mean... another user can just read the file in loop, and the camera application will not get any useful information. > Also, not all devices may be able to report the faults that happened > earlier but are not valid at the time of I2C readout. In that case the > user will never now that the fault has ever occurred, unless they read > the flash_fault attribute at the proper moment. > > In this case we cannot enforce consistent policy for all devices. Too bad. But lets do a good job at least for devices where we can do a good job, ok? > Please describe the use case when clearing the fault on read can be > harmful, if you have any. while true; grep -ri foo /sys; done And no, your application trying to read the faults will very probably read nothing. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: Reading /sys with side effects (was Re: [PATCH 1/2] Documentation: leds: Add description of LED Flash class extension) @ 2015-01-29 21:14 ` Pavel Machek 0 siblings, 0 replies; 19+ messages in thread From: Pavel Machek @ 2015-01-29 21:14 UTC (permalink / raw) To: Jacek Anaszewski Cc: Greg KH, kernel list, linux-leds, devicetree, kyungmin.park, b.zolnierkie, cooloney, rpurdie, sakari.ailus, s.nawrocki Hi! > >>+ - flash_fault - list of flash faults that may have occurred: > >>+ * led-over-voltage - flash controller voltage to the flash LED > >>+ has exceededthe limit specific to the flash controller > >>+ * flash-timeout-exceeded - the flash strobe was still on when > >>+ the timeout set by the user has expired; not all flash > >>+ controllers may set this in all such conditions > >>+ * controller-over-temperature - the flash controller has > >>+ overheated > >>+ * controller-short-circuit - the short circuit protection > >>+ of the flash controller has been triggered > >>+ * led-power-supply-over-current - current in the LED power > >>+ supply has exceeded the limit specific to the flash > >>+ controller > >>+ * indicator-led-fault - the flash controller has detected > >>+ a short or open circuit condition on the indicator LED > >>+ * led-under-voltage - flash controller voltage to the flash > >>+ LED has been below the minimum limit specific to > >>+ the flash > >>+ * controller-under-voltage - the input voltage of the flash > >>+ controller is below the limit under which strobing the > >>+ flash at full current will not be possible. The condition > >>+ persists until this flag is no longer set > >>+ * led-over-temperature - the temperature of the LED has exceeded > >>+ its allowed upper limit > >>+ > >>+ Flash faults are cleared, if possible, by reading the attribute. > > > >That's bad. Now you can no longer present flash_fault file as readable > >to non-root users, and grep -ri foo /sys will interfere with your > >camera application. > > > >Bad interface, just fix it. > > In my opinion it isn't crucial for the user to be aware of the > fact that some non-persistent fault happened right after strobing the > flash (e.g. over temperature). > > I cannot see anything harmful in the situation when someone does grep > on /sys and clears non-persistent fault on a flash LED device. So why export the faults at all? I mean... another user can just read the file in loop, and the camera application will not get any useful information. > Also, not all devices may be able to report the faults that happened > earlier but are not valid at the time of I2C readout. In that case the > user will never now that the fault has ever occurred, unless they read > the flash_fault attribute at the proper moment. > > In this case we cannot enforce consistent policy for all devices. Too bad. But lets do a good job at least for devices where we can do a good job, ok? > Please describe the use case when clearing the fault on read can be > harmful, if you have any. while true; grep -ri foo /sys; done And no, your application trying to read the faults will very probably read nothing. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: Reading /sys with side effects (was Re: [PATCH 1/2] Documentation: leds: Add description of LED Flash class extension) 2015-01-29 21:14 ` Pavel Machek (?) @ 2015-01-30 8:55 ` Jacek Anaszewski 2015-01-30 16:40 ` Greg KH -1 siblings, 1 reply; 19+ messages in thread From: Jacek Anaszewski @ 2015-01-30 8:55 UTC (permalink / raw) To: Pavel Machek Cc: Greg KH, kernel list, linux-leds, devicetree, kyungmin.park, b.zolnierkie, cooloney, rpurdie, sakari.ailus, s.nawrocki Hi Pavel, On 01/29/2015 10:14 PM, Pavel Machek wrote: > Hi! > >>>> + - flash_fault - list of flash faults that may have occurred: >>>> + * led-over-voltage - flash controller voltage to the flash LED >>>> + has exceededthe limit specific to the flash controller >>>> + * flash-timeout-exceeded - the flash strobe was still on when >>>> + the timeout set by the user has expired; not all flash >>>> + controllers may set this in all such conditions >>>> + * controller-over-temperature - the flash controller has >>>> + overheated >>>> + * controller-short-circuit - the short circuit protection >>>> + of the flash controller has been triggered >>>> + * led-power-supply-over-current - current in the LED power >>>> + supply has exceeded the limit specific to the flash >>>> + controller >>>> + * indicator-led-fault - the flash controller has detected >>>> + a short or open circuit condition on the indicator LED >>>> + * led-under-voltage - flash controller voltage to the flash >>>> + LED has been below the minimum limit specific to >>>> + the flash >>>> + * controller-under-voltage - the input voltage of the flash >>>> + controller is below the limit under which strobing the >>>> + flash at full current will not be possible. The condition >>>> + persists until this flag is no longer set >>>> + * led-over-temperature - the temperature of the LED has exceeded >>>> + its allowed upper limit >>>> + >>>> + Flash faults are cleared, if possible, by reading the attribute. >>> >>> That's bad. Now you can no longer present flash_fault file as readable >>> to non-root users, and grep -ri foo /sys will interfere with your >>> camera application. >>> >>> Bad interface, just fix it. >> >> In my opinion it isn't crucial for the user to be aware of the >> fact that some non-persistent fault happened right after strobing the >> flash (e.g. over temperature). >> >> I cannot see anything harmful in the situation when someone does grep >> on /sys and clears non-persistent fault on a flash LED device. > > So why export the faults at all? Faults may prevent strobing the flash in case of some devices. The example of such a device is ADP1663 (drivers/media/i2c/adp1653.c). This driver reads the faults before strobing the flash and if a fault preventing strobing has occurred it returns -EBUSY. If this driver was made a LED Flash class driver, then it would expose flash_faults attribute. The driver would probably need redesigning - checking the faults before strobing would have to be avoided and it should be left to the userspace. > I mean... another user can just read the file in loop, and the camera > application will not get any useful information. If the fault is no longer valid at the time of access from camera application, then why it should be reported then? >> Also, not all devices may be able to report the faults that happened >> earlier but are not valid at the time of I2C readout. In that case the >> user will never now that the fault has ever occurred, unless they read >> the flash_fault attribute at the proper moment. >> >> In this case we cannot enforce consistent policy for all devices. > > Too bad. But lets do a good job at least for devices where we can do a > good job, ok? > >> Please describe the use case when clearing the fault on read can be >> harmful, if you have any. > > while true; grep -ri foo /sys; done > > And no, your application trying to read the faults will very probably > read nothing. And this is OK. If a non-persistent fault was read by grep, then it will not be reported anymore. If someone wanted to maintain the history of flash faults for a device, then they are free to do it on their own by periodically reading the attribute, however I don't think it would be practical during every day use. -- Best Regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: Reading /sys with side effects (was Re: [PATCH 1/2] Documentation: leds: Add description of LED Flash class extension) 2015-01-30 8:55 ` Jacek Anaszewski @ 2015-01-30 16:40 ` Greg KH 2015-02-02 9:07 ` Jacek Anaszewski 0 siblings, 1 reply; 19+ messages in thread From: Greg KH @ 2015-01-30 16:40 UTC (permalink / raw) To: Jacek Anaszewski Cc: Pavel Machek, kernel list, linux-leds, devicetree, kyungmin.park, b.zolnierkie, cooloney, rpurdie, sakari.ailus, s.nawrocki On Fri, Jan 30, 2015 at 09:55:30AM +0100, Jacek Anaszewski wrote: > Hi Pavel, > > On 01/29/2015 10:14 PM, Pavel Machek wrote: > >Hi! > > > >>>>+ - flash_fault - list of flash faults that may have occurred: > >>>>+ * led-over-voltage - flash controller voltage to the flash LED > >>>>+ has exceededthe limit specific to the flash controller > >>>>+ * flash-timeout-exceeded - the flash strobe was still on when > >>>>+ the timeout set by the user has expired; not all flash > >>>>+ controllers may set this in all such conditions > >>>>+ * controller-over-temperature - the flash controller has > >>>>+ overheated > >>>>+ * controller-short-circuit - the short circuit protection > >>>>+ of the flash controller has been triggered > >>>>+ * led-power-supply-over-current - current in the LED power > >>>>+ supply has exceeded the limit specific to the flash > >>>>+ controller > >>>>+ * indicator-led-fault - the flash controller has detected > >>>>+ a short or open circuit condition on the indicator LED > >>>>+ * led-under-voltage - flash controller voltage to the flash > >>>>+ LED has been below the minimum limit specific to > >>>>+ the flash > >>>>+ * controller-under-voltage - the input voltage of the flash > >>>>+ controller is below the limit under which strobing the > >>>>+ flash at full current will not be possible. The condition > >>>>+ persists until this flag is no longer set > >>>>+ * led-over-temperature - the temperature of the LED has exceeded > >>>>+ its allowed upper limit > >>>>+ > >>>>+ Flash faults are cleared, if possible, by reading the attribute. > >>> > >>>That's bad. Now you can no longer present flash_fault file as readable > >>>to non-root users, and grep -ri foo /sys will interfere with your > >>>camera application. > >>> > >>>Bad interface, just fix it. > >> > >>In my opinion it isn't crucial for the user to be aware of the > >>fact that some non-persistent fault happened right after strobing the > >>flash (e.g. over temperature). > >> > >>I cannot see anything harmful in the situation when someone does grep > >>on /sys and clears non-persistent fault on a flash LED device. > > > >So why export the faults at all? > > Faults may prevent strobing the flash in case of some devices. > The example of such a device is ADP1663 (drivers/media/i2c/adp1653.c). > This driver reads the faults before strobing the flash and if a > fault preventing strobing has occurred it returns -EBUSY. > > If this driver was made a LED Flash class driver, then it would > expose flash_faults attribute. The driver would probably need > redesigning - checking the faults before strobing would have to be > avoided and it should be left to the userspace. That's fine, but Pavel's point is that you shouldn't "clear a fault" by reading a sysfs file as you don't control who reads all sysfs files (hint, libudev might cache all attributes when they are found / change, which could prevent anyone else from seeing that fault.) So please fix this, make a write to clear a fault or some other such explicit action, not a simple read. That's not an acceptable api. thanks, greg k-h ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: Reading /sys with side effects (was Re: [PATCH 1/2] Documentation: leds: Add description of LED Flash class extension) 2015-01-30 16:40 ` Greg KH @ 2015-02-02 9:07 ` Jacek Anaszewski 2015-02-02 9:44 ` Pavel Machek 0 siblings, 1 reply; 19+ messages in thread From: Jacek Anaszewski @ 2015-02-02 9:07 UTC (permalink / raw) To: Greg KH Cc: Pavel Machek, kernel list, linux-leds, devicetree, kyungmin.park, b.zolnierkie, cooloney, rpurdie, sakari.ailus, s.nawrocki On 01/30/2015 05:40 PM, Greg KH wrote: > On Fri, Jan 30, 2015 at 09:55:30AM +0100, Jacek Anaszewski wrote: >> Hi Pavel, >> >> On 01/29/2015 10:14 PM, Pavel Machek wrote: >>> Hi! >>> >>>>>> + - flash_fault - list of flash faults that may have occurred: >>>>>> + * led-over-voltage - flash controller voltage to the flash LED >>>>>> + has exceededthe limit specific to the flash controller >>>>>> + * flash-timeout-exceeded - the flash strobe was still on when >>>>>> + the timeout set by the user has expired; not all flash >>>>>> + controllers may set this in all such conditions >>>>>> + * controller-over-temperature - the flash controller has >>>>>> + overheated >>>>>> + * controller-short-circuit - the short circuit protection >>>>>> + of the flash controller has been triggered >>>>>> + * led-power-supply-over-current - current in the LED power >>>>>> + supply has exceeded the limit specific to the flash >>>>>> + controller >>>>>> + * indicator-led-fault - the flash controller has detected >>>>>> + a short or open circuit condition on the indicator LED >>>>>> + * led-under-voltage - flash controller voltage to the flash >>>>>> + LED has been below the minimum limit specific to >>>>>> + the flash >>>>>> + * controller-under-voltage - the input voltage of the flash >>>>>> + controller is below the limit under which strobing the >>>>>> + flash at full current will not be possible. The condition >>>>>> + persists until this flag is no longer set >>>>>> + * led-over-temperature - the temperature of the LED has exceeded >>>>>> + its allowed upper limit >>>>>> + >>>>>> + Flash faults are cleared, if possible, by reading the attribute. >>>>> >>>>> That's bad. Now you can no longer present flash_fault file as readable >>>>> to non-root users, and grep -ri foo /sys will interfere with your >>>>> camera application. >>>>> >>>>> Bad interface, just fix it. >>>> >>>> In my opinion it isn't crucial for the user to be aware of the >>>> fact that some non-persistent fault happened right after strobing the >>>> flash (e.g. over temperature). >>>> >>>> I cannot see anything harmful in the situation when someone does grep >>>> on /sys and clears non-persistent fault on a flash LED device. >>> >>> So why export the faults at all? >> >> Faults may prevent strobing the flash in case of some devices. >> The example of such a device is ADP1663 (drivers/media/i2c/adp1653.c). >> This driver reads the faults before strobing the flash and if a >> fault preventing strobing has occurred it returns -EBUSY. >> >> If this driver was made a LED Flash class driver, then it would >> expose flash_faults attribute. The driver would probably need >> redesigning - checking the faults before strobing would have to be >> avoided and it should be left to the userspace. > > That's fine, but Pavel's point is that you shouldn't "clear a fault" by > reading a sysfs file as you don't control who reads all sysfs files > (hint, libudev might cache all attributes when they are found / change, > which could prevent anyone else from seeing that fault.) > > So please fix this, make a write to clear a fault or some other such > explicit action, not a simple read. That's not an acceptable api. I am aware what Pavel'a point was, I just presented the arguments justifying existence of the flash_faults attribute at all. In my opinion flash_faults attribute should report the current state of the device. For the devices which clear the faults on I2C readout the faults read would have to be cached in the driver, until they are explicitly cleared, to keep the sysfs interface consistent. Nonetheless, there can be also devices which don't require clearing the faults - they are reported only when the actual condition occurs, e.g. over temperature or under voltage. When the related value gets back to the acceptable level the fault is no longer reported by the device. In this case some faults will remain unnoticed by the user space. This is the argument in favour of my statement that caching the faults does not make a sense and is not crucial. The user's vital interest is to know whether the flash LED is operational right before strobing. Since we cannot guarantee reporting all the faults that occurred for all possible flash LED devices, the only sensible solution is to report only the currently valid fault. -- Best Regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: Reading /sys with side effects (was Re: [PATCH 1/2] Documentation: leds: Add description of LED Flash class extension) 2015-02-02 9:07 ` Jacek Anaszewski @ 2015-02-02 9:44 ` Pavel Machek 2015-02-02 11:55 ` Jacek Anaszewski 0 siblings, 1 reply; 19+ messages in thread From: Pavel Machek @ 2015-02-02 9:44 UTC (permalink / raw) To: Jacek Anaszewski Cc: Greg KH, kernel list, linux-leds, devicetree, kyungmin.park, b.zolnierkie, cooloney, rpurdie, sakari.ailus, s.nawrocki On Mon 2015-02-02 10:07:02, Jacek Anaszewski wrote: > On 01/30/2015 05:40 PM, Greg KH wrote: > >On Fri, Jan 30, 2015 at 09:55:30AM +0100, Jacek Anaszewski wrote: > >>Hi Pavel, > >> > >>On 01/29/2015 10:14 PM, Pavel Machek wrote: > >>>Hi! > >>> > >>>>>>+ - flash_fault - list of flash faults that may have occurred: > >>>>>>+ * led-over-voltage - flash controller voltage to the flash LED > >>>>>>+ has exceededthe limit specific to the flash controller > >>>>>>+ * flash-timeout-exceeded - the flash strobe was still on when > >>>>>>+ the timeout set by the user has expired; not all flash > >>>>>>+ controllers may set this in all such conditions > >>>>>>+ * controller-over-temperature - the flash controller has > >>>>>>+ overheated > >>>>>>+ * controller-short-circuit - the short circuit protection > >>>>>>+ of the flash controller has been triggered > >>>>>>+ * led-power-supply-over-current - current in the LED power > >>>>>>+ supply has exceeded the limit specific to the flash > >>>>>>+ controller > >>>>>>+ * indicator-led-fault - the flash controller has detected > >>>>>>+ a short or open circuit condition on the indicator LED > >>>>>>+ * led-under-voltage - flash controller voltage to the flash > >>>>>>+ LED has been below the minimum limit specific to > >>>>>>+ the flash > >>>>>>+ * controller-under-voltage - the input voltage of the flash > >>>>>>+ controller is below the limit under which strobing the > >>>>>>+ flash at full current will not be possible. The condition > >>>>>>+ persists until this flag is no longer set > >>>>>>+ * led-over-temperature - the temperature of the LED has exceeded > >>>>>>+ its allowed upper limit > >>>>>>+ > >>>>>>+ Flash faults are cleared, if possible, by reading the attribute. > >>>>> > >>>>>That's bad. Now you can no longer present flash_fault file as readable > >>>>>to non-root users, and grep -ri foo /sys will interfere with your > >>>>>camera application. > >>>>> > >>>>>Bad interface, just fix it. > >>>> > >>>>In my opinion it isn't crucial for the user to be aware of the > >>>>fact that some non-persistent fault happened right after strobing the > >>>>flash (e.g. over temperature). > >>>> > >>>>I cannot see anything harmful in the situation when someone does grep > >>>>on /sys and clears non-persistent fault on a flash LED device. > >>> > >>>So why export the faults at all? > >> > >>Faults may prevent strobing the flash in case of some devices. > >>The example of such a device is ADP1663 (drivers/media/i2c/adp1653.c). > >>This driver reads the faults before strobing the flash and if a > >>fault preventing strobing has occurred it returns -EBUSY. > >> > >>If this driver was made a LED Flash class driver, then it would > >>expose flash_faults attribute. The driver would probably need > >>redesigning - checking the faults before strobing would have to be > >>avoided and it should be left to the userspace. > > > >That's fine, but Pavel's point is that you shouldn't "clear a fault" by > >reading a sysfs file as you don't control who reads all sysfs files > >(hint, libudev might cache all attributes when they are found / change, > >which could prevent anyone else from seeing that fault.) > > > >So please fix this, make a write to clear a fault or some other such > >explicit action, not a simple read. That's not an acceptable api. > > I am aware what Pavel'a point was, I just presented the arguments > justifying existence of the flash_faults attribute at all. Fine. Then, you should understand what you need to fix at this point. > In my opinion flash_faults attribute should report the current state of > the device. For the devices which clear the faults on I2C readout the > faults read would have to be cached in the driver, until they are > explicitly cleared, to keep the sysfs interface consistent. Yes, just do the caching. > Nonetheless, there can be also devices which don't require clearing the > faults - they are reported only when the actual condition occurs, > e.g. over temperature or under voltage. When the related value gets > back to the acceptable level the fault is no longer reported by the > device. > > In this case some faults will remain unnoticed by the user space. This > is the argument in favour of my statement that caching the faults does > not make a sense and is not crucial. The user's vital interest is to > know whether the flash LED is operational right before strobing. You can still do caching, exactly the same way. If you want current and previous faults, do the read. If you want currently active faults, you do write then read. > Since we cannot guarantee reporting all the faults that occurred for > all possible flash LED devices, the only sensible solution is to report > only the currently valid fault. How do you propose to do that on devices that clear on read? [Actually, you could _always_ do two reads on those devices, discard first result, and return the second. But I'm not sure how hardware will like that.] Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: Reading /sys with side effects (was Re: [PATCH 1/2] Documentation: leds: Add description of LED Flash class extension) 2015-02-02 9:44 ` Pavel Machek @ 2015-02-02 11:55 ` Jacek Anaszewski 2015-02-02 13:51 ` Pavel Machek 0 siblings, 1 reply; 19+ messages in thread From: Jacek Anaszewski @ 2015-02-02 11:55 UTC (permalink / raw) To: Pavel Machek Cc: Greg KH, kernel list, linux-leds, devicetree, kyungmin.park, b.zolnierkie, cooloney, rpurdie, sakari.ailus, s.nawrocki On 02/02/2015 10:44 AM, Pavel Machek wrote: > On Mon 2015-02-02 10:07:02, Jacek Anaszewski wrote: >> On 01/30/2015 05:40 PM, Greg KH wrote: >>> On Fri, Jan 30, 2015 at 09:55:30AM +0100, Jacek Anaszewski wrote: >>>> Hi Pavel, >>>> >>>> On 01/29/2015 10:14 PM, Pavel Machek wrote: >>>>> Hi! >>>>> >>>>>>>> + - flash_fault - list of flash faults that may have occurred: >>>>>>>> + * led-over-voltage - flash controller voltage to the flash LED >>>>>>>> + has exceededthe limit specific to the flash controller >>>>>>>> + * flash-timeout-exceeded - the flash strobe was still on when >>>>>>>> + the timeout set by the user has expired; not all flash >>>>>>>> + controllers may set this in all such conditions >>>>>>>> + * controller-over-temperature - the flash controller has >>>>>>>> + overheated >>>>>>>> + * controller-short-circuit - the short circuit protection >>>>>>>> + of the flash controller has been triggered >>>>>>>> + * led-power-supply-over-current - current in the LED power >>>>>>>> + supply has exceeded the limit specific to the flash >>>>>>>> + controller >>>>>>>> + * indicator-led-fault - the flash controller has detected >>>>>>>> + a short or open circuit condition on the indicator LED >>>>>>>> + * led-under-voltage - flash controller voltage to the flash >>>>>>>> + LED has been below the minimum limit specific to >>>>>>>> + the flash >>>>>>>> + * controller-under-voltage - the input voltage of the flash >>>>>>>> + controller is below the limit under which strobing the >>>>>>>> + flash at full current will not be possible. The condition >>>>>>>> + persists until this flag is no longer set >>>>>>>> + * led-over-temperature - the temperature of the LED has exceeded >>>>>>>> + its allowed upper limit >>>>>>>> + >>>>>>>> + Flash faults are cleared, if possible, by reading the attribute. >>>>>>> >>>>>>> That's bad. Now you can no longer present flash_fault file as readable >>>>>>> to non-root users, and grep -ri foo /sys will interfere with your >>>>>>> camera application. >>>>>>> >>>>>>> Bad interface, just fix it. >>>>>> >>>>>> In my opinion it isn't crucial for the user to be aware of the >>>>>> fact that some non-persistent fault happened right after strobing the >>>>>> flash (e.g. over temperature). >>>>>> >>>>>> I cannot see anything harmful in the situation when someone does grep >>>>>> on /sys and clears non-persistent fault on a flash LED device. >>>>> >>>>> So why export the faults at all? >>>> >>>> Faults may prevent strobing the flash in case of some devices. >>>> The example of such a device is ADP1663 (drivers/media/i2c/adp1653.c). >>>> This driver reads the faults before strobing the flash and if a >>>> fault preventing strobing has occurred it returns -EBUSY. >>>> >>>> If this driver was made a LED Flash class driver, then it would >>>> expose flash_faults attribute. The driver would probably need >>>> redesigning - checking the faults before strobing would have to be >>>> avoided and it should be left to the userspace. >>> >>> That's fine, but Pavel's point is that you shouldn't "clear a fault" by >>> reading a sysfs file as you don't control who reads all sysfs files >>> (hint, libudev might cache all attributes when they are found / change, >>> which could prevent anyone else from seeing that fault.) >>> >>> So please fix this, make a write to clear a fault or some other such >>> explicit action, not a simple read. That's not an acceptable api. >> >> I am aware what Pavel'a point was, I just presented the arguments >> justifying existence of the flash_faults attribute at all. > > Fine. Then, you should understand what you need to fix at this point. > >> In my opinion flash_faults attribute should report the current state of >> the device. For the devices which clear the faults on I2C readout the >> faults read would have to be cached in the driver, until they are >> explicitly cleared, to keep the sysfs interface consistent. > > Yes, just do the caching. > >> Nonetheless, there can be also devices which don't require clearing the >> faults - they are reported only when the actual condition occurs, >> e.g. over temperature or under voltage. When the related value gets >> back to the acceptable level the fault is no longer reported by the >> device. >> >> In this case some faults will remain unnoticed by the user space. This >> is the argument in favour of my statement that caching the faults does >> not make a sense and is not crucial. The user's vital interest is to >> know whether the flash LED is operational right before strobing. > > You can still do caching, exactly the same way. If you want current > and previous faults, do the read. If you want currently active faults, > you do write then read. > >> Since we cannot guarantee reporting all the faults that occurred for >> all possible flash LED devices, the only sensible solution is to report >> only the currently valid fault. > > How do you propose to do that on devices that clear on read? > > [Actually, you could _always_ do two reads on those devices, discard > first result, and return the second. But I'm not sure how hardware > will like that.] This would be the most sensible option. However, let's analyze the typical use cases for flash strobing: ------------------------------------------------------------- >>>>>>>> Version without faults caching: ============ Driver side: ============ read_faults() faults = read_i2c(); //read faults if faults write_i2c(); //clear faults, only for some devices faults = read_i2c(); //read faults return faults ================ User space side: ================ 1. faults = `cat flash_faults` //read_faults() 2. if faults then print "Unable to strobe the flash LED due to faults" else echo 1 > flash_strobe >>>>>>>> Version with faults caching: ============ Driver side: ============ read_faults() faults |= read_i2c(); //read faults clear_faults() write_i2c(); //clear faults faults = 0; ================ User space side: ================ 1. faults = `cat flash_faults` //read_faults() 2. if faults then echo 0 > flash_faults //clear_faults() faults = `cat flash_faults` //read_faults() 3, if !faults echo 1 > flash_strobe else print "Unable to strobe the flash LED due to faults" ------------------------------------------------------------- From the above it seems that version with clearing faults on read results in the simpler flash strobing procedure on userspace side, by the cost of additional bus access on the driver side. If the more complicated flash strobing procedure is acceptable for everyone I will change the interface. BTW, I think, that we don't need additional attribute, just writing the flash_faults attribute can do the clearing. -- Best Regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: Reading /sys with side effects (was Re: [PATCH 1/2] Documentation: leds: Add description of LED Flash class extension) 2015-02-02 11:55 ` Jacek Anaszewski @ 2015-02-02 13:51 ` Pavel Machek 2015-02-02 14:51 ` Jacek Anaszewski 0 siblings, 1 reply; 19+ messages in thread From: Pavel Machek @ 2015-02-02 13:51 UTC (permalink / raw) To: Jacek Anaszewski Cc: Greg KH, kernel list, linux-leds, devicetree, kyungmin.park, b.zolnierkie, cooloney, rpurdie, sakari.ailus, s.nawrocki Hi! > >[Actually, you could _always_ do two reads on those devices, discard > >first result, and return the second. But I'm not sure how hardware > >will like that.] > > This would be the most sensible option. > > > However, let's analyze the typical use cases for flash strobing: > > > ------------------------------------------------------------- > > > >>>>>>>> Version without faults caching: > > ============ > Driver side: > ============ > > read_faults() > faults = read_i2c(); //read faults > if faults > write_i2c(); //clear faults, only for some devices > faults = read_i2c(); //read faults > return faults > > ================ > User space side: > ================ > > 1. faults = `cat flash_faults` //read_faults() > 2. if faults then > print "Unable to strobe the flash LED due to faults" > else > echo 1 > flash_strobe > > > >>>>>>>> Version with faults caching: > > ============ > Driver side: > ============ > > read_faults() > faults |= read_i2c(); //read faults > > clear_faults() > write_i2c(); //clear faults > faults = 0; > > > ================ > User space side: > ================ > > 1. faults = `cat flash_faults` //read_faults() > 2. if faults then > echo 0 > flash_faults //clear_faults() > faults = `cat flash_faults` //read_faults() > 3, if !faults > echo 1 > flash_strobe > else > print "Unable to strobe the flash LED due to faults" > > > ------------------------------------------------------------- > > From the above it seems that version with clearing faults on read > results in the simpler flash strobing procedure on userspace side, > by the cost of additional bus access on the driver side. I like caching version more (as it will allow by-hand debugging of "why did not flash fire? Aha, lets see in the file, there was fault), but both should be acceptable. > we don't need additional attribute, just writing the flash_faults > attribute can do the clearing. Yes, writing flash_faults to clear is acceptable. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: Reading /sys with side effects (was Re: [PATCH 1/2] Documentation: leds: Add description of LED Flash class extension) 2015-02-02 13:51 ` Pavel Machek @ 2015-02-02 14:51 ` Jacek Anaszewski 0 siblings, 0 replies; 19+ messages in thread From: Jacek Anaszewski @ 2015-02-02 14:51 UTC (permalink / raw) To: Pavel Machek Cc: Greg KH, kernel list, linux-leds, devicetree, kyungmin.park, b.zolnierkie, cooloney, rpurdie, sakari.ailus, s.nawrocki Hi Pavel, On 02/02/2015 02:51 PM, Pavel Machek wrote: > Hi! > >>> [Actually, you could _always_ do two reads on those devices, discard >>> first result, and return the second. But I'm not sure how hardware >>> will like that.] >> >> This would be the most sensible option. >> >> >> However, let's analyze the typical use cases for flash strobing: >> >> >> ------------------------------------------------------------- >> >> >>>>>>>>>> Version without faults caching: >> >> ============ >> Driver side: >> ============ >> >> read_faults() >> faults = read_i2c(); //read faults >> if faults >> write_i2c(); //clear faults, only for some devices >> faults = read_i2c(); //read faults >> return faults >> >> ================ >> User space side: >> ================ >> >> 1. faults = `cat flash_faults` //read_faults() >> 2. if faults then >> print "Unable to strobe the flash LED due to faults" >> else >> echo 1 > flash_strobe >> >> >>>>>>>>>> Version with faults caching: >> >> ============ >> Driver side: >> ============ >> >> read_faults() >> faults |= read_i2c(); //read faults >> >> clear_faults() >> write_i2c(); //clear faults >> faults = 0; >> >> >> ================ >> User space side: >> ================ >> >> 1. faults = `cat flash_faults` //read_faults() >> 2. if faults then >> echo 0 > flash_faults //clear_faults() >> faults = `cat flash_faults` //read_faults() >> 3, if !faults >> echo 1 > flash_strobe >> else >> print "Unable to strobe the flash LED due to faults" >> >> >> ------------------------------------------------------------- >> >> From the above it seems that version with clearing faults on read >> results in the simpler flash strobing procedure on userspace side, >> by the cost of additional bus access on the driver side. > > I like caching version more (as it will allow by-hand debugging of > "why did not flash fire? Aha, lets see in the file, there was fault), > but both should be acceptable. > >> we don't need additional attribute, just writing the flash_faults >> attribute can do the clearing. > > Yes, writing flash_faults to clear is acceptable. I've been just inspired with another approach: Faults register is read in the strobe_set callback, right after sending flash strobe command to the device. The userspace can read the cached faults through flash_faults attribute. This way, we avoid reading sysfs attribute with side effect and gain the possibility of giving immediate feedback to the user. Exemplary use case: 1. echo 1 > flash_strobe write_i2c(); //strobe flash faults = read_i2c(); //read faults if faults return -EINVAL; return 0; 2. cat flash_faults return faults; -- Best Regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2015-02-02 14:51 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-27 8:07 [PATCH 1/2] Documentation: leds: Add description of LED Flash class extension Jacek Anaszewski
2015-01-27 8:07 ` [PATCH 2/2] DT: leds: Add led-sources property Jacek Anaszewski
2015-01-29 14:55 ` Jacek Anaszewski
2015-01-29 20:28 ` Pavel Machek
2015-01-29 21:03 ` Rob Herring
2015-01-29 22:14 ` Bryan Wu
[not found] ` <CAK5ve-LKBfjRzxK_b0ByEF5uWHfAaBGS2OPznOiYNg9+FJP5Gg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2015-01-29 22:30 ` Pavel Machek
2015-01-29 22:59 ` Bryan Wu
2015-01-27 22:37 ` Reading /sys with side effects (was Re: [PATCH 1/2] Documentation: leds: Add description of LED Flash class extension) Pavel Machek
2015-01-28 8:43 ` Jacek Anaszewski
[not found] ` <54C8A130.8000807-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
2015-01-29 21:14 ` Pavel Machek
2015-01-29 21:14 ` Pavel Machek
2015-01-30 8:55 ` Jacek Anaszewski
2015-01-30 16:40 ` Greg KH
2015-02-02 9:07 ` Jacek Anaszewski
2015-02-02 9:44 ` Pavel Machek
2015-02-02 11:55 ` Jacek Anaszewski
2015-02-02 13:51 ` Pavel Machek
2015-02-02 14:51 ` Jacek Anaszewski
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.