diff mbox

[1/2] mmc: sdhci: Added set_power sdhci_ops handler.

Message ID 1369244832-23868-2-git-send-email-eu@felipetonello.com (mailing list archive)
State New, archived
Headers show

Commit Message

Felipe F. Tonello May 22, 2013, 5:47 p.m. UTC 
From: "Felipe F. Tonello" <eu@felipetonello.com>

This is useful for power managment purposes if a sdhci child host wants to
turn off some other peripheral also.

Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
---
 drivers/mmc/host/sdhci.c | 8 ++++++++
 drivers/mmc/host/sdhci.h | 1 +
 2 files changed, 9 insertions(+)

Comments

Guennadi Liakhovetski May 22, 2013, 8:30 p.m. UTC | #1 
On Wed, 22 May 2013, Felipe F. Tonello wrote:

> From: "Felipe F. Tonello" <eu@felipetonello.com>
> 
> This is useful for power managment purposes if a sdhci child host wants to
> turn off some other peripheral also.

Sorry, could you elaborate a bit? In what situations is it exactly useful? 
And why cannot the regulator API be used there?

Thanks
Guennadi

> 
> Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
> ---
>  drivers/mmc/host/sdhci.c | 8 ++++++++
>  drivers/mmc/host/sdhci.h | 1 +
>  2 files changed, 9 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 2ea429c..0a026c6 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1244,6 +1244,10 @@ static int sdhci_set_power(struct sdhci_host *host, unsigned short power)
>  	u8 pwr = 0;
>  
>  	if (power != (unsigned short)-1) {
> +
> +		if (host->ops->set_power)
> +			host->ops->set_power(host, true);
> +
>  		switch (1 << power) {
>  		case MMC_VDD_165_195:
>  			pwr = SDHCI_POWER_180;
> @@ -1259,6 +1263,10 @@ static int sdhci_set_power(struct sdhci_host *host, unsigned short power)
>  		default:
>  			BUG();
>  		}
> +	} else {
> +
> +		if (host->ops->set_power)
> +			host->ops->set_power(host, false);
>  	}
>  
>  	if (host->pwr == pwr)
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index 379e09d..293d56d 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -294,6 +294,7 @@ struct sdhci_ops {
>  	void	(*platform_resume)(struct sdhci_host *host);
>  	void    (*adma_workaround)(struct sdhci_host *host, u32 intmask);
>  	void	(*platform_init)(struct sdhci_host *host);
> +	void	(*set_power)(struct sdhci_host *host, bool power);
>  };
>  
>  #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
> -- 
> 1.8.1.4
> 
> --
> 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
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
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
Felipe F. Tonello May 22, 2013, 9:22 p.m. UTC | #2 
Hi Guennadi,

On Wednesday, May 22, 2013 10:30:40 PM Guennadi Liakhovetski wrote:
> On Wed, 22 May 2013, Felipe F. Tonello wrote:
> > From: "Felipe F. Tonello" <eu@felipetonello.com>
> > 
> > This is useful for power managment purposes if a sdhci child host wants to
> > turn off some other peripheral also.
> 
> Sorry, could you elaborate a bit? In what situations is it exactly useful?
> And why cannot the regulator API be used there?

Sorry about that.

One example that I can think of is when you have a wifi module connected as a 
mmc card via sdio. So you can register a callback function in your machine 
source code to turn on/off the wifi module based on the mmc host power.

I've seen this implementation in others mmc hosts, such as omap.

Regards,

Felipe

