Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754206AbdGLVZC (ORCPT ); Wed, 12 Jul 2017 17:25:02 -0400 Received: from mail-pf0-f175.google.com ([209.85.192.175]:33835 "EHLO mail-pf0-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753920AbdGLVY6 (ORCPT ); Wed, 12 Jul 2017 17:24:58 -0400 Date: Wed, 12 Jul 2017 14:24:54 -0700 From: Bjorn Andersson To: fenglinw@codeaurora.org Cc: linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org, Linus Walleij , Rob Herring , Mark Rutland , Andy Gross , David Brown , Srinivas Kandagatla , linux-gpio@vger.kernel.org, devicetree@vger.kernel.org, linux-soc@vger.kernel.org, collinsd@quicinc.com, aghayal@qti.qualcomm.com, wruan@quicinc.com, kgunda@qti.qualcomm.com Subject: Re: [PATCH V1 2/3] pinctrl: qcom: spmi-gpio: Add dtest route for digital input Message-ID: <20170712212454.GK1618@tuxbook> References: <20170613061707.13892-1-fenglinw@codeaurora.org> <20170613061707.13892-3-fenglinw@codeaurora.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170613061707.13892-3-fenglinw@codeaurora.org> User-Agent: Mutt/1.8.3 (2017-05-23) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4504 Lines: 121 On Mon 12 Jun 23:16 PDT 2017, fenglinw@codeaurora.org wrote: > From: Fenglin Wu > > Add property "qcom,dtest-buffer" to specify which dtest rail to feed > when the pin is configured as a digital input. > > Signed-off-by: Fenglin Wu > --- > .../devicetree/bindings/pinctrl/qcom,pmic-gpio.txt | 15 ++++++++ > drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 45 ++++++++++++++++++++++ > include/dt-bindings/pinctrl/qcom,pmic-gpio.h | 6 +++ > 3 files changed, 66 insertions(+) > > diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.txt b/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.txt > index 1493c0a..521c783 100644 > --- a/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.txt > +++ b/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.txt > @@ -195,6 +195,21 @@ to specify in a pin configuration subnode: > Valid values are 0-3 corresponding to PMIC_GPIO_AOUT_ATESTx > defined in . > > +- qcom,dtest-buffer: > + Usage: optional > + Value type: > + Definition: Selects DTEST rail to route to GPIO when it's configured > + as a digital input. > + For LV/MV GPIO subtypes, the valid values are 0-3 > + corresponding to PMIC_GPIO_DIN_DTESTx defined in > + . Only one > + DTEST rail can be selected at a time. As with the analog lines, this is a natural number and I think we should encode it as such in the DeviceTree. > + For 4CH/8CH GPIO subtypes, supported values are 1-15. > + 4 DTEST rails are supported in total and more than 1 DTEST > + rail can be selected simultaneously. Each bit of the > + 4 LSBs represent one DTEST rail, such as [3:0] = 0101 > + means both DTEST1 and DTEST3 are selected. I'm not too keen on encoding this as a bitmask. I would much rather encode it as multiple values - although that complicates the implementation. Or can we just ignore it? (Is the lack of support in the newer chips a result of no-one using this?) > + > Example: > > pm8921_gpio: gpio@150 { > diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c [..] > @@ -512,6 +526,13 @@ static int pmic_gpio_config_set(struct pinctrl_dev *pctldev, unsigned int pin, > return -EINVAL; > pad->atest = arg; > break; > + case PMIC_GPIO_CONF_DTEST_BUFFER: > + if ((pad->lv_mv_type && arg > PMIC_GPIO_DIN_DTEST4) > + || (!pad->lv_mv_type && arg > > + PMIC_GPIO_DIG_IN_DTEST_SEL_MASK)) > + return -EINVAL; > + pad->dtest_buffer = arg; > + break; > default: > return -EINVAL; > } > @@ -544,6 +565,17 @@ static int pmic_gpio_config_set(struct pinctrl_dev *pctldev, unsigned int pin, > val = PMIC_GPIO_MODE_DIGITAL_OUTPUT; > } > Remember that you're supposed to be able to have two different states defines, one with dtest-buffer and one without - and switching between them should enable _and_ disable the dtest buffer. So you need to detect the absence of qcom,dtest-buffer and you need to write the register in this case as well. So before looping over all the config parameters, set pad->dtest_buffer to "disabled" and when you get here it will either be disabled or have the specified value. > + if (pad->dtest_buffer != INT_MAX) { > + val = pad->dtest_buffer; > + if (pad->lv_mv_type) > + val |= PMIC_GPIO_LV_MV_DIG_IN_DTEST_EN; > + Instead of representing "disabled" as INT_MAX, you could keep track of it in the same representation as the hardware, i.e. 0 would be disabled. The additional effort would be in the lv_mv case that you need to mask in PMIC_GPIO_LV_MV_DIG_IN_DTEST_EN in the few places where you actually enable dtest buffering. > + ret = pmic_gpio_write(state, pad, > + PMIC_GPIO_REG_DIG_IN_CTL, val); > + if (ret < 0) > + return ret; > + } > + [..] > diff --git a/include/dt-bindings/pinctrl/qcom,pmic-gpio.h b/include/dt-bindings/pinctrl/qcom,pmic-gpio.h > index 85d8809..21738ed 100644 > --- a/include/dt-bindings/pinctrl/qcom,pmic-gpio.h > +++ b/include/dt-bindings/pinctrl/qcom,pmic-gpio.h > @@ -99,6 +99,12 @@ > #define PMIC_GPIO_AOUT_ATEST3 2 > #define PMIC_GPIO_AOUT_ATEST4 3 > > +/* DTEST buffer for digital input mode */ > +#define PMIC_GPIO_DIN_DTEST1 0 > +#define PMIC_GPIO_DIN_DTEST2 1 > +#define PMIC_GPIO_DIN_DTEST3 2 > +#define PMIC_GPIO_DIN_DTEST4 3 > + Please drop these defines. Regards, Bjorn