Re: [PATCH 3/3] USB: driver for Freescale QUICC Engine USB Host Controller

[Andrew Morton <akpm@linux-foundation.org>]

On Thu, 18 Sep 2008 19:17:46 +0400
Anton Vorontsov <avorontsov@ru.mvista.com> wrote:

> This patch adds support for the FHCI USB controller, as found
> in the Freescale MPC836x and MPC832x processors. It can support
> Full or Low speed modes.
> 
> Quite a lot the hardware is doing by itself (SOF generation, CRC
> generation and checking), though scheduling and retransmission is on
> software's shoulders.
> 
> This controller does not integrate the root hub, so this driver also
> fakes one-port hub. External hub is required to support more than
> one device.
>
> ...
>

Please, no.

> new file mode 100644
> index 0000000..23716fa
> --- /dev/null
> +++ b/drivers/usb/host/fhci-cq.c
> @@ -0,0 +1,105 @@
> +/*
> + * Freescale QUICC Engine USB Host Controller Driver
> + *
> + * Copyright (c) Freescale Semicondutor, Inc. 2006.
> + *               Shlomi Gridish <gridish@freescale.com>
> + *               Jerry Huang <Chang-Ming.Huang@freescale.com>
> + * Copyright (c) Logic Product Development, Inc. 2007
> + *               Peter Barada <peterb@logicpd.com>
> + * Copyright (c) MontaVista Software, Inc. 2008.
> + *               Anton Vorontsov <avorontsov@ru.mvista.com>
> + *
> + * This program is free software; you can redistribute  it and/or modify it
> + * under  the terms of  the GNU General  Public License as published by the
> + * Free Software Foundation;  either version 2 of the  License, or (at your
> + * option) any later version.
> + */
> +
> +/* circular queue structure */

Yet another private ringbuffer implementation.  Sigh.

> +struct cir_q {
> +       int max;		/* size of queue */
> +       int max_in;	/* max items in queue */
> +       int first;	/* index of first in queue */
> +       int last;	/* index after last in queue */
> +       int read;	/* current reading position */
> +       void *table[1];	/* fake size */
> +};
> +
> +/* circular queue handle */
> +static int cq_howmany(struct cir_q *cq)
> +{
> +	int l = cq->last;
> +	int f = cq->first;
> +
> +	return l >= f ? l - f : l + cq->max - f;
> +}
> +
> +static struct cir_q *cq_new(int size)
> +{
> +	struct cir_q *cq;
> +
> +	cq = kzalloc((sizeof(*cq) + size * sizeof(void *)), GFP_KERNEL);
> +	if (cq) {
> +		cq->max = size;
> +		cq->first = 0;
> +		cq->last = 0;
> +		cq->read = 0;
> +		cq->max_in = 0;

The above four statements are unneeded.

> +	}
> +
> +	return cq;
> +}
>
> ...
>
> +++ b/drivers/usb/host/fhci-hcd.c
> @@ -0,0 +1,798 @@
> +/*
> + * Freescale QUICC Engine USB Host Controller Driver
> + *
> + * Copyright (c) Freescale Semicondutor, Inc. 2006.
> + *               Shlomi Gridish <gridish@freescale.com>
> + *               Jerry Huang <Chang-Ming.Huang@freescale.com>
> + * Copyright (c) Logic Product Development, Inc. 2007
> + *               Peter Barada <peterb@logicpd.com>
> + * Copyright (c) MontaVista Software, Inc. 2008.
> + *               Anton Vorontsov <avorontsov@ru.mvista.com>
> + *
> + * This program is free software; you can redistribute  it and/or modify it
> + * under  the terms of  the GNU General  Public License as published by the
> + * Free Software Foundation;  either version 2 of the  License, or (at your
> + * option) any later version.
> + */
> +
> +#if defined(CONFIG_FHCI_DEBUG) && !defined(DEBUG)
> +#define DEBUG
> +#endif
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/delay.h>
> +#include <linux/errno.h>
> +#include <linux/list.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/usb.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_gpio.h>
> +#include <asm/qe.h>
> +#include <asm/fsl_gtm.h>
> +#include "../core/hcd.h"
> +#include "fhci.h"
> +#include "fhci-hub.c"
> +#include "fhci-q.c"
> +#include "fhci-dbg.c"
> +#include "fhci-mem.c"
> +#include "fhci-cq.c"
> +#include "fhci-tds.c"
> +#include "fhci-sched.c"

hack, gack, nack.

We know that USB likes to prevertedly #include C files all over the
place, but this doesn't mean that it's a sane, tasteful or desirable
thing to do.

I stopped looking there.  Guys - get a grip :(