| Message ID | 1434599897-22505-2-git-send-email-mranostay@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Matt, On 06/18/2015 05:58 AM, Matt Ranostay wrote: > Signed-off-by: Matt Ranostay <mranostay@gmail.com> > --- > .../devicetree/bindings/input/cap11xx.txt | 25 ++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/Documentation/devicetree/bindings/input/cap11xx.txt b/Documentation/devicetree/bindings/input/cap11xx.txt > index 7d0a300..09cdc43 100644 > --- a/Documentation/devicetree/bindings/input/cap11xx.txt > +++ b/Documentation/devicetree/bindings/input/cap11xx.txt > @@ -38,6 +38,11 @@ Optional properties: > defaults. The array must have exactly six > entries. > > + linux,led-brightness: Defines the ON brightness when the optional LED > + functionality is used. Valid values are 1-15. > + By default a value of 15 is set. > + > + > Example: > > i2c_controller { > @@ -55,5 +60,25 @@ i2c_controller { > <105>, /* KEY_LEFT */ > <109>, /* KEY_PAGEDOWN */ > <104>; /* KEY_PAGEUP */ > + > + linux,led-brightness = <15>; > + #address-cells = <1>; > + #size-cells = <0>; > + > + usr@0 { > + label = "cap11xx:green:usr0"; > + reg = <0>; > + }; > + > + usr@1 { > + label = "cap11xx:green:usr1"; > + reg = <1>; > + }; > + > + alive@2 { > + label = "cap11xx:green:alive"; > + reg = <2>; > + linux,default_trigger = "heartbeat"; > + }; > }; > } > Acked-by: Jacek Anaszewski <j.anaszewski@samsung.com>
On Wed, Jun 17, 2015 at 08:58:16PM -0700, Matt Ranostay wrote: > Signed-off-by: Matt Ranostay <mranostay@gmail.com> > --- > .../devicetree/bindings/input/cap11xx.txt | 25 ++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/Documentation/devicetree/bindings/input/cap11xx.txt b/Documentation/devicetree/bindings/input/cap11xx.txt > index 7d0a300..09cdc43 100644 > --- a/Documentation/devicetree/bindings/input/cap11xx.txt > +++ b/Documentation/devicetree/bindings/input/cap11xx.txt > @@ -38,6 +38,11 @@ Optional properties: > defaults. The array must have exactly six > entries. > > + linux,led-brightness: Defines the ON brightness when the optional LED > + functionality is used. Valid values are 1-15. > + By default a value of 15 is set. Please mention the device does not allow controlling brightness of leds individually and that is why this property is at device level, not individual led level. Also, why does it have "linux" prefix? It does not appear to control any linux-specific functionality. > + > + > Example: > > i2c_controller { > @@ -55,5 +60,25 @@ i2c_controller { > <105>, /* KEY_LEFT */ > <109>, /* KEY_PAGEDOWN */ > <104>; /* KEY_PAGEUP */ > + > + linux,led-brightness = <15>; > + #address-cells = <1>; > + #size-cells = <0>; > + > + usr@0 { > + label = "cap11xx:green:usr0"; > + reg = <0>; > + }; > + > + usr@1 { > + label = "cap11xx:green:usr1"; > + reg = <1>; > + }; > + > + alive@2 { > + label = "cap11xx:green:alive"; > + reg = <2>; > + linux,default_trigger = "heartbeat"; > + }; > }; > } > -- > 1.9.1 > Thanks.
On 06/22/2015 07:59 PM, Dmitry Torokhov wrote: > On Wed, Jun 17, 2015 at 08:58:16PM -0700, Matt Ranostay wrote: >> Signed-off-by: Matt Ranostay <mranostay@gmail.com> >> --- >> .../devicetree/bindings/input/cap11xx.txt | 25 ++++++++++++++++++++++ >> 1 file changed, 25 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/input/cap11xx.txt b/Documentation/devicetree/bindings/input/cap11xx.txt >> index 7d0a300..09cdc43 100644 >> --- a/Documentation/devicetree/bindings/input/cap11xx.txt >> +++ b/Documentation/devicetree/bindings/input/cap11xx.txt >> @@ -38,6 +38,11 @@ Optional properties: >> defaults. The array must have exactly six >> entries. >> >> + linux,led-brightness: Defines the ON brightness when the optional LED >> + functionality is used. Valid values are 1-15. >> + By default a value of 15 is set. > > Please mention the device does not allow controlling brightness of leds > individually and that is why this property is at device level, not > individual led level. I've just noticed that we have drivers/leds/leds-netxbig.c driver, which also doesn't allow controlling the LEDs on extension board individually, but it still does allow changing their brightness. I am leaning towards allowing this also for this driver and adding similar comment in the source code like at the line 218 of the aforementioned driver. As a result this property wouldn't be required. > Also, why does it have "linux" prefix? It does not appear to control > any linux-specific functionality.
On Tue, Jun 23, 2015 at 1:36 AM, Jacek Anaszewski <j.anaszewski@samsung.com> wrote: > On 06/22/2015 07:59 PM, Dmitry Torokhov wrote: >> >> On Wed, Jun 17, 2015 at 08:58:16PM -0700, Matt Ranostay wrote: >>> >>> Signed-off-by: Matt Ranostay <mranostay@gmail.com> >>> --- >>> .../devicetree/bindings/input/cap11xx.txt | 25 >>> ++++++++++++++++++++++ >>> 1 file changed, 25 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/input/cap11xx.txt >>> b/Documentation/devicetree/bindings/input/cap11xx.txt >>> index 7d0a300..09cdc43 100644 >>> --- a/Documentation/devicetree/bindings/input/cap11xx.txt >>> +++ b/Documentation/devicetree/bindings/input/cap11xx.txt >>> @@ -38,6 +38,11 @@ Optional properties: >>> defaults. The array must have exactly six >>> entries. >>> >>> + linux,led-brightness: Defines the ON brightness when the >>> optional LED >>> + functionality is used. Valid values are >>> 1-15. >>> + By default a value of 15 is set. >> >> >> Please mention the device does not allow controlling brightness of leds >> individually and that is why this property is at device level, not >> individual led level. > > > I've just noticed that we have drivers/leds/leds-netxbig.c driver, which > also doesn't allow controlling the LEDs on extension board individually, > but it still does allow changing their brightness. I am leaning towards > allowing this also for this driver and adding similar comment in the > source code like at the line 218 of the aforementioned driver. > As a result this property wouldn't be required. > Ok that should be pretty simple to do. But seems kind weird to have each led channel to be changing the brightness of all. Wouldn't the brightness sysfs entries of the other led channels be showing incorrect values? >> Also, why does it have "linux" prefix? It does not appear to control >> any linux-specific functionality. > > > > -- > Best Regards, > Jacek Anaszewski -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 23, 2015 at 10:23:57AM -0700, Matt Ranostay wrote: > On Tue, Jun 23, 2015 at 1:36 AM, Jacek Anaszewski > <j.anaszewski@samsung.com> wrote: > > On 06/22/2015 07:59 PM, Dmitry Torokhov wrote: > >> > >> On Wed, Jun 17, 2015 at 08:58:16PM -0700, Matt Ranostay wrote: > >>> > >>> Signed-off-by: Matt Ranostay <mranostay@gmail.com> > >>> --- > >>> .../devicetree/bindings/input/cap11xx.txt | 25 > >>> ++++++++++++++++++++++ > >>> 1 file changed, 25 insertions(+) > >>> > >>> diff --git a/Documentation/devicetree/bindings/input/cap11xx.txt > >>> b/Documentation/devicetree/bindings/input/cap11xx.txt > >>> index 7d0a300..09cdc43 100644 > >>> --- a/Documentation/devicetree/bindings/input/cap11xx.txt > >>> +++ b/Documentation/devicetree/bindings/input/cap11xx.txt > >>> @@ -38,6 +38,11 @@ Optional properties: > >>> defaults. The array must have exactly six > >>> entries. > >>> > >>> + linux,led-brightness: Defines the ON brightness when the > >>> optional LED > >>> + functionality is used. Valid values are > >>> 1-15. > >>> + By default a value of 15 is set. > >> > >> > >> Please mention the device does not allow controlling brightness of leds > >> individually and that is why this property is at device level, not > >> individual led level. > > > > > > I've just noticed that we have drivers/leds/leds-netxbig.c driver, which > > also doesn't allow controlling the LEDs on extension board individually, > > but it still does allow changing their brightness. I am leaning towards > > allowing this also for this driver and adding similar comment in the > > source code like at the line 218 of the aforementioned driver. > > As a result this property wouldn't be required. > > > > Ok that should be pretty simple to do. But seems kind weird to have > each led channel to be changing the brightness of all. Wouldn't the > brightness sysfs entries of the other led channels be showing > incorrect values? I agree, this is kind of weird. Maybe we should have a device-specific attribute (on the platform device level) that allows controlling overall brightness, but I think LEDs should be just on/off with max brightness of 1. Userspace should not have to be aware about the fact that on that particular device LEDs are not completely independent as far as their brightness goes. Thanks.
On Tue, Jun 23, 2015 at 11:07 AM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > On Tue, Jun 23, 2015 at 10:23:57AM -0700, Matt Ranostay wrote: >> On Tue, Jun 23, 2015 at 1:36 AM, Jacek Anaszewski >> <j.anaszewski@samsung.com> wrote: >> > On 06/22/2015 07:59 PM, Dmitry Torokhov wrote: >> >> >> >> On Wed, Jun 17, 2015 at 08:58:16PM -0700, Matt Ranostay wrote: >> >>> >> >>> Signed-off-by: Matt Ranostay <mranostay@gmail.com> >> >>> --- >> >>> .../devicetree/bindings/input/cap11xx.txt | 25 >> >>> ++++++++++++++++++++++ >> >>> 1 file changed, 25 insertions(+) >> >>> >> >>> diff --git a/Documentation/devicetree/bindings/input/cap11xx.txt >> >>> b/Documentation/devicetree/bindings/input/cap11xx.txt >> >>> index 7d0a300..09cdc43 100644 >> >>> --- a/Documentation/devicetree/bindings/input/cap11xx.txt >> >>> +++ b/Documentation/devicetree/bindings/input/cap11xx.txt >> >>> @@ -38,6 +38,11 @@ Optional properties: >> >>> defaults. The array must have exactly six >> >>> entries. >> >>> >> >>> + linux,led-brightness: Defines the ON brightness when the >> >>> optional LED >> >>> + functionality is used. Valid values are >> >>> 1-15. >> >>> + By default a value of 15 is set. >> >> >> >> >> >> Please mention the device does not allow controlling brightness of leds >> >> individually and that is why this property is at device level, not >> >> individual led level. >> > >> > >> > I've just noticed that we have drivers/leds/leds-netxbig.c driver, which >> > also doesn't allow controlling the LEDs on extension board individually, >> > but it still does allow changing their brightness. I am leaning towards >> > allowing this also for this driver and adding similar comment in the >> > source code like at the line 218 of the aforementioned driver. >> > As a result this property wouldn't be required. >> > >> >> Ok that should be pretty simple to do. But seems kind weird to have >> each led channel to be changing the brightness of all. Wouldn't the >> brightness sysfs entries of the other led channels be showing >> incorrect values? > > I agree, this is kind of weird. Maybe we should have a device-specific > attribute (on the platform device level) that allows controlling overall > brightness, but I think LEDs should be just on/off with max brightness > of 1. Userspace should not have to be aware about the fact that on that > particular device LEDs are not completely independent as far as their > brightness goes. So should I drop the devicetree part of the patch in v6? > > Thanks. > > -- > Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 23, 2015 at 11:24:42AM -0700, Matt Ranostay wrote: > On Tue, Jun 23, 2015 at 11:07 AM, Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > > On Tue, Jun 23, 2015 at 10:23:57AM -0700, Matt Ranostay wrote: > >> On Tue, Jun 23, 2015 at 1:36 AM, Jacek Anaszewski > >> <j.anaszewski@samsung.com> wrote: > >> > On 06/22/2015 07:59 PM, Dmitry Torokhov wrote: > >> >> > >> >> On Wed, Jun 17, 2015 at 08:58:16PM -0700, Matt Ranostay wrote: > >> >>> > >> >>> Signed-off-by: Matt Ranostay <mranostay@gmail.com> > >> >>> --- > >> >>> .../devicetree/bindings/input/cap11xx.txt | 25 > >> >>> ++++++++++++++++++++++ > >> >>> 1 file changed, 25 insertions(+) > >> >>> > >> >>> diff --git a/Documentation/devicetree/bindings/input/cap11xx.txt > >> >>> b/Documentation/devicetree/bindings/input/cap11xx.txt > >> >>> index 7d0a300..09cdc43 100644 > >> >>> --- a/Documentation/devicetree/bindings/input/cap11xx.txt > >> >>> +++ b/Documentation/devicetree/bindings/input/cap11xx.txt > >> >>> @@ -38,6 +38,11 @@ Optional properties: > >> >>> defaults. The array must have exactly six > >> >>> entries. > >> >>> > >> >>> + linux,led-brightness: Defines the ON brightness when the > >> >>> optional LED > >> >>> + functionality is used. Valid values are > >> >>> 1-15. > >> >>> + By default a value of 15 is set. > >> >> > >> >> > >> >> Please mention the device does not allow controlling brightness of leds > >> >> individually and that is why this property is at device level, not > >> >> individual led level. > >> > > >> > > >> > I've just noticed that we have drivers/leds/leds-netxbig.c driver, which > >> > also doesn't allow controlling the LEDs on extension board individually, > >> > but it still does allow changing their brightness. I am leaning towards > >> > allowing this also for this driver and adding similar comment in the > >> > source code like at the line 218 of the aforementioned driver. > >> > As a result this property wouldn't be required. > >> > > >> > >> Ok that should be pretty simple to do. But seems kind weird to have > >> each led channel to be changing the brightness of all. Wouldn't the > >> brightness sysfs entries of the other led channels be showing > >> incorrect values? > > > > I agree, this is kind of weird. Maybe we should have a device-specific > > attribute (on the platform device level) that allows controlling overall > > brightness, but I think LEDs should be just on/off with max brightness > > of 1. Userspace should not have to be aware about the fact that on that > > particular device LEDs are not completely independent as far as their > > brightness goes. > > So should I drop the devicetree part of the patch in v6? I think so. Let's have the device come up with LEDs brightness at default value and then users can adjust it as they need via sysfs. Thanks.
On 06/23/2015 08:07 PM, Dmitry Torokhov wrote: > On Tue, Jun 23, 2015 at 10:23:57AM -0700, Matt Ranostay wrote: >> On Tue, Jun 23, 2015 at 1:36 AM, Jacek Anaszewski >> <j.anaszewski@samsung.com> wrote: >>> On 06/22/2015 07:59 PM, Dmitry Torokhov wrote: >>>> >>>> On Wed, Jun 17, 2015 at 08:58:16PM -0700, Matt Ranostay wrote: >>>>> >>>>> Signed-off-by: Matt Ranostay <mranostay@gmail.com> >>>>> --- >>>>> .../devicetree/bindings/input/cap11xx.txt | 25 >>>>> ++++++++++++++++++++++ >>>>> 1 file changed, 25 insertions(+) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/input/cap11xx.txt >>>>> b/Documentation/devicetree/bindings/input/cap11xx.txt >>>>> index 7d0a300..09cdc43 100644 >>>>> --- a/Documentation/devicetree/bindings/input/cap11xx.txt >>>>> +++ b/Documentation/devicetree/bindings/input/cap11xx.txt >>>>> @@ -38,6 +38,11 @@ Optional properties: >>>>> defaults. The array must have exactly six >>>>> entries. >>>>> >>>>> + linux,led-brightness: Defines the ON brightness when the >>>>> optional LED >>>>> + functionality is used. Valid values are >>>>> 1-15. >>>>> + By default a value of 15 is set. >>>> >>>> >>>> Please mention the device does not allow controlling brightness of leds >>>> individually and that is why this property is at device level, not >>>> individual led level. >>> >>> >>> I've just noticed that we have drivers/leds/leds-netxbig.c driver, which >>> also doesn't allow controlling the LEDs on extension board individually, >>> but it still does allow changing their brightness. I am leaning towards >>> allowing this also for this driver and adding similar comment in the >>> source code like at the line 218 of the aforementioned driver. >>> As a result this property wouldn't be required. >>> >> >> Ok that should be pretty simple to do. But seems kind weird to have >> each led channel to be changing the brightness of all. Wouldn't the >> brightness sysfs entries of the other led channels be showing >> incorrect values? If you implemented brightness_get op it would show proper value. Assuming that the device allows reading current brightness. > I agree, this is kind of weird. Maybe we should have a device-specific > attribute (on the platform device level) that allows controlling overall > brightness, but I think LEDs should be just on/off with max brightness > of 1. Userspace should not have to be aware about the fact that on that > particular device LEDs are not completely independent as far as their > brightness goes. This way we are removing the possibility of controlling LED brightness at all, the feature that the hardware supports. Anyway, we will have to add a mechanism to the LED subsystem for detecting which LED class devices are controlled by the same hardware. This will allow for describing this type of dependencies. This is on "to do" list.
On 06/24/2015 09:28 AM, Jacek Anaszewski wrote: > On 06/23/2015 08:07 PM, Dmitry Torokhov wrote: >> On Tue, Jun 23, 2015 at 10:23:57AM -0700, Matt Ranostay wrote: >>> On Tue, Jun 23, 2015 at 1:36 AM, Jacek Anaszewski >>> <j.anaszewski@samsung.com> wrote: >>>> On 06/22/2015 07:59 PM, Dmitry Torokhov wrote: >>>>> >>>>> On Wed, Jun 17, 2015 at 08:58:16PM -0700, Matt Ranostay wrote: >>>>>> >>>>>> Signed-off-by: Matt Ranostay <mranostay@gmail.com> >>>>>> --- >>>>>> .../devicetree/bindings/input/cap11xx.txt | 25 >>>>>> ++++++++++++++++++++++ >>>>>> 1 file changed, 25 insertions(+) >>>>>> >>>>>> diff --git a/Documentation/devicetree/bindings/input/cap11xx.txt >>>>>> b/Documentation/devicetree/bindings/input/cap11xx.txt >>>>>> index 7d0a300..09cdc43 100644 >>>>>> --- a/Documentation/devicetree/bindings/input/cap11xx.txt >>>>>> +++ b/Documentation/devicetree/bindings/input/cap11xx.txt >>>>>> @@ -38,6 +38,11 @@ Optional properties: >>>>>> defaults. The array must have >>>>>> exactly six >>>>>> entries. >>>>>> >>>>>> + linux,led-brightness: Defines the ON brightness when the >>>>>> optional LED >>>>>> + functionality is used. Valid >>>>>> values are >>>>>> 1-15. >>>>>> + By default a value of 15 is set. >>>>> >>>>> >>>>> Please mention the device does not allow controlling brightness of >>>>> leds >>>>> individually and that is why this property is at device level, not >>>>> individual led level. >>>> >>>> >>>> I've just noticed that we have drivers/leds/leds-netxbig.c driver, >>>> which >>>> also doesn't allow controlling the LEDs on extension board >>>> individually, >>>> but it still does allow changing their brightness. I am leaning towards >>>> allowing this also for this driver and adding similar comment in the >>>> source code like at the line 218 of the aforementioned driver. >>>> As a result this property wouldn't be required. >>>> >>> >>> Ok that should be pretty simple to do. But seems kind weird to have >>> each led channel to be changing the brightness of all. Wouldn't the >>> brightness sysfs entries of the other led channels be showing >>> incorrect values? > > If you implemented brightness_get op it would show proper value. > Assuming that the device allows reading current brightness. Of course brightness_get wouldn't have to read the brightness from the device, but return the value cached on on each call to brightness_set for any LED class device exposed by the driver.
diff --git a/Documentation/devicetree/bindings/input/cap11xx.txt b/Documentation/devicetree/bindings/input/cap11xx.txt index 7d0a300..09cdc43 100644 --- a/Documentation/devicetree/bindings/input/cap11xx.txt +++ b/Documentation/devicetree/bindings/input/cap11xx.txt @@ -38,6 +38,11 @@ Optional properties: defaults. The array must have exactly six entries. + linux,led-brightness: Defines the ON brightness when the optional LED + functionality is used. Valid values are 1-15. + By default a value of 15 is set. + + Example: i2c_controller { @@ -55,5 +60,25 @@ i2c_controller { <105>, /* KEY_LEFT */ <109>, /* KEY_PAGEDOWN */ <104>; /* KEY_PAGEUP */ + + linux,led-brightness = <15>; + #address-cells = <1>; + #size-cells = <0>; + + usr@0 { + label = "cap11xx:green:usr0"; + reg = <0>; + }; + + usr@1 { + label = "cap11xx:green:usr1"; + reg = <1>; + }; + + alive@2 { + label = "cap11xx:green:alive"; + reg = <2>; + linux,default_trigger = "heartbeat"; + }; }; }
Signed-off-by: Matt Ranostay <mranostay@gmail.com> --- .../devicetree/bindings/input/cap11xx.txt | 25 ++++++++++++++++++++++ 1 file changed, 25 insertions(+)