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@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> > Reported-by: kernel test robot <lkp@xxxxxxxxx> # 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@xxxxxxxxx>
Thank you for your review!
Best regards,
Yoshihiro Shimoda
