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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux