[PATCH 0/2] mmc: add helpers mmc_regulator_set_ocr_vmmc_up/off

[PATCH 0/2] mmc: add helpers mmc_regulator_set_ocr_vmmc_up/off

[Heiner Kallweit]

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

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

[Heiner Kallweit]

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

[PATCH 2/2] mmc: meson-gx: use new helpers mmc_regulator_set_ocr_vmmc_off/up

[Heiner Kallweit]

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

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

[Ulf Hansson]

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

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

[Heiner Kallweit]

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

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

[Ulf Hansson]

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

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

[Heiner Kallweit]

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