On Fri, 17 Feb 2023 at 21:09, Heiner Kallweit <hkallweit1@xxxxxxxxx> wrote:
>
> On 17.02.2023 11:47, Ulf Hansson wrote:
> > On Wed, 15 Feb 2023 at 21:14, Heiner Kallweit <hkallweit1@xxxxxxxxx> wrote:
> >>
> >> A lot of drivers use this code, therefore let's factor it out to
> >> helpers.
> >>
> >> Signed-off-by: Heiner Kallweit <hkallweit1@xxxxxxxxx>
> >> ---
> >> 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
