RE: [PATCH v3] mmc: host: renesas_sdhi: Refactor renesas_sdhi_probe()

[Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>]

Hi Geert-san,


> From: Geert Uytterhoeven, Sent: Tuesday, July 20, 2021 7:35 PM
> 
> On Fri, Jul 2, 2021 at 1:31 PM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
> > Refactor renesas_sdhi_probe() to avoid increasing numbers of
> > sdhi_quirks_match[] entry when we add other stable SoCs like
> > r8a779m*.
> 
> Cool, then we won't need "[PATCH 04/14] mmc: renesas_sdhi: Add support
> for R-Car H3e-2G and M3e-2G".

Indeed!

> > Note that the sdhi_quirks_match[] is only needed on
> > renesas_sdhi_internal_dmac.c so that of_data of
> > renesas_sdhi_sys_dmac.c keeps as-is.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > Reported-by: kernel test robot <lkp@intel.com> # build fix on RFC
> 
> I would drop this tag, as it is basically a test/review comment for
> an older version of a patch.

I got it. I'll drop it.

> > --- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c
> > +++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
> > @@ -406,15 +516,25 @@ static const struct soc_device_attribute soc_dma_quirks[] = {
> >  static int renesas_sdhi_internal_dmac_probe(struct platform_device *pdev)
> >  {
> >         const struct soc_device_attribute *soc = soc_device_match(soc_dma_quirks);
> > +       const struct soc_device_attribute *attr = soc_device_match(sdhi_quirks_match);
> > +       const struct renesas_sdhi_of_data_with_quirks *of_data_quirks;
> > +       const struct renesas_sdhi_quirks *quirks = NULL;
> >         struct device *dev = &pdev->dev;
> >
> >         if (soc)
> >                 global_flags |= (unsigned long)soc->data;
> >
> > +       of_data_quirks = of_device_get_match_data(&pdev->dev);
> > +
> > +       if (attr)
> > +               quirks = attr->data;
> > +
> 
> I think the code would be easier to read without the interleaving of
> of_device_get_match_data() and soc_device_match() based matching, ...
> 
> >         /* value is max of SD_SECCNT. Confirmed by HW engineers */
> >         dma_set_max_seg_size(dev, 0xffffffff);
> >
> > -       return renesas_sdhi_probe(pdev, &renesas_sdhi_internal_dmac_dma_ops);
> > +       return renesas_sdhi_probe(pdev, &renesas_sdhi_internal_dmac_dma_ops,
> > +                                 of_data_quirks->of_data,
> > +                                 quirks ? : of_data_quirks->quirks);
> 
> ... and without using the ternary operator, like:
> 
>     of_data_quirks = of_device_get_match_data(&pdev->dev);
>     quirks = of_data_quirks->quirks;
> 
>     attr = soc_device_match(soc_dma_quirks);
>     if (attr)
>             global_flags |= (unsigned long)attr->data;
> 
>     attr = soc_device_match(sdhi_quirks_match);
>     if (attr)
>             quirks = attr->data;
> 
>     [...]
> 
>     return renesas_sdhi_probe(pdev, &renesas_sdhi_internal_dmac_dma_ops,
>                               of_data_quirks->of_data, quirks);

Thank you for your suggestion! I'll update this patch.

> Regardless, as this doesn't impact functionality:
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Thank you for your review!

Best regards,
Yoshihiro Shimoda