Re: [PATCH 3/3] pinctrl: add mt2712 pinctrl driver

From: Zhiyong Tao
Date: Thu Sep 21 2017 - 04:49:46 EST

On Thu, 2017-08-03 at 10:00 +0800, Yingjoe Chen wrote:
> On Wed, 2017-08-02 at 14:03 +0800, Zhiyong Tao wrote:
> > On Tue, 2017-08-01 at 17:14 +0800, Yingjoe Chen wrote:
> > >
> > > Hi Zhiyong,
> > >
> > >
> > >
> > > On Mon, 2017-07-31 at 16:22 +0800, Zhiyong Tao wrote:
> > > <...>
> > > > 3)Add "spec_dir_set" and "spec_dir_get" in "mtk_pinctrl_devdata".
> > > > 4)Change "spec_dir_set" and add "spec_dir_get" in "pinctrl-mt2701.c"
> > > > and "pinctrl-mtk-common.c".
> > >
> > > I think these deserve another patch.
> > > Please also explain why we need this.
> >
> > ==> ok, I will separate it in another patch in the next version.
> > Because we should control another gpio base register for gpio16 and 17
> > in mt2712 E1. It is special for the direction control in gpio16 and
> > gpio17.
> > >
> > >
> > > > 5)Change "port_mask" from "7" to "6" for EINT.
> > >
> > > I'm assuming this is a bug fix for mt2701?
> > > If yes, this should be a separate patch.
> >
> > ==> yes, it is a bug fix for mt2701. When I use EINT bothe edge triggle,
> > offset can't get the offset address which offset address is 1/3/5/7.
> > I will separate it in another patch in the next version.
> > >
> > > > 6)Remove generic pull config condition in "mtk_pconf_set_pull_select".
> > > > 7)Change "arg" to "MTK_PUPD_SET_R1R0_00" of "mtk_pconf_set_pull_select".
> > >
> > > Why we need to change arg?
> >
> > ==> to parse the "bias-disable" property in dts for special pins.
> >
> > >
> > >
> > > >
> > > > Signed-off-by: Zhiyong Tao <zhiyong.tao@xxxxxxxxxxxx>
> > > > ---
> > > > drivers/pinctrl/mediatek/Kconfig | 8 +
> > > > drivers/pinctrl/mediatek/Makefile | 1 +
> > > > drivers/pinctrl/mediatek/pinctrl-mt2701.c | 21 +-
> > > > drivers/pinctrl/mediatek/pinctrl-mt2712.c | 670 +++++++++
> > > > drivers/pinctrl/mediatek/pinctrl-mtk-common.c | 16 +-
> > > > drivers/pinctrl/mediatek/pinctrl-mtk-common.h | 44 +-
> > > > drivers/pinctrl/mediatek/pinctrl-mtk-mt2712.h | 1858 +++++++++++++++++++++++++
> > > > 7 files changed, 2586 insertions(+), 32 deletions(-)
> > > > create mode 100644 drivers/pinctrl/mediatek/pinctrl-mt2712.c
> > > > create mode 100644 drivers/pinctrl/mediatek/pinctrl-mtk-mt2712.h
> > > >
> > >
> > > <...>
> > >
> > > > diff --git a/drivers/pinctrl/mediatek/pinctrl-mt2701.c b/drivers/pinctrl/mediatek/pinctrl-mt2701.c
> > > > index f86f3b3..4a43f5c 100644
> > > > --- a/drivers/pinctrl/mediatek/pinctrl-mt2701.c
> > > > +++ b/drivers/pinctrl/mediatek/pinctrl-mt2701.c
> > > > @@ -503,10 +503,26 @@ static void mt2701_spec_pinmux_set(struct regmap *reg, unsigned int pin,
> > > > regmap_update_bits(reg, mt2701_spec_pinmux[i].offset, mask, value);
> > > > }
> > > >
> > > > -static void mt2701_spec_dir_set(unsigned int *reg_addr, unsigned int pin)
> > > > +static int mt2701_spec_dir_set(struct mtk_pinctrl *pctl,
> > > > + unsigned int *reg_addr,
> > > > + unsigned int pin,
> > > > + bool input)
> > > > {
> > > > if (pin > 175)
> > > > *reg_addr += 0x10;
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int mt2701_spec_dir_get(struct mtk_pinctrl *pctl,
> > > > + unsigned int *reg_addr,
> > > > + unsigned int pin,
> > > > + bool input)
> > >
> > > incorrect prototype?
> > >
> > > > +{
> > > > + if (pin > 175)
> > > > + *reg_addr += 0x10;
> > > > +
> > > > + return 0;
> > > > }
> > > >
> > > > static const struct mtk_pinctrl_devdata mt2701_pinctrl_data = {
> > > > @@ -520,6 +536,7 @@ static void mt2701_spec_dir_set(unsigned int *reg_addr, unsigned int pin)
> > > > .spec_ies_smt_set = mt2701_ies_smt_set,
> > > > .spec_pinmux_set = mt2701_spec_pinmux_set,
> > > > .spec_dir_set = mt2701_spec_dir_set,
> > > > + .spec_dir_get = mt2701_spec_dir_get,
> > > > .dir_offset = 0x0000,
> > > > .pullen_offset = 0x0150,
> > > > .pullsel_offset = 0x0280,
> > > > @@ -551,7 +568,7 @@ static void mt2701_spec_dir_set(unsigned int *reg_addr, unsigned int pin)
> > > > .dbnc_ctrl = 0x500,
> > > > .dbnc_set = 0x600,
> > > > .dbnc_clr = 0x700,
> > > > - .port_mask = 6,
> > > > + .port_mask = 7,
> > > > .ports = 6,
> > > > },
> > > > .ap_num = 169,
> > > > diff --git a/drivers/pinctrl/mediatek/pinctrl-mt2712.c b/drivers/pinctrl/mediatek/pinctrl-mt2712.c
> > > > new file mode 100644
> > > > index 0000000..c933b75
> > > > --- /dev/null
> > > > +++ b/drivers/pinctrl/mediatek/pinctrl-mt2712.c
> > >
> > > <...>
> > >
> > > > +
> > > > +static int mt2712_spec_dir_set(struct mtk_pinctrl *pctl,
> > > > + unsigned int *reg_addr,
> > > > + unsigned int pin,
> > > > + bool input)
> > > > +{
> > > > + u32 reg_val;
> > > > +
> > > > + if (pin == 16) {
> > > > + regmap_read(pctl->regmap2, 0x940, &reg_val);
> > > > + reg_val |= BIT(15);
> > > > + if (input == true)
> > > > + reg_val &= ~BIT(14);
> > > > + else
> > > > + reg_val |= BIT(14);
> > > > + regmap_write(pctl->regmap2, 0x940, reg_val);
> > > > + }
> > > > +
> > > > + if (pin == 17) {
> > > > + regmap_read(pctl->regmap2, 0x940, &reg_val);
> > > > + reg_val |= BIT(7);
> > > > + if (input == true)
> > > > + reg_val &= ~BIT(6);
> > > > + else
> > > > + reg_val |= BIT(6);
> > > > + regmap_write(pctl->regmap2, 0x940, reg_val);
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > >
> > > Does this means pin 16, 17 is in different/special reg/bit location?
> > > I didn't see spec_dir_get in your patch, does this means they are in
> > > standard location or you just forgot it?
> > >
> > > The original idea of spec_dir_set is to get the register offset for the
> > > pin, so both set_direction and get_direction are using the same
> > > extension function. Instead of adding a new spec_dir_get, can we just
> > > extend the function to also include bit location?
> >
> > ==> In mt2712 E1 gpi16 and gpio17 direction control is special. The
> > based register is different. so we add "struct mtk_pinctrl *pctl"
> > parameter to get the regmap2. The direction status is also different.
> > we forgot to add spec_dir_get, we will add it in the next version.
>
> In your device tree, you only provide one pctl-regmap, so who will setup
> this regmap2? If you need 2 pctl-regmap, you should add it in binding.

==> Dear Joe.c,
I will add another pctl-regmap in device tree in v2.
Which binding file we should add? Is it
"Documentation/devicetree/bindings/pinctrl/pinctrl-mt65xx.txt"?
>
> Your code will access register twice when setting pin 16,17, One in the
> 0x940 here and original location in normal code.
>
> The spec_dir_set was intend to be a remap function, ie, find the correct
> register for the pin. If possible, it would be better to expand the
> remap function for new chip instead of accessing the register directly.
> This way you don't need to have 2 functions for set and get, don't need
> to have extra code access register and don't have access twice bug.
>
==> Thanks for your comment.
we will try to expand the remap function for new chip in v2.
>
>
> > >
> > > <...>
> > >
> > > > diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> > > > index 3cf384f..aeec87e 100644
> > > > --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> > > > +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> > > > @@ -84,7 +84,7 @@ static int mtk_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
> > > > bit = BIT(offset & 0xf);
> > > >
> > > > if (pctl->devdata->spec_dir_set)
> > > > - pctl->devdata->spec_dir_set(&reg_addr, offset);
> > > > + pctl->devdata->spec_dir_set(pctl, &reg_addr, offset, input);
> > > >
> > > > if (input)
> > > > /* Different SoC has different alignment offset. */
> > > > @@ -307,13 +307,6 @@ static int mtk_pconf_set_pull_select(struct mtk_pinctrl *pctl,
> > > > return 0;
> > > > }
> > > >
> > > > - /* For generic pull config, default arg value should be 0 or 1. */
> > > > - if (arg != 0 && arg != 1) {
> > > > - dev_err(pctl->dev, "invalid pull-up argument %d on pin %d .\n",
> > > > - arg, pin);
> > > > - return -EINVAL;
> > > > - }
> > > > -
> > >
> > >
> > > Why we need to remove this?
> > ==> In order to parse "bias-disable" property. we change "arg" to
> > "MTK_PUPD_SET_R1R0_00". for normal pins, If we don't remove it.
> > It will return here.
>
> Please don't. We still need the check for other property.
==> ok, we will use other method, and retain the code, and not remove it
in v2.
> > >
> > > > bit = BIT(pin & 0xf);
> > > > if (enable)
> > > > reg_pullen = SET_ADDR(mtk_get_port(pctl, pin) +
> > > > @@ -343,7 +336,8 @@ static int mtk_pconf_parse_conf(struct pinctrl_dev *pctldev,
> > > >
> > > > switch (param) {
> > > > case PIN_CONFIG_BIAS_DISABLE:
> > > > - ret = mtk_pconf_set_pull_select(pctl, pin, false, false, arg);
> > > > + ret = mtk_pconf_set_pull_select(pctl, pin, false, false,
> > > > + MTK_PUPD_SET_R1R0_00);
> > >
> > > Why we need to change this?
> > >
> > ==> if only just add "bias-disable" property in dts. "arg" is 0 or 1,
> > It can't to parse the "bias-disable" property. so we change it to
> > "MTK_PUPD_SET_R1R0_00".
>
> Why not?
> The enable is false here, you should check that.
> R1R0_00 means set R1R0 to 00, it does not mean disable. Using it as
> disable is a bug.
>
> Joe.C

==> Thanks for your comment.
we will use other method, and not change it in v2. Thanks.
>
>
>