Re: [PATCH v13 3/3] mmc: mediatek: add support for SDIO eint wakup IRQ

[Axe Yang <axe.yang@mediatek.com>]

On Mon, 2022-07-25 at 14:46 +0200, Linus Walleij wrote:
> On Mon, Jul 25, 2022 at 11:13 AM Axe Yang <axe.yang@mediatek.com>
> wrote:
> > On Fri, 2022-07-22 at 13:21 +0200, Linus Walleij wrote:
> > > On Thu, Jun 23, 2022 at 11:10 AM Axe Yang <axe.yang@mediatek.com>
> > > wrote:
> > SDIO DAT1 pin mode is changed to GPIO mode in
> > dev_pm_set_dedicated_wake_irq_reverse():
> > 
> > 
https://urldefense.com/v3/__https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c*L339__;Iw!!CTRNKA9wMg0ARbw!zE3kmi37pZw4HiBNeRipWbi3gbAqrljLVQc5JVz-WP_NaIWTVhXshkakjFNh478e$
> >  
> > 
> > dev_pm_set_dedicated_wake_irq_reverse() -> ...
> > ->request_threaded_irq()
> > -> __setup_irq() -> irq_request_resources() ->
> > mtk_eint_irq_request_resources()-> mtk_xt_set_gpio_as_eint()
> 
> This doesn't seem to have much to do with pin control?
> No pin control functions are called on this execution path,
> no pin control state is changed, right?

That's right, no pin control state is changed.

> 
> If what you mean is that
> it happens to poke into the same hardware registers that is
> mainly a matter of concurrency in the driver, sometimes two
> abstractions happen to have to poke into the same hardware
> registers and then it is up to the driver to maintain state for
> the hardware, this is not a question for the framework.
> 
> How is Mediatek developers thinking about this thing in general?
> You are a few people who developed the driver so certainly
> you must have some design idea to why irq_request_resources()
> poke around in these registers? Does it even perform pin
> control behind the back of the pin control framework?

I see. It is sensible to reset pin function to GPIO mode when trying to
register the pin to eint mode, and the operation is out of pinctrl
framework.

Seems like maintain the pin state in driver is the only way to fix the
pin function conflict.

> 
> > To restore SDIO DAT1 pin to uhs mode. I have to call
> > pinctrl_select_state() twice(change pinctrl to another state, then
> > change back to uhs mode). Ulf worried we might be doing something
> > at
> > the mmc driver level, which should really be managed at the pinctrl
> > layer.
> > 
> > Do you have any comment or suggestion on this?
> 
> The pin control state transitions are really just finite automata.
> 
> Your pin control needs to be different when using wakeup from
> when being used for SDIO and this is perfectly fine, it's no
> different from the fact that the regulator and clock might need
> to be in different states, so I don't quite understand the
> question?

I see. At first I thought that pinctrl framework should be aware of
the hidden modification of pin function. But as you said, it is just
a finite automata. Driver should correct GPIO settings by itself if pin
state be changed outside pin control state mechanical.
Sorry for the noise, and thanks for your comment again.

> 
> 
Regards,
Axe