Re: [PATCH] spi: ti-qspi: improve ->remove() callback

[Grygorii Strashko]

On 11/02/2015 05:20 PM, Felipe Balbi wrote:
> Grygorii Strashko  writes:
> 
>> On 10/29/2015 03:57 PM, Felipe Balbi wrote:
>>> there's no need to call pm_runtime_get_sync()
>>> followed by pm_runtime_put(). We should, instead,
>>> just call pm_runtime_put_sync() and pm_runtime_disable().
>>
>> Sry, but why do we need to call pm_runtime_put[_sync]() here?
>>
>> My be just pm_runtime_disable() will be ok?
> 
> and disable with unbalanced pm_runtime_get() ?
> 

Which one is unbalanced pm_runtime_get()?
There are no  pm_runtime_get() in probe, so there you are
going to introduce unbalanced pm_runtime_put_sync() actually :(

-- 
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: ti-qspi: improve ->remove() callback

[Grygorii Strashko]

On 10/29/2015 03:57 PM, Felipe Balbi wrote:

there's no need to call pm_runtime_get_sync()
followed by pm_runtime_put(). We should, instead,
just call pm_runtime_put_sync() and pm_runtime_disable().


Sry, but why do we need to call pm_runtime_put[_sync]() here?

My be just pm_runtime_disable() will be ok?




Signed-off-by: Felipe Balbi 
---
  drivers/spi/spi-ti-qspi.c | 11 +--
  1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/spi/spi-ti-qspi.c b/drivers/spi/spi-ti-qspi.c
index 69c1a95b0615..64318fcfacf2 100644
--- a/drivers/spi/spi-ti-qspi.c
+++ b/drivers/spi/spi-ti-qspi.c
@@ -554,16 +554,7 @@ free_master:

  static int ti_qspi_remove(struct platform_device *pdev)
  {
-   struct ti_qspi *qspi = platform_get_drvdata(pdev);
-   int ret;
-
-   ret = pm_runtime_get_sync(qspi->dev);
-   if (ret < 0) {
-   dev_err(qspi->dev, "pm_runtime_get_sync() failed\n");
-   return ret;
-   }
-
-   pm_runtime_put(qspi->dev);
+   pm_runtime_put_sync(>dev);
pm_runtime_disable(>dev);

return 0;




--
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: ti-qspi: improve ->remove() callback

[Felipe Balbi]

Grygorii Strashko  writes:

> On 10/29/2015 03:57 PM, Felipe Balbi wrote:
>> there's no need to call pm_runtime_get_sync()
>> followed by pm_runtime_put(). We should, instead,
>> just call pm_runtime_put_sync() and pm_runtime_disable().
>
> Sry, but why do we need to call pm_runtime_put[_sync]() here?
>
> My be just pm_runtime_disable() will be ok?

and disable with unbalanced pm_runtime_get() ?

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] spi: ti-qspi: improve ->remove() callback

[Grygorii Strashko]

On 11/02/2015 06:06 PM, Felipe Balbi wrote:
> 
> hi,
> 
> Grygorii Strashko  writes:
>> On 11/02/2015 05:20 PM, Felipe Balbi wrote:
>>> Grygorii Strashko  writes:
>>>
 On 10/29/2015 03:57 PM, Felipe Balbi wrote:
> there's no need to call pm_runtime_get_sync()
> followed by pm_runtime_put(). We should, instead,
> just call pm_runtime_put_sync() and pm_runtime_disable().

 Sry, but why do we need to call pm_runtime_put[_sync]() here?

 My be just pm_runtime_disable() will be ok?
>>>
>>> and disable with unbalanced pm_runtime_get() ?
>>>
>>
>> Which one is unbalanced pm_runtime_get()?
>> There are no  pm_runtime_get() in probe, so there you are
>> going to introduce unbalanced pm_runtime_put_sync() actually :(
> 
> look at ti_qspi_setup(). I _do_ see, however, that it calls
> pm_runtime_put_autosuspend() in the same function; what happens if
> driver is removed after ti_qspi_setup() runs but before
> put_autosuspend() has time to actually run ?
> 

Seems nothing :) If I understand code in __device_release_driver() right:
the .remove() callback will be called after pm_runtime_put_sync() and
device should be disabled at this moment.

Also, note that ti_qspi_setup() will be called for each SPI device attached
to SPI master and further PM management of SPI master is performed by SPI core
from __spi_pump_messages().

-- 
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: ti-qspi: improve ->remove() callback

[Felipe Balbi]

Hi,

Grygorii Strashko  writes:
> On 11/02/2015 06:06 PM, Felipe Balbi wrote:
>> 
>> hi,
>> 
>> Grygorii Strashko  writes:
>>> On 11/02/2015 05:20 PM, Felipe Balbi wrote:
 Grygorii Strashko  writes:

> On 10/29/2015 03:57 PM, Felipe Balbi wrote:
>> there's no need to call pm_runtime_get_sync()
>> followed by pm_runtime_put(). We should, instead,
>> just call pm_runtime_put_sync() and pm_runtime_disable().
>
> Sry, but why do we need to call pm_runtime_put[_sync]() here?
>
> My be just pm_runtime_disable() will be ok?

 and disable with unbalanced pm_runtime_get() ?

>>>
>>> Which one is unbalanced pm_runtime_get()?
>>> There are no  pm_runtime_get() in probe, so there you are
>>> going to introduce unbalanced pm_runtime_put_sync() actually :(
>> 
>> look at ti_qspi_setup(). I _do_ see, however, that it calls
>> pm_runtime_put_autosuspend() in the same function; what happens if
>> driver is removed after ti_qspi_setup() runs but before
>> put_autosuspend() has time to actually run ?
>> 
>
> Seems nothing :) If I understand code in __device_release_driver()
> right: the .remove() callback will be called after
> pm_runtime_put_sync() and device should be disabled at this moment.
>
> Also, note that ti_qspi_setup() will be called for each SPI device
> attached to SPI master and further PM management of SPI master is
> performed by SPI core from __spi_pump_messages().

all right, do you want to send a patch fixing it ?

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] spi: ti-qspi: improve ->remove() callback

