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
