RE: [PATCH v4 4/7] scsi: ufs-renesas: Add support for Renesas R-Car UFS controller

From: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
To: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Alim Akhtar <alim.akhtar@samsung.com>,
	"avri.altman@wdc.com" <avri.altman@wdc.com>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzk+dt@kernel.org>,
	"James E.J. Bottomley" <jejb@linux.ibm.com>,
	"Martin K. Petersen" <martin.petersen@oracle.com>,
	scsi <linux-scsi@vger.kernel.org>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
	<devicetree@vger.kernel.org>,
	Linux-Renesas <linux-renesas-soc@vger.kernel.org>
Subject: RE: [PATCH v4 4/7] scsi: ufs-renesas: Add support for Renesas R-Car UFS controller
Date: Thu, 28 Apr 2022 06:45:50 +0000	[thread overview]
Message-ID: <TYBPR01MB5341B5236609633655ADF75DD8FD9@TYBPR01MB5341.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <CAMuHMdXn=qm_gutcq64y+XCa=CchKPqPnZqoc7jMMjcw-G0vmA@mail.gmail.com>

Hi Geert-san,

Thank you for your review!

> From: Geert Uytterhoeven, Sent: Wednesday, April 27, 2022 10:10 PM
> 
> On Wed, Apr 20, 2022 at 11:39 AM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
<snip>
> > +static void ufs_renesas_reg_control(struct ufs_hba *hba,
> > +                                   const struct ufs_renesas_init_param *p)
> > +{
> > +       static u32 save[MAX_INDEX];
> > +       int ret;
> > +       u32 val;
> > +
> > +       pr_debug("%s: %d %04x %08x, %08x, %d\n", __func__, p->mode, p->reg,
> > +                p->u.val, p->mask, p->index);
> 
> Do you need this?
> If yes, perhaps dev_dbg(hba->dev, ...)?

Umm, I don't think so because the parameters are hardcoded.
So, I'll remove all pr_debug.

> > +
> > +       WARN_ON(p->index >= MAX_INDEX);
> > +
> > +       switch (p->mode) {
> > +       case MODE_RESTORE:
> > +               ufshcd_writel(hba, save[p->index], p->reg);
> > +               break;
> > +       case MODE_SET:
> > +               pr_debug("%s: %d %x %x\n", __func__, p->index, save[p->index],
> > +                        p->u.set);
> 
> Likewise.

I'll remove it.

> > +               save[p->index] |= p->u.set;
> > +               break;
> > +       case MODE_SAVE:
> > +               save[p->index] = ufshcd_readl(hba, p->reg) & p->mask;
> > +               pr_debug("%s: index = %d, save = %08x\n", __func__,
> > +                        p->index, save[p->index]);
> 
> Likewise.

I'll remove it too.

> > +               break;
> > +       case MODE_POLL:
> > +               ret = readl_poll_timeout_atomic(hba->mmio_base + p->reg,
> > +                                               val,
> > +                                               (val & p->mask) == p->u.expected,
> > +                                               10, 1000);
> > +               if (ret)
> > +                       pr_err("%s: poll failed %d (%08x, %08x, %08x)\n",
> > +                              __func__, ret, val, p->mask, p->u.expected);
> > +               break;
> > +       case MODE_WAIT:
> > +               if (p->u.delay_us > 1000)
> > +                       mdelay(p->u.delay_us / 1000);
> 
> mdelay(DIV_ROUND_UP(p->u.delay_us, 1000));
> (cfr. include/linux/delay.h:ndelay())

I got it. I'll fix it.

> 
> > +               else
> > +                       udelay(p->u.delay_us);
> > +               break;
> > +       case MODE_WRITE:
> > +               ufshcd_writel(hba, p->u.val, p->reg);
> > +               break;
> > +       default:
> > +               break;
> > +       }
> > +}
> > +
> > +static void ufs_renesas_pre_init(struct ufs_hba *hba)
> > +{
> > +       const struct ufs_renesas_init_param *p = ufs_param;
> > +       int i;
> 
> unsigned int i

I'll fix it.

> > +
> > +       for (i = 0; i < ARRAY_SIZE(ufs_param); i++)
> > +               ufs_renesas_reg_control(hba, &p[i]);
> > +}
> 
> > +static const struct of_device_id __maybe_unused ufs_renesas_of_match[] = {
> > +       { .compatible = "renesas,r8a779f0-ufs" },
> 
> As pointed out by the kernel test robot, this lack a sentinel.

Yes. I have already fixed this on v5.

Best regards,
Yoshihiro Shimoda