[PATCH v4] mmc: add a function to get regulators, supplying card's power

[PATCH v4] mmc: add a function to get regulators, supplying card's power

[Guennadi Liakhovetski]

Add a function to get regulators, supplying card's Vdd and Vccq on a
specific host. If a Vdd supplying regulator is found, the function checks,
whether a valid OCR mask can be obtained from it. The Vccq regulator is
optional. A failure to get it is not fatal.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---

v4:

1. add _GPL to EXPORT_SYMBOL (note: a recently posted patch changes 
another occurrence of EXPORT_SYMBOL too, so, this patch will apply with a 
fuzz, if merged after)

2. add an optional vqmmc regulator, which also changes the function 
prototype

3. print a warning, if failed to get an OCR mask

Thanks to all, who commented

 drivers/mmc/core/core.c  |   27 +++++++++++++++++++++++++++
 include/linux/mmc/host.h |   12 ++++++++++++
 2 files changed, 39 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 0b6141d..d77a238 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1013,6 +1013,33 @@ int mmc_regulator_set_ocr(struct mmc_host *mmc,
 }
 EXPORT_SYMBOL(mmc_regulator_set_ocr);
 
+int mmc_regulator_get_supply(struct mmc_host *mmc, struct mmc_supply *s)
+{
+	struct device *dev = mmc_dev(mmc);
+	struct regulator *supply;
+	int ret;
+
+	if (!mmc || !s)
+		return -EINVAL;
+
+	supply = devm_regulator_get(dev, "vmmc");
+
+	if (IS_ERR(supply))
+		return PTR_ERR(supply);
+
+	s->vmmc = supply;
+	s->vqmmc = devm_regulator_get(dev, "vqmmc");
+
+	ret = mmc_regulator_get_ocrmask(supply);
+	if (ret > 0)
+		mmc->ocr_avail = ret;
+	else
+		dev_warn(mmc_dev(mmc), "Failed getting OCR mask: %d\n", ret);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(mmc_regulator_get_supply);
+
 #endif /* CONFIG_REGULATOR */
 
 /*
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 0707d22..3192335 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -359,11 +359,17 @@ static inline void mmc_signal_sdio_irq(struct mmc_host *host)
 
 struct regulator;
 
+struct mmc_supply {
+	struct regulator *vmmc;		/* Card power supply */
+	struct regulator *vqmmc;	/* Optional Vccq supply */
+};
+
 #ifdef CONFIG_REGULATOR
 int mmc_regulator_get_ocrmask(struct regulator *supply);
 int mmc_regulator_set_ocr(struct mmc_host *mmc,
 			struct regulator *supply,
 			unsigned short vdd_bit);
