| Message ID | 02b46550-e0b5-4251-8156-56eb0f690d89@gmail.com |
|---|---|
| Headers | show |
| Series | mmc: add helpers mmc_regulator_set_ocr_vmmc_up/off | expand |
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
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
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
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