Re: [RFC] USB Support

[Felix Zielcke <fzielcke@z-51.de>]

Am Mittwoch, den 27.08.2008, 21:56 +0200 schrieb Marco Gerards:
> Hi,

Hello Marco,

not that big as you might have expected, but maybe not that bad start.
I try to get more used to this :)


> - Broken OHCI support, it works in QEMu but I haven't tested on real
>   hardware.  Please do not review ohci.c, it won't be committed but I
>   included it now in case someone wants to test it anyways :-)

;)

Index: conf/i386-pc.rmk
===================================================================
--- conf/i386-pc.rmk	(revision 1830)
+++ conf/i386-pc.rmk	(working copy)

@@ -140,14 +140,21 @@ grub_emu_SOURCES = commands/boot.c comma
 	fs/ufs.c fs/xfs.c fs/afs.c					\
 	\
 	util/console.c util/hostfs.c util/grub-emu.c util/misc.c	\
-	util/biosdisk.c util/getroot.c					\
+	util/biosdisk.c util/getroot.c 					\

Janitor work? :)

@@ -165,7 +172,8 @@ pkglib_MODULES = biosdisk.mod _chain.mod
 	vbe.mod vbetest.mod vbeinfo.mod video.mod gfxterm.mod \
 	videotest.mod play.mod bitmap.mod tga.mod cpuid.mod serial.mod	\
 	ata.mod vga.mod memdisk.mod jpeg.mod png.mod pci.mod lspci.mod \
-	aout.mod _bsd.mod bsd.mod pxe.mod pxecmd.mod datetime.mod date.mod \
+	aout.mod _bsd.mod bsd.mod usb.mod uhci.mod ohci.mod usbtest.mod usbms.mod \
+	pxe.mod pxecmd.mod datetime.mod date.mod \
 	datehook.mod

I think datehook.mod feels more comfortable if it would be along with
datetime.mod and date.mod

Index: disk/usbms.c
===================================================================
--- disk/usbms.c	(revision 0)
+++ disk/usbms.c	(revision 0)

+  int usb_iterate (grub_usb_device_t usbdev)
+    {
+      grub_usb_err_t err;
+      struct grub_usb_desc_device *descdev = &usbdev->descdev;
+      int i;
+
+      if (descdev->class != 0 || descdev->subclass || descdev->protocol != 0)
+	return 0;

Please be consistent, != 0 isn't needed anyway.

+  grub_usb_iterate (usb_iterate);
+}
+
+\f

Weird `\f' character which seems not to make sense.

+
+static int
+grub_usbms_iterate (int (*hook) (const char *name, int luns))



+  /* XXX: Magic and check this code.  */
+  if (status.status == 2)

A structure named status with a field status looks a bit funny,
but I don't have a suggestion for you :(


+static struct grub_scsi_dev grub_usbms_dev =
+  {
+    .name = "usb",

Shouldn't this be "usbms" ?

+    .iterate = grub_usbms_iterate,
+    .open = grub_usbms_open,
+    .close = grub_usbms_close,
+    .read = grub_usbms_read,
+    .write = grub_usbms_write
+  }; 

+grub_usb_err_t
+grub_usb_root_hub (grub_usb_controller_t controller);
+
+\f
Again weird `\f'
+/* XXX: All handled by libusb for now.  */
+struct grub_usb_controller_dev

+struct grub_usb_controller
+{
+  /* The underlying USB Host Controller device.  */
+  grub_usb_controller_dev_t dev;
+
+  /* Data used by the USB Host Controller Driver.  */
+  void *data;
+};
+\f

Again.

+
+struct grub_usb_interface
+{
+  struct grub_usb_desc_if *descif;
+
+  struct grub_usb_desc_endp *descendp;
+};


+};
+
+\f

Again.

+
+typedef enum
+  {
+typedef struct grub_usb_transfer *grub_usb_transfer_t;
+
+\f

Again.

+#define GRUB_USB_REQTYPE_IN		(1 << 7)
+#define GRUB_USB_REQTYPE_OUT		(0 << 7)

Index: bus/usb/uhci.c
===================================================================
--- bus/usb/uhci.c	(revision 0)
+++ bus/usb/uhci.c	(revision 0)


+ fail:
+  if (u)
+    {
+      grub_free ((void *) u->qh);
+      grub_free (u->framelist);
+    }
+  grub_free (u);

util/misc.c: grub_free () doestn't check *ptr before calling free ()
But better util/misc.c get's changed then your code and bus/usb/uhci
isn't compiled for grub-emu anyway :)

+static int
+grub_uhci_hubports (grub_usb_controller_t dev __attribute__((unused)))
+{
+  /* The root hub has exactly two ports.  */
+  return 2;
+}
+
+\f

Again weird `\f' character.

+static struct grub_usb_controller_dev usb_controller =
+{
+  .name = "uhci",
+  .iterate = grub_uhci_iterate,
+  .transfer = grub_uhci_transfer,
+  .hubports = grub_uhci_hubports,
+  .portstatus = grub_uhci_portstatus,
+  .detect_dev = grub_uhci_detect_dev
+};

Index: bus/usb/usbhub.c
===================================================================
--- bus/usb/usbhub.c	(revision 0)
+++ bus/usb/usbhub.c	(revision 0)

+  return dev;
+}
+
+\f

Again.

+static grub_err_t
+grub_usb_add_hub (grub_usb_device_t dev)
+{

Index: bus/usb/usb.c
===================================================================
--- bus/usb/usb.c	(revision 0)
+++ bus/usb/usb.c	(revision 0)

+  return 0;
+}
+#endif
+
+\f

And again weird `\f'

+grub_usb_err_t
+grub_usb_clear_halt (grub_usb_device_t dev, int endpoint)
+{

Index: commands/usbtest.c
===================================================================
--- commands/usbtest.c	(revision 0)
+++ commands/usbtest.c	(revision 0)

+static const char *usb_classes[] =
+  {
+    "",
+    "Audio",
+    "Communication Interface",
+    "HID",
+    "",

Does this empty string makes sense?

+    "Physical",
+    "Image",
+    "Printer",
+    "Mass Storage",
+    "Hub",
+    "Data Interface",
+    "Smart Card",
+    "Content Security",
+    "Video"
+  };


Index: util/usb.c
===================================================================
--- util/usb.c	(revision 0)
+++ util/usb.c	(revision 0)

+#include <config.h>
+#include <grub/misc.h>
+#include <grub/mm.h>
+#include <usb.h>
+#include <grub/usb.h>
+#include <grub/dl.h>
+
+\f

Again weird `\f'

+grub_err_t
+grub_libusb_fini (void)
+{
+  return 0;
+}
+
+\f

Again.


-- 
Felix Zielcke