| Message ID | 20250121225510.751444-1-wse@tuxedocomputers.com |
|---|---|
| Headers | show |
| Series | platform/x86/tuxedo: Add virtual LampArray for TUXEDO NB04 devices | expand |
Am 21.01.25 um 23:31 schrieb Werner Sembach: > Hi, > after some other work, picked this up again. > Only coding style changes vs v4. > > > I now got my feet a little wet with hid-bpf regarding something else, and > with that knowledge I would leave the long arrays in the beginning in the > kernel code for the time being: > > sirius_16_ansii_kbl_mapping and sirius_16_iso_kbl_mapping are required > during initialization so they have to exist in the kernel code anyway. > > report_descriptor will most likly not change even for future models and > afaik having report_descriptors in kernel drivers is not unheard of. > > So the only things that could be meaningfully moved to a hid-bpf program > are the sirius_16_*_kbl_mapping_pos_* arrays. But for these is have to give > out some fallback value anyway for the case where a hid-bpf file is missing > or fails to load. So why not use real world values from my test device for > these values? > > As soon as there is a future device that can use the same driver with just > these pos arrays different, then I would implement that change via a bpf > program instead of a change to the kernel driver. > > Let me know if you too think this is a sensefull approach? > > > Another question: Would this patch need to wait for a userspace > implementation of lamp array before it can get accepted? It would be nice if you could test the LampArray implementation. But other than that userspace can catch up later. Still, i am interested in the opinion of the LED maintainers regarding the fake HID interface. Thanks, Armin Wolf > > The folder structure and naming scheme with nb04 is im preparation for > other parts of tuxedo-drivers to be upstreamed. NB04 is one of the > board_vendor dmi strings on TUXEDO devices that aligns with which part of > tuxedo-drivers implements the features of that device. They are independent > of each other so I plan to put them in different subfolders to reflect > that. > > Best regards, > Werner Sembach > > Werner Sembach (1): > platform/x86/tuxedo: Add virtual LampArray for TUXEDO NB04 devices > > MAINTAINERS | 6 + > drivers/platform/x86/Kconfig | 2 + > drivers/platform/x86/Makefile | 3 + > drivers/platform/x86/tuxedo/Kbuild | 6 + > drivers/platform/x86/tuxedo/Kconfig | 6 + > drivers/platform/x86/tuxedo/nb04/Kbuild | 9 + > drivers/platform/x86/tuxedo/nb04/Kconfig | 14 + > .../platform/x86/tuxedo/nb04/wmi_ab_init.c | 103 +++ > .../platform/x86/tuxedo/nb04/wmi_ab_init.h | 18 + > .../x86/tuxedo/nb04/wmi_ab_virt_lamparray.c | 772 ++++++++++++++++++ > .../x86/tuxedo/nb04/wmi_ab_virt_lamparray.h | 18 + > .../platform/x86/tuxedo/nb04/wmi_xx_util.c | 97 +++ > .../platform/x86/tuxedo/nb04/wmi_xx_util.h | 112 +++ > 13 files changed, 1166 insertions(+) > create mode 100644 drivers/platform/x86/tuxedo/Kbuild > create mode 100644 drivers/platform/x86/tuxedo/Kconfig > create mode 100644 drivers/platform/x86/tuxedo/nb04/Kbuild > create mode 100644 drivers/platform/x86/tuxedo/nb04/Kconfig > create mode 100644 drivers/platform/x86/tuxedo/nb04/wmi_ab_init.c > create mode 100644 drivers/platform/x86/tuxedo/nb04/wmi_ab_init.h > create mode 100644 drivers/platform/x86/tuxedo/nb04/wmi_ab_virt_lamparray.c > create mode 100644 drivers/platform/x86/tuxedo/nb04/wmi_ab_virt_lamparray.h > create mode 100644 drivers/platform/x86/tuxedo/nb04/wmi_xx_util.c > create mode 100644 drivers/platform/x86/tuxedo/nb04/wmi_xx_util.h >
Hi! > > I now got my feet a little wet with hid-bpf regarding something else, and > > with that knowledge I would leave the long arrays in the beginning in the > > kernel code for the time being: > > > > sirius_16_ansii_kbl_mapping and sirius_16_iso_kbl_mapping are required > > during initialization so they have to exist in the kernel code anyway. > > > > report_descriptor will most likly not change even for future models and > > afaik having report_descriptors in kernel drivers is not unheard of. > > > > So the only things that could be meaningfully moved to a hid-bpf program > > are the sirius_16_*_kbl_mapping_pos_* arrays. But for these is have to give > > out some fallback value anyway for the case where a hid-bpf file is missing > > or fails to load. So why not use real world values from my test device for > > these values? > > > > As soon as there is a future device that can use the same driver with just > > these pos arrays different, then I would implement that change via a bpf > > program instead of a change to the kernel driver. > > > > Let me know if you too think this is a sensefull approach? > > > > > > Another question: Would this patch need to wait for a userspace > > implementation of lamp array before it can get accepted? > > It would be nice if you could test the LampArray implementation. But other than that > userspace can catch up later. > > Still, i am interested in the opinion of the LED maintainers > regarding the fake HID interface. Comments from previous review were not addressed. Most importantly, this is not a way to do kernel interface. We want reasonable interface that can be documented and modified as needed. We want to pass /dev/input to userspace, not raw HID. This is not ok. Best regards, Pavel
Am 01.02.25 um 05:39 schrieb Armin Wolf: > Am 21.01.25 um 23:31 schrieb Werner Sembach: > >> Hi, >> after some other work, picked this up again. >> Only coding style changes vs v4. >> >> >> I now got my feet a little wet with hid-bpf regarding something else, and >> with that knowledge I would leave the long arrays in the beginning in the >> kernel code for the time being: >> >> sirius_16_ansii_kbl_mapping and sirius_16_iso_kbl_mapping are required >> during initialization so they have to exist in the kernel code anyway. >> >> report_descriptor will most likly not change even for future models and >> afaik having report_descriptors in kernel drivers is not unheard of. >> >> So the only things that could be meaningfully moved to a hid-bpf program >> are the sirius_16_*_kbl_mapping_pos_* arrays. But for these is have to give >> out some fallback value anyway for the case where a hid-bpf file is missing >> or fails to load. So why not use real world values from my test device for >> these values? >> >> As soon as there is a future device that can use the same driver with just >> these pos arrays different, then I would implement that change via a bpf >> program instead of a change to the kernel driver. >> >> Let me know if you too think this is a sensefull approach? >> >> >> Another question: Would this patch need to wait for a userspace >> implementation of lamp array before it can get accepted? > > It would be nice if you could test the LampArray implementation. But other > than that > userspace can catch up later. I tested with the attached programs (I directly edited the source files to test different things, this is just a snapshot). > > Still, i am interested in the opinion of the LED maintainers regarding the > fake HID interface. > > Thanks, > Armin Wolf > >> >> The folder structure and naming scheme with nb04 is im preparation for >> other parts of tuxedo-drivers to be upstreamed. NB04 is one of the >> board_vendor dmi strings on TUXEDO devices that aligns with which part of >> tuxedo-drivers implements the features of that device. They are independent >> of each other so I plan to put them in different subfolders to reflect >> that. >> >> Best regards, >> Werner Sembach >> >> Werner Sembach (1): >> platform/x86/tuxedo: Add virtual LampArray for TUXEDO NB04 devices >> >> MAINTAINERS | 6 + >> drivers/platform/x86/Kconfig | 2 + >> drivers/platform/x86/Makefile | 3 + >> drivers/platform/x86/tuxedo/Kbuild | 6 + >> drivers/platform/x86/tuxedo/Kconfig | 6 + >> drivers/platform/x86/tuxedo/nb04/Kbuild | 9 + >> drivers/platform/x86/tuxedo/nb04/Kconfig | 14 + >> .../platform/x86/tuxedo/nb04/wmi_ab_init.c | 103 +++ >> .../platform/x86/tuxedo/nb04/wmi_ab_init.h | 18 + >> .../x86/tuxedo/nb04/wmi_ab_virt_lamparray.c | 772 ++++++++++++++++++ >> .../x86/tuxedo/nb04/wmi_ab_virt_lamparray.h | 18 + >> .../platform/x86/tuxedo/nb04/wmi_xx_util.c | 97 +++ >> .../platform/x86/tuxedo/nb04/wmi_xx_util.h | 112 +++ >> 13 files changed, 1166 insertions(+) >> create mode 100644 drivers/platform/x86/tuxedo/Kbuild >> create mode 100644 drivers/platform/x86/tuxedo/Kconfig >> create mode 100644 drivers/platform/x86/tuxedo/nb04/Kbuild >> create mode 100644 drivers/platform/x86/tuxedo/nb04/Kconfig >> create mode 100644 drivers/platform/x86/tuxedo/nb04/wmi_ab_init.c >> create mode 100644 drivers/platform/x86/tuxedo/nb04/wmi_ab_init.h >> create mode 100644 drivers/platform/x86/tuxedo/nb04/wmi_ab_virt_lamparray.c >> create mode 100644 drivers/platform/x86/tuxedo/nb04/wmi_ab_virt_lamparray.h >> create mode 100644 drivers/platform/x86/tuxedo/nb04/wmi_xx_util.c >> create mode 100644 drivers/platform/x86/tuxedo/nb04/wmi_xx_util.h >> cmake_minimum_required(VERSION 3.5) project(LampArrayPlayground) add_executable(lap lap.cpp) target_compile_features(lap PRIVATE cxx_std_17) add_executable(laploop laploop.cpp) target_compile_features(laploop PRIVATE cxx_std_17) add_executable(laploopsingle laploopsingle.cpp) target_compile_features(laploopsingle PRIVATE cxx_std_17)
Hi, Am 01.02.25 um 09:48 schrieb Pavel Machek: > Hi! > >>> I now got my feet a little wet with hid-bpf regarding something else, and >>> with that knowledge I would leave the long arrays in the beginning in the >>> kernel code for the time being: >>> >>> sirius_16_ansii_kbl_mapping and sirius_16_iso_kbl_mapping are required >>> during initialization so they have to exist in the kernel code anyway. >>> >>> report_descriptor will most likly not change even for future models and >>> afaik having report_descriptors in kernel drivers is not unheard of. >>> >>> So the only things that could be meaningfully moved to a hid-bpf program >>> are the sirius_16_*_kbl_mapping_pos_* arrays. But for these is have to give >>> out some fallback value anyway for the case where a hid-bpf file is missing >>> or fails to load. So why not use real world values from my test device for >>> these values? >>> >>> As soon as there is a future device that can use the same driver with just >>> these pos arrays different, then I would implement that change via a bpf >>> program instead of a change to the kernel driver. >>> >>> Let me know if you too think this is a sensefull approach? >>> >>> >>> Another question: Would this patch need to wait for a userspace >>> implementation of lamp array before it can get accepted? >> It would be nice if you could test the LampArray implementation. But other than that >> userspace can catch up later. >> >> Still, i am interested in the opinion of the LED maintainers >> regarding the fake HID interface. > Comments from previous review were not addressed. > > Most importantly, this is not a way to do kernel interface. We want > reasonable interface that can be documented and modified as needed. We > want to pass /dev/input to userspace, not raw HID. This is not ok. There are already 2 endless discussions about this: https://lore.kernel.org/all/1fb08a74-62c7-4d0c-ba5d-648e23082dcb@tuxedocomputers.com/ https://lore.kernel.org/all/73c36418-34d6-46cf-9f10-6ca5e569274f@tuxedocomputers.com/ And a shorter one before that: https://lore.kernel.org/all/30cbbf20-08cf-a69b-4f58-359a9802e86f@tuxedocomputers.com/ The brief: - LampArray is a standard that will hit the Linux world anyway. - The alternative proposal via a led matrix does not even really fit keyboards, and does not at all fit all other device types. Hans and Benjamin already agree with me that LampArray is the way to go. So after over 2 years can I please have a final decision on how to implement this? Regards, Werner > > Best regards, > Pavel
Hi! > > Comments from previous review were not addressed. > > > > Most importantly, this is not a way to do kernel interface. We want > > reasonable interface that can be documented and modified as needed. We > > want to pass /dev/input to userspace, not raw HID. This is not ok. > > There are already 2 endless discussions about this: > > https://lore.kernel.org/all/1fb08a74-62c7-4d0c-ba5d-648e23082dcb@tuxedocomputers.com/ > > https://lore.kernel.org/all/73c36418-34d6-46cf-9f10-6ca5e569274f@tuxedocomputers.com/ > > And a shorter one before that: > > https://lore.kernel.org/all/30cbbf20-08cf-a69b-4f58-359a9802e86f@tuxedocomputers.com/ > > The brief: > > - LampArray is a standard that will hit the Linux world anyway. Maybe. Still have to see device implementing that. LampArray will still need /sys/class/leds for compatibility. LampArray still does not solve effects. More importantly, it is not okay to say "kernel interface is specified by that crazy document from 3rd party". > - The alternative proposal via a led matrix does not even really fit > keyboards, and does not at all fit all other device types. We are solving keyboards, not the other device types. The other devices can likely be handled by existing /sys/class/leds interfaces. > Hans and Benjamin already agree with me that LampArray is the way to go. > > So after over 2 years can I please have a final decision on how to implement this? For final decisions, you'd have to talk to Linus. (And sorry for the delay, btw). If you want to move this forward, place a driver in drivers/leds/keyboard. Implement /sys/class/leds interface, but make sure interface is clearly separated from the code talking to the firmware. Then we can review that, perhaps merge, so users will have something, and decide what interface to use for per-key control. LampArray is no-go. Other options are open. Best regards, Pavel