Re: [PATCH v3 2/3] mmc: Add realtek USB sdmmc host driver
On 02/10/2014 10:58 PM, Ulf Hansson wrote:
On 6 February 2014 15:35, rogera...@realtek.com wrote:
From: Roger Tseng rogera...@realtek.com
Realtek USB SD/MMC host driver provides mmc host support based on the Realtek
USB card reader MFD driver.
Signed-off-by: Roger Tseng rogera...@realtek.com
---
drivers/mmc/host/Kconfig |7 +
drivers/mmc/host/Makefile |1 +
drivers/mmc/host/rtsx_usb_sdmmc.c | 1500 +
3 files changed, 1508 insertions(+)
create mode 100644 drivers/mmc/host/rtsx_usb_sdmmc.c
[snip]
+#ifdef CONFIG_PM_RUNTIME
There are stubs for pm_runtime* functions, thus the ifdefs can be removed.
Please go though the complete patch and remove all instances.
+ pm_runtime_put(sdmmc_dev(host));
I don't know so much about USB mmc hosts hardware, but I just wanted
to find out if I have understood this correct.
You can't do fine grained power management of the USB parent device,
since it needs to be runtime resumed to be able keep the power the
card? Once it becomes runtime suspended, the power to the card will
thus also be dropped?
Yes, and to keep some internal state of the controller.
[snip]
+#ifdef CONFIG_PM
I suppose this should be CONFIG_PM_SLEEP?
...
+ err = mmc_suspend_host(mmc);
This won't compile. The mmc_suspend_host API has been removed.
...
+ return mmc_resume_host(mmc);
This won't compile. The mmc_resume_host API has been removed.
...
+static struct platform_driver rtsx_usb_sdmmc_driver = {
+ .probe = rtsx_usb_sdmmc_drv_probe,
+ .remove = rtsx_usb_sdmmc_drv_remove,
+ .id_table = rtsx_usb_sdmmc_ids,
+ .suspend= rtsx_usb_sdmmc_suspend,
+ .resume = rtsx_usb_sdmmc_resume,
Please use the modern pm_ops instead of the legacy suspend/resume callbacks.
I suggest you then also switch to use the SIMPLE_DEV_PM_OPS macro.
I just missed the removal of mmc_suspend|resume_host.
I'll remove all these unnecessary mmc host pm things as done in commit
ff71c4bcb0af2730d047989e485303ae4e1ce794 for drivers of our PCIe devices.
Thanks for pointing this out.
Kind regards
Ulf Hansson
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 2/3] mmc: Add realtek USB sdmmc host driver
On 11 February 2014 10:27, Roger rogera...@realtek.com wrote:
On 02/10/2014 10:58 PM, Ulf Hansson wrote:
On 6 February 2014 15:35, rogera...@realtek.com wrote:
From: Roger Tseng rogera...@realtek.com
Realtek USB SD/MMC host driver provides mmc host support based on the
Realtek
USB card reader MFD driver.
Signed-off-by: Roger Tseng rogera...@realtek.com
---
drivers/mmc/host/Kconfig |7 +
drivers/mmc/host/Makefile |1 +
drivers/mmc/host/rtsx_usb_sdmmc.c | 1500
+
3 files changed, 1508 insertions(+)
create mode 100644 drivers/mmc/host/rtsx_usb_sdmmc.c
[snip]
+#ifdef CONFIG_PM_RUNTIME
There are stubs for pm_runtime* functions, thus the ifdefs can be removed.
Please go though the complete patch and remove all instances.
+ pm_runtime_put(sdmmc_dev(host));
I don't know so much about USB mmc hosts hardware, but I just wanted
to find out if I have understood this correct.
You can't do fine grained power management of the USB parent device,
since it needs to be runtime resumed to be able keep the power the
card? Once it becomes runtime suspended, the power to the card will
thus also be dropped?
Yes, and to keep some internal state of the controller.
Okay.
But the internal state of the controller should be possible to restore
at runtime_resume, so that should not be the reason, right?
[snip]
+#ifdef CONFIG_PM
I suppose this should be CONFIG_PM_SLEEP?
...
+ err = mmc_suspend_host(mmc);
This won't compile. The mmc_suspend_host API has been removed.
...
+ return mmc_resume_host(mmc);
This won't compile. The mmc_resume_host API has been removed.
...
+static struct platform_driver rtsx_usb_sdmmc_driver = {
+ .probe = rtsx_usb_sdmmc_drv_probe,
+ .remove = rtsx_usb_sdmmc_drv_remove,
+ .id_table = rtsx_usb_sdmmc_ids,
+ .suspend= rtsx_usb_sdmmc_suspend,
+ .resume = rtsx_usb_sdmmc_resume,
Please use the modern pm_ops instead of the legacy suspend/resume
callbacks.
I suggest you then also switch to use the SIMPLE_DEV_PM_OPS macro.
I just missed the removal of mmc_suspend|resume_host.
I'll remove all these unnecessary mmc host pm things as done in commit
ff71c4bcb0af2730d047989e485303ae4e1ce794 for drivers of our PCIe devices.
Thanks for pointing this out.
Kind regards
Ulf Hansson
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 2/3] mmc: Add realtek USB sdmmc host driver
On 02/11/2014 05:50 PM, Ulf Hansson wrote: On 11 February 2014 10:27, Roger rogera...@realtek.com wrote: On 02/10/2014 10:58 PM, Ulf Hansson wrote: On 6 February 2014 15:35, rogera...@realtek.com wrote: From: Roger Tseng rogera...@realtek.com Realtek USB SD/MMC host driver provides mmc host support based on the Realtek USB card reader MFD driver. Signed-off-by: Roger Tseng rogera...@realtek.com --- drivers/mmc/host/Kconfig |7 + drivers/mmc/host/Makefile |1 + drivers/mmc/host/rtsx_usb_sdmmc.c | 1500 + 3 files changed, 1508 insertions(+) create mode 100644 drivers/mmc/host/rtsx_usb_sdmmc.c [snip] +#ifdef CONFIG_PM_RUNTIME There are stubs for pm_runtime* functions, thus the ifdefs can be removed. Please go though the complete patch and remove all instances. + pm_runtime_put(sdmmc_dev(host)); I don't know so much about USB mmc hosts hardware, but I just wanted to find out if I have understood this correct. You can't do fine grained power management of the USB parent device, since it needs to be runtime resumed to be able keep the power the card? Once it becomes runtime suspended, the power to the card will thus also be dropped? Yes, and to keep some internal state of the controller. Okay. But the internal state of the controller should be possible to restore at runtime_resume, so that should not be the reason, right? I checked again. Internal registers will hold its state during suspend. Card power is the exact reason. Kind regards Ulf Hansson . ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
