[PATCH v5 07/11] mmc: sdio: Convert to dev_pm_domain_attach|detach()

[PATCH v5 07/11] mmc: sdio: Convert to dev_pm_domain_attach|detach()

[Ulf Hansson]

Previously only the ACPI PM domain was supported by the sdio bus.

Let's convert to the common attach/detach functions for PM domains,
which currently means we are extending the support to include the
generic PM domain as well.

Cc: linux-mmc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Reviewed-by: Kevin Hilman <khilman-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/mmc/core/sdio_bus.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c
index 4fa8fef9..1df0fc6 100644
--- a/drivers/mmc/core/sdio_bus.c
+++ b/drivers/mmc/core/sdio_bus.c
@@ -315,7 +315,7 @@ int sdio_add_func(struct sdio_func *func)
 	ret = device_add(&func->dev);
 	if (ret == 0) {
 		sdio_func_set_present(func);
-		acpi_dev_pm_attach(&func->dev, false);
+		dev_pm_domain_attach(&func->dev, false);
 	}
 
 	return ret;
@@ -332,7 +332,7 @@ void sdio_remove_func(struct sdio_func *func)
 	if (!sdio_func_present(func))
 		return;
 
-	acpi_dev_pm_detach(&func->dev, false);
+	dev_pm_domain_detach(&func->dev, false);
 	device_del(&func->dev);
 	put_device(&func->dev);
 }
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 07/11] mmc: sdio: Convert to dev_pm_domain_attach|detach()

[Dmitry Torokhov]

Hi Ulf,

