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

[Yingjoe Chen <yingjoe.chen@mediatek.com>]

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.


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

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


> 
> Signed-off-by: Zhiyong Tao <zhiyong.tao@mediatek.com>
> ---
>  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?



<...>

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

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

Joe.C