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

[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@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;
> }

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

[......]