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

[Heiner Kallweit <hkallweit1@gmail.com>]

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