On 19/10/23 18:24, Ulf Hansson wrote: > On Thu, 19 Oct 2023 at 16:48, Adrian Hunter <adrian.hunter@xxxxxxxxx> wrote: >> >> On 18/10/23 17:42, Ulf Hansson wrote: >>> On Wed, 18 Oct 2023 at 04:07, Bough Chen <haibo.chen@xxxxxxx> wrote: >>>> >>>>> -----Original Message----- >>>>> From: Ulf Hansson <ulf.hansson@xxxxxxxxxx> >>>>> Sent: 2023年10月17日 19:35 >>>>> To: Bough Chen <haibo.chen@xxxxxxx> >>>>> Cc: adrian.hunter@xxxxxxxxx; linux-mmc@xxxxxxxxxxxxxxx; dl-linux-imx >>>>> <linux-imx@xxxxxxx>; shawnguo@xxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx; >>>>> kernel@xxxxxxxxxxxxxx; festevam@xxxxxxxxx >>>>> Subject: Re: [PATCH] mmc: host: sdhci-esdhc-imx: let usr to config the wakeup >>>>> for gpio cd pin through sysfs >>>>> >>>>> On Tue, 17 Oct 2023 at 09:26, <haibo.chen@xxxxxxx> wrote: >>>>>> >>>>>> From: Haibo Chen <haibo.chen@xxxxxxx> >>>>>> >>>>>> Currently default enable the gpio cd pin wakeup, this will waste power >>>>>> after system suspend if usr do not need this cd pin wakeup feature. >>>>>> Now let usr to config the wakeup for gpio cd pin through sysfs. >>>>>> >>>>>> Signed-off-by: Haibo Chen <haibo.chen@xxxxxxx> >>>>>> --- >>>>>> drivers/mmc/host/sdhci-esdhc-imx.c | 14 ++++++++++---- >>>>>> 1 file changed, 10 insertions(+), 4 deletions(-) >>>>>> >>>>>> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c >>>>>> b/drivers/mmc/host/sdhci-esdhc-imx.c >>>>>> index 40a6e2f8145a..2e46648344ba 100644 >>>>>> --- a/drivers/mmc/host/sdhci-esdhc-imx.c >>>>>> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c >>>>>> @@ -1797,9 +1797,12 @@ static int sdhci_esdhc_imx_probe(struct >>>>> platform_device *pdev) >>>>>> /* >>>>>> * Setup the wakeup capability here, let user to decide >>>>>> * whether need to enable this wakeup through sysfs interface. >>>>>> + * First check the SDIO device, second check the gpio CD pin. >>>>>> */ >>>>>> - if ((host->mmc->pm_caps & MMC_PM_KEEP_POWER) && >>>>>> - (host->mmc->pm_caps & >>>>> MMC_PM_WAKE_SDIO_IRQ)) >>>>>> + if (((host->mmc->pm_caps & MMC_PM_KEEP_POWER) && >>>>>> + (host->mmc->pm_caps & >>>>> MMC_PM_WAKE_SDIO_IRQ)) || >>>>>> + ((host->mmc->caps & MMC_CAP_CD_WAKE) && >>>>>> + host->mmc->slot.cd_irq >= 0)) >>>>>> device_set_wakeup_capable(&pdev->dev, true); >>>>> >>>>> If the wakeup is GPIO based, it doesn't mean that the device is wakeup capable, >>>>> so this is wrong. >>>>> >>>>> In principle this must be managed through the GPIO irqchip driver instead. >>>> >>>> Yes, I know GPIO and USDHC are different device, combine GPIO wakeup to USDHC is not that reasonable. >>>> I send this patch because I notice there is similar code in sdhci-pci-core.c, refer to sdhci_pci_suspend_host() >>>> >>>> Seems PCI subsystem combine GPIO wakeup to PCI controller. >>> >>> Good point! I am not sure that is the correct way to do it either, but >>> maybe it's fine for PCI based devices. Adrian probably knows this >>> better. >> >> PCI / ACPI layers handle configuration of wakeup signaling, so a >> device will be put to the appropriate power state. It won't be >> forced to be "on" by registering a wakeup source. >> > > Okay, I see. > > So you are saying that PCI bus/layers are aware of the GPIO pin/irq > being used for wakeup for the device in question? No, it is more like PCI / ACPI take care of most things but the wakeup framework provides sysfs wakeup API. We can add that and use it to determine if wakeup is enabled (by the user) to then enable the CD GPIO IRQ wakeup. That way, the sysfs wakeup API is on the correct device, and the user doesn't need to know anything about GPIO.
