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@xxxxxxxxxxxxx>
> ---
>
> 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;
+}
