| Message ID | 1445625991-5343-3-git-send-email-marex@denx.de |
|---|---|
| State | Accepted, archived |
| Delegated to: | Andreas Bießmann |
| Headers | show |
On 23.10.15 20:46, Marek Vasut wrote: > Instead of passing just the register area as a private data, introduce > a proper struct atmel_mci_priv structure instead. This will become useful > in the subsequent patch, where we eliminate the global variable from this > driver. > > Signed-off-by: Marek Vasut <marex@denx.de> Reveiwed-by: Andreas Bießmann <andreas.devel@googlemail.com> > --- > drivers/mmc/gen_atmel_mci.c | 34 ++++++++++++++++++++++------------ > 1 file changed, 22 insertions(+), 12 deletions(-) > > diff --git a/drivers/mmc/gen_atmel_mci.c b/drivers/mmc/gen_atmel_mci.c > index 8b05fcd..abc77cc 100644 > --- a/drivers/mmc/gen_atmel_mci.c > +++ b/drivers/mmc/gen_atmel_mci.c > @@ -32,6 +32,11 @@ > # define MCI_BUS 0 > #endif > > +struct atmel_mci_priv { > + struct mmc_config cfg; > + struct atmel_mci *mci; > +}; > + > static int initialized = 0; > > /* Read Atmel MCI IP version */ > @@ -55,7 +60,8 @@ static void dump_cmd(u32 cmdr, u32 arg, u32 status, const char* msg) > /* Setup for MCI Clock and Block Size */ > static void mci_set_mode(struct mmc *mmc, u32 hz, u32 blklen) > { > - atmel_mci_t *mci = mmc->priv; > + struct atmel_mci_priv *priv = mmc->priv; > + atmel_mci_t *mci = priv->mci; > u32 bus_hz = get_mci_clk_rate(); > u32 clkdiv = 255; > unsigned int version = atmel_mci_get_version(mci); > @@ -198,7 +204,8 @@ io_fail: > static int > mci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data) > { > - atmel_mci_t *mci = mmc->priv; > + struct atmel_mci_priv *priv = mmc->priv; > + atmel_mci_t *mci = priv->mci; > u32 cmdr; > u32 error_flags = 0; > u32 status; > @@ -323,7 +330,8 @@ mci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data) > /* Entered into mmc structure during driver init */ > static void mci_set_ios(struct mmc *mmc) > { > - atmel_mci_t *mci = mmc->priv; > + struct atmel_mci_priv *priv = mmc->priv; > + atmel_mci_t *mci = priv->mci; > int bus_width = mmc->bus_width; > unsigned int version = atmel_mci_get_version(mci); > int busw; > @@ -359,7 +367,8 @@ static void mci_set_ios(struct mmc *mmc) > /* Entered into mmc structure during driver init */ > static int mci_init(struct mmc *mmc) > { > - atmel_mci_t *mci = mmc->priv; > + struct atmel_mci_priv *priv = mmc->priv; > + atmel_mci_t *mci = priv->mci; > > /* Initialize controller */ > writel(MMCI_BIT(SWRST), &mci->cr); /* soft reset */ > @@ -393,22 +402,23 @@ int atmel_mci_init(void *regs) > { > struct mmc *mmc; > struct mmc_config *cfg; > - struct atmel_mci *mci; > + struct atmel_mci_priv *priv; > unsigned int version; > > - cfg = malloc(sizeof(*cfg)); > - if (cfg == NULL) > - return -1; > - memset(cfg, 0, sizeof(*cfg)); > + priv = calloc(1, sizeof(*priv)); > + if (!priv) > + return -ENOMEM; > > - mci = (struct atmel_mci *)regs; > + cfg = &priv->cfg; > > cfg->name = "mci"; > cfg->ops = &atmel_mci_ops; > > + priv->mci = (struct atmel_mci *)regs; > + > /* need to be able to pass these in on a board by board basis */ > cfg->voltages = MMC_VDD_32_33 | MMC_VDD_33_34; > - version = atmel_mci_get_version(mci); > + version = atmel_mci_get_version(priv->mci); > if ((version & 0xf00) >= 0x300) { > cfg->host_caps = MMC_MODE_8BIT; > cfg->host_caps |= MMC_MODE_HS | MMC_MODE_HS_52MHz; > @@ -425,7 +435,7 @@ int atmel_mci_init(void *regs) > > cfg->b_max = CONFIG_SYS_MMC_MAX_BLK_COUNT; > > - mmc = mmc_create(cfg, regs); > + mmc = mmc_create(cfg, priv); > > if (mmc == NULL) { > free(cfg); We shouldn't free cfg here but priv, the rest looks sane to me. Eventual return -ENODEV on !mmc and adopt the following comment, we may leak priv now as there is no de-init. Andreas >
On Saturday, October 24, 2015 at 12:59:14 AM, Andreas Bießmann wrote: > On 23.10.15 20:46, Marek Vasut wrote: > > Instead of passing just the register area as a private data, introduce > > a proper struct atmel_mci_priv structure instead. This will become useful > > in the subsequent patch, where we eliminate the global variable from this > > driver. > > > > Signed-off-by: Marek Vasut <marex@denx.de> > > Reveiwed-by: Andreas Bießmann <andreas.devel@googlemail.com> > > > --- > > > > drivers/mmc/gen_atmel_mci.c | 34 ++++++++++++++++++++++------------ > > 1 file changed, 22 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/mmc/gen_atmel_mci.c b/drivers/mmc/gen_atmel_mci.c > > index 8b05fcd..abc77cc 100644 > > --- a/drivers/mmc/gen_atmel_mci.c > > +++ b/drivers/mmc/gen_atmel_mci.c > > @@ -32,6 +32,11 @@ > > > > # define MCI_BUS 0 > > #endif > > > > +struct atmel_mci_priv { > > + struct mmc_config cfg; > > + struct atmel_mci *mci; > > +}; > > + > > > > static int initialized = 0; > > > > /* Read Atmel MCI IP version */ > > > > @@ -55,7 +60,8 @@ static void dump_cmd(u32 cmdr, u32 arg, u32 status, > > const char* msg) > > > > /* Setup for MCI Clock and Block Size */ > > static void mci_set_mode(struct mmc *mmc, u32 hz, u32 blklen) > > { > > > > - atmel_mci_t *mci = mmc->priv; > > + struct atmel_mci_priv *priv = mmc->priv; > > + atmel_mci_t *mci = priv->mci; > > > > u32 bus_hz = get_mci_clk_rate(); > > u32 clkdiv = 255; > > unsigned int version = atmel_mci_get_version(mci); > > > > @@ -198,7 +204,8 @@ io_fail: > > static int > > mci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data > > *data) { > > > > - atmel_mci_t *mci = mmc->priv; > > + struct atmel_mci_priv *priv = mmc->priv; > > + atmel_mci_t *mci = priv->mci; > > > > u32 cmdr; > > u32 error_flags = 0; > > u32 status; > > > > @@ -323,7 +330,8 @@ mci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, > > struct mmc_data *data) > > > > /* Entered into mmc structure during driver init */ > > static void mci_set_ios(struct mmc *mmc) > > { > > > > - atmel_mci_t *mci = mmc->priv; > > + struct atmel_mci_priv *priv = mmc->priv; > > + atmel_mci_t *mci = priv->mci; > > > > int bus_width = mmc->bus_width; > > unsigned int version = atmel_mci_get_version(mci); > > int busw; > > > > @@ -359,7 +367,8 @@ static void mci_set_ios(struct mmc *mmc) > > > > /* Entered into mmc structure during driver init */ > > static int mci_init(struct mmc *mmc) > > { > > > > - atmel_mci_t *mci = mmc->priv; > > + struct atmel_mci_priv *priv = mmc->priv; > > + atmel_mci_t *mci = priv->mci; > > > > /* Initialize controller */ > > writel(MMCI_BIT(SWRST), &mci->cr); /* soft reset */ > > > > @@ -393,22 +402,23 @@ int atmel_mci_init(void *regs) > > > > { > > > > struct mmc *mmc; > > struct mmc_config *cfg; > > > > - struct atmel_mci *mci; > > + struct atmel_mci_priv *priv; > > > > unsigned int version; > > > > - cfg = malloc(sizeof(*cfg)); > > - if (cfg == NULL) > > - return -1; > > - memset(cfg, 0, sizeof(*cfg)); > > + priv = calloc(1, sizeof(*priv)); > > + if (!priv) > > + return -ENOMEM; > > > > - mci = (struct atmel_mci *)regs; > > + cfg = &priv->cfg; > > > > cfg->name = "mci"; > > cfg->ops = &atmel_mci_ops; > > > > + priv->mci = (struct atmel_mci *)regs; > > + > > > > /* need to be able to pass these in on a board by board basis */ > > cfg->voltages = MMC_VDD_32_33 | MMC_VDD_33_34; > > > > - version = atmel_mci_get_version(mci); > > + version = atmel_mci_get_version(priv->mci); > > > > if ((version & 0xf00) >= 0x300) { > > > > cfg->host_caps = MMC_MODE_8BIT; > > cfg->host_caps |= MMC_MODE_HS | MMC_MODE_HS_52MHz; > > > > @@ -425,7 +435,7 @@ int atmel_mci_init(void *regs) > > > > cfg->b_max = CONFIG_SYS_MMC_MAX_BLK_COUNT; > > > > - mmc = mmc_create(cfg, regs); > > + mmc = mmc_create(cfg, priv); > > > > if (mmc == NULL) { > > > > free(cfg); > > We shouldn't free cfg here but priv, the rest looks sane to me. Eventual > return -ENODEV on !mmc and adopt the following comment, we may leak priv > now as there is no de-init. Uh right, can you fix that while applying the patchset or do you want me to repost ?
Hi Marek, On 24.10.15 01:29, Marek Vasut wrote: > On Saturday, October 24, 2015 at 12:59:14 AM, Andreas Bießmann wrote: >> On 23.10.15 20:46, Marek Vasut wrote: >>> Instead of passing just the register area as a private data, introduce >>> a proper struct atmel_mci_priv structure instead. This will become useful >>> in the subsequent patch, where we eliminate the global variable from this >>> driver. >>> >>> Signed-off-by: Marek Vasut <marex@denx.de> >> >> Reveiwed-by: Andreas Bießmann <andreas.devel@googlemail.com> >> >>> --- >>> >>> drivers/mmc/gen_atmel_mci.c | 34 ++++++++++++++++++++++------------ >>> 1 file changed, 22 insertions(+), 12 deletions(-) >>> >>> diff --git a/drivers/mmc/gen_atmel_mci.c b/drivers/mmc/gen_atmel_mci.c >>> index 8b05fcd..abc77cc 100644 >>> --- a/drivers/mmc/gen_atmel_mci.c >>> +++ b/drivers/mmc/gen_atmel_mci.c >>> @@ -425,7 +435,7 @@ int atmel_mci_init(void *regs) >>> >>> cfg->b_max = CONFIG_SYS_MMC_MAX_BLK_COUNT; >>> >>> - mmc = mmc_create(cfg, regs); >>> + mmc = mmc_create(cfg, priv); >>> >>> if (mmc == NULL) { >>> >>> free(cfg); >> >> We shouldn't free cfg here but priv, the rest looks sane to me. Eventual >> return -ENODEV on !mmc and adopt the following comment, we may leak priv >> now as there is no de-init. > > Uh right, can you fix that while applying the patchset or do you want me to > repost ? I'll fix it. Andreas
On Saturday, October 24, 2015 at 10:35:45 AM, Andreas Bießmann wrote: > Hi Marek, Hi! > On 24.10.15 01:29, Marek Vasut wrote: > > On Saturday, October 24, 2015 at 12:59:14 AM, Andreas Bießmann wrote: > >> On 23.10.15 20:46, Marek Vasut wrote: > >>> Instead of passing just the register area as a private data, introduce > >>> a proper struct atmel_mci_priv structure instead. This will become > >>> useful in the subsequent patch, where we eliminate the global variable > >>> from this driver. > >>> > >>> Signed-off-by: Marek Vasut <marex@denx.de> > >> > >> Reveiwed-by: Andreas Bießmann <andreas.devel@googlemail.com> > >> > >>> --- > >>> > >>> drivers/mmc/gen_atmel_mci.c | 34 ++++++++++++++++++++++------------ > >>> 1 file changed, 22 insertions(+), 12 deletions(-) > >>> > >>> diff --git a/drivers/mmc/gen_atmel_mci.c b/drivers/mmc/gen_atmel_mci.c > >>> index 8b05fcd..abc77cc 100644 > >>> --- a/drivers/mmc/gen_atmel_mci.c > >>> +++ b/drivers/mmc/gen_atmel_mci.c > >>> > >>> @@ -425,7 +435,7 @@ int atmel_mci_init(void *regs) > >>> > >>> cfg->b_max = CONFIG_SYS_MMC_MAX_BLK_COUNT; > >>> > >>> - mmc = mmc_create(cfg, regs); > >>> + mmc = mmc_create(cfg, priv); > >>> > >>> if (mmc == NULL) { > >>> > >>> free(cfg); > >> > >> We shouldn't free cfg here but priv, the rest looks sane to me. Eventual > >> return -ENODEV on !mmc and adopt the following comment, we may leak priv > >> now as there is no de-init. > > > > Uh right, can you fix that while applying the patchset or do you want me > > to repost ? > > I'll fix it. Thanks! Best regards, Marek Vasut
Dear Marek Vasut, Marek Vasut <marex@denx.de> writes: >Instead of passing just the register area as a private data, introduce >a proper struct atmel_mci_priv structure instead. This will become useful >in the subsequent patch, where we eliminate the global variable from this >driver. > >Signed-off-by: Marek Vasut <marex@denx.de> >Reviewed-by: Andreas Bießmann <andreas.devel@googlemail.com> >[fix free()] >Signed-off-by: Andreas Bießmann <andreas.devel@googlemail.com> >--- > drivers/mmc/gen_atmel_mci.c | 40 +++++++++++++++++++++++++--------------- > 1 file changed, 25 insertions(+), 15 deletions(-) applied to u-boot-atmel/master, thanks! Also fix the missed free(). Best regards, Andreas Bießmann
diff --git a/drivers/mmc/gen_atmel_mci.c b/drivers/mmc/gen_atmel_mci.c index 8b05fcd..abc77cc 100644 --- a/drivers/mmc/gen_atmel_mci.c +++ b/drivers/mmc/gen_atmel_mci.c @@ -32,6 +32,11 @@ # define MCI_BUS 0 #endif +struct atmel_mci_priv { + struct mmc_config cfg; + struct atmel_mci *mci; +}; + static int initialized = 0; /* Read Atmel MCI IP version */ @@ -55,7 +60,8 @@ static void dump_cmd(u32 cmdr, u32 arg, u32 status, const char* msg) /* Setup for MCI Clock and Block Size */ static void mci_set_mode(struct mmc *mmc, u32 hz, u32 blklen) { - atmel_mci_t *mci = mmc->priv; + struct atmel_mci_priv *priv = mmc->priv; + atmel_mci_t *mci = priv->mci; u32 bus_hz = get_mci_clk_rate(); u32 clkdiv = 255; unsigned int version = atmel_mci_get_version(mci); @@ -198,7 +204,8 @@ io_fail: static int mci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data) { - atmel_mci_t *mci = mmc->priv; + struct atmel_mci_priv *priv = mmc->priv; + atmel_mci_t *mci = priv->mci; u32 cmdr; u32 error_flags = 0; u32 status; @@ -323,7 +330,8 @@ mci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data) /* Entered into mmc structure during driver init */ static void mci_set_ios(struct mmc *mmc) { - atmel_mci_t *mci = mmc->priv; + struct atmel_mci_priv *priv = mmc->priv; + atmel_mci_t *mci = priv->mci; int bus_width = mmc->bus_width; unsigned int version = atmel_mci_get_version(mci); int busw; @@ -359,7 +367,8 @@ static void mci_set_ios(struct mmc *mmc) /* Entered into mmc structure during driver init */ static int mci_init(struct mmc *mmc) { - atmel_mci_t *mci = mmc->priv; + struct atmel_mci_priv *priv = mmc->priv; + atmel_mci_t *mci = priv->mci; /* Initialize controller */ writel(MMCI_BIT(SWRST), &mci->cr); /* soft reset */ @@ -393,22 +402,23 @@ int atmel_mci_init(void *regs) { struct mmc *mmc; struct mmc_config *cfg; - struct atmel_mci *mci; + struct atmel_mci_priv *priv; unsigned int version; - cfg = malloc(sizeof(*cfg)); - if (cfg == NULL) - return -1; - memset(cfg, 0, sizeof(*cfg)); + priv = calloc(1, sizeof(*priv)); + if (!priv) + return -ENOMEM; - mci = (struct atmel_mci *)regs; + cfg = &priv->cfg; cfg->name = "mci"; cfg->ops = &atmel_mci_ops; + priv->mci = (struct atmel_mci *)regs; + /* need to be able to pass these in on a board by board basis */ cfg->voltages = MMC_VDD_32_33 | MMC_VDD_33_34; - version = atmel_mci_get_version(mci); + version = atmel_mci_get_version(priv->mci); if ((version & 0xf00) >= 0x300) { cfg->host_caps = MMC_MODE_8BIT; cfg->host_caps |= MMC_MODE_HS | MMC_MODE_HS_52MHz; @@ -425,7 +435,7 @@ int atmel_mci_init(void *regs) cfg->b_max = CONFIG_SYS_MMC_MAX_BLK_COUNT; - mmc = mmc_create(cfg, regs); + mmc = mmc_create(cfg, priv); if (mmc == NULL) { free(cfg);
Instead of passing just the register area as a private data, introduce a proper struct atmel_mci_priv structure instead. This will become useful in the subsequent patch, where we eliminate the global variable from this driver. Signed-off-by: Marek Vasut <marex@denx.de> --- drivers/mmc/gen_atmel_mci.c | 34 ++++++++++++++++++++++------------ 1 file changed, 22 insertions(+), 12 deletions(-)