[RFC 1/7] mmc: fsl_esdhc_imx: add OF_PLATDATA support

[Simon Glass]

Hi Walter,

On Tue, 7 Apr 2020 at 14:05, Walter Lozano <walter.lozano at collabora.com> wrote:
>
> Hi Simon,
>
> Thank you for taking the time to review this series.
>
> On 6/4/20 00:42, Simon Glass wrote:
> > Hi Walter,
> >
> > On Sun, 29 Mar 2020 at 21:32, Walter Lozano<walter.lozano at collabora.com>  wrote:
> >> Signed-off-by: Walter Lozano<walter.lozano at collabora.com>
> >> ---
> >>   drivers/mmc/fsl_esdhc_imx.c | 46 +++++++++++++++++++++++++++++++++----
> >>   1 file changed, 42 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c
> >> index 4900498e9b..761a4b46e9 100644
> >> --- a/drivers/mmc/fsl_esdhc_imx.c
> >> +++ b/drivers/mmc/fsl_esdhc_imx.c
> >> @@ -29,6 +29,8 @@
> >>   #include <dm.h>
> >>   #include <asm-generic/gpio.h>
> >>   #include <dm/pinctrl.h>
> >> +#include <dt-structs.h>
> >> +#include <mapmem.h>
> >>
> >>   #if !CONFIG_IS_ENABLED(BLK)
> >>   #include "mmc_private.h"
> >> @@ -98,6 +100,11 @@ struct fsl_esdhc {
> >>   };
> >>
> >>   struct fsl_esdhc_plat {
> >> +#if CONFIG_IS_ENABLED(OF_PLATDATA)
> >> +       /* Put this first since driver model will copy the data here */
> >> +       struct dtd_fsl_imx6q_usdhc dtplat;
> >> +#endif
> >> +
> >>          struct mmc_config cfg;
> >>          struct mmc mmc;
> >>   };
> >> @@ -1377,14 +1384,18 @@ static int fsl_esdhc_probe(struct udevice *dev)
> >>          struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev);
> >>          struct fsl_esdhc_plat *plat = dev_get_platdata(dev);
> >>          struct fsl_esdhc_priv *priv = dev_get_priv(dev);
> >> +#if !CONFIG_IS_ENABLED(OF_PLATDATA)
> >>          const void *fdt = gd->fdt_blob;
> >>          int node = dev_of_offset(dev);
> >> +       fdt_addr_t addr;
> >> +#else
> >> +       struct dtd_fsl_imx6q_usdhc *dtplat = &plat->dtplat;
> >> +#endif
> >>          struct esdhc_soc_data *data =
> >>                  (struct esdhc_soc_data *)dev_get_driver_data(dev);
> >>   #if CONFIG_IS_ENABLED(DM_REGULATOR)
> >>          struct udevice *vqmmc_dev;
> >>   #endif
> >> -       fdt_addr_t addr;
> >>          unsigned int val;
> >>          struct mmc *mmc;
> >>   #if !CONFIG_IS_ENABLED(BLK)
> >> @@ -1392,14 +1403,23 @@ static int fsl_esdhc_probe(struct udevice *dev)
> >>   #endif
> >>          int ret;
> >>
> >> +#if CONFIG_IS_ENABLED(OF_PLATDATA)
> >> +       priv->esdhc_regs = map_sysmem(dtplat->reg[0], dtplat->reg[1]);
> >> +       val = plat->dtplat.bus_width;
> >> +       if (val == 8)
> >> +               priv->bus_width = 8;
> >> +       else if (val == 4)
> >> +               priv->bus_width = 4;
> >> +       else
> >> +               priv->bus_width = 1;
> >> +       priv->non_removable = 1;
> >> +#else
> >>          addr = dev_read_addr(dev);
> >>          if (addr == FDT_ADDR_T_NONE)
> >>                  return -EINVAL;
> >>          priv->esdhc_regs = (struct fsl_esdhc *)addr;
> >>          priv->dev = dev;
> >>          priv->mode = -1;
> >> -       if (data)
> >> -               priv->flags = data->flags;
> >>
> >>          val = dev_read_u32_default(dev, "bus-width", -1);
> >>          if (val == 8)
> >> @@ -1462,7 +1482,9 @@ static int fsl_esdhc_probe(struct udevice *dev)
> >>                          priv->vs18_enable = 1;
> >>          }
> >>   #endif
> >> -
> >> +#endif
> >> +       if (data)
> >> +               priv->flags = data->flags;
> >>          /*
> >>           * TODO:
> >>           * Because lack of clk driver, if SDHC clk is not enabled,
> >> @@ -1513,9 +1535,11 @@ static int fsl_esdhc_probe(struct udevice *dev)
> >>                  return ret;
> >>          }
> >>
> >> +#if !CONFIG_IS_ENABLED(OF_PLATDATA)
> > Maybe can use if() for this one?
>
> Thank you for the suggestion
>
> >>          ret = mmc_of_parse(dev, &plat->cfg);
> >>          if (ret)
> >>                  return ret;
> >> +#endif
> >>
> >>          mmc = &plat->mmc;
> >>          mmc->cfg = &plat->cfg;
> >> @@ -1648,4 +1672,18 @@ U_BOOT_DRIVER(fsl_esdhc) = {
> >>          .platdata_auto_alloc_size = sizeof(struct fsl_esdhc_plat),
> >>          .priv_auto_alloc_size = sizeof(struct fsl_esdhc_priv),
> >>   };
> >> +
> >> +#if CONFIG_IS_ENABLED(OF_PLATDATA)
> > Don't you already have a U_BOOT_DRIVER declaration?
> >
> > You may need to change the name of your existing driver though (see
> > of-platdata docs).
> >
> > So if it is because of that, please add a comment.
>
> I have my doubts regarding this issue. As I see, this driver is used by
> many different DT with different compatible strings, so I'm thinking in
> trying to find a more generic approach. Would it be useful to have a
> more elaborated solution searching for the compatible string when
> matching drivers with device?

Yes I think so.

Actually searching for a string is not great anyway. I wonder if we
can use the linker-list idea in the previous email somehow?

Regards,
SImon