From mboxrd@z Thu Jan  1 00:00:00 1970
From: =?ISO-8859-2?Q?=A3ukasz_Da=B3ek?= <luk0104@gmail.com>
Date: Fri, 17 Aug 2012 23:39:30 +0200
Subject: [U-Boot] [PATCH 3/3] pxa25x: Add USB ethernet gadget driver
In-Reply-To: <201208172257.56285.marex@denx.de>
References: <1345235499-32556-1-git-send-email-luk0104@gmail.com>
	<1345235499-32556-3-git-send-email-luk0104@gmail.com>
	<201208172257.56285.marex@denx.de>
Message-ID: <502EBA12.90805@gmail.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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.

> +/* 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?
>> +#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 */