Messages in this thread | | | Date | Wed, 19 Jan 2022 21:47:56 +0200 | From | Andy Shevchenko <> | Subject | Re: [PATCH v4 3/3] mmc: mediatek: add support for SDIO eint IRQ |
| |
On Wed, Jan 19, 2022 at 06:32:12PM +0800, Axe Yang wrote: > Add support for eint IRQ when MSDC is used as an SDIO host. This > feature requires SDIO device support async IRQ function. With this > feature, SDIO host can be awakened by SDIO card in suspend state, > without additional pin. > > MSDC driver will time-share the SDIO DAT1 pin. During suspend, MSDC > turn off clock and switch SDIO DAT1 pin to GPIO mode. And during > resume, switch GPIO function back to DAT1 mode then turn on clock. > > Some device tree property should be added or modified in MSDC node > to support SDIO eint IRQ. Pinctrls named state_dat1 and state_eint > are mandatory. And cap-sdio-async-irq flag is necessary since this > feature depends on asynchronous interrupt: > &mmcX { > ... > pinctrl-names = "default", "state_uhs", "state_eint", > "state_dat1"; > ... > pinctrl-2 = <&mmc2_pins_eint>; > pinctrl-3 = <&mmc2_pins_dat1>; > ... > cap-sdio-async-irq; > ... > }; > > Signed-off-by: Axe Yang <axe.yang@mediatek.com>
The submitters SoB must be last among all SoB tags. Please, read Submitting Patches document carefully.
> Signed-off-by: Yong Mao <yong.mao@mediatek.com>
Who is they, why their SoB appeared here?
...
> /* > - * Copyright (c) 2014-2015 MediaTek Inc. > + * Copyright (c) 2022 MediaTek Inc.
This doesn't feel right. Why did you remove old years?
> * Author: Chaotian.Jing <chaotian.jing@mediatek.com> > */
...
> + desc = devm_gpiod_get(host->dev, "eint", GPIOD_IN); > + if (IS_ERR(desc)) > + return PTR_ERR(desc);
> + ret = gpiod_to_irq(desc); > + if (ret < 0) > + return ret; > + > + irq = ret;
Since both of them are local variables and there is no specific use of the returned value, I believe it's fine just to
irq = gpiod_to_irq(desc); ...
Hmm... I was wondering if you can use fwnode_irq_get_byname(). Ah, it's not (yet) in upstream.
...
> static int __maybe_unused msdc_runtime_suspend(struct device *dev) > { > + unsigned long flags;
Can you keep reversed xmas tree order?
> struct mmc_host *mmc = dev_get_drvdata(dev); > struct msdc_host *host = mmc_priv(mmc);
(it means to add new variable here)
> return 0; > }
...
> static int __maybe_unused msdc_runtime_resume(struct device *dev) > { > + unsigned long flags;
Ditto.
> struct mmc_host *mmc = dev_get_drvdata(dev); > struct msdc_host *host = mmc_priv(mmc); > int ret;
> return 0; > }
-- With Best Regards, Andy Shevchenko
| |