Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753326AbdGLUzt (ORCPT ); Wed, 12 Jul 2017 16:55:49 -0400 Received: from mail-pf0-f171.google.com ([209.85.192.171]:36247 "EHLO mail-pf0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752363AbdGLUzo (ORCPT ); Wed, 12 Jul 2017 16:55:44 -0400 Date: Wed, 12 Jul 2017 13:55:38 -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 1/3] pinctrl: qcom: spmi-gpio: Add support for GPIO LV/MV subtype Message-ID: <20170712205538.GJ1618@tuxbook> References: <20170613061707.13892-1-fenglinw@codeaurora.org> <20170613061707.13892-2-fenglinw@codeaurora.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170613061707.13892-2-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: 21323 Lines: 632 On Mon 12 Jun 23:16 PDT 2017, fenglinw@codeaurora.org wrote: > From: Fenglin Wu > > GPIO LV (low voltage)/MV (medium voltage) subtypes have different > features and register mappings than 4CH/8CH subtypes. Add support > for LV and MV subtypes. > > Signed-off-by: Fenglin Wu > --- > .../devicetree/bindings/pinctrl/qcom,pmic-gpio.txt | 28 ++- > drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 269 ++++++++++++++++++--- > include/dt-bindings/pinctrl/qcom,pmic-gpio.h | 9 + > 3 files changed, 264 insertions(+), 42 deletions(-) > > diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.txt b/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.txt > index 8d893a8..1493c0a 100644 > --- a/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.txt > +++ b/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.txt > @@ -91,14 +91,18 @@ to specify in a pin configuration subnode: > Value type: > Definition: Specify the alternative function to be configured for the > specified pins. Valid values are: > - "normal", > - "paired", > - "func1", > - "func2", > - "dtest1", > - "dtest2", > - "dtest3", > - "dtest4" > + "normal", > + "paired", > + "func1", > + "func2", > + "dtest1", > + "dtest2", > + "dtest3", > + "dtest4", > + And following values are supported by LV/MV GPIO subtypes: > + "func3", > + "func4", > + "analog" Please keep the indentation of the existing lines. > > - bias-disable: > Usage: optional > @@ -183,6 +187,14 @@ to specify in a pin configuration subnode: > Value type: > Definition: The specified pins are configured in open-source mode. > > +- qcom,atest: > + Usage: optional > + Value type: > + Definition: Selects ATEST rail to route to GPIO when it's configured > + in analog-pass-through mode by specifying "analog" function. > + Valid values are 0-3 corresponding to PMIC_GPIO_AOUT_ATESTx > + defined in . > + > Example: > > pm8921_gpio: gpio@150 { > diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c > index 664b641..caa07e9 100644 > --- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c > +++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c > @@ -40,6 +40,8 @@ > #define PMIC_GPIO_SUBTYPE_GPIOC_4CH 0x5 > #define PMIC_GPIO_SUBTYPE_GPIO_8CH 0x9 > #define PMIC_GPIO_SUBTYPE_GPIOC_8CH 0xd > +#define PMIC_GPIO_SUBTYPE_GPIO_LV 0x10 > +#define PMIC_GPIO_SUBTYPE_GPIO_MV 0x11 > > #define PMIC_MPP_REG_RT_STS 0x10 > #define PMIC_MPP_REG_RT_STS_VAL_MASK 0x1 > @@ -48,8 +50,10 @@ > #define PMIC_GPIO_REG_MODE_CTL 0x40 > #define PMIC_GPIO_REG_DIG_VIN_CTL 0x41 > #define PMIC_GPIO_REG_DIG_PULL_CTL 0x42 > +#define PMIC_GPIO_REG_LV_MV_DIG_OUT_SOURCE_CTL 0x44 > #define PMIC_GPIO_REG_DIG_OUT_CTL 0x45 > #define PMIC_GPIO_REG_EN_CTL 0x46 > +#define PMIC_GPIO_REG_LV_MV_ANA_PASS_THRU_SEL 0x4A > > /* PMIC_GPIO_REG_MODE_CTL */ > #define PMIC_GPIO_REG_MODE_VALUE_SHIFT 0x1 > @@ -58,6 +62,13 @@ > #define PMIC_GPIO_REG_MODE_DIR_SHIFT 4 > #define PMIC_GPIO_REG_MODE_DIR_MASK 0x7 > > +#define PMIC_GPIO_MODE_DIGITAL_INPUT 0 > +#define PMIC_GPIO_MODE_DIGITAL_OUTPUT 1 > +#define PMIC_GPIO_MODE_DIGITAL_INPUT_OUTPUT 2 > +#define PMIC_GPIO_MODE_ANALOG_PASS_THRU 3 > + > +#define PMIC_GPIO_REG_LV_MV_MODE_DIR_MASK 0x3 > + > /* PMIC_GPIO_REG_DIG_VIN_CTL */ > #define PMIC_GPIO_REG_VIN_SHIFT 0 > #define PMIC_GPIO_REG_VIN_MASK 0x7 > @@ -69,6 +80,11 @@ > #define PMIC_GPIO_PULL_DOWN 4 > #define PMIC_GPIO_PULL_DISABLE 5 > > +/* PMIC_GPIO_REG_LV_MV_DIG_OUT_SOURCE_CTL for LV/MV */ > +#define PMIC_GPIO_LV_MV_OUTPUT_INVERT 0x80 > +#define PMIC_GPIO_LV_MV_OUTPUT_INVERT_SHIFT 7 > +#define PMIC_GPIO_LV_MV_OUTPUT_SOURCE_SEL_MASK 0xF > + > /* PMIC_GPIO_REG_DIG_OUT_CTL */ > #define PMIC_GPIO_REG_OUT_STRENGTH_SHIFT 0 > #define PMIC_GPIO_REG_OUT_STRENGTH_MASK 0x3 > @@ -88,9 +104,28 @@ > > #define PMIC_GPIO_PHYSICAL_OFFSET 1 > > +/* PMIC_GPIO_REG_LV_MV_ANA_PASS_THRU_SEL */ > +#define PMIC_GPIO_LV_MV_ANA_MUX_SEL_MASK 0x3 > + > /* Qualcomm specific pin configurations */ > #define PMIC_GPIO_CONF_PULL_UP (PIN_CONFIG_END + 1) > #define PMIC_GPIO_CONF_STRENGTH (PIN_CONFIG_END + 2) > +#define PMIC_GPIO_CONF_ATEST (PIN_CONFIG_END + 3) > + > +/* The index of each function in pmic_gpio_functions[] array */ > +enum pmic_gpio_func_index { > + PMIC_GPIO_FUNC_INDEX_NORMAL = 0x00, > + PMIC_GPIO_FUNC_INDEX_PAIRED = 0x01, > + PMIC_GPIO_FUNC_INDEX_FUNC1 = 0x02, > + PMIC_GPIO_FUNC_INDEX_FUNC2 = 0x03, > + PMIC_GPIO_FUNC_INDEX_FUNC3 = 0x04, > + PMIC_GPIO_FUNC_INDEX_FUNC4 = 0x05, > + PMIC_GPIO_FUNC_INDEX_DTEST1 = 0x06, > + PMIC_GPIO_FUNC_INDEX_DTEST2 = 0x07, > + PMIC_GPIO_FUNC_INDEX_DTEST3 = 0x08, > + PMIC_GPIO_FUNC_INDEX_DTEST4 = 0x09, > + PMIC_GPIO_FUNC_INDEX_ANALOG = 0x10, As this is an enum you should not have to specify the value of each constant. The jump from 9 to 0x10 is confusing, but I presume it's to make the made up function "analog" end up outside the 4 bits of function selector available - which I'm not sure I like, see below. > +}; > > /** > * struct pmic_gpio_pad - keep current GPIO settings > @@ -102,12 +137,14 @@ > * open-drain or open-source mode. > * @output_enabled: Set to true if GPIO output logic is enabled. > * @input_enabled: Set to true if GPIO input buffer logic is enabled. > + * @lv_mv_type: Set to true if GPIO subtype is GPIO_LV(0x10) or GPIO_MV(0x11). > * @num_sources: Number of power-sources supported by this GPIO. > * @power_source: Current power-source used. > * @buffer_type: Push-pull, open-drain or open-source. > * @pullup: Constant current which flow trough GPIO output buffer. > * @strength: No, Low, Medium, High > * @function: See pmic_gpio_functions[] > + * @atest: the ATEST selection for GPIO analog-pass-through mode > */ > struct pmic_gpio_pad { > u16 base; > @@ -117,12 +154,14 @@ struct pmic_gpio_pad { > bool have_buffer; > bool output_enabled; > bool input_enabled; > + bool lv_mv_type; > unsigned int num_sources; > unsigned int power_source; > unsigned int buffer_type; > unsigned int pullup; > unsigned int strength; > unsigned int function; > + unsigned int atest; > }; > > struct pmic_gpio_state { > @@ -135,12 +174,14 @@ struct pmic_gpio_state { > static const struct pinconf_generic_params pmic_gpio_bindings[] = { > {"qcom,pull-up-strength", PMIC_GPIO_CONF_PULL_UP, 0}, > {"qcom,drive-strength", PMIC_GPIO_CONF_STRENGTH, 0}, > + {"qcom,atest", PMIC_GPIO_CONF_ATEST, 0}, > }; > > #ifdef CONFIG_DEBUG_FS > static const struct pin_config_item pmic_conf_items[ARRAY_SIZE(pmic_gpio_bindings)] = { > PCONFDUMP(PMIC_GPIO_CONF_PULL_UP, "pull up strength", NULL, true), > PCONFDUMP(PMIC_GPIO_CONF_STRENGTH, "drive-strength", NULL, true), > + PCONFDUMP(PMIC_GPIO_CONF_ATEST, "atest", NULL, true), > }; > #endif > > @@ -152,11 +193,25 @@ struct pmic_gpio_state { > "gpio30", "gpio31", "gpio32", "gpio33", "gpio34", "gpio35", "gpio36", > }; > > +/* > + * Treat LV/MV GPIO analog-pass-through mode as a function, add it > + * to the end of the function list. Add placeholder for the reserved > + * functions defined in LV/MV OUTPUT_SOURCE_SEL register. > + */ > static const char *const pmic_gpio_functions[] = { > - PMIC_GPIO_FUNC_NORMAL, PMIC_GPIO_FUNC_PAIRED, > - PMIC_GPIO_FUNC_FUNC1, PMIC_GPIO_FUNC_FUNC2, > - PMIC_GPIO_FUNC_DTEST1, PMIC_GPIO_FUNC_DTEST2, > - PMIC_GPIO_FUNC_DTEST3, PMIC_GPIO_FUNC_DTEST4, > + [PMIC_GPIO_FUNC_INDEX_NORMAL] = PMIC_GPIO_FUNC_NORMAL, > + [PMIC_GPIO_FUNC_INDEX_PAIRED] = PMIC_GPIO_FUNC_PAIRED, > + [PMIC_GPIO_FUNC_INDEX_FUNC1] = PMIC_GPIO_FUNC_FUNC1, > + [PMIC_GPIO_FUNC_INDEX_FUNC2] = PMIC_GPIO_FUNC_FUNC2, > + [PMIC_GPIO_FUNC_INDEX_FUNC3] = PMIC_GPIO_FUNC_FUNC3, > + [PMIC_GPIO_FUNC_INDEX_FUNC4] = PMIC_GPIO_FUNC_FUNC4, > + [PMIC_GPIO_FUNC_INDEX_DTEST1] = PMIC_GPIO_FUNC_DTEST1, > + [PMIC_GPIO_FUNC_INDEX_DTEST2] = PMIC_GPIO_FUNC_DTEST2, > + [PMIC_GPIO_FUNC_INDEX_DTEST3] = PMIC_GPIO_FUNC_DTEST3, > + [PMIC_GPIO_FUNC_INDEX_DTEST4] = PMIC_GPIO_FUNC_DTEST4, > + "reserved-a", "reserved-b", "reserved-c", > + "reserved-d", "reserved-e", "reserved-f", PMIC_GPIO_FUNC_INDEX_ANALOG is just a made up value kept within the driver, so there is no problem to change it in the future. Therefor I don't think you need to represent the reserved functions here. > + [PMIC_GPIO_FUNC_INDEX_ANALOG] = PMIC_GPIO_FUNC_ANALOG, I can see the value of saying 'function = "analog";' in DT, but I'm not sure that representing it inside the driver as a function is the best, please see below. > }; > > static int pmic_gpio_read(struct pmic_gpio_state *state, > @@ -248,21 +303,74 @@ static int pmic_gpio_set_mux(struct pinctrl_dev *pctldev, unsigned function, > > pad->function = function; > > - val = 0; > + val = PMIC_GPIO_MODE_DIGITAL_INPUT; > if (pad->output_enabled) { > if (pad->input_enabled) > - val = 2; > + val = PMIC_GPIO_MODE_DIGITAL_INPUT_OUTPUT; Giving these hard coded constants defines does look good, but are unrelated to the introduction of LV/MV support, so please split this out into its own patch. > else > - val = 1; > + val = PMIC_GPIO_MODE_DIGITAL_OUTPUT; > } > > - val = val << PMIC_GPIO_REG_MODE_DIR_SHIFT; > - val |= pad->function << PMIC_GPIO_REG_MODE_FUNCTION_SHIFT; > - val |= pad->out_value & PMIC_GPIO_REG_MODE_VALUE_SHIFT; > + if (function > PMIC_GPIO_FUNC_INDEX_DTEST4 && > + function < PMIC_GPIO_FUNC_INDEX_ANALOG) { > + pr_err("reserved function: %s hasn't been enabled\n", > + pmic_gpio_functions[function]); > + return -EINVAL; > + } This check for selecting an invalid function is new, but the problem exists before the introduction of LV/MV support. So please split this out in its own patch. > > - ret = pmic_gpio_write(state, pad, PMIC_GPIO_REG_MODE_CTL, val); > - if (ret < 0) > - return ret; > + if (pad->lv_mv_type) { > + if (pad->function == PMIC_GPIO_FUNC_INDEX_ANALOG) { I believe it would be cleaner if you represent "analog passthrough" in the driver by a boolean similar to "output_enable", and you give it highest priority. Above you would end up having: if (pad->analog_pass) val = 3; else if (pad->output_enabled && pad->input_enabled) val = 2; else if (pad->output) val = 1; else val = 0; Then the MODE_CTL part of these two blocks becomes the same and there's no harm in doing both the writes in both cases - so you could drop the inner if-statement all together. > + val = PMIC_GPIO_MODE_ANALOG_PASS_THRU; > + ret = pmic_gpio_write(state, pad, > + PMIC_GPIO_REG_MODE_CTL, val); > + if (ret < 0) > + return ret; > + > + ret = pmic_gpio_write(state, pad, > + PMIC_GPIO_REG_LV_MV_ANA_PASS_THRU_SEL, > + pad->atest); > + if (ret < 0) > + return ret; > + } else { > + ret = pmic_gpio_write(state, pad, > + PMIC_GPIO_REG_MODE_CTL, val); > + if (ret < 0) > + return ret; > + > + val = pad->out_value > + << PMIC_GPIO_LV_MV_OUTPUT_INVERT_SHIFT; > + val |= pad->function > + & PMIC_GPIO_LV_MV_OUTPUT_SOURCE_SEL_MASK; > + ret = pmic_gpio_write(state, pad, > + PMIC_GPIO_REG_LV_MV_DIG_OUT_SOURCE_CTL, val); > + if (ret < 0) > + return ret; > + } > + } else { > + /* > + * GPIO not of LV/MV subtype doesn't have "func3", "func4" > + * "analog" functions, and "dtest1" to "dtest4" functions > + * have register value 2 bits lower than the function index > + * in pmic_gpio_functions[]. > + */ > + if (function == PMIC_GPIO_FUNC_INDEX_FUNC3 > + || function == PMIC_GPIO_FUNC_INDEX_FUNC4 > + || function == PMIC_GPIO_FUNC_INDEX_ANALOG) { > + return -EINVAL; Please make sure you never reach this point with invalid function (i.e detect this at the top of the function). > + } else if (function >= PMIC_GPIO_FUNC_INDEX_DTEST1 && > + function <= PMIC_GPIO_FUNC_INDEX_DTEST4) { > + pad->function -= (PMIC_GPIO_FUNC_INDEX_DTEST1 - > + PMIC_GPIO_FUNC_INDEX_FUNC3); > + } > + > + val = val << PMIC_GPIO_REG_MODE_DIR_SHIFT; > + val |= pad->function << PMIC_GPIO_REG_MODE_FUNCTION_SHIFT; > + val |= pad->out_value & PMIC_GPIO_REG_MODE_VALUE_SHIFT; > + > + ret = pmic_gpio_write(state, pad, PMIC_GPIO_REG_MODE_CTL, val); > + if (ret < 0) > + return ret; > + } > > val = pad->is_enabled << PMIC_GPIO_REG_MASTER_EN_SHIFT; > > @@ -322,6 +430,9 @@ static int pmic_gpio_config_get(struct pinctrl_dev *pctldev, > case PMIC_GPIO_CONF_STRENGTH: > arg = pad->strength; > break; > + case PMIC_GPIO_CONF_ATEST: > + arg = pad->atest; > + break; > default: > return -EINVAL; > } > @@ -396,6 +507,11 @@ static int pmic_gpio_config_set(struct pinctrl_dev *pctldev, unsigned int pin, > return -EINVAL; > pad->strength = arg; > break; > + case PMIC_GPIO_CONF_ATEST: > + if (arg > PMIC_GPIO_AOUT_ATEST4) Please make this pad->lv_mv_type || arg > PMIC_GPIO_AOUT_ATEST4, to catch invalid (although ignored) configurations. > + return -EINVAL; > + pad->atest = arg; > + break; > default: > return -EINVAL; > } > @@ -420,19 +536,53 @@ static int pmic_gpio_config_set(struct pinctrl_dev *pctldev, unsigned int pin, > if (ret < 0) > return ret; > > - val = 0; > + val = PMIC_GPIO_MODE_DIGITAL_INPUT; > if (pad->output_enabled) { > if (pad->input_enabled) > - val = 2; > + val = PMIC_GPIO_MODE_DIGITAL_INPUT_OUTPUT; > else > - val = 1; > + val = PMIC_GPIO_MODE_DIGITAL_OUTPUT; > } > > - val = val << PMIC_GPIO_REG_MODE_DIR_SHIFT; > - val |= pad->function << PMIC_GPIO_REG_MODE_FUNCTION_SHIFT; > - val |= pad->out_value & PMIC_GPIO_REG_MODE_VALUE_SHIFT; > + if (pad->lv_mv_type) { > + if (pad->function == PMIC_GPIO_FUNC_INDEX_ANALOG) { Please make this follow the suggestion in set_mux() > + val = PMIC_GPIO_MODE_ANALOG_PASS_THRU; > + ret = pmic_gpio_write(state, pad, > + PMIC_GPIO_REG_MODE_CTL, val); > + if (ret < 0) > + return ret; > > - return pmic_gpio_write(state, pad, PMIC_GPIO_REG_MODE_CTL, val); > + ret = pmic_gpio_write(state, pad, > + PMIC_GPIO_REG_LV_MV_ANA_PASS_THRU_SEL, > + pad->atest); > + if (ret < 0) > + return ret; > + } else { > + ret = pmic_gpio_write(state, pad, > + PMIC_GPIO_REG_MODE_CTL, val); > + if (ret < 0) > + return ret; > + > + val = pad->out_value > + << PMIC_GPIO_LV_MV_OUTPUT_INVERT_SHIFT; > + val |= pad->function > + & PMIC_GPIO_LV_MV_OUTPUT_SOURCE_SEL_MASK; > + ret = pmic_gpio_write(state, pad, > + PMIC_GPIO_REG_LV_MV_DIG_OUT_SOURCE_CTL, val); > + if (ret < 0) > + return ret; > + } > + } else { > + val = val << PMIC_GPIO_REG_MODE_DIR_SHIFT; > + val |= pad->function << PMIC_GPIO_REG_MODE_FUNCTION_SHIFT; > + val |= pad->out_value & PMIC_GPIO_REG_MODE_VALUE_SHIFT; > + > + ret = pmic_gpio_write(state, pad, PMIC_GPIO_REG_MODE_CTL, val); > + if (ret < 0) > + return ret; > + } > + > + return ret; > } > > static void pmic_gpio_config_dbg_show(struct pinctrl_dev *pctldev, > @@ -440,7 +590,7 @@ static void pmic_gpio_config_dbg_show(struct pinctrl_dev *pctldev, > { > struct pmic_gpio_state *state = pinctrl_dev_get_drvdata(pctldev); > struct pmic_gpio_pad *pad; > - int ret, val; > + int ret, val, function; > > static const char *const biases[] = { > "pull-up 30uA", "pull-up 1.5uA", "pull-up 31.5uA", > @@ -471,9 +621,21 @@ static void pmic_gpio_config_dbg_show(struct pinctrl_dev *pctldev, > ret &= PMIC_MPP_REG_RT_STS_VAL_MASK; > pad->out_value = ret; > } > + /* > + * For GPIO not of LV/MV subtypes, the register value of > + * the function mapping from "dtest1" to "dtest4" is 2 bits "2 bits" means something else, so I would suggest something like: /* * For the non-LV/MV subtypes only 2 special functions are available, * offsetting the dtest values by two */ And then implement this as: function = pad->function; if (!pad->lv_mv_type && pad->function >= PMIC_GPIO_FUNC_INDEX_FUNC3) function -= PMIC_GPIO_FUNC_INDEX_DTEST1 - PMIC_GPIO_FUNC_INDEX_FUNC3; > + * lower than the function index in pmic_gpio_functions[]. > + */ > + if (!pad->lv_mv_type && > + pad->function >= PMIC_GPIO_FUNC_INDEX_FUNC3) { > + function = pad->function + (PMIC_GPIO_FUNC_INDEX_DTEST1 > + - PMIC_GPIO_FUNC_INDEX_FUNC3); > + } else { > + function = pad->function; > + } > > seq_printf(s, " %-4s", pad->output_enabled ? "out" : "in"); > - seq_printf(s, " %-7s", pmic_gpio_functions[pad->function]); > + seq_printf(s, " %-7s", pmic_gpio_functions[function]); > seq_printf(s, " vin-%d", pad->power_source); > seq_printf(s, " %-27s", biases[pad->pullup]); > seq_printf(s, " %-10s", buffer_types[pad->buffer_type]); > @@ -618,40 +780,72 @@ static int pmic_gpio_populate(struct pmic_gpio_state *state, > case PMIC_GPIO_SUBTYPE_GPIOC_8CH: > pad->num_sources = 8; > break; > + case PMIC_GPIO_SUBTYPE_GPIO_LV: > + pad->num_sources = 1; > + pad->have_buffer = true; > + pad->lv_mv_type = true; > + break; > + case PMIC_GPIO_SUBTYPE_GPIO_MV: > + pad->num_sources = 2; > + pad->have_buffer = true; > + pad->lv_mv_type = true; > + break; > default: > dev_err(state->dev, "unknown GPIO type 0x%x\n", subtype); > return -ENODEV; > } > > - val = pmic_gpio_read(state, pad, PMIC_GPIO_REG_MODE_CTL); > - if (val < 0) > - return val; > + if (pad->lv_mv_type) { > + val = pmic_gpio_read(state, pad, > + PMIC_GPIO_REG_LV_MV_DIG_OUT_SOURCE_CTL); > + if (val < 0) > + return val; > + > + pad->out_value = !!(val & PMIC_GPIO_LV_MV_OUTPUT_INVERT); > + pad->function = val & PMIC_GPIO_LV_MV_OUTPUT_SOURCE_SEL_MASK; > + > + val = pmic_gpio_read(state, pad, PMIC_GPIO_REG_MODE_CTL); > + if (val < 0) > + return val; > + > + dir = val & PMIC_GPIO_REG_LV_MV_MODE_DIR_MASK; > + } else { > + val = pmic_gpio_read(state, pad, PMIC_GPIO_REG_MODE_CTL); > + if (val < 0) > + return val; > + > + pad->out_value = val & PMIC_GPIO_REG_MODE_VALUE_SHIFT; > > - pad->out_value = val & PMIC_GPIO_REG_MODE_VALUE_SHIFT; > + dir = val >> PMIC_GPIO_REG_MODE_DIR_SHIFT; > + dir &= PMIC_GPIO_REG_MODE_DIR_MASK; > + pad->function = val >> PMIC_GPIO_REG_MODE_FUNCTION_SHIFT; > + pad->function &= PMIC_GPIO_REG_MODE_FUNCTION_MASK; > + } > > - dir = val >> PMIC_GPIO_REG_MODE_DIR_SHIFT; > - dir &= PMIC_GPIO_REG_MODE_DIR_MASK; > switch (dir) { > - case 0: > + case PMIC_GPIO_MODE_DIGITAL_INPUT: > pad->input_enabled = true; > pad->output_enabled = false; > break; > - case 1: > + case PMIC_GPIO_MODE_DIGITAL_OUTPUT: > pad->input_enabled = false; > pad->output_enabled = true; > break; > - case 2: > + case PMIC_GPIO_MODE_DIGITAL_INPUT_OUTPUT: > pad->input_enabled = true; > pad->output_enabled = true; > break; > + case PMIC_GPIO_MODE_ANALOG_PASS_THRU: > + if (pad->lv_mv_type) > + pad->function = PMIC_GPIO_FUNC_INDEX_ANALOG; > + else > + return -ENODEV; Please handle the invalid case first and leave the valid case unindented. > + break; > default: > dev_err(state->dev, "unknown GPIO direction\n"); > return -ENODEV; > } > > - pad->function = val >> PMIC_GPIO_REG_MODE_FUNCTION_SHIFT; > - pad->function &= PMIC_GPIO_REG_MODE_FUNCTION_MASK; > - > val = pmic_gpio_read(state, pad, PMIC_GPIO_REG_DIG_VIN_CTL); > if (val < 0) > return val; > @@ -676,6 +870,13 @@ static int pmic_gpio_populate(struct pmic_gpio_state *state, > pad->buffer_type = val >> PMIC_GPIO_REG_OUT_TYPE_SHIFT; > pad->buffer_type &= PMIC_GPIO_REG_OUT_TYPE_MASK; > > + if (pad->function == PMIC_GPIO_FUNC_INDEX_ANALOG) { I would suggest that you do this always on lv_mv_type. > + val = pmic_gpio_read(state, pad, > + PMIC_GPIO_REG_LV_MV_ANA_PASS_THRU_SEL); > + if (val < 0) > + return val; > + pad->atest = val & PMIC_GPIO_LV_MV_ANA_MUX_SEL_MASK; > + } And please leave an empty line between unrelated paragraphs of code. > /* Pin could be disabled with PIN_CONFIG_BIAS_HIGH_IMPEDANCE */ > pad->is_enabled = true; > return 0; > diff --git a/include/dt-bindings/pinctrl/qcom,pmic-gpio.h b/include/dt-bindings/pinctrl/qcom,pmic-gpio.h > index d33f17c..85d8809 100644 > --- a/include/dt-bindings/pinctrl/qcom,pmic-gpio.h > +++ b/include/dt-bindings/pinctrl/qcom,pmic-gpio.h > @@ -93,15 +93,24 @@ > #define PM8994_GPIO_S4 2 > #define PM8994_GPIO_L12 3 > > +/* ATEST MUX selection for analog-pass-through mode */ > +#define PMIC_GPIO_AOUT_ATEST1 0 > +#define PMIC_GPIO_AOUT_ATEST2 1 > +#define PMIC_GPIO_AOUT_ATEST3 2 > +#define PMIC_GPIO_AOUT_ATEST4 3 > + These values are natural numbers, so please use that in DeviceTree. Drop the defines and make the code translate the value when filling out pmic_gpio_pad->atest (so it has the same representation as the hardware). > /* To be used with "function" */ > #define PMIC_GPIO_FUNC_NORMAL "normal" > #define PMIC_GPIO_FUNC_PAIRED "paired" > #define PMIC_GPIO_FUNC_FUNC1 "func1" > #define PMIC_GPIO_FUNC_FUNC2 "func2" > +#define PMIC_GPIO_FUNC_FUNC3 "func3" > +#define PMIC_GPIO_FUNC_FUNC4 "func4" > #define PMIC_GPIO_FUNC_DTEST1 "dtest1" > #define PMIC_GPIO_FUNC_DTEST2 "dtest2" > #define PMIC_GPIO_FUNC_DTEST3 "dtest3" > #define PMIC_GPIO_FUNC_DTEST4 "dtest4" > +#define PMIC_GPIO_FUNC_ANALOG "analog" > Regards, Bjorn