* [PATCH V1 0/3] Add LV/MV subtypes support for QCOM PMIC GPIOs
@ 2017-06-13 6:16 fenglinw
2017-06-13 6:16 ` [PATCH V1 1/3] pinctrl: qcom: spmi-gpio: Add support for GPIO LV/MV subtype fenglinw
` (2 more replies)
0 siblings, 3 replies; 23+ messages in thread
From: fenglinw @ 2017-06-13 6:16 UTC (permalink / raw)
To: linux-arm-msm, linux-kernel; +Cc: collinsd, aghayal, wruan, kgunda, Fenglin Wu
From: Fenglin Wu <fenglinw@codeaurora.org>
LV (low voltage) and MV (medium voltage) subtype GPIO modules are support
in QCOM PMICs like PMI8998, PM660. These modules support addtional features
(func3, func4, analog-pass-through) and the register mappings are
also different compared with legacy 4CH/8CH subtypes.
These patches add the support for the new functions in LV/MV subtypes,
and do some feature extension and bug fixing.
Fenglin Wu (3):
pinctrl: qcom: spmi-gpio: Add support for GPIO LV/MV subtype
pinctrl: qcom: spmi-gpio: Add dtest route for digital input
pinctrl: qcom: spmi-gpio: Correct power_source range check
.../devicetree/bindings/pinctrl/qcom,pmic-gpio.txt | 43 ++-
drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 316 ++++++++++++++++++---
include/dt-bindings/pinctrl/qcom,pmic-gpio.h | 15 +
3 files changed, 331 insertions(+), 43 deletions(-)
--
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH V1 1/3] pinctrl: qcom: spmi-gpio: Add support for GPIO LV/MV subtype
2017-06-13 6:16 [PATCH V1 0/3] Add LV/MV subtypes support for QCOM PMIC GPIOs fenglinw
@ 2017-06-13 6:16 ` fenglinw
2017-06-18 14:04 ` Rob Herring
` (2 more replies)
2017-06-13 6:16 ` [PATCH V1 2/3] pinctrl: qcom: spmi-gpio: Add dtest route for digital input fenglinw
2017-06-13 6:16 ` [PATCH V1 3/3] pinctrl: qcom: spmi-gpio: Correct power_source range check fenglinw
2 siblings, 3 replies; 23+ messages in thread
From: fenglinw @ 2017-06-13 6:16 UTC (permalink / raw)
To: linux-arm-msm, linux-kernel, Linus Walleij, Rob Herring,
Mark Rutland, Andy Gross, David Brown, Fenglin Wu,
Srinivas Kandagatla, linux-gpio, devicetree, linux-soc
Cc: collinsd, aghayal, wruan, kgunda
From: Fenglin Wu <fenglinw@codeaurora.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@codeaurora.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"
- 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,
+};
/**
* 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] = PMIC_GPIO_FUNC_ANALOG,
};
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;
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;
+ }
- 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) {
+ 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;
+ } 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)
+ 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) {
+ 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
+ * 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;
+ 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) {
+ 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;
+ }
/* 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
+
/* 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"
#define PM8038_GPIO1_2_LPG_DRV PMIC_GPIO_FUNC_FUNC1
#define PM8038_GPIO3_5V_BOOST_EN PMIC_GPIO_FUNC_FUNC1
--
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH V1 2/3] pinctrl: qcom: spmi-gpio: Add dtest route for digital input
2017-06-13 6:16 [PATCH V1 0/3] Add LV/MV subtypes support for QCOM PMIC GPIOs fenglinw
2017-06-13 6:16 ` [PATCH V1 1/3] pinctrl: qcom: spmi-gpio: Add support for GPIO LV/MV subtype fenglinw
@ 2017-06-13 6:16 ` fenglinw
2017-06-18 14:04 ` Rob Herring
` (2 more replies)
2017-06-13 6:16 ` [PATCH V1 3/3] pinctrl: qcom: spmi-gpio: Correct power_source range check fenglinw
2 siblings, 3 replies; 23+ messages in thread
From: fenglinw @ 2017-06-13 6:16 UTC (permalink / raw)
To: linux-arm-msm, linux-kernel, Linus Walleij, Rob Herring,
Mark Rutland, Andy Gross, David Brown, Fenglin Wu,
Srinivas Kandagatla, linux-gpio, devicetree, linux-soc
Cc: collinsd, aghayal, wruan, kgunda
From: Fenglin Wu <fenglinw@codeaurora.org>
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 <fenglinw@codeaurora.org>
---
.../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 <dt-bindings/pinctrl/qcom,pmic-gpio.h>.
+- qcom,dtest-buffer:
+ Usage: optional
+ Value type: <u32>
+ 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
+ <dt-bindings/pinctrl/qcom,pmic-gpio.h>. Only one
+ DTEST rail can be selected at a time.
+ 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.
+
Example:
pm8921_gpio: gpio@150 {
diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
index caa07e9..581309d 100644
--- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
+++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
@@ -51,6 +51,7 @@
#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_IN_CTL 0x43
#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
@@ -85,6 +86,11 @@
#define PMIC_GPIO_LV_MV_OUTPUT_INVERT_SHIFT 7
#define PMIC_GPIO_LV_MV_OUTPUT_SOURCE_SEL_MASK 0xF
+/* PMIC_GPIO_REG_DIG_IN_CTL */
+#define PMIC_GPIO_LV_MV_DIG_IN_DTEST_EN 0x80
+#define PMIC_GPIO_LV_MV_DIG_IN_DTEST_SEL_MASK 0x7
+#define PMIC_GPIO_DIG_IN_DTEST_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
@@ -111,6 +117,7 @@
#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)
+#define PMIC_GPIO_CONF_DTEST_BUFFER (PIN_CONFIG_END + 4)
/* The index of each function in pmic_gpio_functions[] array */
enum pmic_gpio_func_index {
@@ -145,6 +152,8 @@ enum pmic_gpio_func_index {
* @strength: No, Low, Medium, High
* @function: See pmic_gpio_functions[]
* @atest: the ATEST selection for GPIO analog-pass-through mode
+ * @dtest_buffer: the DTEST buffer selection for digital input mode,
+ * the default value is INT_MAX if not used.
*/
struct pmic_gpio_pad {
u16 base;
@@ -162,6 +171,7 @@ struct pmic_gpio_pad {
unsigned int strength;
unsigned int function;
unsigned int atest;
+ unsigned int dtest_buffer;
};
struct pmic_gpio_state {
@@ -175,6 +185,7 @@ struct pmic_gpio_state {
{"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},
+ {"qcom,dtest-buffer", PMIC_GPIO_CONF_DTEST_BUFFER, 0},
};
#ifdef CONFIG_DEBUG_FS
@@ -433,6 +444,9 @@ static int pmic_gpio_config_get(struct pinctrl_dev *pctldev,
case PMIC_GPIO_CONF_ATEST:
arg = pad->atest;
break;
+ case PMIC_GPIO_CONF_DTEST_BUFFER:
+ arg = pad->dtest_buffer;
+ break;
default:
return -EINVAL;
}
@@ -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;
}
+ if (pad->dtest_buffer != INT_MAX) {
+ val = pad->dtest_buffer;
+ if (pad->lv_mv_type)
+ val |= PMIC_GPIO_LV_MV_DIG_IN_DTEST_EN;
+
+ ret = pmic_gpio_write(state, pad,
+ PMIC_GPIO_REG_DIG_IN_CTL, val);
+ if (ret < 0)
+ return ret;
+ }
+
if (pad->lv_mv_type) {
if (pad->function == PMIC_GPIO_FUNC_INDEX_ANALOG) {
val = PMIC_GPIO_MODE_ANALOG_PASS_THRU;
@@ -641,6 +673,8 @@ static void pmic_gpio_config_dbg_show(struct pinctrl_dev *pctldev,
seq_printf(s, " %-10s", buffer_types[pad->buffer_type]);
seq_printf(s, " %-4s", pad->out_value ? "high" : "low");
seq_printf(s, " %-7s", strengths[pad->strength]);
+ if (pad->dtest_buffer != INT_MAX)
+ seq_printf(s, " dtest buffer %d", pad->dtest_buffer);
}
}
@@ -860,6 +894,17 @@ static int pmic_gpio_populate(struct pmic_gpio_state *state,
pad->pullup = val >> PMIC_GPIO_REG_PULL_SHIFT;
pad->pullup &= PMIC_GPIO_REG_PULL_MASK;
+ val = pmic_gpio_read(state, pad, PMIC_GPIO_REG_DIG_IN_CTL);
+ if (val < 0)
+ return val;
+
+ if (pad->lv_mv_type && (val & PMIC_GPIO_LV_MV_DIG_IN_DTEST_EN))
+ pad->dtest_buffer = val & PMIC_GPIO_LV_MV_DIG_IN_DTEST_SEL_MASK;
+ else if (!pad->lv_mv_type)
+ pad->dtest_buffer = val & PMIC_GPIO_DIG_IN_DTEST_SEL_MASK;
+ else
+ pad->dtest_buffer = INT_MAX;
+
val = pmic_gpio_read(state, pad, PMIC_GPIO_REG_DIG_OUT_CTL);
if (val < 0)
return val;
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
+
/* To be used with "function" */
#define PMIC_GPIO_FUNC_NORMAL "normal"
#define PMIC_GPIO_FUNC_PAIRED "paired"
--
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH V1 3/3] pinctrl: qcom: spmi-gpio: Correct power_source range check
2017-06-13 6:16 [PATCH V1 0/3] Add LV/MV subtypes support for QCOM PMIC GPIOs fenglinw
2017-06-13 6:16 ` [PATCH V1 1/3] pinctrl: qcom: spmi-gpio: Add support for GPIO LV/MV subtype fenglinw
2017-06-13 6:16 ` [PATCH V1 2/3] pinctrl: qcom: spmi-gpio: Add dtest route for digital input fenglinw
@ 2017-06-13 6:16 ` fenglinw
2017-06-20 9:33 ` Linus Walleij
2017-07-12 21:33 ` Bjorn Andersson
2 siblings, 2 replies; 23+ messages in thread
From: fenglinw @ 2017-06-13 6:16 UTC (permalink / raw)
To: linux-arm-msm, linux-kernel, Andy Gross, David Brown,
Linus Walleij, linux-soc, linux-gpio
Cc: collinsd, aghayal, wruan, kgunda, Fenglin Wu
From: Fenglin Wu <fenglinw@codeaurora.org>
Power source selection in DIG_VIN_CTL is indexed from 0, in the range
check it shouldn't be equal to the total number of power sources.
Signed-off-by: Fenglin Wu <fenglinw@codeaurora.org>
---
drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
index 581309d..1fd677c 100644
--- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
+++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
@@ -500,7 +500,7 @@ static int pmic_gpio_config_set(struct pinctrl_dev *pctldev, unsigned int pin,
pad->is_enabled = false;
break;
case PIN_CONFIG_POWER_SOURCE:
- if (arg > pad->num_sources)
+ if (arg >= pad->num_sources)
return -EINVAL;
pad->power_source = arg;
break;
--
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH V1 1/3] pinctrl: qcom: spmi-gpio: Add support for GPIO LV/MV subtype
2017-06-13 6:16 ` [PATCH V1 1/3] pinctrl: qcom: spmi-gpio: Add support for GPIO LV/MV subtype fenglinw
@ 2017-06-18 14:04 ` Rob Herring
2017-06-19 1:00 ` Fenglin Wu
2017-06-20 11:15 ` Linus Walleij
[not found] ` <20170613061707.13892-2-fenglinw-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2 siblings, 1 reply; 23+ messages in thread
From: Rob Herring @ 2017-06-18 14:04 UTC (permalink / raw)
To: fenglinw
Cc: linux-arm-msm, linux-kernel, Linus Walleij, Mark Rutland,
Andy Gross, David Brown, Srinivas Kandagatla, linux-gpio,
devicetree, linux-soc, collinsd, aghayal, wruan, kgunda
On Tue, Jun 13, 2017 at 02:16:03PM +0800, fenglinw@codeaurora.org wrote:
> From: Fenglin Wu <fenglinw@codeaurora.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@codeaurora.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/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
> +
> /* 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"
defines for strings? That's really pointless. I'd prefer you drop using
them than add more.
Rob
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V1 2/3] pinctrl: qcom: spmi-gpio: Add dtest route for digital input
2017-06-13 6:16 ` [PATCH V1 2/3] pinctrl: qcom: spmi-gpio: Add dtest route for digital input fenglinw
@ 2017-06-18 14:04 ` Rob Herring
2017-06-20 11:14 ` Linus Walleij
[not found] ` <20170613061707.13892-3-fenglinw-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2 siblings, 0 replies; 23+ messages in thread
From: Rob Herring @ 2017-06-18 14:04 UTC (permalink / raw)
To: fenglinw
Cc: linux-arm-msm, linux-kernel, Linus Walleij, Mark Rutland,
Andy Gross, David Brown, Srinivas Kandagatla, linux-gpio,
devicetree, linux-soc, collinsd, aghayal, wruan, kgunda
On Tue, Jun 13, 2017 at 02:16:04PM +0800, fenglinw@codeaurora.org wrote:
> From: Fenglin Wu <fenglinw@codeaurora.org>
>
> 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 <fenglinw@codeaurora.org>
> ---
> .../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(+)
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V1 1/3] pinctrl: qcom: spmi-gpio: Add support for GPIO LV/MV subtype
2017-06-18 14:04 ` Rob Herring
@ 2017-06-19 1:00 ` Fenglin Wu
0 siblings, 0 replies; 23+ messages in thread
From: Fenglin Wu @ 2017-06-19 1:00 UTC (permalink / raw)
To: Rob Herring
Cc: linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Linus Walleij, Mark Rutland,
Andy Gross, David Brown, Srinivas Kandagatla,
linux-gpio-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-soc-u79uwXL29TY76Z2rM5mHXA,
collinsd-sgV2jX0FEOL9JmXXK+q4OQ, aghayal-sgV2jX0FEOL9JmXXK+q4OQ,
wruan-sgV2jX0FEOL9JmXXK+q4OQ, kgunda-sgV2jX0FEOL9JmXXK+q4OQ
On 6/18/2017 10:04 PM, Rob Herring wrote:
> On Tue, Jun 13, 2017 at 02:16:03PM +0800, fenglinw-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org wrote:
>> From: Fenglin Wu <fenglinw-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.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-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.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/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
>> +
>> /* 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"
>
> defines for strings? That's really pointless. I'd prefer you drop using
> them than add more.
>
Thanks for the suggestion, I will remove these string definitions in
next patch.
Does other part look good? I would post a new patch if no other comments.
> Rob
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\n
a Linux Foundation Collaborative Project.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V1 1/3] pinctrl: qcom: spmi-gpio: Add support for GPIO LV/MV subtype
@ 2017-06-19 1:00 ` Fenglin Wu
0 siblings, 0 replies; 23+ messages in thread
From: Fenglin Wu @ 2017-06-19 1:00 UTC (permalink / raw)
To: Rob Herring
Cc: linux-arm-msm, linux-kernel, Linus Walleij, Mark Rutland,
Andy Gross, David Brown, Srinivas Kandagatla, linux-gpio,
devicetree, linux-soc, collinsd, aghayal, wruan, kgunda
On 6/18/2017 10:04 PM, Rob Herring wrote:
> On Tue, Jun 13, 2017 at 02:16:03PM +0800, fenglinw@codeaurora.org wrote:
>> From: Fenglin Wu <fenglinw@codeaurora.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@codeaurora.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/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
>> +
>> /* 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"
>
> defines for strings? That's really pointless. I'd prefer you drop using
> them than add more.
>
Thanks for the suggestion, I will remove these string definitions in
next patch.
Does other part look good? I would post a new patch if no other comments.
> Rob
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\n
a Linux Foundation Collaborative Project.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V1 1/3] pinctrl: qcom: spmi-gpio: Add support for GPIO LV/MV subtype
2017-06-19 1:00 ` Fenglin Wu
(?)
@ 2017-06-19 2:07 ` Fenglin Wu
-1 siblings, 0 replies; 23+ messages in thread
From: Fenglin Wu @ 2017-06-19 2:07 UTC (permalink / raw)
To: Rob Herring
Cc: linux-arm-msm, linux-kernel, Linus Walleij, Mark Rutland,
Andy Gross, David Brown, Srinivas Kandagatla, linux-gpio,
devicetree, linux-soc, collinsd, aghayal, wruan, kgunda
On 6/19/2017 9:00 AM, Fenglin Wu wrote:
> On 6/18/2017 10:04 PM, Rob Herring wrote:
>> On Tue, Jun 13, 2017 at 02:16:03PM +0800, fenglinw@codeaurora.org wrote:
>>> From: Fenglin Wu <fenglinw@codeaurora.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@codeaurora.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/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
>>> +
>>> /* 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"
>>
>> defines for strings? That's really pointless. I'd prefer you drop using
>> them than add more.
>>
> Thanks for the suggestion, I will remove these string definitions in
> next patch.
> Does other part look good? I would post a new patch if no other comments.
>
Sorry, I hadn't noticed there are so many definitions depend on them. I
take my word back and I think it deserves more discussion.
The function names "func1/func2/func3/func4" defined for GPIO hardware
module are very generic. Each GPIO located in different PMICs would have
its special function according to different hardware design, such as:
batt_alarm, keypad_drv, div_clk, etc.
The dt-binding header file defines the PMIC GPIOs' special functions
which depending on these string definitions (samples list below). This
gives a good understanding to end user if they requires to set the GPIO
special function but not having a hardware specification to explain the
details.
If we remove these string definitions, we would have another place to
explain these mapping of "funcx" to any GPIOs' special functions in each
PMICs, would dt-binding document be a good place to have them?
#define PM8038_GPIO1_2_LPG_DRV PMIC_GPIO_FUNC_FUNC1
#define PM8038_GPIO3_5V_BOOST_EN PMIC_GPIO_FUNC_FUNC1
#define PM8038_GPIO4_SSBI_ALT_CLK PMIC_GPIO_FUNC_FUNC1
#define PM8038_GPIO5_6_EXT_REG_EN PMIC_GPIO_FUNC_FUNC1
#define PM8038_GPIO10_11_EXT_REG_EN PMIC_GPIO_FUNC_FUNC1
#define PM8038_GPIO6_7_CLK PMIC_GPIO_FUNC_FUNC1
...
>> Rob
>> --
>> To unsubscribe from this list: send the line "unsubscribe
>> linux-arm-msm" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
--
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V1 3/3] pinctrl: qcom: spmi-gpio: Correct power_source range check
2017-06-13 6:16 ` [PATCH V1 3/3] pinctrl: qcom: spmi-gpio: Correct power_source range check fenglinw
@ 2017-06-20 9:33 ` Linus Walleij
2017-07-12 21:33 ` Bjorn Andersson
1 sibling, 0 replies; 23+ messages in thread
From: Linus Walleij @ 2017-06-20 9:33 UTC (permalink / raw)
To: fenglinw, Bjorn Andersson
Cc: linux-arm-msm, linux-kernel, Andy Gross, David Brown,
open list:ARM/QUALCOMM SUPPORT, linux-gpio, collinsd, aghayal,
wruan, kgunda
On Tue, Jun 13, 2017 at 8:16 AM, <fenglinw@codeaurora.org> wrote:
> From: Fenglin Wu <fenglinw@codeaurora.org>
>
> Power source selection in DIG_VIN_CTL is indexed from 0, in the range
> check it shouldn't be equal to the total number of power sources.
>
> Signed-off-by: Fenglin Wu <fenglinw@codeaurora.org>
> ---
> drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> index 581309d..1fd677c 100644
> --- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> +++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> @@ -500,7 +500,7 @@ static int pmic_gpio_config_set(struct pinctrl_dev *pctldev, unsigned int pin,
> pad->is_enabled = false;
> break;
> case PIN_CONFIG_POWER_SOURCE:
> - if (arg > pad->num_sources)
> + if (arg >= pad->num_sources)
> return -EINVAL;
> pad->power_source = arg;
> break;
Björn can you ACK this?
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V1 2/3] pinctrl: qcom: spmi-gpio: Add dtest route for digital input
2017-06-13 6:16 ` [PATCH V1 2/3] pinctrl: qcom: spmi-gpio: Add dtest route for digital input fenglinw
2017-06-18 14:04 ` Rob Herring
@ 2017-06-20 11:14 ` Linus Walleij
[not found] ` <20170613061707.13892-3-fenglinw-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2 siblings, 0 replies; 23+ messages in thread
From: Linus Walleij @ 2017-06-20 11:14 UTC (permalink / raw)
To: fenglinw, Bjorn Andersson
Cc: linux-arm-msm, linux-kernel, Rob Herring, Mark Rutland,
Andy Gross, David Brown, Srinivas Kandagatla, linux-gpio,
devicetree, open list:ARM/QUALCOMM SUPPORT, collinsd, aghayal,
wruan, kgunda
Björn can you look at this patch?
...and make a patch adding yourself to MAINTAINERS for drivers/pinctrl/qcom/*
as so many people seem to miss including you on reviews.
Yours,
Linus Walleij
On Tue, Jun 13, 2017 at 8:16 AM, <fenglinw@codeaurora.org> wrote:
> From: Fenglin Wu <fenglinw@codeaurora.org>
>
> 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 <fenglinw@codeaurora.org>
> ---
> .../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 <dt-bindings/pinctrl/qcom,pmic-gpio.h>.
>
> +- qcom,dtest-buffer:
> + Usage: optional
> + Value type: <u32>
> + 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
> + <dt-bindings/pinctrl/qcom,pmic-gpio.h>. Only one
> + DTEST rail can be selected at a time.
> + 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.
> +
> Example:
>
> pm8921_gpio: gpio@150 {
> diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> index caa07e9..581309d 100644
> --- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> +++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> @@ -51,6 +51,7 @@
> #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_IN_CTL 0x43
> #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
> @@ -85,6 +86,11 @@
> #define PMIC_GPIO_LV_MV_OUTPUT_INVERT_SHIFT 7
> #define PMIC_GPIO_LV_MV_OUTPUT_SOURCE_SEL_MASK 0xF
>
> +/* PMIC_GPIO_REG_DIG_IN_CTL */
> +#define PMIC_GPIO_LV_MV_DIG_IN_DTEST_EN 0x80
> +#define PMIC_GPIO_LV_MV_DIG_IN_DTEST_SEL_MASK 0x7
> +#define PMIC_GPIO_DIG_IN_DTEST_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
> @@ -111,6 +117,7 @@
> #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)
> +#define PMIC_GPIO_CONF_DTEST_BUFFER (PIN_CONFIG_END + 4)
>
> /* The index of each function in pmic_gpio_functions[] array */
> enum pmic_gpio_func_index {
> @@ -145,6 +152,8 @@ enum pmic_gpio_func_index {
> * @strength: No, Low, Medium, High
> * @function: See pmic_gpio_functions[]
> * @atest: the ATEST selection for GPIO analog-pass-through mode
> + * @dtest_buffer: the DTEST buffer selection for digital input mode,
> + * the default value is INT_MAX if not used.
> */
> struct pmic_gpio_pad {
> u16 base;
> @@ -162,6 +171,7 @@ struct pmic_gpio_pad {
> unsigned int strength;
> unsigned int function;
> unsigned int atest;
> + unsigned int dtest_buffer;
> };
>
> struct pmic_gpio_state {
> @@ -175,6 +185,7 @@ struct pmic_gpio_state {
> {"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},
> + {"qcom,dtest-buffer", PMIC_GPIO_CONF_DTEST_BUFFER, 0},
> };
>
> #ifdef CONFIG_DEBUG_FS
> @@ -433,6 +444,9 @@ static int pmic_gpio_config_get(struct pinctrl_dev *pctldev,
> case PMIC_GPIO_CONF_ATEST:
> arg = pad->atest;
> break;
> + case PMIC_GPIO_CONF_DTEST_BUFFER:
> + arg = pad->dtest_buffer;
> + break;
> default:
> return -EINVAL;
> }
> @@ -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;
> }
>
> + if (pad->dtest_buffer != INT_MAX) {
> + val = pad->dtest_buffer;
> + if (pad->lv_mv_type)
> + val |= PMIC_GPIO_LV_MV_DIG_IN_DTEST_EN;
> +
> + ret = pmic_gpio_write(state, pad,
> + PMIC_GPIO_REG_DIG_IN_CTL, val);
> + if (ret < 0)
> + return ret;
> + }
> +
> if (pad->lv_mv_type) {
> if (pad->function == PMIC_GPIO_FUNC_INDEX_ANALOG) {
> val = PMIC_GPIO_MODE_ANALOG_PASS_THRU;
> @@ -641,6 +673,8 @@ static void pmic_gpio_config_dbg_show(struct pinctrl_dev *pctldev,
> seq_printf(s, " %-10s", buffer_types[pad->buffer_type]);
> seq_printf(s, " %-4s", pad->out_value ? "high" : "low");
> seq_printf(s, " %-7s", strengths[pad->strength]);
> + if (pad->dtest_buffer != INT_MAX)
> + seq_printf(s, " dtest buffer %d", pad->dtest_buffer);
> }
> }
>
> @@ -860,6 +894,17 @@ static int pmic_gpio_populate(struct pmic_gpio_state *state,
> pad->pullup = val >> PMIC_GPIO_REG_PULL_SHIFT;
> pad->pullup &= PMIC_GPIO_REG_PULL_MASK;
>
> + val = pmic_gpio_read(state, pad, PMIC_GPIO_REG_DIG_IN_CTL);
> + if (val < 0)
> + return val;
> +
> + if (pad->lv_mv_type && (val & PMIC_GPIO_LV_MV_DIG_IN_DTEST_EN))
> + pad->dtest_buffer = val & PMIC_GPIO_LV_MV_DIG_IN_DTEST_SEL_MASK;
> + else if (!pad->lv_mv_type)
> + pad->dtest_buffer = val & PMIC_GPIO_DIG_IN_DTEST_SEL_MASK;
> + else
> + pad->dtest_buffer = INT_MAX;
> +
> val = pmic_gpio_read(state, pad, PMIC_GPIO_REG_DIG_OUT_CTL);
> if (val < 0)
> return val;
> 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
> +
> /* To be used with "function" */
> #define PMIC_GPIO_FUNC_NORMAL "normal"
> #define PMIC_GPIO_FUNC_PAIRED "paired"
> --
> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project.
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V1 1/3] pinctrl: qcom: spmi-gpio: Add support for GPIO LV/MV subtype
2017-06-13 6:16 ` [PATCH V1 1/3] pinctrl: qcom: spmi-gpio: Add support for GPIO LV/MV subtype fenglinw
2017-06-18 14:04 ` Rob Herring
@ 2017-06-20 11:15 ` Linus Walleij
2017-07-05 23:51 ` Fenglin Wu
[not found] ` <20170613061707.13892-2-fenglinw-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2 siblings, 1 reply; 23+ messages in thread
From: Linus Walleij @ 2017-06-20 11:15 UTC (permalink / raw)
To: fenglinw, Bjorn Andersson, Ivan Ivanov
Cc: linux-arm-msm, linux-kernel, Rob Herring, Mark Rutland,
Andy Gross, David Brown, Srinivas Kandagatla, linux-gpio,
devicetree, open list:ARM/QUALCOMM SUPPORT, collinsd, aghayal,
wruan, kgunda
Bjrön and/or Ivan: please look at this.
Yours,
Linus Walleij
On Tue, Jun 13, 2017 at 8:16 AM, <fenglinw@codeaurora.org> wrote:
> From: Fenglin Wu <fenglinw@codeaurora.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@codeaurora.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"
>
> - 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,
> +};
>
> /**
> * 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] = PMIC_GPIO_FUNC_ANALOG,
> };
>
> 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;
> 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;
> + }
>
> - 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) {
> + 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;
> + } 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)
> + 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) {
> + 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
> + * 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;
> + 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) {
> + 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;
> + }
> /* 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
> +
> /* 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"
>
> #define PM8038_GPIO1_2_LPG_DRV PMIC_GPIO_FUNC_FUNC1
> #define PM8038_GPIO3_5V_BOOST_EN PMIC_GPIO_FUNC_FUNC1
> --
> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project.
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V1 1/3] pinctrl: qcom: spmi-gpio: Add support for GPIO LV/MV subtype
2017-06-20 11:15 ` Linus Walleij
@ 2017-07-05 23:51 ` Fenglin Wu
2017-07-06 5:02 ` Bjorn Andersson
0 siblings, 1 reply; 23+ messages in thread
From: Fenglin Wu @ 2017-07-05 23:51 UTC (permalink / raw)
To: Linus Walleij, Bjorn Andersson, Ivan Ivanov
Cc: linux-arm-msm, linux-kernel, Rob Herring, Mark Rutland,
Andy Gross, David Brown, Srinivas Kandagatla, linux-gpio,
devicetree, open list:ARM/QUALCOMM SUPPORT, collinsd, aghayal,
wruan, kgunda
Hi Bjorn and Ivan,
Could you help to take some time to look at these spmi-gpio pinctrl
patches?
Thanks.
On 6/20/2017 7:15 PM, Linus Walleij wrote:
> Bjrön and/or Ivan: please look at this.
>
> Yours,
> Linus Walleij
>
> On Tue, Jun 13, 2017 at 8:16 AM, <fenglinw@codeaurora.org> wrote:
>
>> From: Fenglin Wu <fenglinw@codeaurora.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@codeaurora.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"
>>
>> - 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,
>> +};
>>
>> /**
>> * 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] = PMIC_GPIO_FUNC_ANALOG,
>> };
>>
>> 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;
>> 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;
>> + }
>>
>> - 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) {
>> + 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;
>> + } 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)
>> + 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) {
>> + 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
>> + * 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;
>> + 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) {
>> + 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;
>> + }
>> /* 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
>> +
>> /* 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"
>>
>> #define PM8038_GPIO1_2_LPG_DRV PMIC_GPIO_FUNC_FUNC1
>> #define PM8038_GPIO3_5V_BOOST_EN PMIC_GPIO_FUNC_FUNC1
>> --
>> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project.
>>
--
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V1 1/3] pinctrl: qcom: spmi-gpio: Add support for GPIO LV/MV subtype
2017-07-05 23:51 ` Fenglin Wu
@ 2017-07-06 5:02 ` Bjorn Andersson
0 siblings, 0 replies; 23+ messages in thread
From: Bjorn Andersson @ 2017-07-06 5:02 UTC (permalink / raw)
To: Fenglin Wu
Cc: Linus Walleij, Ivan Ivanov, linux-arm-msm, linux-kernel,
Rob Herring, Mark Rutland, Andy Gross, David Brown,
Srinivas Kandagatla, linux-gpio, devicetree,
open list:ARM/QUALCOMM SUPPORT, collinsd, aghayal, wruan, kgunda
On Wed 05 Jul 16:51 PDT 2017, Fenglin Wu wrote:
> Hi Bjorn and Ivan,
>
> Could you help to take some time to look at these spmi-gpio pinctrl
> patches?
> Thanks.
Hi Fenglin,
Sorry for not getting to this earlier, I'll try to finalize my review of
this tomorrow.
Regards,
Bjorn
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V1 1/3] pinctrl: qcom: spmi-gpio: Add support for GPIO LV/MV subtype
2017-06-13 6:16 ` [PATCH V1 1/3] pinctrl: qcom: spmi-gpio: Add support for GPIO LV/MV subtype fenglinw
@ 2017-07-12 20:55 ` Bjorn Andersson
2017-06-20 11:15 ` Linus Walleij
[not found] ` <20170613061707.13892-2-fenglinw-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2 siblings, 0 replies; 23+ messages in thread
From: Bjorn Andersson @ 2017-07-12 20:55 UTC (permalink / raw)
To: fenglinw-sgV2jX0FEOL9JmXXK+q4OQ
Cc: linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Linus Walleij, Rob Herring,
Mark Rutland, Andy Gross, David Brown, Srinivas Kandagatla,
linux-gpio-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-soc-u79uwXL29TY76Z2rM5mHXA,
collinsd-jfJNa2p1gH1BDgjK7y7TUQ, aghayal-Rm6X0d1/PG5y9aJCnZT0Uw,
wruan-jfJNa2p1gH1BDgjK7y7TUQ, kgunda-Rm6X0d1/PG5y9aJCnZT0Uw
On Mon 12 Jun 23:16 PDT 2017, fenglinw-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org wrote:
> From: Fenglin Wu <fenglinw-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.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-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V1 1/3] pinctrl: qcom: spmi-gpio: Add support for GPIO LV/MV subtype
@ 2017-07-12 20:55 ` Bjorn Andersson
0 siblings, 0 replies; 23+ messages in thread
From: Bjorn Andersson @ 2017-07-12 20:55 UTC (permalink / raw)
To: fenglinw
Cc: linux-arm-msm, linux-kernel, Linus Walleij, Rob Herring,
Mark Rutland, Andy Gross, David Brown, Srinivas Kandagatla,
linux-gpio, devicetree, linux-soc, collinsd, aghayal, wruan,
kgunda
On Mon 12 Jun 23:16 PDT 2017, fenglinw@codeaurora.org wrote:
> From: Fenglin Wu <fenglinw@codeaurora.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@codeaurora.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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V1 2/3] pinctrl: qcom: spmi-gpio: Add dtest route for digital input
2017-06-13 6:16 ` [PATCH V1 2/3] pinctrl: qcom: spmi-gpio: Add dtest route for digital input fenglinw
@ 2017-07-12 21:24 ` Bjorn Andersson
2017-06-20 11:14 ` Linus Walleij
[not found] ` <20170613061707.13892-3-fenglinw-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2 siblings, 0 replies; 23+ messages in thread
From: Bjorn Andersson @ 2017-07-12 21:24 UTC (permalink / raw)
To: fenglinw-sgV2jX0FEOL9JmXXK+q4OQ
Cc: linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Linus Walleij, Rob Herring,
Mark Rutland, Andy Gross, David Brown, Srinivas Kandagatla,
linux-gpio-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-soc-u79uwXL29TY76Z2rM5mHXA,
collinsd-jfJNa2p1gH1BDgjK7y7TUQ, aghayal-Rm6X0d1/PG5y9aJCnZT0Uw,
wruan-jfJNa2p1gH1BDgjK7y7TUQ, kgunda-Rm6X0d1/PG5y9aJCnZT0Uw
On Mon 12 Jun 23:16 PDT 2017, fenglinw-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org wrote:
> From: Fenglin Wu <fenglinw-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>
> 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 <fenglinw-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> ---
> .../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 <dt-bindings/pinctrl/qcom,pmic-gpio.h>.
>
> +- qcom,dtest-buffer:
> + Usage: optional
> + Value type: <u32>
> + 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
> + <dt-bindings/pinctrl/qcom,pmic-gpio.h>. 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V1 2/3] pinctrl: qcom: spmi-gpio: Add dtest route for digital input
@ 2017-07-12 21:24 ` Bjorn Andersson
0 siblings, 0 replies; 23+ messages in thread
From: Bjorn Andersson @ 2017-07-12 21:24 UTC (permalink / raw)
To: fenglinw
Cc: linux-arm-msm, linux-kernel, Linus Walleij, Rob Herring,
Mark Rutland, Andy Gross, David Brown, Srinivas Kandagatla,
linux-gpio, devicetree, linux-soc, collinsd, aghayal, wruan,
kgunda
On Mon 12 Jun 23:16 PDT 2017, fenglinw@codeaurora.org wrote:
> From: Fenglin Wu <fenglinw@codeaurora.org>
>
> 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 <fenglinw@codeaurora.org>
> ---
> .../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 <dt-bindings/pinctrl/qcom,pmic-gpio.h>.
>
> +- qcom,dtest-buffer:
> + Usage: optional
> + Value type: <u32>
> + 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
> + <dt-bindings/pinctrl/qcom,pmic-gpio.h>. 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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V1 3/3] pinctrl: qcom: spmi-gpio: Correct power_source range check
2017-06-13 6:16 ` [PATCH V1 3/3] pinctrl: qcom: spmi-gpio: Correct power_source range check fenglinw
2017-06-20 9:33 ` Linus Walleij
@ 2017-07-12 21:33 ` Bjorn Andersson
2017-07-13 5:55 ` Fenglin Wu
1 sibling, 1 reply; 23+ messages in thread
From: Bjorn Andersson @ 2017-07-12 21:33 UTC (permalink / raw)
To: fenglinw, Linus Walleij
Cc: linux-arm-msm, linux-kernel, Andy Gross, David Brown, linux-soc,
linux-gpio, collinsd, aghayal, wruan, kgunda
On Mon 12 Jun 23:16 PDT 2017, fenglinw@codeaurora.org wrote:
> From: Fenglin Wu <fenglinw@codeaurora.org>
>
> Power source selection in DIG_VIN_CTL is indexed from 0, in the range
> check it shouldn't be equal to the total number of power sources.
>
> Signed-off-by: Fenglin Wu <fenglinw@codeaurora.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
This patch is unrelated to the other patches in the series, when this is
the case it's better to send it on its own.
Regards,
Bjorn
> ---
> drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> index 581309d..1fd677c 100644
> --- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> +++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> @@ -500,7 +500,7 @@ static int pmic_gpio_config_set(struct pinctrl_dev *pctldev, unsigned int pin,
> pad->is_enabled = false;
> break;
> case PIN_CONFIG_POWER_SOURCE:
> - if (arg > pad->num_sources)
> + if (arg >= pad->num_sources)
> return -EINVAL;
> pad->power_source = arg;
> break;
> --
> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project.
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V1 1/3] pinctrl: qcom: spmi-gpio: Add support for GPIO LV/MV subtype
2017-07-12 20:55 ` Bjorn Andersson
(?)
@ 2017-07-13 3:21 ` Fenglin Wu
-1 siblings, 0 replies; 23+ messages in thread
From: Fenglin Wu @ 2017-07-13 3:21 UTC (permalink / raw)
To: Bjorn Andersson
Cc: linux-arm-msm, linux-kernel, Linus Walleij, Rob Herring,
Mark Rutland, Andy Gross, David Brown, Srinivas Kandagatla,
linux-gpio, devicetree, linux-soc, collinsd, aghayal, wruan,
kgunda
On 7/13/2017 4:55 AM, Bjorn Andersson wrote:
> On Mon 12 Jun 23:16 PDT 2017, fenglinw@codeaurora.org wrote:
>
>> From: Fenglin Wu <fenglinw@codeaurora.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@codeaurora.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.
Sure, I will fix the indent.
>
>>
>> - 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.
>
Yes, I made up the "analog" function and append it at the end of
function list, 0xa ~ 0xf is reserved and 0xf is the max
possible function index so I assign "analog" as 0x10 which won't cause
any conflict if the reserved bits are used in next hw generation.
Analog-pass-through is a mode defined in the new LV/MV GPIO sub-type
besides to input/output/input&&output, but it actually acting as special
functions which need to connect to any ATESTx lines, it's very similar
to the DTESTx functions listed above. To me, it's more reasonable than
adding a setting of "analog-pass-through" with a "normal" function.
>> +};
>>
>> /**
>> * 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.
I agree adding "analog" function would bring any code complexity but it
won't that much.
>
>> };
>>
>> 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.
>
These changes (replace the hard code as definitions, checking invalid
function) are along with the LV/MV subtype patch. I admit I won't change
that if my change won't touch this part at all. It's good to have them
but I am not sure if it worth to have an independent patch for them.
>>
>> - 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.
>
I agree this is straightforward, I would change it only if you still
disagree on adding it as "anglog" function.
>> + 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).
Sure, I will place it along with the invalid function checking at the
beginning.
>
>> + } 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.
>
Sure.
>> + 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;
>
Thanks for the suggestion. This looks better.
>> + * 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.
>
Sure.
>> + 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.
ATEST mux selection is only valid when the GPIO is used as
analog-pass-through mode. Any other modes (input/output) have other
sources (normal level, dtest lines, special func) and reading ATEST
lines out for them is not useful.
>
>> + 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).
Do you mean assign atest to 1 ~ 4 in devicetree and the code translate
the value to 0~3 for hardware programming?
>
>> /* 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
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V1 2/3] pinctrl: qcom: spmi-gpio: Add dtest route for digital input
2017-07-12 21:24 ` Bjorn Andersson
(?)
@ 2017-07-13 5:27 ` Fenglin Wu
2017-07-14 18:19 ` Bjorn Andersson
-1 siblings, 1 reply; 23+ messages in thread
From: Fenglin Wu @ 2017-07-13 5:27 UTC (permalink / raw)
To: Bjorn Andersson
Cc: linux-arm-msm, linux-kernel, Linus Walleij, Rob Herring,
Mark Rutland, Andy Gross, David Brown, Srinivas Kandagatla,
linux-gpio, devicetree, linux-soc, collinsd, aghayal, wruan,
kgunda
On 7/13/2017 5:24 AM, Bjorn Andersson wrote:
> On Mon 12 Jun 23:16 PDT 2017, fenglinw@codeaurora.org wrote:
>
>> From: Fenglin Wu <fenglinw@codeaurora.org>
>>
>> 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 <fenglinw@codeaurora.org>
>> ---
>> .../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 <dt-bindings/pinctrl/qcom,pmic-gpio.h>.
>>
>> +- qcom,dtest-buffer:
>> + Usage: optional
>> + Value type: <u32>
>> + 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
>> + <dt-bindings/pinctrl/qcom,pmic-gpio.h>. 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?)
I am not quite sure if any real cases would route multiple DTEST line to
single GPIO, but the register mapping uses the bit mask for 4CH/8CH
subtypes and I think we should support it accordingly. Even if I drop
the support, we still have the difference of the register mapping on the
dtest selection between MV/LV subtypes and legacy 4CH/8CH subtypes,
which means we need a place to unify the dtest definition. I put the
complication here in dtsi which the end user would choose the right
value according to the hardware. I am also fine with putting the
complication in C code, but that would drop the supporting of multiple
DTEST lines selection for 4CH/8CH subtype.
>
>> +
>> 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.
>
Thanks for your suggestion, I got the issue here.
PMIC_GPIO_LV_MV_DIG_IN_DTEST_EN need to be unmasked in the case of dtest
disabling.
If we decided to drop the multiple dtest line selection (actually I am
fine with it) and unify the dtest lines values to 1~4 from dtsi, I am Ok
with the suggestion of using 0 to represent "disabled".
>> + 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
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V1 3/3] pinctrl: qcom: spmi-gpio: Correct power_source range check
2017-07-12 21:33 ` Bjorn Andersson
@ 2017-07-13 5:55 ` Fenglin Wu
0 siblings, 0 replies; 23+ messages in thread
From: Fenglin Wu @ 2017-07-13 5:55 UTC (permalink / raw)
To: Bjorn Andersson, Linus Walleij
Cc: linux-arm-msm, linux-kernel, Andy Gross, David Brown, linux-soc,
linux-gpio, collinsd, aghayal, wruan, kgunda
On 7/13/2017 5:33 AM, Bjorn Andersson wrote:
> On Mon 12 Jun 23:16 PDT 2017, fenglinw@codeaurora.org wrote:
>
>> From: Fenglin Wu <fenglinw@codeaurora.org>
>>
>> Power source selection in DIG_VIN_CTL is indexed from 0, in the range
>> check it shouldn't be equal to the total number of power sources.
>>
>> Signed-off-by: Fenglin Wu <fenglinw@codeaurora.org>
>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>
> This patch is unrelated to the other patches in the series, when this is
> the case it's better to send it on its own.
>
> Regards,
> Bjorn
>
Sure, I will send it as an independent patch.
>> ---
>> drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
>> index 581309d..1fd677c 100644
>> --- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
>> +++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
>> @@ -500,7 +500,7 @@ static int pmic_gpio_config_set(struct pinctrl_dev *pctldev, unsigned int pin,
>> pad->is_enabled = false;
>> break;
>> case PIN_CONFIG_POWER_SOURCE:
>> - if (arg > pad->num_sources)
>> + if (arg >= pad->num_sources)
>> return -EINVAL;
>> pad->power_source = arg;
>> break;
>> --
>> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project.
>>
--
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V1 2/3] pinctrl: qcom: spmi-gpio: Add dtest route for digital input
2017-07-13 5:27 ` Fenglin Wu
@ 2017-07-14 18:19 ` Bjorn Andersson
0 siblings, 0 replies; 23+ messages in thread
From: Bjorn Andersson @ 2017-07-14 18:19 UTC (permalink / raw)
To: Fenglin Wu
Cc: linux-arm-msm, linux-kernel, Linus Walleij, Rob Herring,
Mark Rutland, Andy Gross, David Brown, Srinivas Kandagatla,
linux-gpio, devicetree, linux-soc, collinsd, aghayal, wruan,
kgunda
On Wed 12 Jul 22:27 PDT 2017, Fenglin Wu wrote:
> On 7/13/2017 5:24 AM, Bjorn Andersson wrote:
> > On Mon 12 Jun 23:16 PDT 2017, fenglinw@codeaurora.org wrote:
> >
> > > From: Fenglin Wu <fenglinw@codeaurora.org>
> > >
> > > 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 <fenglinw@codeaurora.org>
> > > ---
> > > .../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 <dt-bindings/pinctrl/qcom,pmic-gpio.h>.
> > > +- qcom,dtest-buffer:
> > > + Usage: optional
> > > + Value type: <u32>
> > > + 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
> > > + <dt-bindings/pinctrl/qcom,pmic-gpio.h>. 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?)
>
> I am not quite sure if any real cases would route multiple DTEST line to
> single GPIO, but the register mapping uses the bit mask for 4CH/8CH
> subtypes and I think we should support it accordingly. Even if I drop
> the support, we still have the difference of the register mapping on the
> dtest selection between MV/LV subtypes and legacy 4CH/8CH subtypes,
> which means we need a place to unify the dtest definition. I put the
> complication here in dtsi which the end user would choose the right
> value according to the hardware. I am also fine with putting the
> complication in C code, but that would drop the supporting of multiple
> DTEST lines selection for 4CH/8CH subtype.
>
I do like when the format of DT properties is obvious, so I think the
appropriate format for multiple dtest lines would be: "qcom,dtest-buffer
= <1, 2>;"
So let's punt on the multiple dtest lines for now and just support one.
If we in the future add support for arrays of lines that would still be
backwards compatible with the single case.
Regards,
Bjorn
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2017-07-14 18:19 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-13 6:16 [PATCH V1 0/3] Add LV/MV subtypes support for QCOM PMIC GPIOs fenglinw
2017-06-13 6:16 ` [PATCH V1 1/3] pinctrl: qcom: spmi-gpio: Add support for GPIO LV/MV subtype fenglinw
2017-06-18 14:04 ` Rob Herring
2017-06-19 1:00 ` Fenglin Wu
2017-06-19 1:00 ` Fenglin Wu
2017-06-19 2:07 ` Fenglin Wu
2017-06-20 11:15 ` Linus Walleij
2017-07-05 23:51 ` Fenglin Wu
2017-07-06 5:02 ` Bjorn Andersson
[not found] ` <20170613061707.13892-2-fenglinw-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2017-07-12 20:55 ` Bjorn Andersson
2017-07-12 20:55 ` Bjorn Andersson
2017-07-13 3:21 ` Fenglin Wu
2017-06-13 6:16 ` [PATCH V1 2/3] pinctrl: qcom: spmi-gpio: Add dtest route for digital input fenglinw
2017-06-18 14:04 ` Rob Herring
2017-06-20 11:14 ` Linus Walleij
[not found] ` <20170613061707.13892-3-fenglinw-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2017-07-12 21:24 ` Bjorn Andersson
2017-07-12 21:24 ` Bjorn Andersson
2017-07-13 5:27 ` Fenglin Wu
2017-07-14 18:19 ` Bjorn Andersson
2017-06-13 6:16 ` [PATCH V1 3/3] pinctrl: qcom: spmi-gpio: Correct power_source range check fenglinw
2017-06-20 9:33 ` Linus Walleij
2017-07-12 21:33 ` Bjorn Andersson
2017-07-13 5:55 ` Fenglin Wu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.