On Fri, Sep 19, 2014 at 08:27:40PM +0200, Ulf Hansson wrote:
> Previously only the ACPI PM domain was supported by the sdio bus.
> 
> Let's convert to the common attach/detach functions for PM domains,
> which currently means we are extending the support to include the
> generic PM domain as well.
> 
> Cc: linux-mmc@vger.kernel.org
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> Reviewed-by: Kevin Hilman <khilman@linaro.org>
> ---
>  drivers/mmc/core/sdio_bus.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c
> index 4fa8fef9..1df0fc6 100644
> --- a/drivers/mmc/core/sdio_bus.c
> +++ b/drivers/mmc/core/sdio_bus.c
> @@ -315,7 +315,7 @@ int sdio_add_func(struct sdio_func *func)
>  	ret = device_add(&func->dev);
>  	if (ret == 0) {
>  		sdio_func_set_present(func);
> -		acpi_dev_pm_attach(&func->dev, false);
> +		dev_pm_domain_attach(&func->dev, false);

Admittedly it is not brought in by your change, but I am a bit worried
about this code. In all other busses we attach power domain to a device
before probing it (and detach if probe fails). Why here we only attach
it to power domain after adding the device to the bus? If driver for the
function has already been loaded the probe() would run without device
being attached to a power domain...

Adding Aaron as he's the author of the original change.

Thanks.

-- 
Dmitry

Re: [PATCH v5 07/11] mmc: sdio: Convert to dev_pm_domain_attach|detach()

[Aaron Lu]

On 10/02/2014 08:27 AM, Dmitry Torokhov wrote:
> Hi Ulf,
> 
> On Fri, Sep 19, 2014 at 08:27:40PM +0200, Ulf Hansson wrote:
>> Previously only the ACPI PM domain was supported by the sdio bus.
>>
>> Let's convert to the common attach/detach functions for PM domains,
>> which currently means we are extending the support to include the
>> generic PM domain as well.
>>
>> Cc: linux-mmc@vger.kernel.org
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> Reviewed-by: Kevin Hilman <khilman@linaro.org>
>> ---
>>  drivers/mmc/core/sdio_bus.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c
>> index 4fa8fef9..1df0fc6 100644
>> --- a/drivers/mmc/core/sdio_bus.c
>> +++ b/drivers/mmc/core/sdio_bus.c
>> @@ -315,7 +315,7 @@ int sdio_add_func(struct sdio_func *func)
>>  	ret = device_add(&func->dev);
>>  	if (ret == 0) {
>>  		sdio_func_set_present(func);
>> -		acpi_dev_pm_attach(&func->dev, false);
>> +		dev_pm_domain_attach(&func->dev, false);
> 
> Admittedly it is not brought in by your change, but I am a bit worried
> about this code. In all other busses we attach power domain to a device
> before probing it (and detach if probe fails). Why here we only attach
> it to power domain after adding the device to the bus? If driver for the
> function has already been loaded the probe() would run without device
> being attached to a power domain...

Sorry for replying late...

I see your concern, but it should be safe here. The dev_pm_domain_attach
does primarily two things(for ACPI based system): assign the pm_domian
field so that later system and runtime PM transitions we have proper
callbacks for this device; power on the device if needed.

I was using Broadcom BCM43241 SDIO wifi card when developing the code,
it doesn't need the pm_domain set in its probe function and since the
SDIO wifi card is powered on after system boot, no power on is needed
either. That's why I added the attach function after device_add to save
a little code(detach if probe fails).

Feel free to change the sequence if the current one is a bad one, I'm OK
with either case.

> 
> Adding Aaron as he's the author of the original change.

Thanks,
Aaron

Re: [PATCH v5 07/11] mmc: sdio: Convert to dev_pm_domain_attach|detach()

[Ulf Hansson]

On 13 October 2014 04:48, Aaron Lu <aaron.lu@intel.com> wrote:
> On 10/02/2014 08:27 AM, Dmitry Torokhov wrote:
>> Hi Ulf,
>>
>> On Fri, Sep 19, 2014 at 08:27:40PM +0200, Ulf Hansson wrote:
>>> Previously only the ACPI PM domain was supported by the sdio bus.
>>>
>>> Let's convert to the common attach/detach functions for PM domains,
>>> which currently means we are extending the support to include the
>>> generic PM domain as well.
>>>
>>> Cc: linux-mmc@vger.kernel.org
>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> Reviewed-by: Kevin Hilman <khilman@linaro.org>
>>> ---
>>>  drivers/mmc/core/sdio_bus.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c
>>> index 4fa8fef9..1df0fc6 100644
>>> --- a/drivers/mmc/core/sdio_bus.c
>>> +++ b/drivers/mmc/core/sdio_bus.c
>>> @@ -315,7 +315,7 @@ int sdio_add_func(struct sdio_func *func)
>>>      ret = device_add(&func->dev);
>>>      if (ret == 0) {
>>>              sdio_func_set_present(func);
>>> -            acpi_dev_pm_attach(&func->dev, false);
>>> +            dev_pm_domain_attach(&func->dev, false);
>>
>> Admittedly it is not brought in by your change, but I am a bit worried
>> about this code. In all other busses we attach power domain to a device
>> before probing it (and detach if probe fails). Why here we only attach
>> it to power domain after adding the device to the bus? If driver for the
>> function has already been loaded the probe() would run without device
>> being attached to a power domain...
>
> Sorry for replying late...
>
> I see your concern, but it should be safe here. The dev_pm_domain_attach
> does primarily two things(for ACPI based system): assign the pm_domian
> field so that later system and runtime PM transitions we have proper
> callbacks for this device; power on the device if needed.
>
> I was using Broadcom BCM43241 SDIO wifi card when developing the code,
> it doesn't need the pm_domain set in its probe function and since the
> SDIO wifi card is powered on after system boot, no power on is needed
> either. That's why I added the attach function after device_add to save
> a little code(detach if probe fails).
>
> Feel free to change the sequence if the current one is a bad one, I'm OK
> with either case.

Thanks for clarifying this. I plan to change the behaviour according
to how other buses handles it.

Though, the patch will be a part another patchset that handles a
related initialization problem for the generic PM domain. I will keep
you guys on cc when I post it.

Kind regards
Uffe