| Message ID | 20190404171007.160878-3-ncrews@chromium.org |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [v5,1/3] platform/chrome: wilco_ec: Standardize mailbox interface | expand |
On Thu, Apr 4, 2019 at 10:11 AM Nick Crews <ncrews@chromium.org> wrote: > > We want all backlights for the system keyboard to > use a common name, so the name "platform::kbd_backlight" > would be better than the current "chromeos::kbd_backlight" > name. Normally this wouldn't be worth changing, but the new > Wilco keyboard backlight driver uses the "platform" name. > We want to make it so all Chrome OS devices are consistent, > so we'll change the name here too. > Wondering - who is the "we" you are talking about ? Guenter > The Power Manager daemon only looks for LEDs that match the > pattern "*:kbd_backlight", so this change won't affect that. > > Signed-off-by: Nick Crews <ncrews@chromium.org> > --- > drivers/platform/chrome/cros_kbd_led_backlight.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/platform/chrome/cros_kbd_led_backlight.c b/drivers/platform/chrome/cros_kbd_led_backlight.c > index aa409f0201fb..c56c8405f39c 100644 > --- a/drivers/platform/chrome/cros_kbd_led_backlight.c > +++ b/drivers/platform/chrome/cros_kbd_led_backlight.c > @@ -77,7 +77,7 @@ static int keyboard_led_probe(struct platform_device *pdev) > if (!cdev) > return -ENOMEM; > > - cdev->name = "chromeos::kbd_backlight"; > + cdev->name = "platform::kbd_backlight"; > cdev->max_brightness = ACPI_KEYBOARD_BACKLIGHT_MAX; > cdev->flags |= LED_CORE_SUSPENDRESUME; > cdev->brightness_set = keyboard_led_set_brightness; > -- > 2.20.1 >
On Thu, Apr 4, 2019 at 10:36 AM Guenter Roeck <groeck@google.com> wrote: > > On Thu, Apr 4, 2019 at 10:11 AM Nick Crews <ncrews@chromium.org> wrote: > > > > We want all backlights for the system keyboard to > > use a common name, so the name "platform::kbd_backlight" > > would be better than the current "chromeos::kbd_backlight" > > name. Normally this wouldn't be worth changing, but the new > > Wilco keyboard backlight driver uses the "platform" name. > > We want to make it so all Chrome OS devices are consistent, > > so we'll change the name here too. > > > > Wondering - who is the "we" you are talking about ? This also has a potential of breaking existing setups if somebody did happen to match on entire name instead of suffix. Such changes have to be considered very carefully; at this point I am against of doing this. Thanks.
On Thu, Apr 4, 2019 at 11:43 AM Dmitry Torokhov <dtor@google.com> wrote: > > On Thu, Apr 4, 2019 at 10:36 AM Guenter Roeck <groeck@google.com> wrote: > > > > On Thu, Apr 4, 2019 at 10:11 AM Nick Crews <ncrews@chromium.org> wrote: > > > > > > We want all backlights for the system keyboard to > > > use a common name, so the name "platform::kbd_backlight" > > > would be better than the current "chromeos::kbd_backlight" > > > name. Normally this wouldn't be worth changing, but the new > > > Wilco keyboard backlight driver uses the "platform" name. > > > We want to make it so all Chrome OS devices are consistent, > > > so we'll change the name here too. > > > > > > > Wondering - who is the "we" you are talking about ? You're right, I should have been more precise. I was referring to Pavel, Enric, and myself. Pavel had this opinion here: https://lkml.org/lkml/2019/4/4/1040. I don't know what Pavel meant by "we" in that comment, but I would guess that could mean the other LED maintainers as well? I talked with Enric 1:1 and he didn't see the problem with changing it, though perhaps he was only considering our use of the LED via powerd, and not users in general. I'm guessing Pavel's and Enric's meanings though, excuse me if I am misinterpreting. > > This also has a potential of breaking existing setups if somebody did > happen to match on entire name instead of suffix. Such changes have to > be considered very carefully; at this point I am against of doing > this. Would it make sense to keep the old name as is, and only make the new Wilco name begin with "platform:"? What would you think is best? > > Thanks. > > -- > Dmitry
On Thu, Apr 4, 2019 at 11:41 AM Nick Crews <ncrews@chromium.org> wrote: > > On Thu, Apr 4, 2019 at 11:43 AM Dmitry Torokhov <dtor@google.com> wrote: > > > > On Thu, Apr 4, 2019 at 10:36 AM Guenter Roeck <groeck@google.com> wrote: > > > > > > On Thu, Apr 4, 2019 at 10:11 AM Nick Crews <ncrews@chromium.org> wrote: > > > > > > > > We want all backlights for the system keyboard to > > > > use a common name, so the name "platform::kbd_backlight" > > > > would be better than the current "chromeos::kbd_backlight" > > > > name. Normally this wouldn't be worth changing, but the new > > > > Wilco keyboard backlight driver uses the "platform" name. > > > > We want to make it so all Chrome OS devices are consistent, > > > > so we'll change the name here too. > > > > > > > > > > Wondering - who is the "we" you are talking about ? > > You're right, I should have been more precise. > I was referring to Pavel, Enric, and myself. Pavel had this opinion here: > https://lkml.org/lkml/2019/4/4/1040. I don't know what Pavel meant by "we" > in that comment, but I would guess that could mean the other LED maintainers > as well? I talked with Enric 1:1 and he didn't see the problem with changing it, > though perhaps he was only considering our use of the LED via powerd, > and not users in general. I'm guessing Pavel's and Enric's meanings though, > excuse me if I am misinterpreting. > > > > > This also has a potential of breaking existing setups if somebody did > > happen to match on entire name instead of suffix. Such changes have to > > be considered very carefully; at this point I am against of doing > > this. > > Would it make sense to keep the old name as is, and only make the new > Wilco name begin with "platform:"? What would you think is best? Given that we do not have a single instance of platform::kbd_backlight in kernel at this time I have no idea why Pavel is trying to push this for Wilco driver. Thanks.
Hi! > You're right, I should have been more precise. > I was referring to Pavel, Enric, and myself. Pavel had this opinion here: > https://lkml.org/lkml/2019/4/4/1040. I don't know what Pavel meant by "we" > in that comment, but I would guess that could mean the other LED maintainers > as well? I talked with Enric 1:1 and he didn't see the problem with changing it, > though perhaps he was only considering our use of the LED via powerd, > and not users in general. I'm guessing Pavel's and Enric's meanings though, > excuse me if I am misinterpreting. > > > > > This also has a potential of breaking existing setups if somebody did > > happen to match on entire name instead of suffix. Such changes have to > > be considered very carefully; at this point I am against of doing > > this. > > Would it make sense to keep the old name as is, and only make the new > Wilco name begin with "platform:"? What would you think is best? That works for me. I think we might be able to get away with changing the name (we have some really bad names in the LED subsystem, and it would be nice to fix them), but my priority at the moment is not to extend the problem. Pavel -*- org -*- It is somehow important to provide consistent interface to the userland. LED devices have one problem there, and that is naming of directories in /sys/class/leds. It would be nice if userland would just know right "name" for given LED function, but situation got more complex. Anyway, if backwards compatibility is not an issue, new code should use one of the "good" names from this list, and you should extend the list where applicable. Bad names are listed, too; in case you are writing application that wants to use particular feature, you should probe for good name, first, but then try the bad ones, too. * Keyboards Good: "input*:*:capslock" Good: "input*:*:scrolllock" Good: "input*:*:numlock" Bad: "shift-key-light" (Motorola Droid 4, capslock) Set of common keyboard LEDs, going back to PC AT or so. Good: "platform::kbd_backlight" Bad: "tpacpi::thinklight" (IBM/Lenovo Thinkpads) Bad: "lp5523:kb{1,2,3,4,5,6}" (Nokia N900) Frontlight/backlight of main keyboard. Bad: "button-backlight" (Motorola Droid 4) Some phones have touch buttons below screen; it is different from main keyboard. And this is their backlight. * Sound subsystem Good: "platform:*:mute" Good: "platform:*:micmute" LEDs on notebook body, indicating that sound input / output is muted. * System notification Good: "status-led:{red,green,blue}" (Motorola Droid 4) Bad: "lp5523:{r,g,b}" (Nokia N900) Phones usually have multi-color status LED. * Power management Good: "platform:*:charging" (allwinner sun50i)
On Thu 2019-04-04 11:55:27, Dmitry Torokhov wrote: > On Thu, Apr 4, 2019 at 11:41 AM Nick Crews <ncrews@chromium.org> wrote: > > > > On Thu, Apr 4, 2019 at 11:43 AM Dmitry Torokhov <dtor@google.com> wrote: > > > > > > On Thu, Apr 4, 2019 at 10:36 AM Guenter Roeck <groeck@google.com> wrote: > > > > > > > > On Thu, Apr 4, 2019 at 10:11 AM Nick Crews <ncrews@chromium.org> wrote: > > > > > > > > > > We want all backlights for the system keyboard to > > > > > use a common name, so the name "platform::kbd_backlight" > > > > > would be better than the current "chromeos::kbd_backlight" > > > > > name. Normally this wouldn't be worth changing, but the new > > > > > Wilco keyboard backlight driver uses the "platform" name. > > > > > We want to make it so all Chrome OS devices are consistent, > > > > > so we'll change the name here too. > > > > > > > > > > > > > Wondering - who is the "we" you are talking about ? > > > > You're right, I should have been more precise. > > I was referring to Pavel, Enric, and myself. Pavel had this opinion here: > > https://lkml.org/lkml/2019/4/4/1040. I don't know what Pavel meant by "we" > > in that comment, but I would guess that could mean the other LED maintainers > > as well? I talked with Enric 1:1 and he didn't see the problem with changing it, > > though perhaps he was only considering our use of the LED via powerd, > > and not users in general. I'm guessing Pavel's and Enric's meanings though, > > excuse me if I am misinterpreting. > > > > > > > > This also has a potential of breaking existing setups if somebody did > > > happen to match on entire name instead of suffix. Such changes have to > > > be considered very carefully; at this point I am against of doing > > > this. > > > > Would it make sense to keep the old name as is, and only make the new > > Wilco name begin with "platform:"? What would you think is best? > > Given that we do not have a single instance of platform::kbd_backlight > in kernel at this time I have no idea why Pavel is trying to push this > for Wilco driver. See the documentation in the email I sent few seconds ago. I hope it explains my reasoning, if not, I'll explain it. Pavel
On Thu, Apr 4, 2019 at 11:59 AM Pavel Machek <pavel@ucw.cz> wrote: > > On Thu 2019-04-04 11:55:27, Dmitry Torokhov wrote: > > On Thu, Apr 4, 2019 at 11:41 AM Nick Crews <ncrews@chromium.org> wrote: > > > > > > On Thu, Apr 4, 2019 at 11:43 AM Dmitry Torokhov <dtor@google.com> wrote: > > > > > > > > On Thu, Apr 4, 2019 at 10:36 AM Guenter Roeck <groeck@google.com> wrote: > > > > > > > > > > On Thu, Apr 4, 2019 at 10:11 AM Nick Crews <ncrews@chromium.org> wrote: > > > > > > > > > > > > We want all backlights for the system keyboard to > > > > > > use a common name, so the name "platform::kbd_backlight" > > > > > > would be better than the current "chromeos::kbd_backlight" > > > > > > name. Normally this wouldn't be worth changing, but the new > > > > > > Wilco keyboard backlight driver uses the "platform" name. > > > > > > We want to make it so all Chrome OS devices are consistent, > > > > > > so we'll change the name here too. > > > > > > > > > > > > > > > > Wondering - who is the "we" you are talking about ? > > > > > > You're right, I should have been more precise. > > > I was referring to Pavel, Enric, and myself. Pavel had this opinion here: > > > https://lkml.org/lkml/2019/4/4/1040. I don't know what Pavel meant by "we" > > > in that comment, but I would guess that could mean the other LED maintainers > > > as well? I talked with Enric 1:1 and he didn't see the problem with changing it, > > > though perhaps he was only considering our use of the LED via powerd, > > > and not users in general. I'm guessing Pavel's and Enric's meanings though, > > > excuse me if I am misinterpreting. > > > > > > > > > > > This also has a potential of breaking existing setups if somebody did > > > > happen to match on entire name instead of suffix. Such changes have to > > > > be considered very carefully; at this point I am against of doing > > > > this. > > > > > > Would it make sense to keep the old name as is, and only make the new > > > Wilco name begin with "platform:"? What would you think is best? > > > > Given that we do not have a single instance of platform::kbd_backlight > > in kernel at this time I have no idea why Pavel is trying to push this > > for Wilco driver. > > See the documentation in the email I sent few seconds ago. I hope it > explains my reasoning, if not, I'll explain it. Yes, I see the doc and I do not think I agree with it. If you look at the LED docs you will see: LED Device Naming ================= Is currently of the form: "devicename:colour:function" It is *function* and maybe color that userspace is interested in, and here we have proper standardization in form of "kbd_backlight". Device name is, well, device name. It should uniquely identify the device led is attached to, but otherwise is rarely interesting. If userspace is really concerned what kind of keyboard backlight it is it should investigate parent device(s) and see what they end up with. Thanks. -- Dmitry
On Thu 2019-04-04 12:05:39, Dmitry Torokhov wrote: > On Thu, Apr 4, 2019 at 11:59 AM Pavel Machek <pavel@ucw.cz> wrote: > > > > On Thu 2019-04-04 11:55:27, Dmitry Torokhov wrote: > > > On Thu, Apr 4, 2019 at 11:41 AM Nick Crews <ncrews@chromium.org> wrote: > > > > > > > > On Thu, Apr 4, 2019 at 11:43 AM Dmitry Torokhov <dtor@google.com> wrote: > > > > > > > > > > On Thu, Apr 4, 2019 at 10:36 AM Guenter Roeck <groeck@google.com> wrote: > > > > > > > > > > > > On Thu, Apr 4, 2019 at 10:11 AM Nick Crews <ncrews@chromium.org> wrote: > > > > > > > > > > > > > > We want all backlights for the system keyboard to > > > > > > > use a common name, so the name "platform::kbd_backlight" > > > > > > > would be better than the current "chromeos::kbd_backlight" > > > > > > > name. Normally this wouldn't be worth changing, but the new > > > > > > > Wilco keyboard backlight driver uses the "platform" name. > > > > > > > We want to make it so all Chrome OS devices are consistent, > > > > > > > so we'll change the name here too. > > > > > > > > > > > > > > > > > > > Wondering - who is the "we" you are talking about ? > > > > > > > > You're right, I should have been more precise. > > > > I was referring to Pavel, Enric, and myself. Pavel had this opinion here: > > > > https://lkml.org/lkml/2019/4/4/1040. I don't know what Pavel meant by "we" > > > > in that comment, but I would guess that could mean the other LED maintainers > > > > as well? I talked with Enric 1:1 and he didn't see the problem with changing it, > > > > though perhaps he was only considering our use of the LED via powerd, > > > > and not users in general. I'm guessing Pavel's and Enric's meanings though, > > > > excuse me if I am misinterpreting. > > > > > > > > > > > > > > This also has a potential of breaking existing setups if somebody did > > > > > happen to match on entire name instead of suffix. Such changes have to > > > > > be considered very carefully; at this point I am against of doing > > > > > this. > > > > > > > > Would it make sense to keep the old name as is, and only make the new > > > > Wilco name begin with "platform:"? What would you think is best? > > > > > > Given that we do not have a single instance of platform::kbd_backlight > > > in kernel at this time I have no idea why Pavel is trying to push this > > > for Wilco driver. > > > > See the documentation in the email I sent few seconds ago. I hope it > > explains my reasoning, if not, I'll explain it. > > Yes, I see the doc and I do not think I agree with it. If you look at > the LED docs you will see: If you take a look at mailing list archive, this is currently being worked on. And you might want to take a look at MAINTAINERS file :-) > LED Device Naming > ================= > > Is currently of the form: > > "devicename:colour:function" > > It is *function* and maybe color that userspace is interested in, and > here we have proper standardization in form of "kbd_backlight". Device > name is, well, device name. It should uniquely identify the device led > is attached to, but otherwise is rarely interesting. If userspace is > really concerned what kind of keyboard backlight it is it should > investigate parent device(s) and see what they end up with. That does not work. Userspace wants to know if it is internal keyboard or USB keyboard, not what kind of i2c controller the LED is connected to. grep for platform::mic_mute . (And platform is even pretty good match for how the LED is connected in your case). Pavel
On Thu, Apr 4, 2019 at 12:19 PM Pavel Machek <pavel@ucw.cz> wrote: > > On Thu 2019-04-04 12:05:39, Dmitry Torokhov wrote: > > On Thu, Apr 4, 2019 at 11:59 AM Pavel Machek <pavel@ucw.cz> wrote: > > > > > > On Thu 2019-04-04 11:55:27, Dmitry Torokhov wrote: > > > > On Thu, Apr 4, 2019 at 11:41 AM Nick Crews <ncrews@chromium.org> wrote: > > > > > > > > > > On Thu, Apr 4, 2019 at 11:43 AM Dmitry Torokhov <dtor@google.com> wrote: > > > > > > > > > > > > On Thu, Apr 4, 2019 at 10:36 AM Guenter Roeck <groeck@google.com> wrote: > > > > > > > > > > > > > > On Thu, Apr 4, 2019 at 10:11 AM Nick Crews <ncrews@chromium.org> wrote: > > > > > > > > > > > > > > > > We want all backlights for the system keyboard to > > > > > > > > use a common name, so the name "platform::kbd_backlight" > > > > > > > > would be better than the current "chromeos::kbd_backlight" > > > > > > > > name. Normally this wouldn't be worth changing, but the new > > > > > > > > Wilco keyboard backlight driver uses the "platform" name. > > > > > > > > We want to make it so all Chrome OS devices are consistent, > > > > > > > > so we'll change the name here too. > > > > > > > > > > > > > > > > > > > > > > Wondering - who is the "we" you are talking about ? > > > > > > > > > > You're right, I should have been more precise. > > > > > I was referring to Pavel, Enric, and myself. Pavel had this opinion here: > > > > > https://lkml.org/lkml/2019/4/4/1040. I don't know what Pavel meant by "we" > > > > > in that comment, but I would guess that could mean the other LED maintainers > > > > > as well? I talked with Enric 1:1 and he didn't see the problem with changing it, > > > > > though perhaps he was only considering our use of the LED via powerd, > > > > > and not users in general. I'm guessing Pavel's and Enric's meanings though, > > > > > excuse me if I am misinterpreting. > > > > > > > > > > > > > > > > > This also has a potential of breaking existing setups if somebody did > > > > > > happen to match on entire name instead of suffix. Such changes have to > > > > > > be considered very carefully; at this point I am against of doing > > > > > > this. > > > > > > > > > > Would it make sense to keep the old name as is, and only make the new > > > > > Wilco name begin with "platform:"? What would you think is best? > > > > > > > > Given that we do not have a single instance of platform::kbd_backlight > > > > in kernel at this time I have no idea why Pavel is trying to push this > > > > for Wilco driver. > > > > > > See the documentation in the email I sent few seconds ago. I hope it > > > explains my reasoning, if not, I'll explain it. > > > > Yes, I see the doc and I do not think I agree with it. If you look at > > the LED docs you will see: > > If you take a look at mailing list archive, this is currently being > worked on. And you might want to take a look at MAINTAINERS file :-) So? Being maintainer does not give you free reign to merge stuff that does not quite make sense, you still need to discuss it and convince people. > > > LED Device Naming > > ================= > > > > Is currently of the form: > > > > "devicename:colour:function" > > > > It is *function* and maybe color that userspace is interested in, and > > here we have proper standardization in form of "kbd_backlight". Device > > name is, well, device name. It should uniquely identify the device led > > is attached to, but otherwise is rarely interesting. If userspace is > > really concerned what kind of keyboard backlight it is it should > > investigate parent device(s) and see what they end up with. > > That does not work. Userspace wants to know if it is internal keyboard > or USB keyboard, not what kind of i2c controller the LED is connected > to. Why does userspace want to know it? What does it do differently with backlit external keyboards vs internal ones? And how does it decide if USB keyboard is internal or not given that there are devices with internal USB keyboards? > > grep for platform::mic_mute . > > (And platform is even pretty good match for how the LED is connected > in your case). Until we get a secondary interface that is also "platform"... Thanks.
Hi! > > > It is *function* and maybe color that userspace is interested in, and > > > here we have proper standardization in form of "kbd_backlight". Device > > > name is, well, device name. It should uniquely identify the device led > > > is attached to, but otherwise is rarely interesting. If userspace is > > > really concerned what kind of keyboard backlight it is it should > > > investigate parent device(s) and see what they end up with. > > > > That does not work. Userspace wants to know if it is internal keyboard > > or USB keyboard, not what kind of i2c controller the LED is connected > > to. > > Why does userspace want to know it? For example to turn off backlight on internal keyboard when the lid is closed. > > grep for platform::mic_mute . > > > > (And platform is even pretty good match for how the LED is connected > > in your case). > > Until we get a secondary interface that is also "platform"... How would that happen? Pavel
On Thu, Apr 4, 2019 at 1:06 PM Pavel Machek <pavel@ucw.cz> wrote: > > Hi! > > > > > It is *function* and maybe color that userspace is interested in, and > > > > here we have proper standardization in form of "kbd_backlight". Device > > > > name is, well, device name. It should uniquely identify the device led > > > > is attached to, but otherwise is rarely interesting. If userspace is > > > > really concerned what kind of keyboard backlight it is it should > > > > investigate parent device(s) and see what they end up with. > > > > > > That does not work. Userspace wants to know if it is internal keyboard > > > or USB keyboard, not what kind of i2c controller the LED is connected > > > to. > > > > Why does userspace want to know it? > > For example to turn off backlight on internal keyboard when the lid is closed. Would you not want to turn off external as well? And what to do if internal keyboard is not platform but USB? Like Google "Whiskers"? (I am not sure why you decided to drop my mention of internal USB keyboards completely off your reply). > > > > grep for platform::mic_mute . > > > > > > (And platform is even pretty good match for how the LED is connected > > > in your case). > > > > Until we get a secondary interface that is also "platform"... > > How would that happen? It won't happen on Wilco, but you can't imagine that several blocks get reused in the same device and they end up clashing? Thanks.
On Thu 2019-04-04 13:13:34, Dmitry Torokhov wrote: > On Thu, Apr 4, 2019 at 1:06 PM Pavel Machek <pavel@ucw.cz> wrote: > > > > Hi! > > > > > > > It is *function* and maybe color that userspace is interested in, and > > > > > here we have proper standardization in form of "kbd_backlight". Device > > > > > name is, well, device name. It should uniquely identify the device led > > > > > is attached to, but otherwise is rarely interesting. If userspace is > > > > > really concerned what kind of keyboard backlight it is it should > > > > > investigate parent device(s) and see what they end up with. > > > > > > > > That does not work. Userspace wants to know if it is internal keyboard > > > > or USB keyboard, not what kind of i2c controller the LED is connected > > > > to. > > > > > > Why does userspace want to know it? > > > > For example to turn off backlight on internal keyboard when the lid is closed. > > Would you not want to turn off external as well? Not really. If I'm using machine with lid closed, external monitor and USB keyboard attached, I want to control backlight of USB keyboard, but backlight of internal keyboard can stay off. > And what to do if internal keyboard is not platform but USB? Like > Google "Whiskers"? (I am not sure why you decided to drop my mention > of internal USB keyboards completely off your reply). I don't have answers for everything. Even if you have USB keyboard, you'll likely still have backlight connected to embedded controller. If not, then maybe you have exception userland needs to know about. Still better than making everything an exception. > > > > grep for platform::mic_mute . > > > > > > > > (And platform is even pretty good match for how the LED is connected > > > > in your case). > > > > > > Until we get a secondary interface that is also "platform"... > > > > How would that happen? > > It won't happen on Wilco, but you can't imagine that several blocks > get reused in the same device and they end up clashing? In the end, there will be just one internal keyboard backlight, right? Pavel
On Thu, Apr 4, 2019 at 1:20 PM Pavel Machek <pavel@ucw.cz> wrote: > > On Thu 2019-04-04 13:13:34, Dmitry Torokhov wrote: > > On Thu, Apr 4, 2019 at 1:06 PM Pavel Machek <pavel@ucw.cz> wrote: > > > > > > Hi! > > > > > > > > > It is *function* and maybe color that userspace is interested in, and > > > > > > here we have proper standardization in form of "kbd_backlight". Device > > > > > > name is, well, device name. It should uniquely identify the device led > > > > > > is attached to, but otherwise is rarely interesting. If userspace is > > > > > > really concerned what kind of keyboard backlight it is it should > > > > > > investigate parent device(s) and see what they end up with. > > > > > > > > > > That does not work. Userspace wants to know if it is internal keyboard > > > > > or USB keyboard, not what kind of i2c controller the LED is connected > > > > > to. > > > > > > > > Why does userspace want to know it? > > > > > > For example to turn off backlight on internal keyboard when the lid is closed. > > > > Would you not want to turn off external as well? > > Not really. If I'm using machine with lid closed, external monitor and > USB keyboard attached, I want to control backlight of USB keyboard, > but backlight of internal keyboard can stay off. > > > And what to do if internal keyboard is not platform but USB? Like > > Google "Whiskers"? (I am not sure why you decided to drop my mention > > of internal USB keyboards completely off your reply). > > I don't have answers for everything. Even if you have USB keyboard, you'll > likely still have backlight connected to embedded controller. If not, > then maybe you have exception userland needs to know about. > > Still better than making everything an exception. You do not need to make everything exception. You just need to look beyond the name, and see how the device is connected. And then apply your exceptions for "weird" devices. > > > > > > grep for platform::mic_mute . > > > > > > > > > > (And platform is even pretty good match for how the LED is connected > > > > > in your case). > > > > > > > > Until we get a secondary interface that is also "platform"... > > > > > > How would that happen? > > > > It won't happen on Wilco, but you can't imagine that several blocks > > get reused in the same device and they end up clashing? > > In the end, there will be just one internal keyboard backlight, right? ¯\_(ツ)_/¯ Maybe. Maybe not. Who knows what HW designers will come up with. Thanks.
Hi! > > > And what to do if internal keyboard is not platform but USB? Like > > > Google "Whiskers"? (I am not sure why you decided to drop my mention > > > of internal USB keyboards completely off your reply). > > > > I don't have answers for everything. Even if you have USB keyboard, you'll > > likely still have backlight connected to embedded controller. If not, > > then maybe you have exception userland needs to know about. > > > > Still better than making everything an exception. > > You do not need to make everything exception. You just need to look > beyond the name, and see how the device is connected. And then apply > your exceptions for "weird" devices. "Where it is connected" is not interesting to the userland. "Is it backlight for internal keyboard" is the right question. It may be connected to embedded controller or some kind of controller over i2c... my shell scripts should not need to know about architecture of every notebook out there. But I don't see why I should do additional work when its trivial for kernel to just name the LED in an useful way. "platform::kbd_backlight" has no disadvantages compared to "wilco::kbd_backlight" ... so lets just use it. Pavel
On Thu, Apr 4, 2019 at 1:42 PM Pavel Machek <pavel@ucw.cz> wrote: > > Hi! > > > > > And what to do if internal keyboard is not platform but USB? Like > > > > Google "Whiskers"? (I am not sure why you decided to drop my mention > > > > of internal USB keyboards completely off your reply). > > > > > > I don't have answers for everything. Even if you have USB keyboard, you'll > > > likely still have backlight connected to embedded controller. If not, > > > then maybe you have exception userland needs to know about. > > > > > > Still better than making everything an exception. > > > > You do not need to make everything exception. You just need to look > > beyond the name, and see how the device is connected. And then apply > > your exceptions for "weird" devices. > > "Where it is connected" is not interesting to the userland. "Is it > backlight for internal keyboard" is the right question. It may be > connected to embedded controller or some kind of controller over > i2c... my shell scripts should not need to know about architecture of > every notebook out there. Then your scripts will be failing for some setups. > > But I don't see why I should do additional work when its trivial for > kernel to just name the LED in an useful way. > > "platform::kbd_backlight" has no disadvantages compared to > "wilco::kbd_backlight" ... so lets just use it. It has disadvantages because it promises more than it can deliver IMO. If device name != "platform::kbd_backlight" it does not mean that it is not internal keyboard. And you still have not resolved how you will handle cases when there is more than one deice that can be considered internal and may have a backlight. Thanks.
On Thu 2019-04-04 14:48:35, Dmitry Torokhov wrote: > On Thu, Apr 4, 2019 at 1:42 PM Pavel Machek <pavel@ucw.cz> wrote: > > > > Hi! > > > > > > > And what to do if internal keyboard is not platform but USB? Like > > > > > Google "Whiskers"? (I am not sure why you decided to drop my mention > > > > > of internal USB keyboards completely off your reply). > > > > > > > > I don't have answers for everything. Even if you have USB keyboard, you'll > > > > likely still have backlight connected to embedded controller. If not, > > > > then maybe you have exception userland needs to know about. > > > > > > > > Still better than making everything an exception. > > > > > > You do not need to make everything exception. You just need to look > > > beyond the name, and see how the device is connected. And then apply > > > your exceptions for "weird" devices. > > > > "Where it is connected" is not interesting to the userland. "Is it > > backlight for internal keyboard" is the right question. It may be > > connected to embedded controller or some kind of controller over > > i2c... my shell scripts should not need to know about architecture of > > every notebook out there. > > Then your scripts will be failing for some setups. Well, yes. Do you want to guess what "lp5523:kb3" is? > > But I don't see why I should do additional work when its trivial for > > kernel to just name the LED in an useful way. > > > > "platform::kbd_backlight" has no disadvantages compared to > > "wilco::kbd_backlight" ... so lets just use it. > > It has disadvantages because it promises more than it can deliver IMO. > If device name != "platform::kbd_backlight" it does not mean that it > is not internal keyboard. My promise is if "platform::kbd_backlight" exists, it is backlight for internal keyboard. (And second half is "if it is easy for kernel, we name backlight for internal keyboard platform::kbd_backlight"). > And you still have not resolved how you will > handle cases when there is more than one deice that can be considered > internal and may have a backlight. Is that realistic? How would that device look like? Pavel
On Thu, Apr 4, 2019 at 3:05 PM Pavel Machek <pavel@ucw.cz> wrote: > > On Thu 2019-04-04 14:48:35, Dmitry Torokhov wrote: > > On Thu, Apr 4, 2019 at 1:42 PM Pavel Machek <pavel@ucw.cz> wrote: > > > > > > Hi! > > > > > > > > > And what to do if internal keyboard is not platform but USB? Like > > > > > > Google "Whiskers"? (I am not sure why you decided to drop my mention > > > > > > of internal USB keyboards completely off your reply). > > > > > > > > > > I don't have answers for everything. Even if you have USB keyboard, you'll > > > > > likely still have backlight connected to embedded controller. If not, > > > > > then maybe you have exception userland needs to know about. > > > > > > > > > > Still better than making everything an exception. > > > > > > > > You do not need to make everything exception. You just need to look > > > > beyond the name, and see how the device is connected. And then apply > > > > your exceptions for "weird" devices. > > > > > > "Where it is connected" is not interesting to the userland. "Is it > > > backlight for internal keyboard" is the right question. It may be > > > connected to embedded controller or some kind of controller over > > > i2c... my shell scripts should not need to know about architecture of > > > every notebook out there. > > > > Then your scripts will be failing for some setups. > > Well, yes. Do you want to guess what "lp5523:kb3" is? > Oh, please. The discussion is about the driver name part, which you want to overload with some string to mean "internal", which in turn is, if anything, part of the functionality. With "platform", you'll at some point have two "platform::kbd_backlight" entries. Remind me to send you a "told you so" when that happens. Guenter > > > But I don't see why I should do additional work when its trivial for > > > kernel to just name the LED in an useful way. > > > > > > "platform::kbd_backlight" has no disadvantages compared to > > > "wilco::kbd_backlight" ... so lets just use it. > > > > It has disadvantages because it promises more than it can deliver IMO. > > If device name != "platform::kbd_backlight" it does not mean that it > > is not internal keyboard. > > My promise is if "platform::kbd_backlight" exists, it is backlight for > internal keyboard. (And second half is "if it is easy for kernel, we > name backlight for internal keyboard platform::kbd_backlight"). > > > And you still have not resolved how you will > > handle cases when there is more than one deice that can be considered > > internal and may have a backlight. > > Is that realistic? How would that device look like? > > Pavel > -- > (english) http://www.livejournal.com/~pavelmachek > (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi, On 5/4/19 0:42, Guenter Roeck wrote: > On Thu, Apr 4, 2019 at 3:05 PM Pavel Machek <pavel@ucw.cz> wrote: >> >> On Thu 2019-04-04 14:48:35, Dmitry Torokhov wrote: >>> On Thu, Apr 4, 2019 at 1:42 PM Pavel Machek <pavel@ucw.cz> wrote: >>>> >>>> Hi! >>>> >>>>>>> And what to do if internal keyboard is not platform but USB? Like >>>>>>> Google "Whiskers"? (I am not sure why you decided to drop my mention >>>>>>> of internal USB keyboards completely off your reply). >>>>>> >>>>>> I don't have answers for everything. Even if you have USB keyboard, you'll >>>>>> likely still have backlight connected to embedded controller. If not, >>>>>> then maybe you have exception userland needs to know about. >>>>>> >>>>>> Still better than making everything an exception. >>>>> >>>>> You do not need to make everything exception. You just need to look >>>>> beyond the name, and see how the device is connected. And then apply >>>>> your exceptions for "weird" devices. >>>> >>>> "Where it is connected" is not interesting to the userland. "Is it >>>> backlight for internal keyboard" is the right question. It may be >>>> connected to embedded controller or some kind of controller over >>>> i2c... my shell scripts should not need to know about architecture of >>>> every notebook out there. >>> >>> Then your scripts will be failing for some setups. >> >> Well, yes. Do you want to guess what "lp5523:kb3" is? >> > > Oh, please. The discussion is about the driver name part, which you > want to overload with some string to mean "internal", which in turn > is, if anything, part of the functionality. > > With "platform", you'll at some point have two > "platform::kbd_backlight" entries. Remind me to send you a "told you > so" when that happens. > > Guenter > >>>> But I don't see why I should do additional work when its trivial for >>>> kernel to just name the LED in an useful way. >>>> >>>> "platform::kbd_backlight" has no disadvantages compared to >>>> "wilco::kbd_backlight" ... so lets just use it. >>> >>> It has disadvantages because it promises more than it can deliver IMO. >>> If device name != "platform::kbd_backlight" it does not mean that it >>> is not internal keyboard. >> >> My promise is if "platform::kbd_backlight" exists, it is backlight for >> internal keyboard. (And second half is "if it is easy for kernel, we >> name backlight for internal keyboard platform::kbd_backlight"). >> >>> And you still have not resolved how you will >>> handle cases when there is more than one deice that can be considered >>> internal and may have a backlight. >> >> Is that realistic? How would that device look like? >> Maybe is something "weird" in the PC/laptop world but in the Embedded world is not as weird as you think. I worked on devices that has two internal backlights, one to lit the qwerty keyboard and the other one to lit the numeric pad. We used the device field to differentiate both. tclkeyboard::kbd_backlight tclnumpad::kbd_backlight Taking this to the extreme you can also think in a device where every key has its own LED backlight, this happens for example in this device [1]. The device can lit only specific keys giving to the user a word prediction experience (i.e After press a key, only the keys that match with a possible word are lit on) - Enric [1] https://www.abilia.com/en/product/lightwriter-sl50 >> Pavel >> -- >> (english) http://www.livejournal.com/~pavelmachek >> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi all, On 4/5/19 10:42 AM, Enric Balletbo i Serra wrote: > Hi, > > On 5/4/19 0:42, Guenter Roeck wrote: >> On Thu, Apr 4, 2019 at 3:05 PM Pavel Machek <pavel@ucw.cz> wrote: >>> >>> On Thu 2019-04-04 14:48:35, Dmitry Torokhov wrote: >>>> On Thu, Apr 4, 2019 at 1:42 PM Pavel Machek <pavel@ucw.cz> wrote: >>>>> >>>>> Hi! >>>>> >>>>>>>> And what to do if internal keyboard is not platform but USB? Like >>>>>>>> Google "Whiskers"? (I am not sure why you decided to drop my mention >>>>>>>> of internal USB keyboards completely off your reply). >>>>>>> >>>>>>> I don't have answers for everything. Even if you have USB keyboard, you'll >>>>>>> likely still have backlight connected to embedded controller. If not, >>>>>>> then maybe you have exception userland needs to know about. >>>>>>> >>>>>>> Still better than making everything an exception. >>>>>> >>>>>> You do not need to make everything exception. You just need to look >>>>>> beyond the name, and see how the device is connected. And then apply >>>>>> your exceptions for "weird" devices. >>>>> >>>>> "Where it is connected" is not interesting to the userland. "Is it >>>>> backlight for internal keyboard" is the right question. It may be >>>>> connected to embedded controller or some kind of controller over >>>>> i2c... my shell scripts should not need to know about architecture of >>>>> every notebook out there. >>>> >>>> Then your scripts will be failing for some setups. >>> >>> Well, yes. Do you want to guess what "lp5523:kb3" is? >>> >> >> Oh, please. The discussion is about the driver name part, which you >> want to overload with some string to mean "internal", which in turn >> is, if anything, part of the functionality. >> >> With "platform", you'll at some point have two >> "platform::kbd_backlight" entries. Remind me to send you a "told you >> so" when that happens. >> >> Guenter >> >>>>> But I don't see why I should do additional work when its trivial for >>>>> kernel to just name the LED in an useful way. >>>>> >>>>> "platform::kbd_backlight" has no disadvantages compared to >>>>> "wilco::kbd_backlight" ... so lets just use it. >>>> >>>> It has disadvantages because it promises more than it can deliver IMO. >>>> If device name != "platform::kbd_backlight" it does not mean that it >>>> is not internal keyboard. >>> >>> My promise is if "platform::kbd_backlight" exists, it is backlight for >>> internal keyboard. (And second half is "if it is easy for kernel, we >>> name backlight for internal keyboard platform::kbd_backlight"). >>> >>>> And you still have not resolved how you will >>>> handle cases when there is more than one deice that can be considered >>>> internal and may have a backlight. >>> >>> Is that realistic? How would that device look like? >>> > > Maybe is something "weird" in the PC/laptop world but in the Embedded world is > not as weird as you think. I worked on devices that has two internal backlights, > one to lit the qwerty keyboard and the other one to lit the numeric pad. We used > the device field to differentiate both. > > keyboardist::kbd_backlight > tclnumpad::kbd_backlight > > Taking this to the extreme you can also think in a device where every key has > its own LED backlight, this happens for example in this device [1]. The device > can lit only specific keys giving to the user a word prediction experience (i.e > After press a key, only the keys that match with a possible word are lit on) While we have your attention at the subject of LED naming I would like to invite you all to reviewing my recent patch set [0], available also on the led_naming_v3 branch of linux-leds.git [1]. The patch set introduces generic, backward compatible mechanism for composing LED class devices names. It also aims to deprecate current LED naming convention and encourage dropping "devicename" section. Patch 5/25 from the discussed patch set includes get_led_device_name_info.sh script proving that parent device name of the LED class device is available in the sysfs and its presence in the LED name is unjustified and redundant. The argument being raised here related to name clash risk when there is no unique devicename section included into the LED name is unjustified since LED core has a protection against that and adds "_n" numerical suffix to the requested LED name when it is already taken. The patch set introduces also a set of predefined LED_FUNCTION names to be used in DT bindings. This along with the removal of devicename section from LED naming pattern will help to keep LED sysfs interface more uniform and not varying depending on underlaying hardware driving the LEDs. Regarding the problem discussed in this thread - I would not necessarily go for "platform" in place of devicename LED name section in the cros_kbd_led_backlight driver. If we change it (should we at all - it is already in 5.0 AFAICS?), then I would opt for dropping the "chromeos:" part. It believe it will be possible to retrieve this name with get_led_device_info.sh script. It would be good exercise to check it out. In cases like above: keyboardist::kbd_backlight tclnumpad::kbd_backlight we could do with the following: :kbd-backlight :numpad-backlight I used hyphens instead of underscores since we will have this convention in the LED_FUNCTION names, which is more common for Device Tree, and some of existing LED triggers. W could also think of placing common LED_FUNCTION definitions in include/leds/led-functions.h and include it in both include/leds/leds.h and include/dt-bindings/leds/common.h, so that they would be more naturally accessible for non DT based drivers. [0] https://lkml.org/lkml/2019/3/31/222 [1] https://git.kernel.org/pub/scm/linux/kernel/git/j.anaszewski/linux-leds.git/log/?h=led_naming_v3
Hi! > The patch set introduces also a set of predefined LED_FUNCTION > names to be used in DT bindings. This along with the removal > of devicename section from LED naming pattern will help to keep > LED sysfs interface more uniform and not varying depending on > underlaying hardware driving the LEDs. > > Regarding the problem discussed in this thread - I would not necessarily > go for "platform" in place of devicename LED name section in the > cros_kbd_led_backlight driver. If we change it (should we at all - it is > already in 5.0 AFAICS?), then I would opt for dropping the "chromeos:" > part. It believe it will be possible to retrieve this name with > get_led_device_info.sh script. It would be good exercise to check > it out. I am not sure about existing driver. Important thing for me is that new drivers use consistent naming. > In cases like above: > > keyboardist::kbd_backlight > tclnumpad::kbd_backlight > > we could do with the following: > > :kbd-backlight > :numpad-backlight > > I used hyphens instead of underscores since we will have this convention > in the LED_FUNCTION names, which is more common for Device Tree, and > some of existing LED triggers. Existing userspace already searches for *:kbd_backlight", AFAICT, so we probably want to keep the "_". I don't care much if we use "platform:" or no prefix at all for backlight of internal keyboard, as long as it is consistent across all devices. We certainly want to use some prefix (probably inputX:) for backlight on USB keyboards. Best regards, Pavel
Hi Pavel, On 4/6/19 11:53 AM, Pavel Machek wrote: > Hi! > >> The patch set introduces also a set of predefined LED_FUNCTION >> names to be used in DT bindings. This along with the removal >> of devicename section from LED naming pattern will help to keep >> LED sysfs interface more uniform and not varying depending on >> underlaying hardware driving the LEDs. >> >> Regarding the problem discussed in this thread - I would not necessarily >> go for "platform" in place of devicename LED name section in the >> cros_kbd_led_backlight driver. If we change it (should we at all - it is >> already in 5.0 AFAICS?), then I would opt for dropping the "chromeos:" >> part. It believe it will be possible to retrieve this name with >> get_led_device_info.sh script. It would be good exercise to check >> it out. > > I am not sure about existing driver. Important thing for me is that > new drivers use consistent naming. > >> In cases like above: >> >> keyboardist::kbd_backlight >> tclnumpad::kbd_backlight >> >> we could do with the following: >> >> :kbd-backlight >> :numpad-backlight >> >> I used hyphens instead of underscores since we will have this convention >> in the LED_FUNCTION names, which is more common for Device Tree, and >> some of existing LED triggers. > > Existing userspace already searches for *:kbd_backlight", AFAICT, so > we probably want to keep the "_". OK, but it should be an exception but not a rule. This "kbd-*" naming is used in input and tty subsystems which register keyboard triggers with this style: ~/kernel$ git grep ".*[\":]kbd-" -- "*.c" drivers/input/input-leds.c: [LED_NUML] = { "numlock", VT_TRIGGER("kbd-numlock") }, drivers/input/input-leds.c: [LED_CAPSL] = { "capslock", VT_TRIGGER("kbd-capslock") }, drivers/input/input-leds.c: [LED_SCROLLL] = { "scrolllock", VT_TRIGGER("kbd-scrolllock") }, drivers/input/input-leds.c: [LED_KANA] = { "kana", VT_TRIGGER("kbd-kanalock") }, drivers/tty/vt/keyboard.c: KBD_LED_TRIGGER(VC_SCROLLOCK, "kbd-scrolllock"), drivers/tty/vt/keyboard.c: KBD_LED_TRIGGER(VC_NUMLOCK, "kbd-numlock"), drivers/tty/vt/keyboard.c: KBD_LED_TRIGGER(VC_CAPSLOCK, "kbd-capslock"), drivers/tty/vt/keyboard.c: KBD_LED_TRIGGER(VC_KANALOCK, "kbd-kanalock"), drivers/tty/vt/keyboard.c: KBD_LOCKSTATE_TRIGGER(VC_SHIFTLOCK, "kbd-shiftlock"), drivers/tty/vt/keyboard.c: KBD_LOCKSTATE_TRIGGER(VC_ALTGRLOCK, "kbd-altgrlock"), drivers/tty/vt/keyboard.c: KBD_LOCKSTATE_TRIGGER(VC_CTRLLOCK, "kbd-ctrllock"), drivers/tty/vt/keyboard.c: KBD_LOCKSTATE_TRIGGER(VC_ALTLOCK, "kbd-altlock"), drivers/tty/vt/keyboard.c: KBD_LOCKSTATE_TRIGGER(VC_SHIFTLLOCK, "kbd-shiftllock"), drivers/tty/vt/keyboard.c: KBD_LOCKSTATE_TRIGGER(VC_SHIFTRLOCK, "kbd-shiftrlock"), drivers/tty/vt/keyboard.c: KBD_LOCKSTATE_TRIGGER(VC_CTRLLLOCK, "kbd-ctrlllock"), drivers/tty/vt/keyboard.c: KBD_LOCKSTATE_TRIGGER(VC_CTRLRLOCK, "kbd-ctrlrlock"), "kbd_" naming is used only in case of backlight LEDs: ~/kernel$ git grep ".*[\":]kbd_" -- "*.c" drivers/hid/hid-asus.c: drvdata->kbd_backlight->cdev.name = "asus::kbd_backlight"; drivers/hid/hid-google-hammer.c: kbd_backlight->cdev.name = "hammer::kbd_backlight"; drivers/hwmon/applesmc.c: .name = "smc::kbd_backlight", drivers/input/misc/ims-pcu.c: "pcu%d::kbd_backlight", pcu->device_no); drivers/platform/chrome/cros_kbd_led_backlight.c: cdev->name = "chromeos::kbd_backlight"; drivers/platform/x86/asus-laptop.c: cdev->name = "asus::kbd_backlight"; drivers/platform/x86/asus-wmi.c: asus->kbd_led.name = "asus::kbd_backlight"; drivers/platform/x86/dell-laptop.c: .name = "dell::kbd_backlight", drivers/platform/x86/samsung-laptop.c: samsung->kbd_led.name = "samsung::kbd_backlight"; drivers/platform/x86/sony-laptop.c: kbdbl_ctl->mode_attr.attr.name = "kbd_backlight"; drivers/platform/x86/sony-laptop.c: kbdbl_ctl->timeout_attr.attr.name = "kbd_backlight_timeout"; drivers/platform/x86/thinkpad_acpi.c: .name = "tpacpi::kbd_backlight", drivers/platform/x86/toshiba_acpi.c: dev->kbd_led.name = "toshiba::kbd_backlight"; With LEDs in platform drivers is that problem that we have the name compiled into the kernel. Maybe to make it more flexible we could use kernel config to choose between new "kbd-" and legacy "kbd_" naming. > I don't care much if we use "platform:" or no prefix at all for > backlight of internal keyboard, as long as it is consistent across all > devices. > > We certainly want to use some prefix (probably inputX:) for backlight > on USB keyboards. For LEDs exposed through the input-LED bridge my get_led_device_info.sh script nicely reports: associated input node: input1 It just does: readlink input1\:\:numlock/device which prints: "../../input1 " And I believe that for backlight LEDs exposed by input subsystem it should be similarly since the input device related to USB keyboard is a child of USB device: /sys/class/leds# readlink input1::numlock ../../devices/pci0000:00/0000:00:14.0/usb2/2-4/2-4:1.0/0003:1C4F:0002.0002/input/input1/input1::numlock
Hi! > >I am not sure about existing driver. Important thing for me is that > >new drivers use consistent naming. > > > >>In cases like above: > >> > >> keyboardist::kbd_backlight > >> tclnumpad::kbd_backlight > >> > >>we could do with the following: > >> > >> :kbd-backlight > >> :numpad-backlight > >> > >>I used hyphens instead of underscores since we will have this convention > >>in the LED_FUNCTION names, which is more common for Device Tree, and > >>some of existing LED triggers. > > > >Existing userspace already searches for *:kbd_backlight", AFAICT, so > >we probably want to keep the "_". > > OK, but it should be an exception but not a rule. > This "kbd-*" naming is used in input and tty subsystems which register > keyboard triggers with this style: > > ~/kernel$ git grep ".*[\":]kbd-" -- "*.c" > drivers/input/input-leds.c: [LED_NUML] = { "numlock", > VT_TRIGGER("kbd-numlock") }, > "kbd_" naming is used only in case of backlight LEDs: > > ~/kernel$ git grep ".*[\":]kbd_" -- "*.c" > drivers/hid/hid-asus.c: drvdata->kbd_backlight->cdev.name = > "asus::kbd_backlight"; Yes, but userland already knows about kbd_backlight, so we really can not change this. > With LEDs in platform drivers is that problem that we have the name > compiled into the kernel. Maybe to make it more flexible we could > use kernel config to choose between new "kbd-" and legacy "kbd_" > naming. No, I don't think that is suitable use for config option. > >I don't care much if we use "platform:" or no prefix at all for > >backlight of internal keyboard, as long as it is consistent across all > >devices. > > > >We certainly want to use some prefix (probably inputX:) for backlight > >on USB keyboards. > > For LEDs exposed through the input-LED bridge my get_led_device_info.sh > script nicely reports: > > associated input node: input1 > > It just does: > > readlink input1\:\:numlock/device > > which prints: "../../input1 " Yes, that will probably work for USB keyboards. (But not for internal keyboards on phones, etc). Pavel
Hi Jacek, On Fri, Apr 5, 2019 at 1:00 PM Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote: > > Hi all, > > On 4/5/19 10:42 AM, Enric Balletbo i Serra wrote: > > Hi, > > > > On 5/4/19 0:42, Guenter Roeck wrote: > >> On Thu, Apr 4, 2019 at 3:05 PM Pavel Machek <pavel@ucw.cz> wrote: > >>> > >>> On Thu 2019-04-04 14:48:35, Dmitry Torokhov wrote: > >>>> On Thu, Apr 4, 2019 at 1:42 PM Pavel Machek <pavel@ucw.cz> wrote: > >>>>> > >>>>> Hi! > >>>>> > >>>>>>>> And what to do if internal keyboard is not platform but USB? Like > >>>>>>>> Google "Whiskers"? (I am not sure why you decided to drop my mention > >>>>>>>> of internal USB keyboards completely off your reply). > >>>>>>> > >>>>>>> I don't have answers for everything. Even if you have USB keyboard, you'll > >>>>>>> likely still have backlight connected to embedded controller. If not, > >>>>>>> then maybe you have exception userland needs to know about. > >>>>>>> > >>>>>>> Still better than making everything an exception. > >>>>>> > >>>>>> You do not need to make everything exception. You just need to look > >>>>>> beyond the name, and see how the device is connected. And then apply > >>>>>> your exceptions for "weird" devices. > >>>>> > >>>>> "Where it is connected" is not interesting to the userland. "Is it > >>>>> backlight for internal keyboard" is the right question. It may be > >>>>> connected to embedded controller or some kind of controller over > >>>>> i2c... my shell scripts should not need to know about architecture of > >>>>> every notebook out there. > >>>> > >>>> Then your scripts will be failing for some setups. > >>> > >>> Well, yes. Do you want to guess what "lp5523:kb3" is? > >>> > >> > >> Oh, please. The discussion is about the driver name part, which you > >> want to overload with some string to mean "internal", which in turn > >> is, if anything, part of the functionality. > >> > >> With "platform", you'll at some point have two > >> "platform::kbd_backlight" entries. Remind me to send you a "told you > >> so" when that happens. > >> > >> Guenter > >> > >>>>> But I don't see why I should do additional work when its trivial for > >>>>> kernel to just name the LED in an useful way. > >>>>> > >>>>> "platform::kbd_backlight" has no disadvantages compared to > >>>>> "wilco::kbd_backlight" ... so lets just use it. > >>>> > >>>> It has disadvantages because it promises more than it can deliver IMO. > >>>> If device name != "platform::kbd_backlight" it does not mean that it > >>>> is not internal keyboard. > >>> > >>> My promise is if "platform::kbd_backlight" exists, it is backlight for > >>> internal keyboard. (And second half is "if it is easy for kernel, we > >>> name backlight for internal keyboard platform::kbd_backlight"). > >>> > >>>> And you still have not resolved how you will > >>>> handle cases when there is more than one deice that can be considered > >>>> internal and may have a backlight. > >>> > >>> Is that realistic? How would that device look like? > >>> > > > > Maybe is something "weird" in the PC/laptop world but in the Embedded world is > > not as weird as you think. I worked on devices that has two internal backlights, > > one to lit the qwerty keyboard and the other one to lit the numeric pad. We used > > the device field to differentiate both. > > > > keyboardist::kbd_backlight > > tclnumpad::kbd_backlight > > > > Taking this to the extreme you can also think in a device where every key has > > its own LED backlight, this happens for example in this device [1]. The device > > can lit only specific keys giving to the user a word prediction experience (i.e > > After press a key, only the keys that match with a possible word are lit on) > > While we have your attention at the subject of LED naming I would like > to invite you all to reviewing my recent patch set [0], available > also on the led_naming_v3 branch of linux-leds.git [1]. > > The patch set introduces generic, backward compatible mechanism for > composing LED class devices names. It also aims to deprecate current > LED naming convention and encourage dropping "devicename" section. From looking at the docs section it looks like you propose to move from "device:color:fucntion" to simply "color:function" naming, and expect to have a suffix "_<n>" to avoid problem with duplicate LED names. I do not think this is quite backward compatible, since previously userspace was supposed to split the device name on the colon boundaries and extract the 3rd component if it wanted to determine function. With the new proposed scheme it has to be modified to try and also fetch the 2nd component if there isn't 3rd one and consider it as function as well. It also need to recognize potential suffixes and drop them before matching on function name. I think if you want flexibility you really need to switch from encoding the information in the name to add LED class attributes describing the LED in more detail. This might include information about LEd placement (internal/external) if such information is available, and other additional attributes, if needed. Updated userspace can make use of these new attributes, leaving existing userspace decoding legacy names. > > Patch 5/25 from the discussed patch set includes > get_led_device_name_info.sh script proving that parent device name > of the LED class device is available in the sysfs and its presence > in the LED name is unjustified and redundant. The argument being raised > here related to name clash risk when there is no unique devicename > section included into the LED name is unjustified since LED core has > a protection against that and adds "_n" numerical suffix to the > requested LED name when it is already taken. This scheme breaks userspace that does not expect additional suffixes attached to function name. > > The patch set introduces also a set of predefined LED_FUNCTION > names to be used in DT bindings. This along with the removal > of devicename section from LED naming pattern will help to keep > LED sysfs interface more uniform and not varying depending on > underlaying hardware driving the LEDs. > > Regarding the problem discussed in this thread - I would not necessarily > go for "platform" in place of devicename LED name section in the > cros_kbd_led_backlight driver. If we change it (should we at all - it is > already in 5.0 AFAICS?), then I would opt for dropping the "chromeos:" > part. It believe it will be possible to retrieve this name with > get_led_device_info.sh script. It would be good exercise to check > it out. > > In cases like above: > > keyboardist::kbd_backlight > tclnumpad::kbd_backlight > > we could do with the following: > > :kbd-backlight > :numpad-backlight > > I used hyphens instead of underscores since we will have this convention > in the LED_FUNCTION names, which is more common for Device Tree, and > some of existing LED triggers. I am not sure what device tree has to do with it. ACPI for example likes all caps and sort names with numbers, but we do not let it propagate into the kernel. We also should not be changing existing function names as existing userspace relies on them. > > W could also think of placing common LED_FUNCTION definitions in > include/leds/led-functions.h and include it in both include/leds/leds.h > and include/dt-bindings/leds/common.h, so that they would be more > naturally accessible for non DT based drivers. This makes total sense. Thanks.
Hi Dmitry, Thanks for the review. On 4/8/19 12:01 AM, Dmitry Torokhov wrote: > Hi Jacek, > > On Fri, Apr 5, 2019 at 1:00 PM Jacek Anaszewski > <jacek.anaszewski@gmail.com> wrote: >> >> Hi all, >> >> On 4/5/19 10:42 AM, Enric Balletbo i Serra wrote: >>> Hi, >>> >>> On 5/4/19 0:42, Guenter Roeck wrote: >>>> On Thu, Apr 4, 2019 at 3:05 PM Pavel Machek <pavel@ucw.cz> wrote: >>>>> >>>>> On Thu 2019-04-04 14:48:35, Dmitry Torokhov wrote: >>>>>> On Thu, Apr 4, 2019 at 1:42 PM Pavel Machek <pavel@ucw.cz> wrote: >>>>>>> >>>>>>> Hi! >>>>>>> >>>>>>>>>> And what to do if internal keyboard is not platform but USB? Like >>>>>>>>>> Google "Whiskers"? (I am not sure why you decided to drop my mention >>>>>>>>>> of internal USB keyboards completely off your reply). >>>>>>>>> >>>>>>>>> I don't have answers for everything. Even if you have USB keyboard, you'll >>>>>>>>> likely still have backlight connected to embedded controller. If not, >>>>>>>>> then maybe you have exception userland needs to know about. >>>>>>>>> >>>>>>>>> Still better than making everything an exception. >>>>>>>> >>>>>>>> You do not need to make everything exception. You just need to look >>>>>>>> beyond the name, and see how the device is connected. And then apply >>>>>>>> your exceptions for "weird" devices. >>>>>>> >>>>>>> "Where it is connected" is not interesting to the userland. "Is it >>>>>>> backlight for internal keyboard" is the right question. It may be >>>>>>> connected to embedded controller or some kind of controller over >>>>>>> i2c... my shell scripts should not need to know about architecture of >>>>>>> every notebook out there. >>>>>> >>>>>> Then your scripts will be failing for some setups. >>>>> >>>>> Well, yes. Do you want to guess what "lp5523:kb3" is? >>>>> >>>> >>>> Oh, please. The discussion is about the driver name part, which you >>>> want to overload with some string to mean "internal", which in turn >>>> is, if anything, part of the functionality. >>>> >>>> With "platform", you'll at some point have two >>>> "platform::kbd_backlight" entries. Remind me to send you a "told you >>>> so" when that happens. >>>> >>>> Guenter >>>> >>>>>>> But I don't see why I should do additional work when its trivial for >>>>>>> kernel to just name the LED in an useful way. >>>>>>> >>>>>>> "platform::kbd_backlight" has no disadvantages compared to >>>>>>> "wilco::kbd_backlight" ... so lets just use it. >>>>>> >>>>>> It has disadvantages because it promises more than it can deliver IMO. >>>>>> If device name != "platform::kbd_backlight" it does not mean that it >>>>>> is not internal keyboard. >>>>> >>>>> My promise is if "platform::kbd_backlight" exists, it is backlight for >>>>> internal keyboard. (And second half is "if it is easy for kernel, we >>>>> name backlight for internal keyboard platform::kbd_backlight"). >>>>> >>>>>> And you still have not resolved how you will >>>>>> handle cases when there is more than one deice that can be considered >>>>>> internal and may have a backlight. >>>>> >>>>> Is that realistic? How would that device look like? >>>>> >>> >>> Maybe is something "weird" in the PC/laptop world but in the Embedded world is >>> not as weird as you think. I worked on devices that has two internal backlights, >>> one to lit the qwerty keyboard and the other one to lit the numeric pad. We used >>> the device field to differentiate both. >>> >>> keyboardist::kbd_backlight >>> tclnumpad::kbd_backlight >>> >>> Taking this to the extreme you can also think in a device where every key has >>> its own LED backlight, this happens for example in this device [1]. The device >>> can lit only specific keys giving to the user a word prediction experience (i.e >>> After press a key, only the keys that match with a possible word are lit on) >> >> While we have your attention at the subject of LED naming I would like >> to invite you all to reviewing my recent patch set [0], available >> also on the led_naming_v3 branch of linux-leds.git [1]. >> >> The patch set introduces generic, backward compatible mechanism for >> composing LED class devices names. It also aims to deprecate current >> LED naming convention and encourage dropping "devicename" section. > >>From looking at the docs section it looks like you propose to move > from "device:color:fucntion" to simply "color:function" naming, and > expect to have a suffix "_<n>" to avoid problem with duplicate LED > names. I do not think this is quite backward compatible, since > previously userspace was supposed to split the device name on the > colon boundaries and extract the 3rd component if it wanted to > determine function. With the new proposed scheme it has to be modified > to try and also fetch the 2nd component if there isn't 3rd one and > consider it as function as well. It also need to recognize potential > suffixes and drop them before matching on function name. The feature adding "_n" suffixes is not being added in my patch set, it only gets documented. It was added back in 2015 to cover the case when a LED with the name already present in the system is being added via DT overlay: commit a96aa64cb5723d941de879a9cd1fea025d6acb1b Author: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> Date: Mon Mar 30 10:45:59 2015 -0700 leds/led-class: Handle LEDs with the same name The current code expected that every LED had an unique name. This is a legit expectation when the device tree can no be modified or extended. But with device tree overlays this requirement can be easily broken. This patch finds out if the name is already in use and adds the suffix _1, _2... if not. Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> Signed-off-by: Bryan Wu <cooloney@gmail.com> If LEDs will be properly assigned function names, there will be no need for resorting to this fallback mechanism. I believe, that at least in case of platform drivers you are not going to have two LEDs with exactly the same function name, which is going to reflect what would be printed on the sticker next to the LED on the device case. One type of devices for which preserving devicename will make sense are USB ones. In case of my wifi dongle I get LED name like mt7601u-phy0. The name which would introduce added value would be: "phy0::wlan". The backward compatibility I mention in the patch refers to keeping the support for devicename:color:function. It means that operation of all existing drivers will not be affected. New drivers will also be able to stick to the old naming convention, however people will be encouraged to drop the devicename section as it has no added value beside more "human readable" LED names. This however is not a question in case of devfs nodes, right? Generally the goal is to gradually get rid of this clumsy naming, which includes frequently the chipset name or other arbitrary names like vendor or platform name. Userspace will always be able to retrieve hardware related details, which are available in the sysfs all the time. Please check the get_led_device_info.sh script being added in the patch set. > I think if you want flexibility you really need to switch from > encoding the information in the name to add LED class attributes > describing the LED in more detail. This might include information > about LEd placement (internal/external) if such information is > available, and other additional attributes, if needed. Updated > userspace can make use of these new attributes, leaving existing > userspace decoding legacy names. > >> >> Patch 5/25 from the discussed patch set includes >> get_led_device_name_info.sh script proving that parent device name >> of the LED class device is available in the sysfs and its presence >> in the LED name is unjustified and redundant. The argument being raised >> here related to name clash risk when there is no unique devicename >> section included into the LED name is unjustified since LED core has >> a protection against that and adds "_n" numerical suffix to the >> requested LED name when it is already taken. > > This scheme breaks userspace that does not expect additional suffixes > attached to function name. Like I explained above the addition of suffixes is not a part of this patch set, but a pre-existing fallback for avoiding LED name clash. In order to get the gist of the changes it is required to go through the first five patches of the patch set and read commit messages as well as the documentation of the functions being added. >> The patch set introduces also a set of predefined LED_FUNCTION >> names to be used in DT bindings. This along with the removal >> of devicename section from LED naming pattern will help to keep >> LED sysfs interface more uniform and not varying depending on >> underlaying hardware driving the LEDs. >> >> Regarding the problem discussed in this thread - I would not necessarily >> go for "platform" in place of devicename LED name section in the >> cros_kbd_led_backlight driver. If we change it (should we at all - it is >> already in 5.0 AFAICS?), then I would opt for dropping the "chromeos:" >> part. It believe it will be possible to retrieve this name with >> get_led_device_info.sh script. It would be good exercise to check >> it out. >> >> In cases like above: >> >> keyboardist::kbd_backlight >> tclnumpad::kbd_backlight >> >> we could do with the following: >> >> :kbd-backlight >> :numpad-backlight >> >> I used hyphens instead of underscores since we will have this convention >> in the LED_FUNCTION names, which is more common for Device Tree, and >> some of existing LED triggers. > > I am not sure what device tree has to do with it. ACPI for example > likes all caps and sort names with numbers, but we do not let it > propagate into the kernel. For DT based LED class devices the LED name comes directly from DT label property. > We also should not be changing existing function names as existing > userspace relies on them. This is obvious. I meant making it configurable via kernel config e.g.: CONFIG_LEDS_LEGACY_NAMES. >> W could also think of placing common LED_FUNCTION definitions in >> include/leds/led-functions.h and include it in both include/leds/leds.h >> and include/dt-bindings/leds/common.h, so that they would be more >> naturally accessible for non DT based drivers. > > This makes total sense. > > Thanks. >
I've just found a few [embarrassing :)] bugs in this version, so after we figure out the naming, please wait for me to send out another patch that fixes these. Thanks, Nick On Thu, Apr 4, 2019 at 11:10 AM Nick Crews <ncrews@chromium.org> wrote: > > We want all backlights for the system keyboard to > use a common name, so the name "platform::kbd_backlight" > would be better than the current "chromeos::kbd_backlight" > name. Normally this wouldn't be worth changing, but the new > Wilco keyboard backlight driver uses the "platform" name. > We want to make it so all Chrome OS devices are consistent, > so we'll change the name here too. > > The Power Manager daemon only looks for LEDs that match the > pattern "*:kbd_backlight", so this change won't affect that. > > Signed-off-by: Nick Crews <ncrews@chromium.org> > --- > drivers/platform/chrome/cros_kbd_led_backlight.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/platform/chrome/cros_kbd_led_backlight.c b/drivers/platform/chrome/cros_kbd_led_backlight.c > index aa409f0201fb..c56c8405f39c 100644 > --- a/drivers/platform/chrome/cros_kbd_led_backlight.c > +++ b/drivers/platform/chrome/cros_kbd_led_backlight.c > @@ -77,7 +77,7 @@ static int keyboard_led_probe(struct platform_device *pdev) > if (!cdev) > return -ENOMEM; > > - cdev->name = "chromeos::kbd_backlight"; > + cdev->name = "platform::kbd_backlight"; > cdev->max_brightness = ACPI_KEYBOARD_BACKLIGHT_MAX; > cdev->flags |= LED_CORE_SUSPENDRESUME; > cdev->brightness_set = keyboard_led_set_brightness; > -- > 2.20.1 >
diff --git a/drivers/platform/chrome/cros_kbd_led_backlight.c b/drivers/platform/chrome/cros_kbd_led_backlight.c index aa409f0201fb..c56c8405f39c 100644 --- a/drivers/platform/chrome/cros_kbd_led_backlight.c +++ b/drivers/platform/chrome/cros_kbd_led_backlight.c @@ -77,7 +77,7 @@ static int keyboard_led_probe(struct platform_device *pdev) if (!cdev) return -ENOMEM; - cdev->name = "chromeos::kbd_backlight"; + cdev->name = "platform::kbd_backlight"; cdev->max_brightness = ACPI_KEYBOARD_BACKLIGHT_MAX; cdev->flags |= LED_CORE_SUSPENDRESUME; cdev->brightness_set = keyboard_led_set_brightness;
We want all backlights for the system keyboard to use a common name, so the name "platform::kbd_backlight" would be better than the current "chromeos::kbd_backlight" name. Normally this wouldn't be worth changing, but the new Wilco keyboard backlight driver uses the "platform" name. We want to make it so all Chrome OS devices are consistent, so we'll change the name here too. The Power Manager daemon only looks for LEDs that match the pattern "*:kbd_backlight", so this change won't affect that. Signed-off-by: Nick Crews <ncrews@chromium.org> --- drivers/platform/chrome/cros_kbd_led_backlight.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)