Re: [PATCH v4 4/4] mmc: dw_mmc-rockchip: Add support for rk3576 SoCs

[Detlev Casanova <detlev.casanova@collabora.com>]

Hi Dragan,

On Friday, 23 August 2024 03:00:57 EDT Dragan Simic wrote:
> Hello Detlev,
> 
> Please see a comment below.
> 
> On 2024-08-22 23:15, Detlev Casanova wrote:
> > On rk3576 the tunable clocks are inside the controller itself, removing
> > the need for the "ciu-drive" and "ciu-sample" clocks.
> > 
> > That makes it a new type of controller that has its own dt_parse
> > function.
> > 
> > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> > ---
> > 
> >  drivers/mmc/host/dw_mmc-rockchip.c | 48 ++++++++++++++++++++++++++----
> >  1 file changed, 43 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/mmc/host/dw_mmc-rockchip.c
> > b/drivers/mmc/host/dw_mmc-rockchip.c
> > index 1458cb5fd5c7..7c8ccf5e71bc 100644
> > --- a/drivers/mmc/host/dw_mmc-rockchip.c
> > +++ b/drivers/mmc/host/dw_mmc-rockchip.c
[...]
> > @@ -435,13 +451,25 @@ static int dw_mci_rk3288_parse_dt(struct dw_mci
> > *host)
> > 
> >  	if (IS_ERR(priv->sample_clk))
> >  	
> >  		dev_dbg(host->dev, "ciu-sample not available\n");
> > 
> > -	host->priv = priv;
> > -
> > 
> >  	priv->internal_phase = false;
> >  	
> >  	return 0;
> >  
> >  }
> > 
> > +static int dw_mci_rk3576_parse_dt(struct dw_mci *host)
> > +{
> > +	struct dw_mci_rockchip_priv_data *priv;
> > +	int err = dw_mci_common_parse_dt(host);
> > +	if (err)
> > +		return err;
> > +
> > +	priv = host->priv;
> > +
> > +	priv->internal_phase = true;
> 
> Defining priv, assigning it and using it seems rather redundant,
> when all that's needed is simple "host->priv->internal_phase = true"
> assignment instead.

Yes, that's what I did at first, but host->priv is declared as void*, which 
means it needs to be cast to struct dw_mci_rockchip_priv_data * and I felt 
that 

((struct dw_mci_rockchip_priv_data *)host->priv)->internal_phase = true;

is not very pretty and harder to read.

> > +
> > +	return 0;
> > +}
> > +
> > 
> >  static int dw_mci_rockchip_init(struct dw_mci *host)
> >  {
> >  
> >  	int ret, i;
> > 
> > @@ -483,11 +511,21 @@ static const struct dw_mci_drv_data
> > rk3288_drv_data = {
> > 
> >  	.init			= dw_mci_rockchip_init,
> >  
> >  };
> > 
> > +static const struct dw_mci_drv_data rk3576_drv_data = {
> > +	.common_caps		= MMC_CAP_CMD23,
> > +	.set_ios		= dw_mci_rk3288_set_ios,
> > +	.execute_tuning		= dw_mci_rk3288_execute_tuning,
> > +	.parse_dt		= dw_mci_rk3576_parse_dt,
> > +	.init			= dw_mci_rockchip_init,
> > +};
> > +
> > 
> >  static const struct of_device_id dw_mci_rockchip_match[] = {
> >  
> >  	{ .compatible = "rockchip,rk2928-dw-mshc",
> >  	
> >  		.data = &rk2928_drv_data },
> >  	
> >  	{ .compatible = "rockchip,rk3288-dw-mshc",
> >  	
> >  		.data = &rk3288_drv_data },
> > 
> > +	{ .compatible = "rockchip,rk3576-dw-mshc",
> > +		.data = &rk3576_drv_data },
> > 
> >  	{},
> >  
> >  };
> >  MODULE_DEVICE_TABLE(of, dw_mci_rockchip_match);