[U-Boot] [PATCH 3/3] pxa25x: Add USB ethernet gadget driver

[Marek Vasut]

Dear Łukasz Dałek,

> On 17.08.2012 22:57, Marek Vasut wrote:
> > + /* let loose that packet. maybe try writing another one,
> > Wrong comment , fix globally ... doens't checkpatch catch those?
> 
> no
> 
> >> +		 * double buffering might work.  TSP, TPC, and TFS
> >> +		 * bit values are the same for all normal IN endpoints.
> >> +		 */
> >> +		writel(UDCCS_BI_TPC, ep->reg_udccs);
> >> +		if (is_short)
> >> +			writel(UDCCS_BI_TSP, ep->reg_udccs);
> >> +
> >> +		/* requests complete when all IN data is in the FIFO */
> >> +		if (is_last) {
> >> +			done(ep, req, 0);
> >> +			if (list_empty(&ep->queue))
> >> +				pio_irq_disable(ep->bEndpointAddress);
> >> +			return 1;
> >> +		}
> >> +
> >> +		/* TODO experiment: how robust can fifo mode tweaking be?
> >> +		 * double buffering is off in the default fifo mode, which
> >> +		 * prevents TFS from being set here. */
> >> +
> >> +	} while (readl(ep->reg_udccs)&  UDCCS_BI_TFS);
> >> +	return 0;
> >> +}
> >> +
> >> +/* caller asserts req->pending (ep0 irq status nyet cleared); starts
> >> + * ep0 data stage.  these chips want very simple state transitions.
> >> + */
> >> +static inline
> >> +void ep0start(struct pxa25x_udc *dev, u32 flags, const char *tag)
> >> +{
> >> +	writel(flags|UDCCS0_SA|UDCCS0_OPR,&dev->regs->udccs0);
> >> +	/* writel(USIR0_IR0,&dev->regs->usir0); */
> > 
> > What the heck ?
> 
> I've left this line commented out because I think that on
> pxa chips with revision other than a0, code can make troubles.
> I have only pxa255 a0 and I can't test it. On my board code
> works very well.

Please remove any dead code, either comment it, put it there, decide based on 
runtime-detected CPU revision or something, but no dead code.

Is there anything about this in the docs?

> > +/* until it's enabled, this UDC should be completely invisible
> > + * to any USB host.
> > + */
> > +static void udc_enable(struct pxa25x_udc *dev)
> > +{
> > +	debug("udc: enabling udc\n");
> > +
> > +	udc_clear_mask_UDCCR(UDCCR_UDE);
> > +
> > +	/* try to clear these bits before we enable the udc */
> > +	udc_ack_int_UDCCR(UDCCR_SUSIR|/*UDCCR_RSTIR|*/UDCCR_RESIR);
> > Why is this commented out ?
> 
> as above
> 
> > Replace with debug()
> 
> I've defined noisy() because they sometimes help me when
> debugging code. Can't be left there?

Try debug_cond() then?

> >> +#ifdef NOISY_DEBUG
> >> +# define noisy(x...)	debug(x)
> >> +#else
> >> +# define noisy(x...)	do {} while (0)
> >> +#endif
> >> +
> >> +#else
> >> +
> >> +#define	dump_udccr(x)	do {} while (0)
> >> +#define	dump_udccs0(x)	do {} while (0)
> >> +#define	dump_state(x)	do {} while (0)
> >> +#define noisy(x...)	do {} while (0)
> >> +
> >> +#endif
> >> +
> >> +#endif /* __LINUX_USB_GADGET_PXA25X_H */

Best regards,
Marek Vasut