+int mmc_regulator_get_supply(struct mmc_host *mmc, struct mmc_supply *s);
 #else
 static inline int mmc_regulator_get_ocrmask(struct regulator *supply)
 {
@@ -376,6 +382,12 @@ static inline int mmc_regulator_set_ocr(struct mmc_host *mmc,
 {
 	return 0;
 }
+
+static inline int mmc_regulator_get_supply(struct mmc_host *mmc,
+					   struct mmc_supply *s)
+{
+	return 0;
+}
 #endif
 
 int mmc_card_awake(struct mmc_host *host);
-- 
1.7.2.5

Re: [PATCH v4] mmc: add a function to get regulators, supplying card's power

[Philip Rakity]

On Jun 12, 2012, at 8:57 AM, Guennadi Liakhovetski wrote:

> Add a function to get regulators, supplying card's Vdd and Vccq on a
> specific host. If a Vdd supplying regulator is found, the function checks,
> whether a valid OCR mask can be obtained from it. The Vccq regulator is
> optional. A failure to get it is not fatal.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
> 
> v4:
> 
> 1. add _GPL to EXPORT_SYMBOL (note: a recently posted patch changes 
> another occurrence of EXPORT_SYMBOL too, so, this patch will apply with a 
> fuzz, if merged after)
> 
> 2. add an optional vqmmc regulator, which also changes the function 
> prototype
> 
> 3. print a warning, if failed to get an OCR mask
> 
> Thanks to all, who commented
> 
> drivers/mmc/core/core.c  |   27 +++++++++++++++++++++++++++
> include/linux/mmc/host.h |   12 ++++++++++++
> 2 files changed, 39 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 0b6141d..d77a238 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1013,6 +1013,33 @@ int mmc_regulator_set_ocr(struct mmc_host *mmc,
> }
> EXPORT_SYMBOL(mmc_regulator_set_ocr);
> 
> +int mmc_regulator_get_supply(struct mmc_host *mmc, struct mmc_supply *s)
> +{
> +	struct device *dev = mmc_dev(mmc);
> +	struct regulator *supply;
> +	int ret;
> +
> +	if (!mmc || !s)
> +		return -EINVAL;
> +
> +	supply = devm_regulator_get(dev, "vmmc");
> +
> +	if (IS_ERR(supply))
> +		return PTR_ERR(supply);
> +
> +	s->vmmc = supply;
> +	s->vqmmc = devm_regulator_get(dev, "vqmmc");

Should we set vqmmc to NULL if an error was returned ?

> +
> +	ret = mmc_regulator_get_ocrmask(supply);
> +	if (ret > 0)
> +		mmc->ocr_avail = ret;
> +	else
> +		dev_warn(mmc_dev(mmc), "Failed getting OCR mask: %d\n", ret);

I was wondering if  sdhci (or the other SD Host drivers can work if no ocr_avail mask is set ?  


> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(mmc_regulator_get_supply);
> +
> #endif /* CONFIG_REGULATOR */
> 
> /*
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 0707d22..3192335 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -359,11 +359,17 @@ static inline void mmc_signal_sdio_irq(struct mmc_host *host)
> 
> struct regulator;
> 
> +struct mmc_supply {
> +	struct regulator *vmmc;		/* Card power supply */
> +	struct regulator *vqmmc;	/* Optional Vccq supply */
> +};
> +
> #ifdef CONFIG_REGULATOR
> int mmc_regulator_get_ocrmask(struct regulator *supply);
> int mmc_regulator_set_ocr(struct mmc_host *mmc,
> 			struct regulator *supply,
> 			unsigned short vdd_bit);
> +int mmc_regulator_get_supply(struct mmc_host *mmc, struct mmc_supply *s);
> #else
> static inline int mmc_regulator_get_ocrmask(struct regulator *supply)
> {
> @@ -376,6 +382,12 @@ static inline int mmc_regulator_set_ocr(struct mmc_host *mmc,
> {
> 	return 0;
> }
> +
> +static inline int mmc_regulator_get_supply(struct mmc_host *mmc,
> +					   struct mmc_supply *s)
> +{
> +	return 0;
> +}
> #endif
> 
> int mmc_card_awake(struct mmc_host *host);
> -- 
> 1.7.2.5
> 

Re: [PATCH v4] mmc: add a function to get regulators, supplying card's power

[Guennadi Liakhovetski]

On Tue, 12 Jun 2012, Philip Rakity wrote:

> 
> On Jun 12, 2012, at 8:57 AM, Guennadi Liakhovetski wrote:
> 
> > Add a function to get regulators, supplying card's Vdd and Vccq on a
> > specific host. If a Vdd supplying regulator is found, the function checks,
> > whether a valid OCR mask can be obtained from it. The Vccq regulator is
> > optional. A failure to get it is not fatal.
> > 
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > ---
> > 
> > v4:
> > 
> > 1. add _GPL to EXPORT_SYMBOL (note: a recently posted patch changes 
> > another occurrence of EXPORT_SYMBOL too, so, this patch will apply with a 
> > fuzz, if merged after)
> > 
> > 2. add an optional vqmmc regulator, which also changes the function 
> > prototype
> > 
> > 3. print a warning, if failed to get an OCR mask
> > 
> > Thanks to all, who commented
> > 
> > drivers/mmc/core/core.c  |   27 +++++++++++++++++++++++++++
> > include/linux/mmc/host.h |   12 ++++++++++++
> > 2 files changed, 39 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> > index 0b6141d..d77a238 100644
> > --- a/drivers/mmc/core/core.c
> > +++ b/drivers/mmc/core/core.c
> > @@ -1013,6 +1013,33 @@ int mmc_regulator_set_ocr(struct mmc_host *mmc,
> > }
> > EXPORT_SYMBOL(mmc_regulator_set_ocr);
> > 
> > +int mmc_regulator_get_supply(struct mmc_host *mmc, struct mmc_supply *s)
> > +{
> > +	struct device *dev = mmc_dev(mmc);
> > +	struct regulator *supply;
> > +	int ret;
> > +
> > +	if (!mmc || !s)
> > +		return -EINVAL;
> > +
> > +	supply = devm_regulator_get(dev, "vmmc");
> > +
> > +	if (IS_ERR(supply))
> > +		return PTR_ERR(supply);
> > +
> > +	s->vmmc = supply;
> > +	s->vqmmc = devm_regulator_get(dev, "vqmmc");
> 
> Should we set vqmmc to NULL if an error was returned ?

Don't think so, drivers can perfectly check for IS_ERR() themselves.

> > +
> > +	ret = mmc_regulator_get_ocrmask(supply);
> > +	if (ret > 0)
> > +		mmc->ocr_avail = ret;
> > +	else
> > +		dev_warn(mmc_dev(mmc), "Failed getting OCR mask: %d\n", ret);
> 
> I was wondering if  sdhci (or the other SD Host drivers can work if no ocr_avail mask is set ?  

Yes, e.g., if it's a dummy regulator and the board actually doesn't need 
one.

> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(mmc_regulator_get_supply);
> > +
> > #endif /* CONFIG_REGULATOR */
> > 
> > /*
> > diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> > index 0707d22..3192335 100644
> > --- a/include/linux/mmc/host.h
> > +++ b/include/linux/mmc/host.h
> > @@ -359,11 +359,17 @@ static inline void mmc_signal_sdio_irq(struct mmc_host *host)
> > 
> > struct regulator;
> > 
> > +struct mmc_supply {
> > +	struct regulator *vmmc;		/* Card power supply */
> > +	struct regulator *vqmmc;	/* Optional Vccq supply */
> > +};
> > +
> > #ifdef CONFIG_REGULATOR
> > int mmc_regulator_get_ocrmask(struct regulator *supply);
> > int mmc_regulator_set_ocr(struct mmc_host *mmc,
> > 			struct regulator *supply,
> > 			unsigned short vdd_bit);
> > +int mmc_regulator_get_supply(struct mmc_host *mmc, struct mmc_supply *s);
> > #else
> > static inline int mmc_regulator_get_ocrmask(struct regulator *supply)
> > {
> > @@ -376,6 +382,12 @@ static inline int mmc_regulator_set_ocr(struct mmc_host *mmc,
> > {
> > 	return 0;
> > }
> > +
> > +static inline int mmc_regulator_get_supply(struct mmc_host *mmc,
> > +					   struct mmc_supply *s)
> > +{
> > +	return 0;
> > +}
> > #endif
> > 
> > int mmc_card_awake(struct mmc_host *host);
> > -- 
> > 1.7.2.5

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH v4] mmc: add a function to get regulators, supplying card's power

[Philip Rakity]

On Jun 12, 2012, at 9:52 AM, Guennadi Liakhovetski wrote:

> On Tue, 12 Jun 2012, Philip Rakity wrote:
> 
>> 
>> On Jun 12, 2012, at 8:57 AM, Guennadi Liakhovetski wrote:
>> 
>>> Add a function to get regulators, supplying card's Vdd and Vccq on a
>>> specific host. If a Vdd supplying regulator is found, the function checks,
>>> whether a valid OCR mask can be obtained from it. The Vccq regulator is
>>> optional. A failure to get it is not fatal.
>>> 
>>> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>>> ---
>>> 
>>> v4:
>>> 
>>> 1. add _GPL to EXPORT_SYMBOL (note: a recently posted patch changes 
>>> another occurrence of EXPORT_SYMBOL too, so, this patch will apply with a 
>>> fuzz, if merged after)
>>> 
>>> 2. add an optional vqmmc regulator, which also changes the function 
>>> prototype
>>> 
>>> 3. print a warning, if failed to get an OCR mask
>>> 
>>> Thanks to all, who commented
>>> 
>>> drivers/mmc/core/core.c  |   27 +++++++++++++++++++++++++++
>>> include/linux/mmc/host.h |   12 ++++++++++++
>>> 2 files changed, 39 insertions(+), 0 deletions(-)
>>> 
>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>> index 0b6141d..d77a238 100644
>>> --- a/drivers/mmc/core/core.c
>>> +++ b/drivers/mmc/core/core.c
>>> @@ -1013,6 +1013,33 @@ int mmc_regulator_set_ocr(struct mmc_host *mmc,
>>> }
>>> EXPORT_SYMBOL(mmc_regulator_set_ocr);
>>> 
>>> +int mmc_regulator_get_supply(struct mmc_host *mmc, struct mmc_supply *s)
>>> +{
>>> +	struct device *dev = mmc_dev(mmc);
>>> +	struct regulator *supply;
>>> +	int ret;
>>> +
>>> +	if (!mmc || !s)
>>> +		return -EINVAL;
>>> +
>>> +	supply = devm_regulator_get(dev, "vmmc");
>>> +
>>> +	if (IS_ERR(supply))
>>> +		return PTR_ERR(supply);
>>> +
>>> +	s->vmmc = supply;
>>> +	s->vqmmc = devm_regulator_get(dev, "vqmmc");
>> 
>> Should we set vqmmc to NULL if an error was returned ?
> 
> Don't think so, drivers can perfectly check for IS_ERR() themselves.

sdhci.c checks for 

	if (host->vmmc) {
		regulator_disable(host->vmmc);
		regulator_put(host->vmmc);
	}

so better to set regulator to NULL if not used.
> 
>>> +
>>> +	ret = mmc_regulator_get_ocrmask(supply);
>>> +	if (ret > 0)
>>> +		mmc->ocr_avail = ret;
>>> +	else
>>> +		dev_warn(mmc_dev(mmc), "Failed getting OCR mask: %d\n", ret);
>> 
>> I was wondering if  sdhci (or the other SD Host drivers can work if no ocr_avail mask is set ?  
> 
> Yes, e.g., if it's a dummy regulator and the board actually doesn't need 
> one.
> 
>>> +
>>> +	return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(mmc_regulator_get_supply);
>>> +
>>> #endif /* CONFIG_REGULATOR */
>>> 
>>> /*
>>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>>> index 0707d22..3192335 100644
>>> --- a/include/linux/mmc/host.h
>>> +++ b/include/linux/mmc/host.h
>>> @@ -359,11 +359,17 @@ static inline void mmc_signal_sdio_irq(struct mmc_host *host)
>>> 
>>> struct regulator;
>>> 
>>> +struct mmc_supply {
>>> +	struct regulator *vmmc;		/* Card power supply */
>>> +	struct regulator *vqmmc;	/* Optional Vccq supply */
>>> +};
>>> +
>>> #ifdef CONFIG_REGULATOR
>>> int mmc_regulator_get_ocrmask(struct regulator *supply);
>>> int mmc_regulator_set_ocr(struct mmc_host *mmc,
>>> 			struct regulator *supply,
>>> 			unsigned short vdd_bit);
>>> +int mmc_regulator_get_supply(struct mmc_host *mmc, struct mmc_supply *s);
>>> #else
>>> static inline int mmc_regulator_get_ocrmask(struct regulator *supply)
>>> {
>>> @@ -376,6 +382,12 @@ static inline int mmc_regulator_set_ocr(struct mmc_host *mmc,
>>> {
>>> 	return 0;
>>> }
>>> +
>>> +static inline int mmc_regulator_get_supply(struct mmc_host *mmc,
>>> +					   struct mmc_supply *s)
>>> +{
>>> +	return 0;
>>> +}
>>> #endif
>>> 
>>> int mmc_card_awake(struct mmc_host *host);
>>> -- 
>>> 1.7.2.5
> 
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/

Re: [PATCH v4] mmc: add a function to get regulators, supplying card's power

[Guennadi Liakhovetski]

On Tue, 12 Jun 2012, Philip Rakity wrote:

> 
> On Jun 12, 2012, at 9:52 AM, Guennadi Liakhovetski wrote:
> 
> > On Tue, 12 Jun 2012, Philip Rakity wrote:
> > 
> >> 
> >> On Jun 12, 2012, at 8:57 AM, Guennadi Liakhovetski wrote:
> >> 
> >>> Add a function to get regulators, supplying card's Vdd and Vccq on a
> >>> specific host. If a Vdd supplying regulator is found, the function checks,
> >>> whether a valid OCR mask can be obtained from it. The Vccq regulator is
> >>> optional. A failure to get it is not fatal.
> >>> 
> >>> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> >>> ---
> >>> 
> >>> v4:
> >>> 
> >>> 1. add _GPL to EXPORT_SYMBOL (note: a recently posted patch changes 
> >>> another occurrence of EXPORT_SYMBOL too, so, this patch will apply with a 
> >>> fuzz, if merged after)
> >>> 
> >>> 2. add an optional vqmmc regulator, which also changes the function 
> >>> prototype
> >>> 
> >>> 3. print a warning, if failed to get an OCR mask
> >>> 
> >>> Thanks to all, who commented
> >>> 
> >>> drivers/mmc/core/core.c  |   27 +++++++++++++++++++++++++++
> >>> include/linux/mmc/host.h |   12 ++++++++++++
> >>> 2 files changed, 39 insertions(+), 0 deletions(-)
> >>> 
> >>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> >>> index 0b6141d..d77a238 100644
> >>> --- a/drivers/mmc/core/core.c
> >>> +++ b/drivers/mmc/core/core.c
> >>> @@ -1013,6 +1013,33 @@ int mmc_regulator_set_ocr(struct mmc_host *mmc,
> >>> }
> >>> EXPORT_SYMBOL(mmc_regulator_set_ocr);
> >>> 
> >>> +int mmc_regulator_get_supply(struct mmc_host *mmc, struct mmc_supply *s)
> >>> +{
> >>> +	struct device *dev = mmc_dev(mmc);
> >>> +	struct regulator *supply;
> >>> +	int ret;
> >>> +
> >>> +	if (!mmc || !s)
> >>> +		return -EINVAL;
> >>> +
> >>> +	supply = devm_regulator_get(dev, "vmmc");
> >>> +
> >>> +	if (IS_ERR(supply))
> >>> +		return PTR_ERR(supply);
> >>> +
> >>> +	s->vmmc = supply;
> >>> +	s->vqmmc = devm_regulator_get(dev, "vqmmc");
> >> 
> >> Should we set vqmmc to NULL if an error was returned ?
> > 
> > Don't think so, drivers can perfectly check for IS_ERR() themselves.
> 
> sdhci.c checks for 
> 
> 	if (host->vmmc) {
> 		regulator_disable(host->vmmc);
> 		regulator_put(host->vmmc);
> 	}
> 
> so better to set regulator to NULL if not used.

I'm not convinced. In fact, I think, it is always better in these cases to 
propagate regulator_get() return codes from any wrappers to the callers. 
So, it might be better for consistency to assign s->vmmc and s->vqmmc even 
in case of an error.

Although, on a second thought, no, I'd keep it as is. In case of a 
returned error you shouldn't make any assumptions about the contents of 
the struct from the function parameters. Only in the case of success all 
its usuaal fields should be filled in.

Thanks
Guennadi

> >>> +
> >>> +	ret = mmc_regulator_get_ocrmask(supply);
> >>> +	if (ret > 0)
> >>> +		mmc->ocr_avail = ret;
> >>> +	else
> >>> +		dev_warn(mmc_dev(mmc), "Failed getting OCR mask: %d\n", ret);
> >> 
> >> I was wondering if  sdhci (or the other SD Host drivers can work if no ocr_avail mask is set ?  
> > 
> > Yes, e.g., if it's a dummy regulator and the board actually doesn't need 
> > one.
> > 
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +EXPORT_SYMBOL_GPL(mmc_regulator_get_supply);
> >>> +
> >>> #endif /* CONFIG_REGULATOR */
> >>> 
> >>> /*
> >>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> >>> index 0707d22..3192335 100644
> >>> --- a/include/linux/mmc/host.h
> >>> +++ b/include/linux/mmc/host.h
> >>> @@ -359,11 +359,17 @@ static inline void mmc_signal_sdio_irq(struct mmc_host *host)
> >>> 
> >>> struct regulator;
> >>> 
> >>> +struct mmc_supply {
> >>> +	struct regulator *vmmc;		/* Card power supply */
> >>> +	struct regulator *vqmmc;	/* Optional Vccq supply */
> >>> +};
> >>> +
> >>> #ifdef CONFIG_REGULATOR
> >>> int mmc_regulator_get_ocrmask(struct regulator *supply);
> >>> int mmc_regulator_set_ocr(struct mmc_host *mmc,
> >>> 			struct regulator *supply,
> >>> 			unsigned short vdd_bit);
> >>> +int mmc_regulator_get_supply(struct mmc_host *mmc, struct mmc_supply *s);
> >>> #else
> >>> static inline int mmc_regulator_get_ocrmask(struct regulator *supply)
> >>> {
> >>> @@ -376,6 +382,12 @@ static inline int mmc_regulator_set_ocr(struct mmc_host *mmc,
> >>> {
> >>> 	return 0;
> >>> }
> >>> +
> >>> +static inline int mmc_regulator_get_supply(struct mmc_host *mmc,
> >>> +					   struct mmc_supply *s)
> >>> +{
> >>> +	return 0;
> >>> +}
> >>> #endif
> >>> 
> >>> int mmc_card_awake(struct mmc_host *host);
> >>> -- 
> >>> 1.7.2.5

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH v4] mmc: add a function to get regulators, supplying card's power

[Ulf Hansson]

Hi Guennadi,

On 06/12/2012 05:57 PM, Guennadi Liakhovetski wrote:
> Add a function to get regulators, supplying card's Vdd and Vccq on a
> specific host. If a Vdd supplying regulator is found, the function checks,
> whether a valid OCR mask can be obtained from it. The Vccq regulator is
> optional. A failure to get it is not fatal.
>
> Signed-off-by: Guennadi Liakhovetski<g.liakhovetski@gmx.de>
> ---
>
> v4:
>
> 1. add _GPL to EXPORT_SYMBOL (note: a recently posted patch changes
> another occurrence of EXPORT_SYMBOL too, so, this patch will apply with a
> fuzz, if merged after)
>
> 2. add an optional vqmmc regulator, which also changes the function
> prototype
>
> 3. print a warning, if failed to get an OCR mask
>
> Thanks to all, who commented
>
>   drivers/mmc/core/core.c  |   27 +++++++++++++++++++++++++++
>   include/linux/mmc/host.h |   12 ++++++++++++
>   2 files changed, 39 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 0b6141d..d77a238 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1013,6 +1013,33 @@ int mmc_regulator_set_ocr(struct mmc_host *mmc,
>   }
>   EXPORT_SYMBOL(mmc_regulator_set_ocr);
>
> +int mmc_regulator_get_supply(struct mmc_host *mmc, struct mmc_supply *s)
> +{
> +	struct device *dev = mmc_dev(mmc);
> +	struct regulator *supply;
> +	int ret;
> +
> +	if (!mmc || !s)
> +		return -EINVAL;
> +
> +	supply = devm_regulator_get(dev, "vmmc");
> +
> +	if (IS_ERR(supply))
> +		return PTR_ERR(supply);
> +
> +	s->vmmc = supply;
> +	s->vqmmc = devm_regulator_get(dev, "vqmmc");
> +
> +	ret = mmc_regulator_get_ocrmask(supply);
> +	if (ret>  0)
> +		mmc->ocr_avail = ret;
> +	else
> +		dev_warn(mmc_dev(mmc), "Failed getting OCR mask: %d\n", ret);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(mmc_regulator_get_supply);
> +
>   #endif /* CONFIG_REGULATOR */
>
>   /*
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 0707d22..3192335 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -359,11 +359,17 @@ static inline void mmc_signal_sdio_irq(struct mmc_host *host)
>
>   struct regulator;
>
> +struct mmc_supply {
> +	struct regulator *vmmc;		/* Card power supply */
> +	struct regulator *vqmmc;	/* Optional Vccq supply */
> +};

I believe your intention is to provide this functionality for the host 
drivers as the common way of handling card regulators. Then, I would 
suggest to include these two new regulators in the mmc_host struct, 
instead of having this in a separate struct, which then also needs to be 
handled by every host driver.

> +
>   #ifdef CONFIG_REGULATOR
>   int mmc_regulator_get_ocrmask(struct regulator *supply);
>   int mmc_regulator_set_ocr(struct mmc_host *mmc,
>   			struct regulator *supply,
>   			unsigned short vdd_bit);
> +int mmc_regulator_get_supply(struct mmc_host *mmc, struct mmc_supply *s);
>   #else
>   static inline int mmc_regulator_get_ocrmask(struct regulator *supply)
>   {
> @@ -376,6 +382,12 @@ static inline int mmc_regulator_set_ocr(struct mmc_host *mmc,
>   {
>   	return 0;
>   }
> +
> +static inline int mmc_regulator_get_supply(struct mmc_host *mmc,
> +					   struct mmc_supply *s)
> +{
> +	return 0;
> +}
>   #endif
>
>   int mmc_card_awake(struct mmc_host *host);

Kind regards
Ulf Hansson

Re: [PATCH v4] mmc: add a function to get regulators, supplying card's power

[Guennadi Liakhovetski]

Hi Ulf

On Wed, 13 Jun 2012, Ulf Hansson wrote:

> Hi Guennadi,
> 
> On 06/12/2012 05:57 PM, Guennadi Liakhovetski wrote:
> > Add a function to get regulators, supplying card's Vdd and Vccq on a
> > specific host. If a Vdd supplying regulator is found, the function checks,
> > whether a valid OCR mask can be obtained from it. The Vccq regulator is
> > optional. A failure to get it is not fatal.
> > 
> > Signed-off-by: Guennadi Liakhovetski<g.liakhovetski@gmx.de>
> > ---
> > 
> > v4:
> > 
> > 1. add _GPL to EXPORT_SYMBOL (note: a recently posted patch changes
> > another occurrence of EXPORT_SYMBOL too, so, this patch will apply with a
> > fuzz, if merged after)
> > 
> > 2. add an optional vqmmc regulator, which also changes the function
> > prototype
> > 
> > 3. print a warning, if failed to get an OCR mask
> > 
> > Thanks to all, who commented
> > 
> >   drivers/mmc/core/core.c  |   27 +++++++++++++++++++++++++++
> >   include/linux/mmc/host.h |   12 ++++++++++++
> >   2 files changed, 39 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> > index 0b6141d..d77a238 100644
> > --- a/drivers/mmc/core/core.c
> > +++ b/drivers/mmc/core/core.c
> > @@ -1013,6 +1013,33 @@ int mmc_regulator_set_ocr(struct mmc_host *mmc,
> >   }
> >   EXPORT_SYMBOL(mmc_regulator_set_ocr);
> > 
> > +int mmc_regulator_get_supply(struct mmc_host *mmc, struct mmc_supply *s)
> > +{
> > +	struct device *dev = mmc_dev(mmc);
> > +	struct regulator *supply;
> > +	int ret;
> > +
> > +	if (!mmc || !s)
> > +		return -EINVAL;
> > +
> > +	supply = devm_regulator_get(dev, "vmmc");
> > +
> > +	if (IS_ERR(supply))
> > +		return PTR_ERR(supply);
> > +
> > +	s->vmmc = supply;
> > +	s->vqmmc = devm_regulator_get(dev, "vqmmc");
> > +
> > +	ret = mmc_regulator_get_ocrmask(supply);
> > +	if (ret>  0)
> > +		mmc->ocr_avail = ret;
> > +	else
> > +		dev_warn(mmc_dev(mmc), "Failed getting OCR mask: %d\n", ret);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(mmc_regulator_get_supply);
> > +
> >   #endif /* CONFIG_REGULATOR */
> > 
> >   /*
> > diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> > index 0707d22..3192335 100644
> > --- a/include/linux/mmc/host.h
> > +++ b/include/linux/mmc/host.h
> > @@ -359,11 +359,17 @@ static inline void mmc_signal_sdio_irq(struct mmc_host
> > *host)
> > 
> >   struct regulator;
> > 
> > +struct mmc_supply {
> > +	struct regulator *vmmc;		/* Card power supply */
> > +	struct regulator *vqmmc;	/* Optional Vccq supply */
> > +};
> 
> I believe your intention is to provide this functionality for the host drivers
> as the common way of handling card regulators. Then, I would suggest to
> include these two new regulators in the mmc_host struct, instead of having
> this in a separate struct, which then also needs to be handled by every host
> driver.

I have no strong preference about this. Having an additional struct is how 
I interpreted Mark's proposal:

http://thread.gmane.org/gmane.linux.kernel.mmc/14624/focus=14876

but I'm also fine with putting it in mmc_host. Chris, what's your 
preference?

> 
> > +
> >   #ifdef CONFIG_REGULATOR
> >   int mmc_regulator_get_ocrmask(struct regulator *supply);
> >   int mmc_regulator_set_ocr(struct mmc_host *mmc,
> >   			struct regulator *supply,
> >   			unsigned short vdd_bit);
> > +int mmc_regulator_get_supply(struct mmc_host *mmc, struct mmc_supply *s);
> >   #else
> >   static inline int mmc_regulator_get_ocrmask(struct regulator *supply)
> >   {
> > @@ -376,6 +382,12 @@ static inline int mmc_regulator_set_ocr(struct mmc_host
> > *mmc,
> >   {
> >   	return 0;
> >   }
> > +
> > +static inline int mmc_regulator_get_supply(struct mmc_host *mmc,
> > +					   struct mmc_supply *s)
> > +{
> > +	return 0;
> > +}
> >   #endif
> > 
> >   int mmc_card_awake(struct mmc_host *host);

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH v4] mmc: add a function to get regulators, supplying card's power

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 430 bytes --]

On Wed, Jun 13, 2012 at 10:28:22AM +0200, Guennadi Liakhovetski wrote:

> I have no strong preference about this. Having an additional struct is how 
> I interpreted Mark's proposal:

> http://thread.gmane.org/gmane.linux.kernel.mmc/14624/focus=14876

> but I'm also fine with putting it in mmc_host. Chris, what's your 
> preference?

I'm just suggesting that because it seems like this is idiomatic for the
MMC API.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v4] mmc: add a function to get regulators, supplying card's power

[Chris Ball]

Hi,

On Wed, Jun 13 2012, Guennadi Liakhovetski wrote:
>> > +struct mmc_supply {
>> > +	struct regulator *vmmc;		/* Card power supply */
>> > +	struct regulator *vqmmc;	/* Optional Vccq supply */
>> > +};
>> 
>> I believe your intention is to provide this functionality for the host drivers
>> as the common way of handling card regulators. Then, I would suggest to
>> include these two new regulators in the mmc_host struct, instead of having
>> this in a separate struct, which then also needs to be handled by every host
>> driver.
>
> I have no strong preference about this. Having an additional struct is how 
> I interpreted Mark's proposal:
>
> http://thread.gmane.org/gmane.linux.kernel.mmc/14624/focus=14876
>
> but I'm also fine with putting it in mmc_host. Chris, what's your 
> preference?

I think Mark was just trying to help with your observation that the
changes are messy.  I don't see any compelling reasons to avoid adding
these to mmc_host -- does anyone else feel strongly?

So, I'd say go ahead and post an updated patch that uses mmc_host,
and we can see if Mark has any thoughts.

Thanks,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child