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

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

[Yoshihiro Shimoda]

Refactor renesas_sdhi_probe() to avoid increasing numbers of
sdhi_quirks_match[] entry when we add other stable SoCs like
r8a779m*.

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@renesas.com>
Reported-by: kernel test robot <lkp@intel.com> # build fix on RFC
---
 - I tested this patch on r8a77951 (ES3.0), r8a77960 (ES1.0) and r8a77965.
 - Also I tested this patch on r8a7791.

 Changes from RFC v2:
 - Remove "RFC" mark from the subject.
 - Add a comment to the Reported-by tag.
 - Move all quirks to internal_dmac.c so that expands the renesas_sdhi_probe()
   arguments. So, update the commit subject and description.
 - Don't modify the renesas_sdhi_sys_dmac.c's of_data.
 - Replace tabs with a space in of_data_with_quirks variables.
 https://lore.kernel.org/linux-renesas-soc/20210629102033.847369-1-yoshihiro.shimoda.uh@renesas.com/

 Changes from RFC v1:
 - Fix build error in sys_dmac.c, reported by kernel test robot, so that
   add Reported-by tag.
 - Always set quirks, not using else statement.
 - Fix a NULL dereference if of_device_get_match_data() returns NULL.
 https://lore.kernel.org/linux-renesas-soc/20210625075508.664674-1-yoshihiro.shimoda.uh@renesas.com/

 drivers/mmc/host/renesas_sdhi.h               |   9 +-
 drivers/mmc/host/renesas_sdhi_core.c          |  90 +-----------
 drivers/mmc/host/renesas_sdhi_internal_dmac.c | 128 +++++++++++++++++-
 drivers/mmc/host/renesas_sdhi_sys_dmac.c      |   3 +-
 4 files changed, 137 insertions(+), 93 deletions(-)

diff --git a/drivers/mmc/host/renesas_sdhi.h b/drivers/mmc/host/renesas_sdhi.h
index 53eded81a53e..0c45e82ff0de 100644
--- a/drivers/mmc/host/renesas_sdhi.h
+++ b/drivers/mmc/host/renesas_sdhi.h
@@ -42,6 +42,11 @@ struct renesas_sdhi_quirks {
 	const u8 (*hs400_calib_table)[SDHI_CALIB_TABLE_MAX];
 };
 