[Felipe Balbi]

hi,

Grygorii Strashko  writes:
> On 11/02/2015 05:20 PM, Felipe Balbi wrote:
>> Grygorii Strashko  writes:
>> 
>>> On 10/29/2015 03:57 PM, Felipe Balbi wrote:
 there's no need to call pm_runtime_get_sync()
 followed by pm_runtime_put(). We should, instead,
 just call pm_runtime_put_sync() and pm_runtime_disable().
>>>
>>> Sry, but why do we need to call pm_runtime_put[_sync]() here?
>>>
>>> My be just pm_runtime_disable() will be ok?
>> 
>> and disable with unbalanced pm_runtime_get() ?
>> 
>
> Which one is unbalanced pm_runtime_get()?
> There are no  pm_runtime_get() in probe, so there you are
> going to introduce unbalanced pm_runtime_put_sync() actually :(

look at ti_qspi_setup(). I _do_ see, however, that it calls
pm_runtime_put_autosuspend() in the same function; what happens if
driver is removed after ti_qspi_setup() runs but before
put_autosuspend() has time to actually run ?

-- 
balbi


signature.asc
Description: PGP signature

[PATCH] spi: ti-qspi: improve ->remove() callback

[Felipe Balbi]

there's no need to call pm_runtime_get_sync()
followed by pm_runtime_put(). We should, instead,
just call pm_runtime_put_sync() and pm_runtime_disable().

Signed-off-by: Felipe Balbi 
---
 drivers/spi/spi-ti-qspi.c | 11 +--
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/spi/spi-ti-qspi.c b/drivers/spi/spi-ti-qspi.c
index 69c1a95b0615..64318fcfacf2 100644
--- a/drivers/spi/spi-ti-qspi.c
+++ b/drivers/spi/spi-ti-qspi.c
@@ -554,16 +554,7 @@ free_master:
 
 static int ti_qspi_remove(struct platform_device *pdev)
 {
-   struct ti_qspi *qspi = platform_get_drvdata(pdev);
-   int ret;
-
-   ret = pm_runtime_get_sync(qspi->dev);
-   if (ret < 0) {
-   dev_err(qspi->dev, "pm_runtime_get_sync() failed\n");
-   return ret;
-   }
-
-   pm_runtime_put(qspi->dev);
+   pm_runtime_put_sync(>dev);
pm_runtime_disable(>dev);
 
return 0;
-- 
2.6.2

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