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

Detlev Casanova
2024-08-23T13:19:41Z

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