Re: [PATCH v4 0/6] USB keyboard improvements for asahi / desktop systems

[Marek Vasut <marex@denx.de>]

On 4/6/24 10:04 PM, Janne Grunau wrote:
> On Sat, Apr 06, 2024 at 08:52:17PM +0200, Marek Vasut wrote:
>> On 4/5/24 9:05 PM, Janne Grunau wrote:
>>> On Fri, Apr 05, 2024 at 04:52:32PM +0200, Marek Vasut wrote:
>>>> On 4/4/24 8:25 AM, Janne Grunau via B4 Relay wrote:
>>>>> Apple USB Keyboards from 2021 need quirks to be useable. The boot HID
>>>>> keyboard protocol is unfortunately not described in the first interface
>>>>> descriptor but the second. This needs several changes. The USB keyboard
>>>>> driver has to look at all (2) interface descriptors during probing.
>>>>> Since I didn't want to rebuild the USB driver probe code the Apple
>>>>> keyboards are bound to the keyboard driver via USB vendor and product
>>>>> IDs.
>>>>> To make the keyboards useable on Apple silicon devices the xhci driver
>>>>> needs to initializes rings for the endpoints of the first two interface
>>>>> descriptors. If this is causes concerns regarding regressions or memory
>>>>> use the USB_MAX_ACTIVE_INTERFACES define could be turned into a CONFIG
>>>>> option.
>>>>> Even after this changes the keyboards still do not probe successfully
>>>>> since they apparently do not behave HID standard compliant. They only
>>>>> generate reports on key events. This leads the final check whether the
>>>>> keyboard is operational to fail unless the user presses keys during the
>>>>> probe. Skip this check for known keyboards.
>>>>> Keychron seems to emulate Apple keyboards (some models even "re-use"
>>>>> Apple's USB vendor ID) so apply this quirk as well.
>>>>>
>>>>> Some devices like Yubikeys emulate a keyboard. since u-boot only binds a
>>>>> single keyboard block this kind of devices from the USB keyboard driver.
>>>>>
>>>>> Signed-off-by: Janne Grunau <j@jannau.net>
>>>>
>>>> I picked the series, but CI indicates build errors, can you have a look ?
>>>>
>>>> https://source.denx.de/u-boot/custodians/u-boot-usb/-/pipelines/20215
>>>
>>> The issue seems to be that the field dev in struct usb_device exists
>>> only for DM_USB. That means we can't use dev_dbg.
>>> Either take the following fixup patch or I can resend the series.
>>
>> I squashed the extra patch in, but I think the CI still complains:
>>
>> Pipeline #20236 (
>> https://source.denx.de/u-boot/custodians/u-boot-usb/-/pipelines/20236 )
>> triggered by Marek Vasut ( https://source.denx.de/marex )
>> had 1 failed job.
>>
>> Job #812215 (
>> https://source.denx.de/u-boot/custodians/u-boot-usb/-/jobs/812215/raw )
> 
> env_get() missing without CONFIG_ENV_SUPPORT. I'm too accustomed to the
> kernel's stub functions. Best option seems to to just #if the
> functionality out in this case. See attached fixup patch.
> 
> Sorry,

No worries.

Does it work if you do this instead ?

  static int usb_device_is_ignored(u16 id_vendor, u16 id_product)
  {
         ulong vid, pid;
+       /* No env support, nothing can be ignored */
+       if (CONFIG_IS_ENABLED(ENV_SUPPORT))
+           return 0;

That way, the function is always compiled and if it is unreachable, then 
compiler throws it out. This should improve code compile coverage.