[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-renesas-soc
Subject:    Re: [PATCH] pinctrl: sh-pfc: Split R-Car H3 support in two independent drivers
From:       Geert Uytterhoeven <geert () linux-m68k ! org>
Date:       2019-12-26 14:34:11
Message-ID: CAMuHMdVnoGpBvU5hH1dBHo6QXFS5voy6SmEDZKyu1JWqLfwhGQ () mail ! gmail ! com
[Download RAW message or body]

Hi Shimoda-san,

On Wed, Dec 25, 2019 at 10:46 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> > From: Geert Uytterhoeven, Sent: Wednesday, December 18, 2019 3:43 AM
> >
> > Despite using the same compatible values ("r8a7795"-based) because of
> > historical reasons, R-Car H3 ES1.x (R8A77950) and R-Car H3 ES2.0+
> > (R8A77951) are really different SoCs, with different part numbers, and
> > with different Pin Function Controller blocks.
> >
> > Reflect this in the pinctrl configuration, by replacing the existing
> > CONFIG_PINCTRL_PFC_R8A7795 symbol by two new config symbols:
> > CONFIG_PINCTRL_PFC_R8A77950 and CONFIG_PINCTRL_PFC_R8A77951.  The latter
> > are selected automatically, depending on the soon-to-be-introduced
> > corresponding SoC-specific config options, and on the current common
> > config option, to relax dependencies.
> >
> > Rename the individual pin control driver source files from
> > pfc-r8a7795-es1.c to pfc-r8a77950.c, and from pfc-r8a7795.c to
> > pfc-r8a77951.c, and make them truly independent.
> > As both SoCs share the same compatible value, special care must be taken
> > to match them to the correct pin control driver, if support for it is
> > included in the running kernel.
> >
> > This will allow making support for early R-Car H3 revisions optional,
> > the largest share of which is taken by the pin control driver.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> > Suggestions for simplifying sh_pfc_quirk_match(), or for alternative
> > solutions are welcome!
>
> I wondered if using weak attribute on both info variables could
> simplify sh_pfc_quirk_match(), but such a code [1] doesn't seem better
> than using #ifdef. Also, using weak attributes waste data size
> if R8A77950=n and R8A77951=y for instance.

Thanks for the great suggestion!

The trick is to add __weak to the existing extern declarations in sh_pfc.h,
instead of adding weak empty structs.
When the structs don't exist, their addresses just become zero.

So I came up with the following (whitespace-damaged) patch, which I intend
to fold into the original, if no one objects.

diff --git a/drivers/pinctrl/sh-pfc/core.c b/drivers/pinctrl/sh-pfc/core.c
index 8da95bf22735fd7b..92f8d5f5b6930849 100644
--- a/drivers/pinctrl/sh-pfc/core.c
+++ b/drivers/pinctrl/sh-pfc/core.c
@@ -1120,19 +1120,11 @@ static const void *sh_pfc_quirk_match(void)
        static const struct soc_device_attribute quirks[] = {
                {
                        .soc_id = "r8a7795", .revision = "ES1.*",
-#ifdef CONFIG_PINCTRL_PFC_R8A77950
                        .data = &r8a77950_pinmux_info,
-#else
-                       .data = (void *)-ENODEV,
-#endif
                },
                {
                        .soc_id = "r8a7795",
-#ifdef CONFIG_PINCTRL_PFC_R8A77951
                        .data = &r8a77951_pinmux_info,
-#else
-                       .data = (void *)-ENODEV,
-#endif
                },

                { /* sentinel */ }
@@ -1140,7 +1132,7 @@ static const void *sh_pfc_quirk_match(void)

        match = soc_device_match(quirks);
        if (match)
-               return match->data;
+               return match->data ?: ERR_PTR(-ENODEV);
 #endif /* CONFIG_PINCTRL_PFC_R8A77950 || CONFIG_PINCTRL_PFC_R8A77951 */

        return NULL;
diff --git a/drivers/pinctrl/sh-pfc/sh_pfc.h b/drivers/pinctrl/sh-pfc/sh_pfc.h
index aa298332f00f1a8e..d57e633e99c0ce66 100644
--- a/drivers/pinctrl/sh-pfc/sh_pfc.h
+++ b/drivers/pinctrl/sh-pfc/sh_pfc.h
@@ -318,8 +318,8 @@ extern const struct sh_pfc_soc_info r8a7791_pinmux_info;
 extern const struct sh_pfc_soc_info r8a7792_pinmux_info;
 extern const struct sh_pfc_soc_info r8a7793_pinmux_info;
 extern const struct sh_pfc_soc_info r8a7794_pinmux_info;
-extern const struct sh_pfc_soc_info r8a77950_pinmux_info;
-extern const struct sh_pfc_soc_info r8a77951_pinmux_info;
+extern const struct sh_pfc_soc_info r8a77950_pinmux_info __weak;
+extern const struct sh_pfc_soc_info r8a77951_pinmux_info __weak;
 extern const struct sh_pfc_soc_info r8a77960_pinmux_info;
 extern const struct sh_pfc_soc_info r8a77961_pinmux_info;
 extern const struct sh_pfc_soc_info r8a77965_pinmux_info;

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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
[prev in list] [next in list] [prev in thread] [next in thread]