diff mbox

mmc: omap_hsmmc: Update driver to support without regulators

Message ID 1436902186-6542-1-git-send-email-fcooper@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Franklin Cooper July 14, 2015, 7:29 p.m. UTC
From: Roger Quadros <rogerq@ti.com>

Update driver to support without regulators.

Without this patch boards that do not enable regulator config options will
fail to boot with a kernel panic.

Signed-off-by: Roger Quadros <rogerq@ti.com>
Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
---
 Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt |  2 ++
 drivers/mmc/host/omap_hsmmc.c                           | 14 ++++++++++----
 2 files changed, 12 insertions(+), 4 deletions(-)

Comments

Ulf Hansson July 21, 2015, 11:40 a.m. UTC | #1
On 14 July 2015 at 21:29, Franklin S Cooper Jr <fcooper@ti.com> wrote:
> From: Roger Quadros <rogerq@ti.com>
>
> Update driver to support without regulators.
>
> Without this patch boards that do not enable regulator config options will
> fail to boot with a kernel panic.

I guess that's because the rootfs is mounted on the card that doesn't
get detected, right?

>
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
> ---
>  Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt |  2 ++
>  drivers/mmc/host/omap_hsmmc.c                           | 14 ++++++++++----
>  2 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt b/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
> index 76bf087..2408e87 100644
> --- a/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
> +++ b/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
> @@ -22,6 +22,8 @@ ti,dual-volt: boolean, supports dual voltage cards
>  ti,non-removable: non-removable slot (like eMMC)
>  ti,needs-special-reset: Requires a special softreset sequence
>  ti,needs-special-hs-handling: HSMMC IP needs special setting for handling High Speed
> +voltage-ranges: Specify the voltage range supported if regulator framework
> +isn't enabled.
>  dmas: List of DMA specifiers with the controller specific format
>  as described in the generic DMA client binding. A tx and rx
>  specifier is required.
> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
> index b2b411d..16c870f 100644
> --- a/drivers/mmc/host/omap_hsmmc.c
> +++ b/drivers/mmc/host/omap_hsmmc.c
> @@ -1551,10 +1551,13 @@ static void omap_hsmmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>         if (ios->power_mode != host->power_mode) {
>                 switch (ios->power_mode) {
>                 case MMC_POWER_OFF:
> -                       mmc_pdata(host)->set_power(host->dev, 0, 0);
> +                       if (host->use_reg)
> +                               mmc_pdata(host)->set_power(host->dev, 0, 0);
>                         break;
>                 case MMC_POWER_UP:
> -                       mmc_pdata(host)->set_power(host->dev, 1, ios->vdd);
> +                       if (host->use_reg)
> +                               mmc_pdata(host)->set_power(host->dev, 1,
> +                                                          ios->vdd);
>                         break;
>                 case MMC_POWER_ON:
>                         do_send_init_stream = 1;
> @@ -2082,10 +2085,13 @@ static int omap_hsmmc_probe(struct platform_device *pdev)
>                 if (ret)
>                         goto err_irq;
>                 host->use_reg = 1;
> +               mmc->ocr_avail = mmc_pdata(host)->ocr_mask;
> +       } else {
> +               ret = mmc_of_parse_voltage(pdev->dev.of_node, &mmc->ocr_avail);
> +               if (ret)
> +                       goto err_irq;
>         }
>
> -       mmc->ocr_avail = mmc_pdata(host)->ocr_mask;
> -
>         omap_hsmmc_disable_irq(host);
>
>         /*
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

No, I don't like this patch. If your board have regulators which can
be used to find out the voltage levels, we shall use them. We shall
not use DT to workaround this.

So, perhaps the Kconfig section for omap_hsmmc should select
CONFIG_REGULATOR (and friends)?

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Franklin Cooper July 21, 2015, 7:39 p.m. UTC | #2
On 07/21/2015 06:40 AM, Ulf Hansson wrote:
> On 14 July 2015 at 21:29, Franklin S Cooper Jr <fcooper@ti.com> wrote:
>> From: Roger Quadros <rogerq@ti.com>
>>
>> Update driver to support without regulators.
>>
>> Without this patch boards that do not enable regulator config options will
>> fail to boot with a kernel panic.
> I guess that's because the rootfs is mounted on the card that doesn't
> get detected, right?
No at ramfs was used to validate this and was loaded via ethernet in U-boot.
>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
>> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
>> ---
>>  Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt |  2 ++
>>  drivers/mmc/host/omap_hsmmc.c                           | 14 ++++++++++----
>>  2 files changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt b/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
>> index 76bf087..2408e87 100644
>> --- a/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
>> +++ b/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
>> @@ -22,6 +22,8 @@ ti,dual-volt: boolean, supports dual voltage cards
>>  ti,non-removable: non-removable slot (like eMMC)
>>  ti,needs-special-reset: Requires a special softreset sequence
>>  ti,needs-special-hs-handling: HSMMC IP needs special setting for handling High Speed
>> +voltage-ranges: Specify the voltage range supported if regulator framework
>> +isn't enabled.
>>  dmas: List of DMA specifiers with the controller specific format
>>  as described in the generic DMA client binding. A tx and rx
>>  specifier is required.
>> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
>> index b2b411d..16c870f 100644
>> --- a/drivers/mmc/host/omap_hsmmc.c
>> +++ b/drivers/mmc/host/omap_hsmmc.c
>> @@ -1551,10 +1551,13 @@ static void omap_hsmmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>>         if (ios->power_mode != host->power_mode) {
>>                 switch (ios->power_mode) {
>>                 case MMC_POWER_OFF:
>> -                       mmc_pdata(host)->set_power(host->dev, 0, 0);
>> +                       if (host->use_reg)
>> +                               mmc_pdata(host)->set_power(host->dev, 0, 0);
>>                         break;
>>                 case MMC_POWER_UP:
>> -                       mmc_pdata(host)->set_power(host->dev, 1, ios->vdd);
>> +                       if (host->use_reg)
>> +                               mmc_pdata(host)->set_power(host->dev, 1,
>> +                                                          ios->vdd);
>>                         break;
>>                 case MMC_POWER_ON:
>>                         do_send_init_stream = 1;
>> @@ -2082,10 +2085,13 @@ static int omap_hsmmc_probe(struct platform_device *pdev)
>>                 if (ret)
>>                         goto err_irq;
>>                 host->use_reg = 1;
>> +               mmc->ocr_avail = mmc_pdata(host)->ocr_mask;
>> +       } else {
>> +               ret = mmc_of_parse_voltage(pdev->dev.of_node, &mmc->ocr_avail);
>> +               if (ret)
>> +                       goto err_irq;
>>         }
>>
>> -       mmc->ocr_avail = mmc_pdata(host)->ocr_mask;
>> -
>>         omap_hsmmc_disable_irq(host);
>>
>>         /*
>> --
>> 1.9.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> No, I don't like this patch. If your board have regulators which can
> be used to find out the voltage levels, we shall use them. We shall
> not use DT to workaround this.
>
> So, perhaps the Kconfig section for omap_hsmmc should select
> CONFIG_REGULATOR (and friends)?
Looking at the code the "ifdefCONFIG_REGULATOR" and the usage of use_reg
(use regulator) shows that this driver is suppose to support boards that
don't have the regulator framework enabled.

This patches insures that this driver can work without the regulator framework
which is currently broken.

>
> Kind regards
> Uffe

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Felipe Balbi July 21, 2015, 7:57 p.m. UTC | #3
On Tue, Jul 14, 2015 at 02:29:46PM -0500, Franklin S Cooper Jr wrote:
> From: Roger Quadros <rogerq@ti.com>
> 
> Update driver to support without regulators.
> 
> Without this patch boards that do not enable regulator config options will
> fail to boot with a kernel panic.
> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
> ---
>  Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt |  2 ++
>  drivers/mmc/host/omap_hsmmc.c                           | 14 ++++++++++----
>  2 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt b/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
> index 76bf087..2408e87 100644
> --- a/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
> +++ b/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
> @@ -22,6 +22,8 @@ ti,dual-volt: boolean, supports dual voltage cards
>  ti,non-removable: non-removable slot (like eMMC)
>  ti,needs-special-reset: Requires a special softreset sequence
>  ti,needs-special-hs-handling: HSMMC IP needs special setting for handling High Speed
> +voltage-ranges: Specify the voltage range supported if regulator framework
> +isn't enabled.
>  dmas: List of DMA specifiers with the controller specific format
>  as described in the generic DMA client binding. A tx and rx
>  specifier is required.
> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
> index b2b411d..16c870f 100644
> --- a/drivers/mmc/host/omap_hsmmc.c
> +++ b/drivers/mmc/host/omap_hsmmc.c
> @@ -1551,10 +1551,13 @@ static void omap_hsmmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>  	if (ios->power_mode != host->power_mode) {
>  		switch (ios->power_mode) {
>  		case MMC_POWER_OFF:
> -			mmc_pdata(host)->set_power(host->dev, 0, 0);
> +			if (host->use_reg)
> +				mmc_pdata(host)->set_power(host->dev, 0, 0);

looks like this driver should just be use regulator_get_optional(), then
->set_power() would still work, no ?
Franklin Cooper July 21, 2015, 8:11 p.m. UTC | #4
On 07/21/2015 02:57 PM, Felipe Balbi wrote:
> On Tue, Jul 14, 2015 at 02:29:46PM -0500, Franklin S Cooper Jr wrote:
>> From: Roger Quadros <rogerq@ti.com>
>>
>> Update driver to support without regulators.
>>
>> Without this patch boards that do not enable regulator config options will
>> fail to boot with a kernel panic.
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
>> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
>> ---
>>  Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt |  2 ++
>>  drivers/mmc/host/omap_hsmmc.c                           | 14 ++++++++++----
>>  2 files changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt b/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
>> index 76bf087..2408e87 100644
>> --- a/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
>> +++ b/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
>> @@ -22,6 +22,8 @@ ti,dual-volt: boolean, supports dual voltage cards
>>  ti,non-removable: non-removable slot (like eMMC)
>>  ti,needs-special-reset: Requires a special softreset sequence
>>  ti,needs-special-hs-handling: HSMMC IP needs special setting for handling High Speed
>> +voltage-ranges: Specify the voltage range supported if regulator framework
>> +isn't enabled.
>>  dmas: List of DMA specifiers with the controller specific format
>>  as described in the generic DMA client binding. A tx and rx
>>  specifier is required.
>> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
>> index b2b411d..16c870f 100644
>> --- a/drivers/mmc/host/omap_hsmmc.c
>> +++ b/drivers/mmc/host/omap_hsmmc.c
>> @@ -1551,10 +1551,13 @@ static void omap_hsmmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>>  	if (ios->power_mode != host->power_mode) {
>>  		switch (ios->power_mode) {
>>  		case MMC_POWER_OFF:
>> -			mmc_pdata(host)->set_power(host->dev, 0, 0);
>> +			if (host->use_reg)
>> +				mmc_pdata(host)->set_power(host->dev, 0, 0);
> looks like this driver should just be use regulator_get_optional(), then
> ->set_power() would still work, no ?
Wouldn't there still be a dependency on the regulator framework to get that to work?

I guess my intention was to fix not having the regulator framework. But I'm sure there are
other ways to support not having a regulator defined as long as CONFIG_REGULATOR is enabled.
For example dummy regulator worked fine when I tried it.
>

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Felipe Balbi July 21, 2015, 8:14 p.m. UTC | #5
On Tue, Jul 21, 2015 at 03:11:21PM -0500, Franklin S Cooper Jr. wrote:
> 
> 
> On 07/21/2015 02:57 PM, Felipe Balbi wrote:
> > On Tue, Jul 14, 2015 at 02:29:46PM -0500, Franklin S Cooper Jr wrote:
> >> From: Roger Quadros <rogerq@ti.com>
> >>
> >> Update driver to support without regulators.
> >>
> >> Without this patch boards that do not enable regulator config options will
> >> fail to boot with a kernel panic.
> >>
> >> Signed-off-by: Roger Quadros <rogerq@ti.com>
> >> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
> >> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
> >> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
> >> ---
> >>  Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt |  2 ++
> >>  drivers/mmc/host/omap_hsmmc.c                           | 14 ++++++++++----
> >>  2 files changed, 12 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt b/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
> >> index 76bf087..2408e87 100644
> >> --- a/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
> >> +++ b/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
> >> @@ -22,6 +22,8 @@ ti,dual-volt: boolean, supports dual voltage cards
> >>  ti,non-removable: non-removable slot (like eMMC)
> >>  ti,needs-special-reset: Requires a special softreset sequence
> >>  ti,needs-special-hs-handling: HSMMC IP needs special setting for handling High Speed
> >> +voltage-ranges: Specify the voltage range supported if regulator framework
> >> +isn't enabled.
> >>  dmas: List of DMA specifiers with the controller specific format
> >>  as described in the generic DMA client binding. A tx and rx
> >>  specifier is required.
> >> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
> >> index b2b411d..16c870f 100644
> >> --- a/drivers/mmc/host/omap_hsmmc.c
> >> +++ b/drivers/mmc/host/omap_hsmmc.c
> >> @@ -1551,10 +1551,13 @@ static void omap_hsmmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> >>  	if (ios->power_mode != host->power_mode) {
> >>  		switch (ios->power_mode) {
> >>  		case MMC_POWER_OFF:
> >> -			mmc_pdata(host)->set_power(host->dev, 0, 0);
> >> +			if (host->use_reg)
> >> +				mmc_pdata(host)->set_power(host->dev, 0, 0);
> > looks like this driver should just be use regulator_get_optional(), then
> > ->set_power() would still work, no ?
> Wouldn't there still be a dependency on the regulator framework to get
> that to work?

I didn't look at the code, but I'd expect us to have a sensible stub on
!REGULATOR builds
Franklin Cooper July 22, 2015, 11:44 a.m. UTC | #6
On 07/21/2015 02:39 PM, Franklin S Cooper Jr. wrote:
>
> On 07/21/2015 06:40 AM, Ulf Hansson wrote:
>> On 14 July 2015 at 21:29, Franklin S Cooper Jr <fcooper@ti.com> wrote:
>>> From: Roger Quadros <rogerq@ti.com>
>>>
>>> Update driver to support without regulators.
>>>
>>> Without this patch boards that do not enable regulator config options will
>>> fail to boot with a kernel panic.
>> I guess that's because the rootfs is mounted on the card that doesn't
>> get detected, right?
> No at ramfs was used to validate this and was loaded via ethernet in U-boot.
>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>>> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
>>> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
>>> ---
>>>  Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt |  2 ++
>>>  drivers/mmc/host/omap_hsmmc.c                           | 14 ++++++++++----
>>>  2 files changed, 12 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt b/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
>>> index 76bf087..2408e87 100644
>>> --- a/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
>>> +++ b/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
>>> @@ -22,6 +22,8 @@ ti,dual-volt: boolean, supports dual voltage cards
>>>  ti,non-removable: non-removable slot (like eMMC)
>>>  ti,needs-special-reset: Requires a special softreset sequence
>>>  ti,needs-special-hs-handling: HSMMC IP needs special setting for handling High Speed
>>> +voltage-ranges: Specify the voltage range supported if regulator framework
>>> +isn't enabled.
>>>  dmas: List of DMA specifiers with the controller specific format
>>>  as described in the generic DMA client binding. A tx and rx
>>>  specifier is required.
>>> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
>>> index b2b411d..16c870f 100644
>>> --- a/drivers/mmc/host/omap_hsmmc.c
>>> +++ b/drivers/mmc/host/omap_hsmmc.c
>>> @@ -1551,10 +1551,13 @@ static void omap_hsmmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>>>         if (ios->power_mode != host->power_mode) {
>>>                 switch (ios->power_mode) {
>>>                 case MMC_POWER_OFF:
>>> -                       mmc_pdata(host)->set_power(host->dev, 0, 0);
>>> +                       if (host->use_reg)
>>> +                               mmc_pdata(host)->set_power(host->dev, 0, 0);
>>>                         break;
>>>                 case MMC_POWER_UP:
>>> -                       mmc_pdata(host)->set_power(host->dev, 1, ios->vdd);
>>> +                       if (host->use_reg)
>>> +                               mmc_pdata(host)->set_power(host->dev, 1,
>>> +                                                          ios->vdd);
>>>                         break;
>>>                 case MMC_POWER_ON:
>>>                         do_send_init_stream = 1;
>>> @@ -2082,10 +2085,13 @@ static int omap_hsmmc_probe(struct platform_device *pdev)
>>>                 if (ret)
>>>                         goto err_irq;
>>>                 host->use_reg = 1;
>>> +               mmc->ocr_avail = mmc_pdata(host)->ocr_mask;
>>> +       } else {
>>> +               ret = mmc_of_parse_voltage(pdev->dev.of_node, &mmc->ocr_avail);
>>> +               if (ret)
>>> +                       goto err_irq;
>>>         }
>>>
>>> -       mmc->ocr_avail = mmc_pdata(host)->ocr_mask;
>>> -
>>>         omap_hsmmc_disable_irq(host);
>>>
>>>         /*
>>> --
>>> 1.9.1
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> No, I don't like this patch. If your board have regulators which can
>> be used to find out the voltage levels, we shall use them. We shall
>> not use DT to workaround this.
>>
>> So, perhaps the Kconfig section for omap_hsmmc should select
>> CONFIG_REGULATOR (and friends)?
> Looking at the code the "ifdefCONFIG_REGULATOR" and the usage of use_reg
> (use regulator) shows that this driver is suppose to support boards that
> don't have the regulator framework enabled.
>
> This patches insures that this driver can work without the regulator framework
> which is currently broken.
Hi Ulf,

Just to clarify do you have an issue with this patch or in general
you rather nothave !CONFIG_REGULATOR supported? If its the latter
then wouldit be better to remove the !CONFIG_REGULATOR portion from
this driver all together.
>> Kind regards
>> Uffe
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ulf Hansson July 22, 2015, 12:07 p.m. UTC | #7
[...]

> Hi Ulf,
>
> Just to clarify do you have an issue with this patch or in general
> you rather nothave !CONFIG_REGULATOR supported? If its the latter
> then wouldit be better to remove the !CONFIG_REGULATOR portion from
> this driver all together.

I don't like this patch as is, as it seems like you are replacing the
VMMC regulator that actually exist in HW by parsing a DT property.

I think mmc_of_parse_voltage() shall *only* be used when the OCR mask
(->ocr_avail) can't be fetched by using a regulator. Typically that's
when the VMMC voltage levels are managed within the mmc controller
itself.

I don't have a strong opinion on how you solve that, but I guess you
have two options.

1.
Make sure the driver can be build and used both for CONFIG_REGULATOR
and !CONFIG_REGULATOR. What you need to take care of then, is to
assign the ->ocr_avail mask to a default value in the case when it
can't be fetched from a regulator. Most mmc drivers has some kind of
fall-back mechanism like this.

For those SoC where you need/can control the VMMC regulator, make sure
those enables CONFIG_REGULATOR to get the proper behaviour by
omap_hsmmc.

2.
Force omap_hsmmc to be build with CONFIG_REGULATOR.

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kishon Vijay Abraham I July 22, 2015, 12:17 p.m. UTC | #8
Hi,

On Wednesday 22 July 2015 05:37 PM, Ulf Hansson wrote:
> [...]
> 
>> Hi Ulf,
>>
>> Just to clarify do you have an issue with this patch or in general
>> you rather nothave !CONFIG_REGULATOR supported? If its the latter
>> then wouldit be better to remove the !CONFIG_REGULATOR portion from
>> this driver all together.
> 
> I don't like this patch as is, as it seems like you are replacing the
> VMMC regulator that actually exist in HW by parsing a DT property.
> 
> I think mmc_of_parse_voltage() shall *only* be used when the OCR mask
> (->ocr_avail) can't be fetched by using a regulator. Typically that's
> when the VMMC voltage levels are managed within the mmc controller
> itself.
> 
> I don't have a strong opinion on how you solve that, but I guess you
> have two options.
> 
> 1.
> Make sure the driver can be build and used both for CONFIG_REGULATOR
> and !CONFIG_REGULATOR. What you need to take care of then, is to
> assign the ->ocr_avail mask to a default value in the case when it
> can't be fetched from a regulator. Most mmc drivers has some kind of
> fall-back mechanism like this.

I'm working on cleaning up omap_hsmmc driver where I have taken care to avoid
dependency with CONFIG_REGULATOR. I'll post the series asap.

Thanks
Kishon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt b/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
index 76bf087..2408e87 100644
--- a/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
+++ b/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
@@ -22,6 +22,8 @@  ti,dual-volt: boolean, supports dual voltage cards
 ti,non-removable: non-removable slot (like eMMC)
 ti,needs-special-reset: Requires a special softreset sequence
 ti,needs-special-hs-handling: HSMMC IP needs special setting for handling High Speed
+voltage-ranges: Specify the voltage range supported if regulator framework
+isn't enabled.
 dmas: List of DMA specifiers with the controller specific format
 as described in the generic DMA client binding. A tx and rx
 specifier is required.
diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index b2b411d..16c870f 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -1551,10 +1551,13 @@  static void omap_hsmmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	if (ios->power_mode != host->power_mode) {
 		switch (ios->power_mode) {
 		case MMC_POWER_OFF:
-			mmc_pdata(host)->set_power(host->dev, 0, 0);
+			if (host->use_reg)
+				mmc_pdata(host)->set_power(host->dev, 0, 0);
 			break;
 		case MMC_POWER_UP:
-			mmc_pdata(host)->set_power(host->dev, 1, ios->vdd);
+			if (host->use_reg)
+				mmc_pdata(host)->set_power(host->dev, 1,
+							   ios->vdd);
 			break;
 		case MMC_POWER_ON:
 			do_send_init_stream = 1;
@@ -2082,10 +2085,13 @@  static int omap_hsmmc_probe(struct platform_device *pdev)
 		if (ret)
 			goto err_irq;
 		host->use_reg = 1;
+		mmc->ocr_avail = mmc_pdata(host)->ocr_mask;
+	} else {
+		ret = mmc_of_parse_voltage(pdev->dev.of_node, &mmc->ocr_avail);
+		if (ret)
+			goto err_irq;
 	}
 
-	mmc->ocr_avail = mmc_pdata(host)->ocr_mask;
-
 	omap_hsmmc_disable_irq(host);
 
 	/*