Hi Shimoda-san, Thanks for your patch! 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" (https://lore.kernel.org/r/22b4c393bf5074b53791d2797d8fe74deb8ea9a7.1623315732.git.geert+renesas@xxxxxxxxx). > > 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. > --- 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); Regardless, as this doesn't impact functionality: Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx> Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
