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

[Dragan Simic <dsimic@manjaro.org>]

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
> @@ -410,7 +410,7 @@ static int dw_mci_rk3288_execute_tuning(struct
> dw_mci_slot *slot, u32 opcode)
>  	return ret;
>  }
> 
> -static int dw_mci_rk3288_parse_dt(struct dw_mci *host)
> +static int dw_mci_common_parse_dt(struct dw_mci *host)
>  {
>  	struct device_node *np = host->dev->of_node;
>  	struct dw_mci_rockchip_priv_data *priv;
> @@ -420,13 +420,29 @@ static int dw_mci_rk3288_parse_dt(struct dw_mci 
> *host)
>  		return -ENOMEM;
> 
>  	if (of_property_read_u32(np, "rockchip,desired-num-phases",
> -					&priv->num_phases))
> +				 &priv->num_phases))
>  		priv->num_phases = 360;
> 
>  	if (of_property_read_u32(np, "rockchip,default-sample-phase",
> -					&priv->default_sample_phase))
> +				 &priv->default_sample_phase))
>  		priv->default_sample_phase = 0;
> 
> +	host->priv = priv;
> +
> +	return 0;
> +}
> +
> +static int dw_mci_rk3288_parse_dt(struct dw_mci *host)
> +{
> +	struct dw_mci_rockchip_priv_data *priv;
> +	int err;
> +
> +	err = dw_mci_common_parse_dt(host);
> +	if (err)
> +		return err;
> +
> +	priv = host->priv;
> +
>  	priv->drv_clk = devm_clk_get(host->dev, "ciu-drive");
>  	if (IS_ERR(priv->drv_clk))
>  		dev_dbg(host->dev, "ciu-drive not available\n");
> @@ -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.

> +
> +	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);