[PATCH 0/2] Added set_power handler to mmc sdhci host

[PATCH 0/2] Added set_power handler to mmc sdhci host

[Felipe F. Tonello]

From: "Felipe F. Tonello" <eu@felipetonello.com>

This set_power handler is useful for modules that need to be turned off when
the mmc host turns off.

Felipe F. Tonello (2):
  mmc: sdhci: Added set_power sdhci_ops handler.
  mmc: sdhci-s3c: Added set_power handler to platdata

 drivers/mmc/host/sdhci-s3c.c                | 8 ++++++++
 drivers/mmc/host/sdhci.c                    | 8 ++++++++
 drivers/mmc/host/sdhci.h                    | 1 +
 include/linux/platform_data/mmc-sdhci-s3c.h | 1 +
 4 files changed, 18 insertions(+)

-- 
1.8.1.4

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

[Felipe F. Tonello]

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(+)

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

[PATCH 2/2] mmc: sdhci-s3c: Added set_power handler to platdata

[Felipe F. Tonello]

From: "Felipe F. Tonello" <eu@felipetonello.com>

This is useful to turn off peripherals that are related to the mmc host. One
common case is when the wifi module is connected as an mmc card to the host.

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

diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c
index c6f6246..f7e740c 100644
--- a/drivers/mmc/host/sdhci-s3c.c
+++ b/drivers/mmc/host/sdhci-s3c.c
@@ -360,11 +360,19 @@ static int sdhci_s3c_platform_bus_width(struct sdhci_host *host, int width)
 	return 0;
 }
 
+static void sdhci_s3c_set_power(struct sdhci_host *host, bool power)
+{
+	struct sdhci_s3c *ourhost = to_s3c(host);
+	if (ourhost->pdata->set_power)
+		ourhost->pdata->set_power(power);
+}
+
 static struct sdhci_ops sdhci_s3c_ops = {
 	.get_max_clock		= sdhci_s3c_get_max_clk,
 	.set_clock		= sdhci_s3c_set_clock,
 	.get_min_clock		= sdhci_s3c_get_min_clock,
 	.platform_bus_width	= sdhci_s3c_platform_bus_width,
+	.set_power		= sdhci_s3c_set_power,
 };
 
 static void sdhci_s3c_notify_change(struct platform_device *dev, int state)
diff --git a/include/linux/platform_data/mmc-sdhci-s3c.h b/include/linux/platform_data/mmc-sdhci-s3c.h
index 249f023..55be925 100644
--- a/include/linux/platform_data/mmc-sdhci-s3c.h
+++ b/include/linux/platform_data/mmc-sdhci-s3c.h
@@ -50,6 +50,7 @@ struct s3c_sdhci_platdata {
 						      int state));
 
 	void	(*cfg_gpio)(struct platform_device *dev, int width);
+	void	(*set_power)(bool power);
 };
 
 
-- 
1.8.1.4

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

[Guennadi Liakhovetski]

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/

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

[Felipe Ferreri Tonello]

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/

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

[Guennadi Liakhovetski]

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/

Re: [PATCH 2/2] mmc: sdhci-s3c: Added set_power handler to platdata

[Jaehoon Chung]

Hi Felipe,

I didn't understand this patch, why need to add set_power?
We can use to control the power with the fixed regulator.
Then we can also use the regulator framework.
And i know also control the module like wifi with rfkill.
In set_power, what is it controlled?

Best Regards,
Jaehoon Chung
On 05/23/2013 02:47 AM, Felipe F. Tonello wrote:
> From: "Felipe F. Tonello" <eu@felipetonello.com>
> 
> This is useful to turn off peripherals that are related to the mmc host. One
> common case is when the wifi module is connected as an mmc card to the host.
> 
> Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
> ---
>  drivers/mmc/host/sdhci-s3c.c                | 8 ++++++++
>  include/linux/platform_data/mmc-sdhci-s3c.h | 1 +
>  2 files changed, 9 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c
> index c6f6246..f7e740c 100644
> --- a/drivers/mmc/host/sdhci-s3c.c
> +++ b/drivers/mmc/host/sdhci-s3c.c
> @@ -360,11 +360,19 @@ static int sdhci_s3c_platform_bus_width(struct sdhci_host *host, int width)
>  	return 0;
>  }
>  
> +static void sdhci_s3c_set_power(struct sdhci_host *host, bool power)
> +{
> +	struct sdhci_s3c *ourhost = to_s3c(host);
> +	if (ourhost->pdata->set_power)
> +		ourhost->pdata->set_power(power);
> +}
> +
>  static struct sdhci_ops sdhci_s3c_ops = {
>  	.get_max_clock		= sdhci_s3c_get_max_clk,
>  	.set_clock		= sdhci_s3c_set_clock,
>  	.get_min_clock		= sdhci_s3c_get_min_clock,
>  	.platform_bus_width	= sdhci_s3c_platform_bus_width,
> +	.set_power		= sdhci_s3c_set_power,
>  };
>  
>  static void sdhci_s3c_notify_change(struct platform_device *dev, int state)
> diff --git a/include/linux/platform_data/mmc-sdhci-s3c.h b/include/linux/platform_data/mmc-sdhci-s3c.h
> index 249f023..55be925 100644
> --- a/include/linux/platform_data/mmc-sdhci-s3c.h
> +++ b/include/linux/platform_data/mmc-sdhci-s3c.h
> @@ -50,6 +50,7 @@ struct s3c_sdhci_platdata {
>  						      int state));
>  
>  	void	(*cfg_gpio)(struct platform_device *dev, int width);
> +	void	(*set_power)(bool power);
>  };
>  
>  
> 

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

[Jaehoon Chung]

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
> 

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

[Felipe Tonello]

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
>>
>