| Message ID | 1566206502-4347-6-git-send-email-mars.cheng@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add basic SoC Support for Mediatek MT6779 SoC | expand |
On Mon, Aug 19, 2019 at 11:22 AM Mars Cheng <mars.cheng@mediatek.com> wrote: > for virtual gpios, they should not do reg setting and > should behave as expected for eint function. > > Signed-off-by: Mars Cheng <mars.cheng@mediatek.com> This does not explain what a "virtual GPIO" is in this context, so please elaborate. What is this? Why does it exist? What is it used for? GPIO is "general purpose input/output" and it is a pretty rubbery category already as it is, so we need to define our terms pretty strictly. > +bool mtk_is_virt_gpio(struct mtk_pinctrl *hw, unsigned int gpio_n) > +{ > + const struct mtk_pin_desc *desc; > + bool virt_gpio = false; > + > + if (gpio_n >= hw->soc->npins) > + return virt_gpio; > + > + desc = (const struct mtk_pin_desc *)&hw->soc->pins[gpio_n]; > + > + if (desc->funcs && > + desc->funcs[desc->eint.eint_m].name == 0) NULL check is done like this: if (desc->funcs && !desc->funcs[desc->eint.eint_m].name) > + virt_gpio = true; So why is this GPIO "virtual" because it does not have a name in the funcs table? > @@ -278,6 +295,9 @@ static int mtk_xt_set_gpio_as_eint(void *data, unsigned long eint_n) > if (err) > return err; > > + if (mtk_is_virt_gpio(hw, gpio_n)) > + return 0; So does this mean we always succeed in setting a GPIO as eint if it is virtual? Why? Explanatory comment is needed. > @@ -693,6 +693,9 @@ static int mtk_gpio_get_direction(struct gpio_chip *chip, unsigned int gpio) > const struct mtk_pin_desc *desc; > int value, err; > > + if (mtk_is_virt_gpio(hw, gpio)) > + return 1; Why are "virtual GPIOs" always inputs? Yours, Linus Walleij
On Fri, 2019-08-23 at 10:57 +0200, Linus Walleij wrote: > On Mon, Aug 19, 2019 at 11:22 AM Mars Cheng <mars.cheng@mediatek.com> wrote: > > > for virtual gpios, they should not do reg setting and > > should behave as expected for eint function. > > > > Signed-off-by: Mars Cheng <mars.cheng@mediatek.com> > > This does not explain what a "virtual GPIO" is in this > context, so please elaborate. What is this? Why does > it exist? What is it used for? > > GPIO is "general purpose input/output" and it is a > pretty rubbery category already as it is, so we need > to define our terms pretty strictly. > Virtual GPIO only used inside SOC and not being exported to outside SOC in MTK platform. Some modules use virtual GPIO as eint (e.g. pmic or usb). In MTK platform, external interrupt (EINT) and GPIO is 1-1 mapping and we can set GPIO as eint. But some modules use specific eint which doesn't have real GPIO pin. So we use virtual GPIO to map it. > > +bool mtk_is_virt_gpio(struct mtk_pinctrl *hw, unsigned int gpio_n) > > +{ > > + const struct mtk_pin_desc *desc; > > + bool virt_gpio = false; > > + > > + if (gpio_n >= hw->soc->npins) > > + return virt_gpio; > > + > > + desc = (const struct mtk_pin_desc *)&hw->soc->pins[gpio_n]; > > + > > + if (desc->funcs && > > + desc->funcs[desc->eint.eint_m].name == 0) > > NULL check is done like this: > > if (desc->funcs && !desc->funcs[desc->eint.eint_m].name) > > > + virt_gpio = true; > got it, will fix it in v3. Thanks for reviewing. > So why is this GPIO "virtual" because it does not have > a name in the funcs table? > yes, it doesn't have real GPIO pin and name, so we set it as virtual GPIO. > > @@ -278,6 +295,9 @@ static int mtk_xt_set_gpio_as_eint(void *data, unsigned long eint_n) > > if (err) > > return err; > > > > + if (mtk_is_virt_gpio(hw, gpio_n)) > > + return 0; > > So does this mean we always succeed in setting a GPIO as eint > if it is virtual? Why? Explanatory comment is needed. > yes, we use virtual GPIO to map it. > > @@ -693,6 +693,9 @@ static int mtk_gpio_get_direction(struct gpio_chip *chip, unsigned int gpio) > > const struct mtk_pin_desc *desc; > > int value, err; > > > > + if (mtk_is_virt_gpio(hw, gpio)) > > + return 1; > > Why are "virtual GPIOs" always inputs? > We set virtual GPIO as eint. It mean virtual GPIO only used inside SOC and not being exported to outside SOC. > Yours, > Linus Walleij Sorry for the late reply. I'm the new bsp contact of mtk phone project. I will be taking over from Mars. Thanks for reviewing.
On Mon, Dec 23, 2019 at 4:11 AM Hanks Chen <hanks.chen@mediatek.com> wrote: > On Fri, 2019-08-23 at 10:57 +0200, Linus Walleij wrote: > > On Mon, Aug 19, 2019 at 11:22 AM Mars Cheng <mars.cheng@mediatek.com> wrote: > > This does not explain what a "virtual GPIO" is in this > > context, so please elaborate. What is this? Why does > > it exist? What is it used for? > > > > GPIO is "general purpose input/output" and it is a > > pretty rubbery category already as it is, so we need > > to define our terms pretty strictly. > > > Virtual GPIO only used inside SOC and not being exported to outside SOC > in MTK platform. Some modules use virtual GPIO as eint (e.g. pmic or > usb). I would call that internal GPIOs, those are very real rails inside the chip made with polysilicone so there is nothing "virtual" about them. If the documentation for the chip calls them virtual then explain in the driver that these are SoC-internal lines so that everyone will get it. Is the PMIC inside the SoC? I thought that was usually outside of it in its own chip. But I suppose there could be some interface to it in the SoC and then that interface has this EINT? > In MTK platform, external interrupt (EINT) and GPIO is 1-1 mapping and > we can set GPIO as eint. > But some modules use specific eint which doesn't have real GPIO pin. > So we use virtual GPIO to map it. OK I get it I think... just put these comments into the code as well so we understand when reading the code what is going on. > > > + if (mtk_is_virt_gpio(hw, gpio)) > > > + return 1; > > > > Why are "virtual GPIOs" always inputs? > > We set virtual GPIO as eint. > It mean virtual GPIO only used inside SOC and not being exported to > outside SOC. Are you saying that: - "Virtual" GPIOs are always and only used for interrupts - Since they are only used for interrupts, they are always inputs Then write that in a comment to the above change so we know this context. Yours, Linus Walleij
On Tue, 2020-01-07 at 11:20 +0100, Linus Walleij wrote: > On Mon, Dec 23, 2019 at 4:11 AM Hanks Chen <hanks.chen@mediatek.com> wrote: > > On Fri, 2019-08-23 at 10:57 +0200, Linus Walleij wrote: > > > On Mon, Aug 19, 2019 at 11:22 AM Mars Cheng <mars.cheng@mediatek.com> wrote: > > > > This does not explain what a "virtual GPIO" is in this > > > context, so please elaborate. What is this? Why does > > > it exist? What is it used for? > > > > > > GPIO is "general purpose input/output" and it is a > > > pretty rubbery category already as it is, so we need > > > to define our terms pretty strictly. > > > > > Virtual GPIO only used inside SOC and not being exported to outside SOC > > in MTK platform. Some modules use virtual GPIO as eint (e.g. pmic or > > usb). > > I would call that internal GPIOs, those are very real rails inside > the chip made with polysilicone so there is nothing "virtual" > about them. If the documentation for the chip calls them virtual > then explain in the driver that these are SoC-internal > lines so that everyone will get it. > Got it, I will add the info into the driver in v3 > Is the PMIC inside the SoC? I thought that was usually outside of it > in its own chip. > > But I suppose there could be some interface to it in the SoC and > then that interface has this EINT? > That's right. I use incorrect word. e.g. pmic interface inside the SOC (PMIF), not pmic... > > In MTK platform, external interrupt (EINT) and GPIO is 1-1 mapping and > > we can set GPIO as eint. > > But some modules use specific eint which doesn't have real GPIO pin. > > So we use virtual GPIO to map it. > > OK I get it I think... just put these comments into the code as well > so we understand when reading the code what is going on. Got it, will add the comments in v3. Thanks for reviewing. > > > > > + if (mtk_is_virt_gpio(hw, gpio)) > > > > + return 1; > > > > > > Why are "virtual GPIOs" always inputs? > > > > We set virtual GPIO as eint. > > It mean virtual GPIO only used inside SOC and not being exported to > > outside SOC. > > Are you saying that: > - "Virtual" GPIOs are always and only used for interrupts > - Since they are only used for interrupts, they are always inputs > > Then write that in a comment to the above change so we know > this context. > Yes, virtual GPIOs are always and only used for interrupts in mtk platform. I'll add the comments in v3. Thanks for reviewing > Yours, > Linus Walleij > > _______________________________________________ > Linux-mediatek mailing list > Linux-mediatek@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-mediatek
diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c b/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c index 20e1c89..04948a6 100644 --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c @@ -226,6 +226,23 @@ static int mtk_xt_find_eint_num(struct mtk_pinctrl *hw, unsigned long eint_n) return EINT_NA; } +bool mtk_is_virt_gpio(struct mtk_pinctrl *hw, unsigned int gpio_n) +{ + const struct mtk_pin_desc *desc; + bool virt_gpio = false; + + if (gpio_n >= hw->soc->npins) + return virt_gpio; + + desc = (const struct mtk_pin_desc *)&hw->soc->pins[gpio_n]; + + if (desc->funcs && + desc->funcs[desc->eint.eint_m].name == 0) + virt_gpio = true; + + return virt_gpio; +} + static int mtk_xt_get_gpio_n(void *data, unsigned long eint_n, unsigned int *gpio_n, struct gpio_chip **gpio_chip) @@ -278,6 +295,9 @@ static int mtk_xt_set_gpio_as_eint(void *data, unsigned long eint_n) if (err) return err; + if (mtk_is_virt_gpio(hw, gpio_n)) + return 0; + desc = (const struct mtk_pin_desc *)&hw->soc->pins[gpio_n]; err = mtk_hw_set_value(hw, desc, PINCTRL_PIN_REG_MODE, diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.h b/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.h index 1b7da42..cda1c7a0 100644 --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.h +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.h @@ -299,4 +299,5 @@ int mtk_pinconf_adv_drive_set(struct mtk_pinctrl *hw, int mtk_pinconf_adv_drive_get(struct mtk_pinctrl *hw, const struct mtk_pin_desc *desc, u32 *val); +bool mtk_is_virt_gpio(struct mtk_pinctrl *hw, unsigned int gpio_n); #endif /* __PINCTRL_MTK_COMMON_V2_H */ diff --git a/drivers/pinctrl/mediatek/pinctrl-paris.c b/drivers/pinctrl/mediatek/pinctrl-paris.c index 923264d..ef479ea 100644 --- a/drivers/pinctrl/mediatek/pinctrl-paris.c +++ b/drivers/pinctrl/mediatek/pinctrl-paris.c @@ -693,6 +693,9 @@ static int mtk_gpio_get_direction(struct gpio_chip *chip, unsigned int gpio) const struct mtk_pin_desc *desc; int value, err; + if (mtk_is_virt_gpio(hw, gpio)) + return 1; + desc = (const struct mtk_pin_desc *)&hw->soc->pins[gpio]; err = mtk_hw_get_value(hw, desc, PINCTRL_PIN_REG_DIR, &value);
for virtual gpios, they should not do reg setting and should behave as expected for eint function. Signed-off-by: Mars Cheng <mars.cheng@mediatek.com> --- drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c | 20 ++++++++++++++++++++ drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.h | 1 + drivers/pinctrl/mediatek/pinctrl-paris.c | 3 +++ 3 files changed, 24 insertions(+)