Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751860AbdHAJON (ORCPT ); Tue, 1 Aug 2017 05:14:13 -0400 Received: from mailgw02.mediatek.com ([210.61.82.184]:20123 "EHLO mailgw02.mediatek.com" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1751101AbdHAJOK (ORCPT ); Tue, 1 Aug 2017 05:14:10 -0400 Message-ID: <1501578845.32089.18.camel@mtksdaap41> Subject: Re: [PATCH 3/3] pinctrl: add mt2712 pinctrl driver From: Yingjoe Chen To: Zhiyong Tao CC: , , , , , , , , , , , , , , , Date: Tue, 1 Aug 2017 17:14:05 +0800 In-Reply-To: <1501489333-23145-4-git-send-email-zhiyong.tao@mediatek.com> References: <1501489333-23145-1-git-send-email-zhiyong.tao@mediatek.com> <1501489333-23145-4-git-send-email-zhiyong.tao@mediatek.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.2.3-0ubuntu6 Content-Transfer-Encoding: 7bit MIME-Version: 1.0 X-MTK: N Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5693 Lines: 191 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 > --- > 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, ®_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, ®_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(®_addr, offset); > + pctl->devdata->spec_dir_set(pctl, ®_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