RE: [PATCH 06/10] usb: renesas_usbhs: Add support for RZ/A2

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Shimodaさん、

> From: Yoshihiro Shimoda
> Sent: Thursday, May 09, 2019 3:04 AM

> > -/* status */
> > -#define usbhsc_flags_init(p)   do {(p)->flags = 0; } while (0)
> > -#define usbhsc_flags_set(p, b) ((p)->flags |=  (b))
> > -#define usbhsc_flags_clr(p, b) ((p)->flags &= ~(b))
> > -#define usbhsc_flags_has(p, b) ((p)->flags &   (b))
> 
> I would like to separate this patch to some patches like below to review
> the patch(es) easily:
> 
> 1. Just move these definitions to common.h.

FYI, checkpatch.pl says this:

  WARNING: Single statement macros should not use a do {} while (0) loop
  #122: FILE: drivers/usb/renesas_usbhs/common.h:350:
  +#define usbhsc_flags_init(p)   do {(p)->flags = 0; } while (0)

So, I will change this code to:

#define usbhsc_flags_init(p)   {(p)->flags = 0;}



> It's the same with RZA1. So, I think we can reuse the code like below.
> What do you think?
> +	if (dparam->type == USBHS_TYPE_RZA1 ||
> +	    dparam->type == USBHS_TYPE_RZA2) {
> 		dparam->pipe_configs = usbhsc_new_pipe;
> 		dparam->pipe_size = ARRAY_SIZE(usbhsc_new_pipe);
> 	}

OK.

#At first, RZA2 had 'dparam->has_usb_dmac = 1'. But, DMA had some
 issues, so I removed it.



> I prefer to add "{ }" on "if" and "else" like below.
> 
> 	if (usbhsc_flags_has(priv, USBHSF_CFIFO_BYTE_ADDR)) {
> 		for (i = 0; i < len; i++)
> 			iowrite8(buf[i], addr + (i & 0x03));
> 	} else {
> 		for (i = 0; i < len; i++)
> 			iowrite8(buf[i], addr + (0x03 - (i & 0x03)));
> 	}

OK.
#I always prefer braces. It is easier to read.


> > +static int usbhs_rza2_power_ctrl(struct platform_device *pdev,
> > +				void __iomem *base, int enable)
> > +{
> > +	struct usbhs_priv *priv = usbhs_pdev_to_priv(pdev);
> > +	int retval = -ENODEV;
> > +
> > +	if (priv->phy) {
> > +		if (enable) {
> > +			retval = phy_init(priv->phy);
> > +			if (enable) {
> > +				usbhs_bset(priv, SUSPMODE, SUSPM, SUSPM);
> > +				/* Wait 100 usec for PLL to become stable */
> > +				udelay(100);
> > +			} else {
> 
> This else code never runs. So,

Yes, thank you.

This code is ugly, so I'm going to change it.

Chris





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux