Re: [PATCH 1/2] mmc: core: add helpers mmc_regulator_set_ocr_vmmc_up/off

[Heiner Kallweit <hkallweit1@gmail.com>]

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