lkml.org 
[lkml]   [2024]   [Apr]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v2 1/1] mmc: sdhci-of-dwcmshc: add callback functions for dwcmshc_priv
From

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.

[......]



\
 
 \ /
  Last update: 2024-05-27 18:08    [W:0.141 / U:0.044 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site