RE: [PATCH] pinctrl: sh-pfc: Split R-Car H3 support in two independent drivers

[Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>]

Hi Geert-san,

> 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@xxxxxxxxx>
> ---
> 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.

So,
Reviewed-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>

Also I tested on both R-Car H3 ES1.1 Salvator-X and R-Car H3 ES3.0 Salvator-XS,
and the kernel can boot on these boards. So,

Tested-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>

Best regards,
Yoshihiro Shimoda
---
[1]
This patch below is based on the topic/r8a7795-rename-v1 branch.

diff --git a/drivers/pinctrl/sh-pfc/core.c b/drivers/pinctrl/sh-pfc/core.c
index 1ae7969..c6e86e0 100644
--- a/drivers/pinctrl/sh-pfc/core.c
+++ b/drivers/pinctrl/sh-pfc/core.c
@@ -888,27 +888,22 @@ static inline void sh_pfc_check_driver(struct platform_driver *pdrv) {}
 #endif /* !DEBUG */
 
 #ifdef CONFIG_OF
-static const void *sh_pfc_quirk_match(void)
-{
 #if defined(CONFIG_PINCTRL_PFC_R8A77950) || \
     defined(CONFIG_PINCTRL_PFC_R8A77951)
+const struct sh_pfc_soc_info __weak r8a77950_pinmux_info;
+const struct sh_pfc_soc_info __weak r8a77951_pinmux_info;
+
+static const void *sh_pfc_quirk_match(void)
+{
 	const struct soc_device_attribute *match;
 	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 */ }
@@ -917,10 +912,12 @@ static const void *sh_pfc_quirk_match(void)
 	match = soc_device_match(quirks);
 	if (match)
 		return match->data;
-#endif /* CONFIG_PINCTRL_PFC_R8A77950 || CONFIG_PINCTRL_PFC_R8A77951 */
 
 	return NULL;
 }
+#else
+#define sh_pfc_quirk_match()	NULL
+#endif /* CONFIG_PINCTRL_PFC_R8A77950 || CONFIG_PINCTRL_PFC_R8A77951 */
 #endif /* CONFIG_OF */
 
 static int sh_pfc_probe(struct platform_device *pdev)
@@ -934,6 +931,8 @@ static int sh_pfc_probe(struct platform_device *pdev)
 		info = sh_pfc_quirk_match();
 		if (IS_ERR(info))
 			return PTR_ERR(info);
+		if (info && !info->name)
+			return -ENODEV;
 
 		if (!info)
 			info = of_device_get_match_data(&pdev->dev);