Re: [PATCH v2 1/1] mmc: sdhci-of-dwcmshc: add callback functions for dwcmshc_priv

[Adrian Hunter <adrian.hunter@intel.com>]

On 9/05/24 05:17, Chen Wang wrote:
> 
> 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?

In sdhci-of-arasan.c, ops are copied from platform data to
driver private data e.g.

static int sdhci_arasan_probe(struct platform_device *pdev)
{
	...
	struct sdhci_arasan_data *sdhci_arasan;
	const struct sdhci_arasan_of_data *data;

	data = of_device_get_match_data(dev);
	if (!data)
		return -EINVAL;
	...
	sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
	...
	sdhci_arasan->clk_ops = data->clk_ops;


Alternatively, a pointer could be put in driver private data
to point to platform data.