Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1332485 > unrolled thread

Re: [PATCH 1/2] pinctrl: mediatek: fix direction control issue

Started byHongzhou Yang <hongzhou.yang@mediatek.com>
First post2016-02-12 03:10 +0100
Last post2016-02-16 13:30 +0100
Articles 3 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/2] pinctrl: mediatek: fix direction control issue Hongzhou Yang <hongzhou.yang@mediatek.com> - 2016-02-12 03:10 +0100
    Re: [PATCH 1/2] pinctrl: mediatek: fix direction control issue Yingjoe Chen <yingjoe.chen@mediatek.com> - 2016-02-16 10:00 +0100
      Re: [PATCH 1/2] pinctrl: mediatek: fix direction control issue Linus Walleij <linus.walleij@linaro.org> - 2016-02-16 13:30 +0100

#1332485 — Re: [PATCH 1/2] pinctrl: mediatek: fix direction control issue

FromHongzhou Yang <hongzhou.yang@mediatek.com>
Date2016-02-12 03:10 +0100
SubjectRe: [PATCH 1/2] pinctrl: mediatek: fix direction control issue
Message-ID<r1blh-wq-7@gated-at.bofh.it>
On Fri, 2016-02-05 at 14:52 +0100, Linus Walleij wrote:
> On Thu, Feb 4, 2016 at 4:03 AM, Hongzhou Yang
> <hongzhou.yang@mediatek.com> wrote:
> > On Wed, 2016-02-03 at 09:24 +0800, Biao Huang wrote:
> >> Since input-enable/disable and input-schmitt-enable/disable are
> >> workable when gpio direction is input, so add direction setting
> >> when do input-enable/disable and input-schmitt-enable/disable
> >> properties.
> >>
> >> Signed-off-by: Biao Huang <biao.huang@mediatek.com>
> >> ---
> >>  drivers/pinctrl/mediatek/pinctrl-mtk-common.c |    2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> >> index 8cac73d..6eb01c9 100644
> >> --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> >> +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> >> @@ -352,6 +352,7 @@ static int mtk_pconf_parse_conf(struct pinctrl_dev *pctldev,
> >>               ret = mtk_pconf_set_pull_select(pctl, pin, true, false, arg);
> >>               break;
> >>       case PIN_CONFIG_INPUT_ENABLE:
> >> +             mtk_pmx_gpio_set_direction(pctldev, NULL, pin, true);
> >>               ret = mtk_pconf_set_ies_smt(pctl, pin, arg, param);
> >
> > Hi Linus,
> >
> > From pinctrl-bindings.txt, 'input-enable' property defined as following:
> > input-enable        - enable input on pin (no effect on output)
> >
> > Since input enable and input direction are two different settings on our
> > SOC,
> 
> What does this mean? How can input have a "direction"?
> 
> Isn't the direction of an input always inbound, into the SoC?
> 
> Please elaborate.

Sorry for the late reply.
There are two conditions if external device input data into SOC.
1. Change GPIO direction to input.
2. Switch on input.


> > could you tell me the exact meaning of this property? Input enable
> > only? Or set input direction at the same time?
> 
> This was added in commit 8ba3f4d00078e7a49c60c0bd6298f29402c3a0a0
> "pinctrl: Adds slew-rate, input-enable/disable"
> by Sherman Yin so let's ask him first.

Thanks for this info.
From Sherman's comment, PIN_CONFIG_INPUT_ENABLE seems only to stand for
the condition 2.

* @PIN_CONFIG_INPUT_ENABLE: enable the pin's input.  Note that this does
not affect the pin's ability to drive output.  1 enables input, 0
disables input.

Then I get confuse if we can change direction under this property.

Thanks.
Hongzhou

[toc] | [next] | [standalone]

#1335169

FromYingjoe Chen <yingjoe.chen@mediatek.com>
Date2016-02-16 10:00 +0100
Message-ID<r2JEf-5wa-19@gated-at.bofh.it>
In reply to#1332485
On Thu, 2016-02-11 at 18:07 -0800, Hongzhou Yang wrote:
> On Fri, 2016-02-05 at 14:52 +0100, Linus Walleij wrote:
> > On Thu, Feb 4, 2016 at 4:03 AM, Hongzhou Yang
> > <hongzhou.yang@mediatek.com> wrote:
> > > On Wed, 2016-02-03 at 09:24 +0800, Biao Huang wrote:
> > >> Since input-enable/disable and input-schmitt-enable/disable are
> > >> workable when gpio direction is input, so add direction setting
> > >> when do input-enable/disable and input-schmitt-enable/disable
> > >> properties.
> > >>
> > >> Signed-off-by: Biao Huang <biao.huang@mediatek.com>
> > >> ---
> > >>  drivers/pinctrl/mediatek/pinctrl-mtk-common.c |    2 ++
> > >>  1 file changed, 2 insertions(+)
> > >>
> > >> diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> > >> index 8cac73d..6eb01c9 100644
> > >> --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> > >> +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> > >> @@ -352,6 +352,7 @@ static int mtk_pconf_parse_conf(struct pinctrl_dev *pctldev,
> > >>               ret = mtk_pconf_set_pull_select(pctl, pin, true, false, arg);
> > >>               break;
> > >>       case PIN_CONFIG_INPUT_ENABLE:
> > >> +             mtk_pmx_gpio_set_direction(pctldev, NULL, pin, true);
> > >>               ret = mtk_pconf_set_ies_smt(pctl, pin, arg, param);
> > >
> > > Hi Linus,
> > >
> > > From pinctrl-bindings.txt, 'input-enable' property defined as following:
> > > input-enable        - enable input on pin (no effect on output)
> > >
> > > Since input enable and input direction are two different settings on our
> > > SOC,
> > 
> > What does this mean? How can input have a "direction"?
> > 
> > Isn't the direction of an input always inbound, into the SoC?
> > 
> > Please elaborate.
> 
> Sorry for the late reply.
> There are two conditions if external device input data into SOC.
> 1. Change GPIO direction to input.
> 2. Switch on input.
> 
> 
> > > could you tell me the exact meaning of this property? Input enable
> > > only? Or set input direction at the same time?
> > 
> > This was added in commit 8ba3f4d00078e7a49c60c0bd6298f29402c3a0a0
> > "pinctrl: Adds slew-rate, input-enable/disable"
> > by Sherman Yin so let's ask him first.
> 
> Thanks for this info.
> From Sherman's comment, PIN_CONFIG_INPUT_ENABLE seems only to stand for
> the condition 2.
> 
> * @PIN_CONFIG_INPUT_ENABLE: enable the pin's input.  Note that this does
> not affect the pin's ability to drive output.  1 enables input, 0
> disables input.
> 
> Then I get confuse if we can change direction under this property.

Hi,

To make it more clear, for a GPIO pin to get input on MTK chip, we must:

- Enable input for the pad (input-enable)
- Set mux to GPIO mode
- Set GPIO direction to input

Currently user need to add a dts node to set pinmux to GPIO mode with
input-enable property, the driver need to call gpio_direction_input to
set direction.

Since input-enable and GPIO direction are 2 different things, we now
think this patch is inappropriate. Instead, user should not required to
add pinmux node in dts if all he want is use it as plain GPIO input.
Patch [2/2] remove the need to set it to GPIO mode, Biao will send a new
patch to remove the need for input-enable.

Joe.C

[toc] | [prev] | [next] | [standalone]

#1335325

FromLinus Walleij <linus.walleij@linaro.org>
Date2016-02-16 13:30 +0100
Message-ID<r2MVs-7Lj-1@gated-at.bofh.it>
In reply to#1335169
On Tue, Feb 16, 2016 at 9:54 AM, Yingjoe Chen <yingjoe.chen@mediatek.com> wrote:

> To make it more clear, for a GPIO pin to get input on MTK chip, we must:
>
> - Enable input for the pad (input-enable)

This should be done by the pin control portions of the driver in
response to the PIN_CONFIG_INPUT_ENABLE generic pin
config setting.

Possibly *also* by just writing the hardware registers
from the callbacks right below here:

> - Set mux to GPIO mode

This should be done by just writing the hardware registers
in these callbacks from struct pinmux_ops:

        int (*gpio_request_enable) (struct pinctrl_dev *pctldev,
                                    struct pinctrl_gpio_range *range,
                                    unsigned offset);
        void (*gpio_disable_free) (struct pinctrl_dev *pctldev,
                                   struct pinctrl_gpio_range *range,
                                   unsigned offset);
        int (*gpio_set_direction) (struct pinctrl_dev *pctldev,
                                   struct pinctrl_gpio_range *range,
                                   unsigned offset,
                                   bool input);

In respons to these functions being called from the corresponding
GPIO driver:

extern int pinctrl_request_gpio(unsigned gpio);
extern void pinctrl_free_gpio(unsigned gpio);
extern int pinctrl_gpio_direction_input(unsigned gpio);
extern int pinctrl_gpio_direction_output(unsigned gpio);

> - Set GPIO direction to input

This should be done in these callbacks from struct gpio_chip:

        int                     (*direction_input)(struct gpio_chip *chip,
                                                unsigned offset);
        int                     (*direction_output)(struct gpio_chip *chip,
                                                unsigned offset, int value);

By just writing the hardware registers.

> Currently user need to add a dts node to set pinmux to GPIO mode with
> input-enable property, the driver need to call gpio_direction_input to
> set direction.

It seems like:

- Your GPIO driver is not calling the pinctrl_request_gpio/
  pinctrl_gpio_direction_* etc.

- Your pinmux driver is not implementing the GPIO-specifif
  .gpio_* functions

> Since input-enable and GPIO direction are 2 different things, we now
> think this patch is inappropriate. Instead, user should not required to
> add pinmux node in dts if all he want is use it as plain GPIO input.

If you sort out the above callbacks (also read
Documentation/pinctrl.txt about GPIO) I think it is
clear what you need to do.

Usually you should only need a special pinmux/pinconf node
for a GPIO line if you need to specify pull-up or similar electronic
things for that GPIO.

Yours,
Linus Walleij

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web