Messages in this thread | | | Date | Tue, 30 Apr 2024 08:41:04 +0800 | Subject | Re: [PATCH v2 1/1] mmc: sdhci-of-dwcmshc: add callback functions for dwcmshc_priv | From | Chen Wang <> |
| |
On 2024/4/29 15:08, Adrian Hunter wrote: > On 28/04/24 05:32, Chen Wang wrote: >> From: Chen Wang <unicorn_wang@outlook.com> >> >> 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@outlook.com> >> --- >> 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.
[......]
| |