> 
> Thanks
> Guennadi
> 
> > Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
> > ---
> > 
> >  drivers/mmc/host/sdhci.c | 8 ++++++++
> >  drivers/mmc/host/sdhci.h | 1 +
> >  2 files changed, 9 insertions(+)
> > 
> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> > index 2ea429c..0a026c6 100644
> > --- a/drivers/mmc/host/sdhci.c
> > +++ b/drivers/mmc/host/sdhci.c
> > @@ -1244,6 +1244,10 @@ static int sdhci_set_power(struct sdhci_host *host,
> > unsigned short power)> 
> >  	u8 pwr = 0;
> >  	
> >  	if (power != (unsigned short)-1) {
> > 
> > +
> > +		if (host->ops->set_power)
> > +			host->ops->set_power(host, true);
> > +
> > 
> >  		switch (1 << power) {
> >  		
> >  		case MMC_VDD_165_195:
> >  			pwr = SDHCI_POWER_180;
> > 
> > @@ -1259,6 +1263,10 @@ static int sdhci_set_power(struct sdhci_host *host,
> > unsigned short power)> 
> >  		default:
> >  			BUG();
> >  		
> >  		}
> > 
> > +	} else {
> > +
> > +		if (host->ops->set_power)
> > +			host->ops->set_power(host, false);
> > 
> >  	}
> >  	
> >  	if (host->pwr == pwr)
> > 
> > diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> > index 379e09d..293d56d 100644
> > --- a/drivers/mmc/host/sdhci.h
> > +++ b/drivers/mmc/host/sdhci.h
> > @@ -294,6 +294,7 @@ struct sdhci_ops {
> > 
> >  	void	(*platform_resume)(struct sdhci_host *host);
> >  	void    (*adma_workaround)(struct sdhci_host *host, u32 intmask);
> >  	void	(*platform_init)(struct sdhci_host *host);
> > 
> > +	void	(*set_power)(struct sdhci_host *host, bool power);
> > 
> >  };
> >  
> >  #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
> 
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
--
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
Guennadi Liakhovetski May 23, 2013, 7:25 a.m. UTC | #3 
On Wed, 22 May 2013, Felipe Ferreri Tonello wrote:

> Hi Guennadi,
> 
> On Wednesday, May 22, 2013 10:30:40 PM Guennadi Liakhovetski wrote:
> > On Wed, 22 May 2013, Felipe F. Tonello wrote:
> > > From: "Felipe F. Tonello" <eu@felipetonello.com>
> > > 
> > > This is useful for power managment purposes if a sdhci child host wants to
> > > turn off some other peripheral also.
> > 
> > Sorry, could you elaborate a bit? In what situations is it exactly useful?
> > And why cannot the regulator API be used there?
> 
> Sorry about that.
> 
> One example that I can think of is when you have a wifi module connected as a 
> mmc card via sdio. So you can register a callback function in your machine 
> source code to turn on/off the wifi module based on the mmc host power.

Ok, understand. Your second patch in this series adds such a callback in 
your SDHCI host driver and there it just calls a platform callback. I 
don't think this is a good idea. First, we want to go away from platform 
callbacks, because they are incompatible with DT. Second, because the 
proper solution IMHO would be for your platform to export a regulator, and 
the SDHCI core driver already includes regulator support.

> I've seen this implementation in others mmc hosts, such as omap.

Which, however, doesn't yet mean, it's a good idea :)

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
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
Jaehoon Chung May 24, 2013, 4:02 a.m. UTC | #4 
On 05/23/2013 04:25 PM, Guennadi Liakhovetski wrote:
> On Wed, 22 May 2013, Felipe Ferreri Tonello wrote:
> 
>> Hi Guennadi,
>>
>> On Wednesday, May 22, 2013 10:30:40 PM Guennadi Liakhovetski wrote:
>>> On Wed, 22 May 2013, Felipe F. Tonello wrote:
>>>> From: "Felipe F. Tonello" <eu@felipetonello.com>
>>>>
>>>> This is useful for power managment purposes if a sdhci child host wants to
>>>> turn off some other peripheral also.
>>>
>>> Sorry, could you elaborate a bit? In what situations is it exactly useful?
>>> And why cannot the regulator API be used there?
>>
>> Sorry about that.
>>
>> One example that I can think of is when you have a wifi module connected as a 
>> mmc card via sdio. So you can register a callback function in your machine 
>> source code to turn on/off the wifi module based on the mmc host power.
> 
> Ok, understand. Your second patch in this series adds such a callback in 
> your SDHCI host driver and there it just calls a platform callback. I 
> don't think this is a good idea. First, we want to go away from platform 
> callbacks, because they are incompatible with DT. Second, because the 
> proper solution IMHO would be for your platform to export a regulator, and 
> the SDHCI core driver already includes regulator support.
We can use the regulator framework.
i think this callback function didn't need.

