On 2024/4/29 15:08, Adrian Hunter wrote:
On 28/04/24 05:32, Chen Wang wrote:
From: Chen Wang <unicorn_wang@xxxxxxxxxxx>
The current framework is not easily extended to support new SOCs.
For example, in the current code we see that the SOC-level
structure `rk35xx_priv` and related logic are distributed in
functions such as dwcmshc_probe/dwcmshc_remove/dwcmshc_suspend/......,
which is inappropriate.
The solution is to abstract some possible common operations of soc
into virtual members of `dwcmshc_priv`. Each soc implements its own
corresponding callback function and registers it in init function.
dwcmshc framework is responsible for calling these callback functions
in those dwcmshc_xxx functions.
Signed-off-by: Chen Wang <unicorn_wang@xxxxxxxxxxx>
---
drivers/mmc/host/sdhci-of-dwcmshc.c | 152 +++++++++++++++++-----------
1 file changed, 91 insertions(+), 61 deletions(-)
diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
index 39edf04fedcf..525f954bcb65 100644
--- a/drivers/mmc/host/sdhci-of-dwcmshc.c
+++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
@@ -214,6 +214,10 @@ struct dwcmshc_priv {
void *priv; /* pointer to SoC private stuff */
u16 delay_line;
u16 flags;
+
+ void (*soc_postinit)(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv);
+ int (*soc_clks_enable)(struct dwcmshc_priv *dwc_priv);
+ void (*soc_clks_disable)(struct dwcmshc_priv *dwc_priv);
Normally the ops would be part of platform data. For example,
sdhci-of-arasan.c has:
struct sdhci_arasan_of_data {
const struct sdhci_arasan_soc_ctl_map *soc_ctl_map;
const struct sdhci_pltfm_data *pdata;
const struct sdhci_arasan_clk_ops *clk_ops;
};
And then:
static struct sdhci_arasan_of_data sdhci_arasan_rk3399_data = {
.soc_ctl_map = &rk3399_soc_ctl_map,
.pdata = &sdhci_arasan_cqe_pdata,
.clk_ops = &arasan_clk_ops,
};
etc
static const struct of_device_id sdhci_arasan_of_match[] = {
/* SoC-specific compatible strings w/ soc_ctl_map */
{
.compatible = "rockchip,rk3399-sdhci-5.1",
.data = &sdhci_arasan_rk3399_data,
},
etc
So, say:
struct dwcmshc_pltfm_data {
const struct sdhci_pltfm_data *pltfm_data;
void (*postinit)(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv);
int (*clks_enable)(struct dwcmshc_priv *dwc_priv);
void (*clks_disable)(struct dwcmshc_priv *dwc_priv);
}
Or if the ops are mostly the same, it might be more convenient to
have them in their own structure:
struct dwcmshc_pltfm_data {
const struct sdhci_pltfm_data *pltfm_data;
const struct dwcmshc_ops *ops;
}
Thanks for your suggestion and it looks more formal, I will investigate
and provide a new revision.
};
/*
@@ -1033,10 +1037,40 @@ static void dwcmshc_cqhci_init(struct sdhci_host *host, struct platform_device *
host->mmc->caps2 &= ~(MMC_CAP2_CQE | MMC_CAP2_CQE_DCMD);
}
-static int dwcmshc_rk35xx_init(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv)
+static int dwcmshc_rk35xx_clks_enable(struct dwcmshc_priv *dwc_priv)
{
- int err;
struct rk35xx_priv *priv = dwc_priv->priv;
+ int ret = 0;
+
+ if (priv)
+ ret = clk_bulk_prepare_enable(RK35xx_MAX_CLKS, priv->rockchip_clks);
+ return ret;
+}
+
+static void dwcmshc_rk35xx_clks_disable(struct dwcmshc_priv *dwc_priv)
+{
+ struct rk35xx_priv *priv = dwc_priv->priv;
+
+ if (priv)
+ clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
+ priv->rockchip_clks);
+}
+
+static void dwcmshc_rk35xx_postinit(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv);
Avoid forward declarations if possible. If necessary, it is
preferable to move the function definition.
Yes, I add this declaration just want to make diff look clearer :).
Anyway, move this postinit to the front should be better.
+static int dwcmshc_rk35xx_init(struct device *dev,
+ struct sdhci_host *host, struct dwcmshc_priv *dwc_priv)
This patch looks like it might be doing too much. Please consider
splitting it so reorganising the code is separate from adding the
callbacks.
Sure, will do like this. Thanks.
[......]
