Re: [PATCH v2 1/5] efi: usb: add header for usb-efi

On Wed, 2026-08-19 at 16:03 +0200, Ahmad Fatoum wrote:
> Hi Fabian,
> 
> On 8/19/26 3:32 PM, Fabian Pflug wrote:
> > Hey,
> > 
> > On Wed, 2026-08-19 at 12:01 +0200, Ahmad Fatoum wrote:
> > > Hello,
> > > 
> > > On 8/17/26 10:47 AM, Fabian Pflug wrote:
> > > > Convert the spec in [1] to a header, that is useable for barebox.
> > > > The async headers are not defined, since it is currently not planned to
> > > > support async in addition to sync.
> > > > 
> > > > [1] https://uefi.org/specs/UEFI/2.11/17_Protocols_USB_Support.html
> > > 
> > > Some nitpicks below. You don't need to resend for just those, but if you
> > > are sending a v2, it would be good to address these as well.
> > > 
> > > > 
> > > > Signed-off-by: Fabian Pflug <[email protected]>
> > > 
> > > Reviewed-by: Ahmad Fatoum <[email protected]>
> > > 
> > > > 
> > > > ---
> > > > v2:
> > > > - Fixed wrong type in header for get_string_descriptor
> > > > ---
> > > >  include/efi/protocol/usb.h | 66 ++++++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 66 insertions(+)
> > > > 
> > > > diff --git a/include/efi/protocol/usb.h b/include/efi/protocol/usb.h
> > > > new file mode 100644
> > > > index 0000000000..78953a4fa7
> > > > --- /dev/null
> > > > +++ b/include/efi/protocol/usb.h
> > > > @@ -0,0 +1,66 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0-only */
> > > > +
> > > > +#ifndef __EFI_PROTOCOL_USB_H_
> > > > +#define __EFI_PROTOCOL_USB_H_
> > > > +
> > > > +#include <efi/types.h>
> > > > +#include <linux/usb/usb.h>
> > > 
> > > Nitpick: I would drop this and just predeclare struct devrequest.
> > 
> > It is also
> > 
> > struct usb_config_descriptor
> > struct usb_device_descriptor
> > struct usb_interface_descriptor
> > struct usb_endpoint_descriptor
> 
> as all of these are used behind a pointer, just predeclare them as well
> (don't define them).
> 

Okay.


> > 
> > > 
> > > > +
> > > > +enum efi_usb_data_direction {
> > > > +	EFI_USB_DATA_IN,
> > > > +	EFI_USB_DATA_OUT,
> > > > +	EFI_USB_NO_DATA
> > > > +};
> > > > +
> > > > +//
> > > > +// Error code for USB Transfer Results
> > > > +//
> > > > +#define EFI_USB_NOERROR 0x0000
> > > > +#define EFI_USB_ERR_NOTEXECUTE 0x0001
> > > > +#define EFI_USB_ERR_STALL 0x0002
> > > > +#define EFI_USB_ERR_BUFFER 0x0004
> > > > +#define EFI_USB_ERR_BABBLE 0x0008
> > > > +#define EFI_USB_ERR_NAK 0x0010
> > > > +#define EFI_USB_ERR_CRC 0x0020
> > > > +#define EFI_USB_ERR_TIMEOUT 0x0040
> > > > +#define EFI_USB_ERR_BITSTUFF 0x0080
> > > > +#define EFI_USB_ERR_SYSTEM 0x0100
> > > 
> > > Nitpick: Add some tabs between the macros and the values for indentation?
> > 
> > I could, but clang-format will remove the tabs. And I like to just call it to format the rest and not have the
> > hassle to
> > revert formatting here.
> 
> Does it also do that, when you sync against an up-to-date .clang-format
> from the kernel?
> 

Since the only difference are the ForEachMacros. Yes. (Tested it nonetheless)

/Fabian

> Cheers,
> Ahmad
> 
> > 
> > /Fabian
> > 
> > > 
> > > > +struct efi_usb_io_protocol {
> > > > +	efi_status_t(EFIAPI *control_transfer)(
> > > > +		struct efi_usb_io_protocol *this, struct devrequest *request,
> > > > +		enum efi_usb_data_direction direction, u32 timeout, void *data,
> > > > +		efi_uintn_t length, u32 *status);
> > > > +	efi_status_t(EFIAPI *bulk_transfer)(struct efi_usb_io_protocol *this,
> > > > +					    u8 device_endpoint, void *data,
> > > > +					    efi_uintn_t *length,
> > > > +					    efi_uintn_t timeout, u32 *status);
> > > > +	void *usb_io_async_interrupt_transfer;
> > > > +	efi_status_t(EFIAPI *sync_interrupt_transfer)(
> > > > +		struct efi_usb_io_protocol *this, u8 device_endpoint,
> > > > +		void *data, efi_uintn_t *length, efi_uintn_t timeout,
> > > > +		u32 *status);
> > > > +	void *efi_usb_io_isochronous_transfer;
> > > > +	void *efi_usb_io_async_isochronous_transfer;
> > > > +	efi_status_t(EFIAPI *get_device_descriptor)(
> > > > +		struct efi_usb_io_protocol *this,
> > > > +		struct usb_device_descriptor *desc);
> > > > +	efi_status_t(EFIAPI *get_config_descriptor)(
> > > > +		struct efi_usb_io_protocol *this,
> > > > +		struct usb_config_descriptor *desc);
> > > > +	efi_status_t(EFIAPI *get_interface_descriptor)(
> > > > +		struct efi_usb_io_protocol *this,
> > > > +		struct usb_interface_descriptor *desc);
> > > > +	efi_status_t(EFIAPI *get_endpoint_descriptor)(
> > > > +		struct efi_usb_io_protocol *this, u8 index,
> > > > +		struct usb_endpoint_descriptor *desc);
> > > > +	efi_status_t(EFIAPI *get_string_descriptor)(
> > > > +		struct efi_usb_io_protocol *this, u16 lang_id, u8 string_id,
> > > > +		wchar_t **desc);
> > > > +	efi_status_t(EFIAPI *get_supported_languages)(
> > > > +		struct efi_usb_io_protocol *this, u16 **lang_id_table,
> > > > +		u16 *table_size);
> > > > +	efi_status_t(EFIAPI *port_reset)(struct efi_usb_io_protocol *this);
> > > > +};
> > > > +
> > > > +#endif
> > > >