Best Regards,
Jaehoon Chung
> 
>> I've seen this implementation in others mmc hosts, such as omap.
> 
> Which, however, doesn't yet mean, it's a good idea :)
> 
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
> --
> 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
> 

--
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
Felipe F. Tonello May 24, 2013, 10:12 p.m. UTC | #5 
Hi all,

On Thu, May 23, 2013 at 9:02 PM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
> On 05/23/2013 04:25 PM, Guennadi Liakhovetski wrote:
>> On Wed, 22 May 2013, Felipe Ferreri Tonello wrote:
>>
>>> Hi Guennadi,
>>>
>>> On Wednesday, May 22, 2013 10:30:40 PM Guennadi Liakhovetski wrote:
>>>> On Wed, 22 May 2013, Felipe F. Tonello wrote:
>>>>> From: "Felipe F. Tonello" <eu@felipetonello.com>
>>>>>
>>>>> This is useful for power managment purposes if a sdhci child host wants to
>>>>> turn off some other peripheral also.
>>>>
>>>> Sorry, could you elaborate a bit? In what situations is it exactly useful?
>>>> And why cannot the regulator API be used there?
>>>
>>> Sorry about that.
>>>
>>> One example that I can think of is when you have a wifi module connected as a
>>> mmc card via sdio. So you can register a callback function in your machine
>>> source code to turn on/off the wifi module based on the mmc host power.
>>
>> Ok, understand. Your second patch in this series adds such a callback in
>> your SDHCI host driver and there it just calls a platform callback. I
>> don't think this is a good idea. First, we want to go away from platform
>> callbacks, because they are incompatible with DT. Second, because the
>> proper solution IMHO would be for your platform to export a regulator, and
>> the SDHCI core driver already includes regulator support.
> We can use the regulator framework.
> i think this callback function didn't need.
>

Ok, thanks for the feed back. Just to get things clear, is this
implementation using regulator framework already done?

BR,

Felipe

> Best Regards,
> Jaehoon Chung
>>
>>> I've seen this implementation in others mmc hosts, such as omap.
>>
>> Which, however, doesn't yet mean, it's a good idea :)
>>
>> Thanks
>> Guennadi
>> ---
>> Guennadi Liakhovetski, Ph.D.
>> Freelance Open-Source Software Developer
>> http://www.open-technology.de/
>> --
>> 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
>>
>
--
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
diff mbox

Patch

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 2ea429c..0a026c6 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1244,6 +1244,10 @@  static int sdhci_set_power(struct sdhci_host *host, unsigned short power)
 	u8 pwr = 0;
 
 	if (power != (unsigned short)-1) {
+
+		if (host->ops->set_power)
+			host->ops->set_power(host, true);
+
 		switch (1 << power) {
 		case MMC_VDD_165_195:
 			pwr = SDHCI_POWER_180;
@@ -1259,6 +1263,10 @@  static int sdhci_set_power(struct sdhci_host *host, unsigned short power)
 		default:
 			BUG();
 		}
+	} else {
+
+		if (host->ops->set_power)
+			host->ops->set_power(host, false);
 	}
 
 	if (host->pwr == pwr)
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 379e09d..293d56d 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -294,6 +294,7 @@  struct sdhci_ops {
 	void	(*platform_resume)(struct sdhci_host *host);
 	void    (*adma_workaround)(struct sdhci_host *host, u32 intmask);
 	void	(*platform_init)(struct sdhci_host *host);
+	void	(*set_power)(struct sdhci_host *host, bool power);
 };
 
 #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS