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

[Zhiyong Tao <zhiyong.tao@xxxxxxxxxxxx>]

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.
> 
> 
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html