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

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

Hi Dragan,

On Monday, 26 August 2024 10:07:49 EDT Dragan Simic wrote:
> Hello Detlev,
> 
> On 2024-08-23 15:20, Detlev Casanova wrote:
> > 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.
> 
> Ah, you're right, and I somehow managed to ignore that.
> 
> I agree with your conclusions, although I'd suggest something like
> this, for slightly improved readability:
> 
>   +static int dw_mci_rk3576_parse_dt(struct dw_mci *host)
>   +{
>   +	struct dw_mci_rockchip_priv_data *priv = host->priv;
>   +	int err;
>   +
>   +	err = dw_mci_common_parse_dt(host);
>   +	if (err)
>   +		return err;
>   +
>   +	priv->internal_phase = true;
>   +
>   +	return 0;
>   +}

That won't work either, because host->priv is initialized in 
dw_mci_common_parse_dt.