* [PATCH 0/2] mmc: add helpers mmc_regulator_set_ocr_vmmc_up/off @ 2023-02-15 20:08 Heiner Kallweit 2023-02-15 20:11 ` [PATCH 1/2] mmc: core: " Heiner Kallweit 2023-02-15 20:13 ` [PATCH 2/2] mmc: meson-gx: use new helpers mmc_regulator_set_ocr_vmmc_off/up Heiner Kallweit 0 siblings, 2 replies; 7+ messages in thread From: Heiner Kallweit @ 2023-02-15 20:08 UTC (permalink / raw) To: Ulf Hansson, Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl Cc: linux-arm-kernel@lists.infradead.org, open list:ARM/Amlogic Meson..., linux-mmc@vger.kernel.org A lot of drivers use this code, therefore let's factor it out to helpers. Heiner Kallweit (2): mmc: core: add helpers mmc_regulator_set_ocr_vmmc_up/off mmc: meson-gx: use new helpers mmc_regulator_set_ocr_vmmc_off/up drivers/mmc/host/meson-gx-mmc.c | 6 ++---- include/linux/mmc/host.h | 17 +++++++++++++++++ 2 files changed, 19 insertions(+), 4 deletions(-) -- 2.39.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] mmc: core: add helpers mmc_regulator_set_ocr_vmmc_up/off 2023-02-15 20:08 [PATCH 0/2] mmc: add helpers mmc_regulator_set_ocr_vmmc_up/off Heiner Kallweit @ 2023-02-15 20:11 ` Heiner Kallweit 2023-02-17 10:47 ` Ulf Hansson 2023-02-15 20:13 ` [PATCH 2/2] mmc: meson-gx: use new helpers mmc_regulator_set_ocr_vmmc_off/up Heiner Kallweit 1 sibling, 1 reply; 7+ messages in thread From: Heiner Kallweit @ 2023-02-15 20:11 UTC (permalink / raw) To: Ulf Hansson, Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl Cc: linux-arm-kernel@lists.infradead.org, open list:ARM/Amlogic Meson..., linux-mmc@vger.kernel.org A lot of drivers use this code, therefore let's factor it out to helpers. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- include/linux/mmc/host.h | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h index 812e6b583..f93fb8c7d 100644 --- a/include/linux/mmc/host.h +++ b/include/linux/mmc/host.h @@ -597,6 +597,23 @@ static inline int mmc_regulator_set_vqmmc(struct mmc_host *mmc, } #endif +static inline int mmc_regulator_set_ocr_vmmc_up(struct mmc_host *mmc, + struct mmc_ios *ios) +{ + if (IS_ERR(mmc->supply.vmmc)) + return 0; + + return mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, ios->vdd); +} + +static inline int mmc_regulator_set_ocr_vmmc_off(struct mmc_host *mmc) +{ + if (IS_ERR(mmc->supply.vmmc)) + return 0; + + return mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, 0); +} + int mmc_regulator_get_supply(struct mmc_host *mmc); static inline int mmc_card_is_removable(struct mmc_host *host) -- 2.39.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] mmc: core: add helpers mmc_regulator_set_ocr_vmmc_up/off 2023-02-15 20:11 ` [PATCH 1/2] mmc: core: " Heiner Kallweit @ 2023-02-17 10:47 ` Ulf Hansson 2023-02-17 20:09 ` Heiner Kallweit 0 siblings, 1 reply; 7+ messages in thread From: Ulf Hansson @ 2023-02-17 10:47 UTC (permalink / raw) To: Heiner Kallweit Cc: Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl, linux-arm-kernel@lists.infradead.org, open list:ARM/Amlogic Meson..., linux-mmc@vger.kernel.org On Wed, 15 Feb 2023 at 21:14, Heiner Kallweit <hkallweit1@gmail.com> wrote: > > A lot of drivers use this code, therefore let's factor it out to > helpers. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > include/linux/mmc/host.h | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h > index 812e6b583..f93fb8c7d 100644 > --- a/include/linux/mmc/host.h > +++ b/include/linux/mmc/host.h > @@ -597,6 +597,23 @@ static inline int mmc_regulator_set_vqmmc(struct mmc_host *mmc, > } > #endif > > +static inline int mmc_regulator_set_ocr_vmmc_up(struct mmc_host *mmc, > + struct mmc_ios *ios) > +{ > + if (IS_ERR(mmc->supply.vmmc)) > + return 0; Rather than adding these two new helper functions, how about adding the similar check in mmc_regulator_set_ocr() instead? That should allow us to simplify some code in the host drivers too, right? > + > + return mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, ios->vdd); > +} > + > +static inline int mmc_regulator_set_ocr_vmmc_off(struct mmc_host *mmc) > +{ > + if (IS_ERR(mmc->supply.vmmc)) > + return 0; > + > + return mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, 0); > +} > + > int mmc_regulator_get_supply(struct mmc_host *mmc); > > static inline int mmc_card_is_removable(struct mmc_host *host) > -- > 2.39.1 > > Kind regards Uffe _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] mmc: core: add helpers mmc_regulator_set_ocr_vmmc_up/off 2023-02-17 10:47 ` Ulf Hansson @ 2023-02-17 20:09 ` Heiner Kallweit 2023-02-27 16:13 ` Ulf Hansson 0 siblings, 1 reply; 7+ messages in thread From: Heiner Kallweit @ 2023-02-17 20:09 UTC (permalink / raw) To: Ulf Hansson Cc: Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl, linux-arm-kernel@lists.infradead.org, open list:ARM/Amlogic Meson..., linux-mmc@vger.kernel.org On 17.02.2023 11:47, Ulf Hansson wrote: > On Wed, 15 Feb 2023 at 21:14, Heiner Kallweit <hkallweit1@gmail.com> wrote: >> >> A lot of drivers use this code, therefore let's factor it out to >> helpers. >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> include/linux/mmc/host.h | 17 +++++++++++++++++ >> 1 file changed, 17 insertions(+) >> >> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h >> index 812e6b583..f93fb8c7d 100644 >> --- a/include/linux/mmc/host.h >> +++ b/include/linux/mmc/host.h >> @@ -597,6 +597,23 @@ static inline int mmc_regulator_set_vqmmc(struct mmc_host *mmc, >> } >> #endif >> >> +static inline int mmc_regulator_set_ocr_vmmc_up(struct mmc_host *mmc, >> + struct mmc_ios *ios) >> +{ >> + if (IS_ERR(mmc->supply.vmmc)) >> + return 0; > > Rather than adding these two new helper functions, how about adding > the similar check in mmc_regulator_set_ocr() instead? > There's a number of drivers having 3 paths here: 1. IS_ERR() is true -> do nothing and go one 2. mmc_regulator_set_ocr() returns 0 -> some action and go on 3. mmc_regulator_set_ocr() returns an error -> bail out So the question is: what should mmc_regulator_set_ocr_vmmc_up return if IS_ERR() is true: 1. An errno? Then this errno would have to be different from the error codes the function can normally return. 2. A positive value? Seems to be the best option Then we could write: ret = mmc_regulator_set_ocr() if (ret < 0) return ret; if (!ret) { some_action(); } ... Works but I'm not sure whether it's very intuitive. The other benefit of the proposed helpers is that they hide the complexity of using mmc->supply.vmmc and ios->vdd. Mileage may vary here. Do you have any preference? > That should allow us to simplify some code in the host drivers too, right? > >> + >> + return mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, ios->vdd); >> +} >> + >> +static inline int mmc_regulator_set_ocr_vmmc_off(struct mmc_host *mmc) >> +{ >> + if (IS_ERR(mmc->supply.vmmc)) >> + return 0; >> + >> + return mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, 0); >> +} >> + >> int mmc_regulator_get_supply(struct mmc_host *mmc); >> >> static inline int mmc_card_is_removable(struct mmc_host *host) >> -- >> 2.39.1 >> >> > > Kind regards > Uffe _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] mmc: core: add helpers mmc_regulator_set_ocr_vmmc_up/off 2023-02-17 20:09 ` Heiner Kallweit @ 2023-02-27 16:13 ` Ulf Hansson 2023-02-27 20:59 ` Heiner Kallweit 0 siblings, 1 reply; 7+ messages in thread From: Ulf Hansson @ 2023-02-27 16:13 UTC (permalink / raw) To: Heiner Kallweit Cc: Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl, linux-arm-kernel@lists.infradead.org, open list:ARM/Amlogic Meson..., linux-mmc@vger.kernel.org On Fri, 17 Feb 2023 at 21:09, Heiner Kallweit <hkallweit1@gmail.com> wrote: > > On 17.02.2023 11:47, Ulf Hansson wrote: > > On Wed, 15 Feb 2023 at 21:14, Heiner Kallweit <hkallweit1@gmail.com> wrote: > >> > >> A lot of drivers use this code, therefore let's factor it out to > >> helpers. > >> > >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > >> --- > >> include/linux/mmc/host.h | 17 +++++++++++++++++ > >> 1 file changed, 17 insertions(+) > >> > >> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h > >> index 812e6b583..f93fb8c7d 100644 > >> --- a/include/linux/mmc/host.h > >> +++ b/include/linux/mmc/host.h > >> @@ -597,6 +597,23 @@ static inline int mmc_regulator_set_vqmmc(struct mmc_host *mmc, > >> } > >> #endif > >> > >> +static inline int mmc_regulator_set_ocr_vmmc_up(struct mmc_host *mmc, > >> + struct mmc_ios *ios) > >> +{ > >> + if (IS_ERR(mmc->supply.vmmc)) > >> + return 0; > > > > Rather than adding these two new helper functions, how about adding > > the similar check in mmc_regulator_set_ocr() instead? > > > There's a number of drivers having 3 paths here: > 1. IS_ERR() is true -> do nothing and go one > 2. mmc_regulator_set_ocr() returns 0 -> some action and go on > 3. mmc_regulator_set_ocr() returns an error -> bail out Right, thanks for pointing this out. The important point I am trying to make is that the mmc core is treating "mmc->supply.vmmc" as optional (see mmc_regulator_get_supply()). To be consistent with that behaviour, I think it would make sense to bail out and return 0, in mmc_regulator_set_ocr() "if (IS_ERR(mmc->supply.vmmc))". We don't need a new set of helper functions to do that. > > So the question is: what should mmc_regulator_set_ocr_vmmc_up return > if IS_ERR() is true: > 1. An errno? Then this errno would have to be different from the > error codes the function can normally return. > 2. A positive value? Seems to be the best option > > Then we could write: > > ret = mmc_regulator_set_ocr() > if (ret < 0) > return ret; > if (!ret) { > some_action(); > } > ... > > Works but I'm not sure whether it's very intuitive. > > The other benefit of the proposed helpers is that they hide the > complexity of using mmc->supply.vmmc and ios->vdd. > > Mileage may vary here. Do you have any preference? Actually, there is no complexity. Drivers should always be able to pass 'ios->vdd' to mmc_regulator_set_ocr() (as it holds the correct value). For some reasons, some driver authors seem to find it clearer (I guess) to call mmc_regulator_set_ocr() with an explicit '0' at MMC_POWER_OFF, but it isn't needed (see mmc_power_off()). [...] Kind regards Uffe _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] mmc: core: add helpers mmc_regulator_set_ocr_vmmc_up/off 2023-02-27 16:13 ` Ulf Hansson @ 2023-02-27 20:59 ` Heiner Kallweit 0 siblings, 0 replies; 7+ messages in thread From: Heiner Kallweit @ 2023-02-27 20:59 UTC (permalink / raw) To: Ulf Hansson Cc: Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl, linux-arm-kernel@lists.infradead.org, open list:ARM/Amlogic Meson..., linux-mmc@vger.kernel.org On 27.02.2023 17:13, Ulf Hansson wrote: > On Fri, 17 Feb 2023 at 21:09, Heiner Kallweit <hkallweit1@gmail.com> wrote: >> >> On 17.02.2023 11:47, Ulf Hansson wrote: >>> On Wed, 15 Feb 2023 at 21:14, Heiner Kallweit <hkallweit1@gmail.com> wrote: >>>> >>>> A lot of drivers use this code, therefore let's factor it out to >>>> helpers. >>>> >>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >>>> --- >>>> include/linux/mmc/host.h | 17 +++++++++++++++++ >>>> 1 file changed, 17 insertions(+) >>>> >>>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h >>>> index 812e6b583..f93fb8c7d 100644 >>>> --- a/include/linux/mmc/host.h >>>> +++ b/include/linux/mmc/host.h >>>> @@ -597,6 +597,23 @@ static inline int mmc_regulator_set_vqmmc(struct mmc_host *mmc, >>>> } >>>> #endif >>>> >>>> +static inline int mmc_regulator_set_ocr_vmmc_up(struct mmc_host *mmc, >>>> + struct mmc_ios *ios) >>>> +{ >>>> + if (IS_ERR(mmc->supply.vmmc)) >>>> + return 0; >>> >>> Rather than adding these two new helper functions, how about adding >>> the similar check in mmc_regulator_set_ocr() instead? >>> >> There's a number of drivers having 3 paths here: >> 1. IS_ERR() is true -> do nothing and go one >> 2. mmc_regulator_set_ocr() returns 0 -> some action and go on >> 3. mmc_regulator_set_ocr() returns an error -> bail out > > Right, thanks for pointing this out. > > The important point I am trying to make is that the mmc core is > treating "mmc->supply.vmmc" as optional (see > mmc_regulator_get_supply()). To be consistent with that behaviour, I > think it would make sense to bail out and return 0, in > mmc_regulator_set_ocr() "if (IS_ERR(mmc->supply.vmmc))". We don't need > a new set of helper functions to do that. > You're right. I'll submit a patch for it. >> >> So the question is: what should mmc_regulator_set_ocr_vmmc_up return >> if IS_ERR() is true: >> 1. An errno? Then this errno would have to be different from the >> error codes the function can normally return. >> 2. A positive value? Seems to be the best option >> >> Then we could write: >> >> ret = mmc_regulator_set_ocr() >> if (ret < 0) >> return ret; >> if (!ret) { >> some_action(); >> } >> ... >> >> Works but I'm not sure whether it's very intuitive. >> >> The other benefit of the proposed helpers is that they hide the >> complexity of using mmc->supply.vmmc and ios->vdd. >> >> Mileage may vary here. Do you have any preference? > > Actually, there is no complexity. Drivers should always be able to > pass 'ios->vdd' to mmc_regulator_set_ocr() (as it holds the correct > value). > > For some reasons, some driver authors seem to find it clearer (I > guess) to call mmc_regulator_set_ocr() with an explicit '0' at > MMC_POWER_OFF, but it isn't needed (see mmc_power_off()). > > [...] > > Kind regards > Uffe _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] mmc: meson-gx: use new helpers mmc_regulator_set_ocr_vmmc_off/up 2023-02-15 20:08 [PATCH 0/2] mmc: add helpers mmc_regulator_set_ocr_vmmc_up/off Heiner Kallweit 2023-02-15 20:11 ` [PATCH 1/2] mmc: core: " Heiner Kallweit @ 2023-02-15 20:13 ` Heiner Kallweit 1 sibling, 0 replies; 7+ messages in thread From: Heiner Kallweit @ 2023-02-15 20:13 UTC (permalink / raw) To: Ulf Hansson, Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl Cc: linux-arm-kernel@lists.infradead.org, open list:ARM/Amlogic Meson..., linux-mmc@vger.kernel.org Make use of the new helpers to simplify the code. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/mmc/host/meson-gx-mmc.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c index 641ea4292..740973d35 100644 --- a/drivers/mmc/host/meson-gx-mmc.c +++ b/drivers/mmc/host/meson-gx-mmc.c @@ -604,8 +604,7 @@ static void meson_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) */ switch (ios->power_mode) { case MMC_POWER_OFF: - if (!IS_ERR(mmc->supply.vmmc)) - mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, 0); + mmc_regulator_set_ocr_vmmc_off(mmc); if (!IS_ERR(mmc->supply.vqmmc) && host->vqmmc_enabled) { regulator_disable(mmc->supply.vqmmc); @@ -615,8 +614,7 @@ static void meson_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) break; case MMC_POWER_UP: - if (!IS_ERR(mmc->supply.vmmc)) - mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, ios->vdd); + mmc_regulator_set_ocr_vmmc_up(mmc, ios); break; -- 2.39.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-02-27 21:01 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-02-15 20:08 [PATCH 0/2] mmc: add helpers mmc_regulator_set_ocr_vmmc_up/off Heiner Kallweit 2023-02-15 20:11 ` [PATCH 1/2] mmc: core: " Heiner Kallweit 2023-02-17 10:47 ` Ulf Hansson 2023-02-17 20:09 ` Heiner Kallweit 2023-02-27 16:13 ` Ulf Hansson 2023-02-27 20:59 ` Heiner Kallweit 2023-02-15 20:13 ` [PATCH 2/2] mmc: meson-gx: use new helpers mmc_regulator_set_ocr_vmmc_off/up Heiner Kallweit
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).