Re: [PATCH] PXA27x UDC driver.

[David Brownell <david-b@pacbell.net>]

On Sunday 01 July 2007, Russell King - ARM Linux wrote:
> >  
> > +#ifdef CONFIG_USB_GADGET_PXA27X
> > +#define DEV_CONFIG_SUBSET
> > +#endif
> > +
> 
> Bad - can we not make this runtime dependent?

The #define controls which descriptors are statically linked
into every object.  I suppose someone could submit a patch which
does that differently ... I'm not keen on having lots of "will
never use" data bloating any driver, but it could be moved to
__initdata, at the cost of adding code to kmalloc (and kfree)
all the individual descriptors.  Restricting code bloat to init
sections would still be worse than no bloat...


> > +static void *pxa27x_ep_alloc_buffer(struct usb_ep *_ep, unsigned bytes,
> > +				    dma_addr_t * dma, unsigned int gfp_flags)
> > +{
> > +	...
> 
> Err, no.  There's a perfectly good replacement.  DMA pools.  Please use
> the provided infrastructure instead of inventing your own solution.

This was cloned from code which predates dma pools.  :)

Regardless, I'm planning to remove that interface from the gadget stack.
It's utility is far exceeded by the number of controller driver bugs
in that area.  Plus, if any gadget driver needs such a mechanism, the
dma_pool stuff that now exists could be called directly.


> Pointers which are cleared are set to NULL not zero.  Zero is an integer.
> NULL is a pointer.  Don't confuse the two.

ISTR this class of error would be reported by sparse ("make check").

- Dave