| Message ID | 1414839138-6587-2-git-send-email-contact@paulk.fr |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
Hi Paul, On 11/01/14 12:52, Paul Kocialkowski wrote: > Some devices may use non-standard combinations of regulators to power MMC: > this allows these devices to provide a board-specific MMC power init function > to set everything up in their own way. > > Signed-off-by: Paul Kocialkowski <contact@paulk.fr> > --- > drivers/mmc/mmc.c | 8 ++++++++ > include/mmc.h | 1 + > 2 files changed, 9 insertions(+) > > diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c > index 44a4feb..125f347 100644 > --- a/drivers/mmc/mmc.c > +++ b/drivers/mmc/mmc.c > @@ -1277,6 +1277,12 @@ block_dev_desc_t *mmc_get_dev(int dev) > } > #endif > > +/* board-specific MMC power initializations. */ > +__weak int board_mmc_power_init(void) > +{ > + return -1; > +} > + > int mmc_start_init(struct mmc *mmc) > { > int err; > @@ -1293,6 +1299,8 @@ int mmc_start_init(struct mmc *mmc) > if (mmc->has_init) > return 0; > > + board_mmc_power_init(); > + Shouldn't we have some error handling here? > /* made sure it's not NULL earlier */ > err = mmc->cfg->ops->init(mmc); > > diff --git a/include/mmc.h b/include/mmc.h > index d74a190..aaea644 100644 > --- a/include/mmc.h > +++ b/include/mmc.h > @@ -385,6 +385,7 @@ struct mmc *mmc_spi_init(uint bus, uint cs, uint speed, uint mode); > int mmc_legacy_init(int verbose); > #endif > > +int board_mmc_power_init(void); > int board_mmc_init(bd_t *bis); > int cpu_mmc_init(bd_t *bis); > int mmc_get_env_addr(struct mmc *mmc, int copy, u32 *env_addr); >
Hi Igor, > On 11/01/14 12:52, Paul Kocialkowski wrote: > > Some devices may use non-standard combinations of regulators to power MMC: > > this allows these devices to provide a board-specific MMC power init function > > to set everything up in their own way. > > > > Signed-off-by: Paul Kocialkowski <contact@paulk.fr> > > --- > > drivers/mmc/mmc.c | 8 ++++++++ > > include/mmc.h | 1 + > > 2 files changed, 9 insertions(+) > > > > diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c > > index 44a4feb..125f347 100644 > > --- a/drivers/mmc/mmc.c > > +++ b/drivers/mmc/mmc.c > > @@ -1277,6 +1277,12 @@ block_dev_desc_t *mmc_get_dev(int dev) > > } > > #endif > > > > +/* board-specific MMC power initializations. */ > > +__weak int board_mmc_power_init(void) > > +{ > > + return -1; > > +} > > + > > int mmc_start_init(struct mmc *mmc) > > { > > int err; > > @@ -1293,6 +1299,8 @@ int mmc_start_init(struct mmc *mmc) > > if (mmc->has_init) > > return 0; > > > > + board_mmc_power_init(); > > + > > Shouldn't we have some error handling here? I noticed how weak implementations tend to return -1 and not have their return code checked so much (see the other two such functions in mmc.c). I would be fine with checking the return code and returning 0 in the weak implementation. If you think that's better, I'll make a new version with that. > > /* made sure it's not NULL earlier */ > > err = mmc->cfg->ops->init(mmc); > > > > diff --git a/include/mmc.h b/include/mmc.h > > index d74a190..aaea644 100644 > > --- a/include/mmc.h > > +++ b/include/mmc.h > > @@ -385,6 +385,7 @@ struct mmc *mmc_spi_init(uint bus, uint cs, uint speed, uint mode); > > int mmc_legacy_init(int verbose); > > #endif > > > > +int board_mmc_power_init(void); > > int board_mmc_init(bd_t *bis); > > int cpu_mmc_init(bd_t *bis); > > int mmc_get_env_addr(struct mmc *mmc, int copy, u32 *env_addr); > > >
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Hi Paul, On 11/02/14 20:51, Paul Kocialkowski wrote: > Hi Igor, > >> On 11/01/14 12:52, Paul Kocialkowski wrote: >>> Some devices may use non-standard combinations of regulators to power MMC: >>> this allows these devices to provide a board-specific MMC power init function >>> to set everything up in their own way. >>> >>> Signed-off-by: Paul Kocialkowski <contact@paulk.fr> >>> --- >>> drivers/mmc/mmc.c | 8 ++++++++ >>> include/mmc.h | 1 + >>> 2 files changed, 9 insertions(+) >>> >>> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c >>> index 44a4feb..125f347 100644 >>> --- a/drivers/mmc/mmc.c >>> +++ b/drivers/mmc/mmc.c >>> @@ -1277,6 +1277,12 @@ block_dev_desc_t *mmc_get_dev(int dev) >>> } >>> #endif >>> >>> +/* board-specific MMC power initializations. */ >>> +__weak int board_mmc_power_init(void) >>> +{ >>> + return -1; >>> +} >>> + >>> int mmc_start_init(struct mmc *mmc) >>> { >>> int err; >>> @@ -1293,6 +1299,8 @@ int mmc_start_init(struct mmc *mmc) >>> if (mmc->has_init) >>> return 0; >>> >>> + board_mmc_power_init(); >>> + >> >> Shouldn't we have some error handling here? > > I noticed how weak implementations tend to return -1 and not have their > return code checked so much (see the other two such functions in mmc.c). > I would be fine with checking the return code and returning 0 in the > weak implementation. > > If you think that's better, I'll make a new version with that. Well, otherwise it does not make sense to return int, does it? IMO it is a good practice to check the return value, and may be emit a warning or something (I'm not really sure if it should abort the init sequence). Since I can't propose a good handling now, may be we should leave it for now and see if someone comes up with a good case for it. I'm fine with your decision. > >>> /* made sure it's not NULL earlier */ >>> err = mmc->cfg->ops->init(mmc); >>> >>> diff --git a/include/mmc.h b/include/mmc.h >>> index d74a190..aaea644 100644 >>> --- a/include/mmc.h >>> +++ b/include/mmc.h >>> @@ -385,6 +385,7 @@ struct mmc *mmc_spi_init(uint bus, uint cs, uint speed, uint mode); >>> int mmc_legacy_init(int verbose); >>> #endif >>> >>> +int board_mmc_power_init(void); >>> int board_mmc_init(bd_t *bis); >>> int cpu_mmc_init(bd_t *bis); >>> int mmc_get_env_addr(struct mmc *mmc, int copy, u32 *env_addr); >>> >> > - -- Regards, Igor. -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJUVzBHAAoJEBDE8YO64EfayhsP/jV2YAR6jH6gZJBNn/wGnHId a8U0YGG+f30RBPCdfCiraSRpL1C876wsf+InNDAHgo4HSaRdRwF4tUqWj78SpH9F QTo3qcA6S8S4GIRJ2+q+3BmtlEuYPWOZOi41erhaUF3kMLw6cMt+gL0Ggx4OXQAo 2bhSeQaaGynoFJwC0dIf0QhDjRkPCHbNh2IgROfRye0a7/0pF4tN/tvjLSxw8w54 3457N4TYreGEmMMrtNi/psekMYTjR6JFve3CwiMvOXiwOJGz/d6qj7vhw4Ba7q5i LE385Zj24Bi/+m7ls7hp5I522O0tkShGgPI3XvCiT4xrjXDBihsWG9RGsLSc5vBY EimiztfLVu3alc1cxclK9tq0p+FwyUNwXsUED+oR6t56/qr/BSWiSIDAMfN1zcfI paK9zXDoGvhg6T0pK+SsRvAE4D7vg+XT8WSU52l6/h8TRKB6r3cupSMXAyjmUpLl 1pEozVsg4qW6rJawQOiCjl0YU3ixeVwk0vmWWh+gBOMMLsuikQpFI5s6zUXteqnl IHFS6oiYW/SVppHrd95624suZQPTsrH6+4CvmBwmtBGbmErchsgfj8qjP/ilpn1a Y6Kv0mj2ndkgdFAc7zIEpwzF9yPsewVg8nc+vYLezyT7ePeTNYLpPWgQTL6qEdHH lcK7qYNEaYWIUMkbQM5P =7kRq -----END PGP SIGNATURE-----
diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index 44a4feb..125f347 100644 --- a/drivers/mmc/mmc.c +++ b/drivers/mmc/mmc.c @@ -1277,6 +1277,12 @@ block_dev_desc_t *mmc_get_dev(int dev) } #endif +/* board-specific MMC power initializations. */ +__weak int board_mmc_power_init(void) +{ + return -1; +} + int mmc_start_init(struct mmc *mmc) { int err; @@ -1293,6 +1299,8 @@ int mmc_start_init(struct mmc *mmc) if (mmc->has_init) return 0; + board_mmc_power_init(); + /* made sure it's not NULL earlier */ err = mmc->cfg->ops->init(mmc); diff --git a/include/mmc.h b/include/mmc.h index d74a190..aaea644 100644 --- a/include/mmc.h +++ b/include/mmc.h @@ -385,6 +385,7 @@ struct mmc *mmc_spi_init(uint bus, uint cs, uint speed, uint mode); int mmc_legacy_init(int verbose); #endif +int board_mmc_power_init(void); int board_mmc_init(bd_t *bis); int cpu_mmc_init(bd_t *bis); int mmc_get_env_addr(struct mmc *mmc, int copy, u32 *env_addr);
Some devices may use non-standard combinations of regulators to power MMC: this allows these devices to provide a board-specific MMC power init function to set everything up in their own way. Signed-off-by: Paul Kocialkowski <contact@paulk.fr> --- drivers/mmc/mmc.c | 8 ++++++++ include/mmc.h | 1 + 2 files changed, 9 insertions(+)