Message-ID: <20170712205538.GJ1618@tuxbook>
Date:   Wed, 12 Jul 2017 13:55:38 -0700
From:   Bjorn Andersson <bjorn.andersson@...aro.org>
To:     fenglinw@...eaurora.org
Cc:     linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
        Linus Walleij <linus.walleij@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Andy Gross <andy.gross@...aro.org>,
        David Brown <david.brown@...aro.org>,
        Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        linux-gpio@...r.kernel.org, devicetree@...r.kernel.org,
        linux-soc@...r.kernel.org, collinsd@...cinc.com,
        aghayal@....qualcomm.com, wruan@...cinc.com,
        kgunda@....qualcomm.com
Subject: Re: [PATCH V1 1/3] pinctrl: qcom: spmi-gpio: Add support for GPIO
 LV/MV subtype

On Mon 12 Jun 23:16 PDT 2017, fenglinw@...eaurora.org wrote:

> From: Fenglin Wu <fenglinw@...eaurora.org>
> 
> 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 <fenglinw@...eaurora.org>
> ---
>  .../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: <string>
>  	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: <none>
>  	Definition: The specified pins are configured in open-source mode.
>  
> +- qcom,atest:
> +	Usage: optional
> +	Value type: <u32>
> +	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 <dt-bindings/pinctrl/qcom,pmic-gpio.h>.
> +
>  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

Powered by blists - more mailing lists