From: Chen Wang <unicorn_wang@outlook.com>
To: Adrian Hunter <adrian.hunter@intel.com>,
Chen Wang <unicornxw@gmail.com>,
ulf.hansson@linaro.org, linux-mmc@vger.kernel.org,
linux-kernel@vger.kernel.org, jszhang@kernel.org,
dfustini@baylibre.com, yifeng.zhao@rock-chips.com,
shawn.lin@rock-chips.com, chao.wei@sophgo.com,
haijiao.liu@sophgo.com, xiaoguang.xing@sophgo.com,
tingzhu.wang@sophgo.com, guoren@kernel.org,
inochiama@outlook.com, unicorn_wang@outlook.com
Subject: Re: [PATCH v2 1/1] mmc: sdhci-of-dwcmshc: add callback functions for dwcmshc_priv
Date: Thu, 9 May 2024 10:17:50 +0800 [thread overview]
Message-ID: <MA0P287MB282273829FCBA4BE58BD9CC2FEE62@MA0P287MB2822.INDP287.PROD.OUTLOOK.COM> (raw)
In-Reply-To: <ed900af1-f090-49a9-bc7e-363a28a4ac2b@intel.com>
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;
> }
hi, Adrian,
I thought about it for a while, and I would like to continue discussing
this issue as follows.
I feel like it would be simpler to put it at the dwcmshc_priv level
based on the ops involved in the code so far. Judging from the SOCs
currently supported by dwcmshc, the ops I abstracted only operate data
below the dwcmshc_priv level, and these ops are not used by most SOCs.
- postinit: only required by rk35xx
- init: involves rk35xx and th1520, and the new soc(sg2042) I want to add.
- clks_enable/clks_disable: only rk35xx and the sg2042 I want to add
In particular, for dwcmshc_suspend/dwcmshc_resume, we have already
obtained dwcmshc_priv. If ops is to be placed at the platformdata level,
we have to use device_get_match_data to obtain data again, which feels a
bit unnecessary.
What do you think?
Thanks,
Chen
[......]
next prev parent reply other threads:[~2024-05-09 2:18 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-28 2:32 [PATCH v2 0/1] mmc: sdhci-of-dwcmshc: enhance framework Chen Wang
2024-04-28 2:32 ` [PATCH v2 1/1] mmc: sdhci-of-dwcmshc: add callback functions for dwcmshc_priv Chen Wang
2024-04-29 1:40 ` Drew Fustini
2024-04-29 8:31 ` Chen Wang
2024-04-29 7:08 ` Adrian Hunter
2024-04-30 0:41 ` Chen Wang
2024-05-09 2:17 ` Chen Wang [this message]
2024-05-09 8:21 ` Adrian Hunter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=MA0P287MB282273829FCBA4BE58BD9CC2FEE62@MA0P287MB2822.INDP287.PROD.OUTLOOK.COM \
--to=unicorn_wang@outlook.com \
--cc=adrian.hunter@intel.com \
--cc=chao.wei@sophgo.com \
--cc=dfustini@baylibre.com \
--cc=guoren@kernel.org \
--cc=haijiao.liu@sophgo.com \
--cc=inochiama@outlook.com \
--cc=jszhang@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mmc@vger.kernel.org \
--cc=shawn.lin@rock-chips.com \
--cc=tingzhu.wang@sophgo.com \
--cc=ulf.hansson@linaro.org \
--cc=unicornxw@gmail.com \
--cc=xiaoguang.xing@sophgo.com \
--cc=yifeng.zhao@rock-chips.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.