[U-Boot] [PATCH v5 1/2] usb: zynqmp: Add XHCI driver support

[Marek Vasut]

On Monday, November 16, 2015 at 08:23:24 AM, Siva Durga Prasad Paladugu wrote:
> Hi Marek,

Hi,

[...]

> > > +	ctx->hcd = (struct xhci_hccr *)ctr_addr[index];
> > > +	ctx->dwc3_reg = (struct dwc3 *)((char *)(ctx->hcd) +
> > > +DWC3_REG_OFFSET);
> > 
> > Should be void *, not char * I think. Also, the parenthesis around
> > ctx->hcd are not needed.
> 
> I think keeping char* would be good for easy understanding and readability.

Using bogus casts is never good for anything.

> > > +	ret = board_usb_init(index, USB_INIT_HOST);
> > > +	if (ret != 0) {
> > > +		puts("Failed to initialize board for USB\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	ret = zynqmp_xhci_core_init(ctx);
> > > +	if (ret < 0) {
> > > +		puts("Failed to initialize xhci\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	*hccr = (struct xhci_hccr *)ctx->hcd;
> > > +	*hcor = (struct xhci_hcor *)((uint32_t) *hccr
> > > +				+ HC_LENGTH(xhci_readl(&(*hccr)-
> > >
> > >cr_capbase)));
> > 
> > You want to use uintptr_t for the same of 64bit parts.
> 
> Ok
> 
> > > +	debug("zynqmp-xhci: init hccr %x and hcor %x hc_length %d\n",
> > > +	      (uint32_t)*hccr, (uint32_t)*hcor,
> > > +	      (uint32_t)HC_LENGTH(xhci_readl(&(*hccr)->cr_capbase)));
> > 
> > We have %p for printing pointers, so drop the casts.
> > 
> > Also, since you're using HC_LENGTH() here twice, you might as well put
> > it's value into temporary variable and use it in both places, it might
> > make the code slightly more readable.
> 
> Ok, will take care in next version

Thanks!

Best regards,
Marek Vasut