+struct renesas_sdhi_of_data_with_quirks {
+	const struct renesas_sdhi_of_data *of_data;
+	const struct renesas_sdhi_quirks *quirks;
+};
+
 struct tmio_mmc_dma {
 	enum dma_slave_buswidth dma_buswidth;
 	bool (*filter)(struct dma_chan *chan, void *arg);
@@ -78,6 +83,8 @@ struct renesas_sdhi {
 	container_of((host)->pdata, struct renesas_sdhi, mmc_data)
 
 int renesas_sdhi_probe(struct platform_device *pdev,
-		       const struct tmio_mmc_dma_ops *dma_ops);
+		       const struct tmio_mmc_dma_ops *dma_ops,
+		       const struct renesas_sdhi_of_data *of_data,
+		       const struct renesas_sdhi_quirks *quirks);
 int renesas_sdhi_remove(struct platform_device *pdev);
 #endif
diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
index e49ca0f7fe9a..6fc4cf3c9dce 100644
--- a/drivers/mmc/host/renesas_sdhi_core.c
+++ b/drivers/mmc/host/renesas_sdhi_core.c
@@ -305,27 +305,6 @@ static int renesas_sdhi_start_signal_voltage_switch(struct mmc_host *mmc,
 #define SH_MOBILE_SDHI_SCC_TMPPORT_CALIB_CODE_MASK	0x1f
 #define SH_MOBILE_SDHI_SCC_TMPPORT_MANUAL_MODE		BIT(7)
 
-static const u8 r8a7796_es13_calib_table[2][SDHI_CALIB_TABLE_MAX] = {
-	{ 3,  3,  3,  3,  3,  3,  3,  4,  4,  5,  6,  7,  8,  9, 10, 15,
-	 16, 16, 16, 16, 16, 16, 17, 18, 18, 19, 20, 21, 22, 23, 24, 25 },
-	{ 5,  5,  5,  5,  5,  5,  5,  5,  5,  5,  5,  5,  6,  7,  8, 11,
-	 12, 17, 18, 18, 18, 18, 18, 18, 18, 19, 20, 21, 22, 23, 25, 25 }
-};
-
-static const u8 r8a77965_calib_table[2][SDHI_CALIB_TABLE_MAX] = {
-	{ 1,  2,  6,  6,  7,  8,  9, 10, 11, 12, 13, 14, 15, 15, 15, 16,
-	 17, 18, 19, 20, 21, 22, 23, 24, 25, 25, 26, 27, 28, 29, 30, 31 },
-	{ 2,  3,  4,  4,  5,  6,  7,  9, 10, 11, 12, 13, 14, 15, 16, 17,
-	 17, 17, 20, 21, 22, 23, 24, 25, 27, 28, 29, 30, 31, 31, 31, 31 }
-};
-
-static const u8 r8a77990_calib_table[2][SDHI_CALIB_TABLE_MAX] = {
-	{ 0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,
-	  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0 },
-	{ 0,  0,  0,  1,  2,  3,  3,  4,  4,  4,  5,  5,  6,  8,  9, 10,
-	 11, 12, 13, 15, 16, 17, 17, 18, 18, 19, 20, 22, 24, 25, 26, 26 }
-};
-
 static inline u32 sd_scc_read32(struct tmio_mmc_host *host,
 				struct renesas_sdhi *priv, int addr)
 {
@@ -895,69 +874,12 @@ static void renesas_sdhi_enable_dma(struct tmio_mmc_host *host, bool enable)
 	renesas_sdhi_sdbuf_width(host, enable ? width : 16);
 }
 
-static const struct renesas_sdhi_quirks sdhi_quirks_4tap_nohs400 = {
-	.hs400_disabled = true,
-	.hs400_4taps = true,
-};
-
-static const struct renesas_sdhi_quirks sdhi_quirks_4tap = {
-	.hs400_4taps = true,
-	.hs400_bad_taps = BIT(2) | BIT(3) | BIT(6) | BIT(7),
-};
-
-static const struct renesas_sdhi_quirks sdhi_quirks_nohs400 = {
-	.hs400_disabled = true,
-};
-
-static const struct renesas_sdhi_quirks sdhi_quirks_bad_taps1357 = {
-	.hs400_bad_taps = BIT(1) | BIT(3) | BIT(5) | BIT(7),
-};
-
-static const struct renesas_sdhi_quirks sdhi_quirks_bad_taps2367 = {
-	.hs400_bad_taps = BIT(2) | BIT(3) | BIT(6) | BIT(7),
-};
-
-static const struct renesas_sdhi_quirks sdhi_quirks_r8a7796_es13 = {
-	.hs400_4taps = true,
-	.hs400_bad_taps = BIT(2) | BIT(3) | BIT(6) | BIT(7),
-	.hs400_calib_table = r8a7796_es13_calib_table,
-};
-
-static const struct renesas_sdhi_quirks sdhi_quirks_r8a77965 = {
-	.hs400_bad_taps = BIT(2) | BIT(3) | BIT(6) | BIT(7),
-	.hs400_calib_table = r8a77965_calib_table,
-};
-
-static const struct renesas_sdhi_quirks sdhi_quirks_r8a77990 = {
-	.hs400_calib_table = r8a77990_calib_table,
-};
-
-/*
- * Note for r8a7796 / r8a774a1: we can't distinguish ES1.1 and 1.2 as of now.
- * So, we want to treat them equally and only have a match for ES1.2 to enforce
- * this if there ever will be a way to distinguish ES1.2.
- */
-static const struct soc_device_attribute sdhi_quirks_match[]  = {
-	{ .soc_id = "r8a774a1", .revision = "ES1.[012]", .data = &sdhi_quirks_4tap_nohs400 },
-	{ .soc_id = "r8a7795", .revision = "ES1.*", .data = &sdhi_quirks_4tap_nohs400 },
-	{ .soc_id = "r8a7795", .revision = "ES2.0", .data = &sdhi_quirks_4tap },
-	{ .soc_id = "r8a7795", .revision = "ES3.*", .data = &sdhi_quirks_bad_taps2367 },
-	{ .soc_id = "r8a7796", .revision = "ES1.[012]", .data = &sdhi_quirks_4tap_nohs400 },
-	{ .soc_id = "r8a7796", .revision = "ES1.*", .data = &sdhi_quirks_r8a7796_es13 },
-	{ .soc_id = "r8a77961", .data = &sdhi_quirks_bad_taps1357 },
-	{ .soc_id = "r8a77965", .data = &sdhi_quirks_r8a77965 },
-	{ .soc_id = "r8a77980", .data = &sdhi_quirks_nohs400 },
-	{ .soc_id = "r8a77990", .data = &sdhi_quirks_r8a77990 },
-	{ /* Sentinel. */ },
-};
-
 int renesas_sdhi_probe(struct platform_device *pdev,
-		       const struct tmio_mmc_dma_ops *dma_ops)
+		       const struct tmio_mmc_dma_ops *dma_ops,
+		       const struct renesas_sdhi_of_data *of_data,
+		       const struct renesas_sdhi_quirks *quirks)
 {
 	struct tmio_mmc_data *mmd = pdev->dev.platform_data;
-	const struct renesas_sdhi_quirks *quirks = NULL;
-	const struct renesas_sdhi_of_data *of_data;
-	const struct soc_device_attribute *attr;
 	struct tmio_mmc_data *mmc_data;
 	struct tmio_mmc_dma *dma_priv;
 	struct tmio_mmc_host *host;
@@ -966,12 +888,6 @@ int renesas_sdhi_probe(struct platform_device *pdev,
 	struct resource *res;
 	u16 ver;
 
-	of_data = of_device_get_match_data(&pdev->dev);
-
-	attr = soc_device_match(sdhi_quirks_match);
-	if (attr)
-		quirks = attr->data;
-
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res)
 		return -EINVAL;
diff --git a/drivers/mmc/host/renesas_sdhi_internal_dmac.c b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
index e8f4863d8f1a..4af40cbcf1c3 100644
--- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c
+++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
@@ -15,6 +15,7 @@
 #include <linux/mmc/host.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
+#include <linux/of_device.h>
 #include <linux/pagemap.h>
 #include <linux/scatterlist.h>
 #include <linux/sys_soc.h>
@@ -92,7 +93,7 @@ static struct renesas_sdhi_scc rcar_gen3_scc_taps[] = {
 	},
 };
 
-static const struct renesas_sdhi_of_data of_rza2_compatible = {
+static const struct renesas_sdhi_of_data of_data_rza2 = {
 	.tmio_flags	= TMIO_MMC_HAS_IDLE_WAIT | TMIO_MMC_CLK_ACTUAL |
 			  TMIO_MMC_HAVE_CBSY,
 	.tmio_ocr_mask	= MMC_VDD_32_33,
@@ -107,7 +108,11 @@ static const struct renesas_sdhi_of_data of_rza2_compatible = {
 	.max_segs	= 1,
 };
 
-static const struct renesas_sdhi_of_data of_rcar_gen3_compatible = {
+static const struct renesas_sdhi_of_data_with_quirks of_rza2_compatible = {
+	.of_data	= &of_data_rza2,
+};
+
+static const struct renesas_sdhi_of_data of_data_rcar_gen3 = {
 	.tmio_flags	= TMIO_MMC_HAS_IDLE_WAIT | TMIO_MMC_CLK_ACTUAL |
 			  TMIO_MMC_HAVE_CBSY | TMIO_MMC_MIN_RCAR2,
 	.capabilities	= MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ |
@@ -122,11 +127,116 @@ static const struct renesas_sdhi_of_data of_rcar_gen3_compatible = {
 	.max_segs	= 1,
 };
 
+static const u8 r8a7796_es13_calib_table[2][SDHI_CALIB_TABLE_MAX] = {
+	{ 3,  3,  3,  3,  3,  3,  3,  4,  4,  5,  6,  7,  8,  9, 10, 15,
+	 16, 16, 16, 16, 16, 16, 17, 18, 18, 19, 20, 21, 22, 23, 24, 25 },
+	{ 5,  5,  5,  5,  5,  5,  5,  5,  5,  5,  5,  5,  6,  7,  8, 11,
+	 12, 17, 18, 18, 18, 18, 18, 18, 18, 19, 20, 21, 22, 23, 25, 25 }
+};
+
+static const u8 r8a77965_calib_table[2][SDHI_CALIB_TABLE_MAX] = {
+	{ 1,  2,  6,  6,  7,  8,  9, 10, 11, 12, 13, 14, 15, 15, 15, 16,
+	 17, 18, 19, 20, 21, 22, 23, 24, 25, 25, 26, 27, 28, 29, 30, 31 },
+	{ 2,  3,  4,  4,  5,  6,  7,  9, 10, 11, 12, 13, 14, 15, 16, 17,
+	 17, 17, 20, 21, 22, 23, 24, 25, 27, 28, 29, 30, 31, 31, 31, 31 }
+};
+
+static const u8 r8a77990_calib_table[2][SDHI_CALIB_TABLE_MAX] = {
+	{ 0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,
+	  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0 },
+	{ 0,  0,  0,  1,  2,  3,  3,  4,  4,  4,  5,  5,  6,  8,  9, 10,
+	 11, 12, 13, 15, 16, 17, 17, 18, 18, 19, 20, 22, 24, 25, 26, 26 }
+};
+
+static const struct renesas_sdhi_quirks sdhi_quirks_4tap_nohs400 = {
+	.hs400_disabled = true,
+	.hs400_4taps = true,
+};
+
+static const struct renesas_sdhi_quirks sdhi_quirks_4tap = {
+	.hs400_4taps = true,
+	.hs400_bad_taps = BIT(2) | BIT(3) | BIT(6) | BIT(7),
+};
+
+static const struct renesas_sdhi_quirks sdhi_quirks_nohs400 = {
+	.hs400_disabled = true,
+};
+
+static const struct renesas_sdhi_quirks sdhi_quirks_bad_taps1357 = {
+	.hs400_bad_taps = BIT(1) | BIT(3) | BIT(5) | BIT(7),
+};
+
+static const struct renesas_sdhi_quirks sdhi_quirks_bad_taps2367 = {
+	.hs400_bad_taps = BIT(2) | BIT(3) | BIT(6) | BIT(7),
+};
+
+static const struct renesas_sdhi_quirks sdhi_quirks_r8a7796_es13 = {
+	.hs400_4taps = true,
+	.hs400_bad_taps = BIT(2) | BIT(3) | BIT(6) | BIT(7),
+	.hs400_calib_table = r8a7796_es13_calib_table,
+};
+
+static const struct renesas_sdhi_quirks sdhi_quirks_r8a77965 = {
+	.hs400_bad_taps = BIT(2) | BIT(3) | BIT(6) | BIT(7),
+	.hs400_calib_table = r8a77965_calib_table,
+};
+
+static const struct renesas_sdhi_quirks sdhi_quirks_r8a77990 = {
+	.hs400_calib_table = r8a77990_calib_table,
+};
+
+/*
+ * Note for r8a7796 / r8a774a1: we can't distinguish ES1.1 and 1.2 as of now.
+ * So, we want to treat them equally and only have a match for ES1.2 to enforce
+ * this if there ever will be a way to distinguish ES1.2.
+ */
+static const struct soc_device_attribute sdhi_quirks_match[]  = {
+	{ .soc_id = "r8a774a1", .revision = "ES1.[012]", .data = &sdhi_quirks_4tap_nohs400 },
+	{ .soc_id = "r8a7795", .revision = "ES1.*", .data = &sdhi_quirks_4tap_nohs400 },
+	{ .soc_id = "r8a7795", .revision = "ES2.0", .data = &sdhi_quirks_4tap },
+	{ .soc_id = "r8a7796", .revision = "ES1.[012]", .data = &sdhi_quirks_4tap_nohs400 },
+	{ .soc_id = "r8a7796", .revision = "ES1.*", .data = &sdhi_quirks_r8a7796_es13 },
+	{ /* Sentinel. */ },
+};
+
+static const struct renesas_sdhi_of_data_with_quirks of_r8a7795_compatible = {
+	.of_data = &of_data_rcar_gen3,
+	.quirks = &sdhi_quirks_bad_taps2367,
+};
+
+static const struct renesas_sdhi_of_data_with_quirks of_r8a77961_compatible = {
+	.of_data = &of_data_rcar_gen3,
+	.quirks = &sdhi_quirks_bad_taps1357,
+};
+
+static const struct renesas_sdhi_of_data_with_quirks of_r8a77965_compatible = {
+	.of_data = &of_data_rcar_gen3,
+	.quirks = &sdhi_quirks_r8a77965,
+};
+
+static const struct renesas_sdhi_of_data_with_quirks of_r8a77980_compatible = {
+	.of_data = &of_data_rcar_gen3,
+	.quirks = &sdhi_quirks_nohs400,
+};
+
+static const struct renesas_sdhi_of_data_with_quirks of_r8a77990_compatible = {
+	.of_data = &of_data_rcar_gen3,
+	.quirks = &sdhi_quirks_r8a77990,
+};
+
+static const struct renesas_sdhi_of_data_with_quirks of_rcar_gen3_compatible = {
+	.of_data = &of_data_rcar_gen3,
+};
+
 static const struct of_device_id renesas_sdhi_internal_dmac_of_match[] = {
 	{ .compatible = "renesas,sdhi-r7s9210", .data = &of_rza2_compatible, },
 	{ .compatible = "renesas,sdhi-mmc-r8a77470", .data = &of_rcar_gen3_compatible, },
-	{ .compatible = "renesas,sdhi-r8a7795", .data = &of_rcar_gen3_compatible, },
+	{ .compatible = "renesas,sdhi-r8a7795", .data = &of_r8a7795_compatible, },
 	{ .compatible = "renesas,sdhi-r8a7796", .data = &of_rcar_gen3_compatible, },
+	{ .compatible = "renesas,sdhi-r8a77961", .data = &of_r8a77961_compatible, },
+	{ .compatible = "renesas,sdhi-r8a77965", .data = &of_r8a77965_compatible, },
+	{ .compatible = "renesas,sdhi-r8a77980", .data = &of_r8a77980_compatible, },
+	{ .compatible = "renesas,sdhi-r8a77990", .data = &of_r8a77990_compatible, },
 	{ .compatible = "renesas,rcar-gen3-sdhi", .data = &of_rcar_gen3_compatible, },
 	{},
 };
@@ -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;
+
 	/* 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);
 }
 
 static const struct dev_pm_ops renesas_sdhi_internal_dmac_dev_pm_ops = {
diff --git a/drivers/mmc/host/renesas_sdhi_sys_dmac.c b/drivers/mmc/host/renesas_sdhi_sys_dmac.c
index ffa64211f4de..b3a152797517 100644
--- a/drivers/mmc/host/renesas_sdhi_sys_dmac.c
+++ b/drivers/mmc/host/renesas_sdhi_sys_dmac.c
@@ -451,7 +451,8 @@ static const struct tmio_mmc_dma_ops renesas_sdhi_sys_dmac_dma_ops = {
 
 static int renesas_sdhi_sys_dmac_probe(struct platform_device *pdev)
 {
-	return renesas_sdhi_probe(pdev, &renesas_sdhi_sys_dmac_dma_ops);
+	return renesas_sdhi_probe(pdev, &renesas_sdhi_sys_dmac_dma_ops,
+				  of_device_get_match_data(&pdev->dev), NULL);
 }
 
 static const struct dev_pm_ops renesas_sdhi_sys_dmac_dev_pm_ops = {
-- 
2.25.1

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

[Geert Uytterhoeven]

Hi Shimoda-san,

Thanks for your patch!

On Fri, Jul 2, 2021 at 1:31 PM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> 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@glider.be).

>
> 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@renesas.com>
> Reported-by: kernel test robot <lkp@intel.com> # 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@glider.be>

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

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

[Yoshihiro Shimoda]

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@renesas.com> 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@renesas.com>
> > Reported-by: kernel test robot <lkp@intel.com> # 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@glider.be>

Thank you for your review!

Best regards,
Yoshihiro Shimoda