On Mon, 11 Mar 2024 at 18:07, Caleb Connolly caleb.connolly@linaro.org wrote:
Hi Sumit,
On 11/03/2024 11:10, Sumit Garg wrote:
Add support for driving the GPIO pins as output low or high.
Ohh, this is why it was never working for me >,<
Yeah you only implemented it for PMIC GPIOs.
Signed-off-by: Sumit Garg sumit.garg@linaro.org
drivers/pinctrl/qcom/pinctrl-apq8016.c | 1 + drivers/pinctrl/qcom/pinctrl-qcom.c | 26 +++++++++++++++++++++----- 2 files changed, 22 insertions(+), 5 deletions(-)
diff --git a/drivers/pinctrl/qcom/pinctrl-apq8016.c b/drivers/pinctrl/qcom/pinctrl-apq8016.c index cb0e2227079..04b501c563d 100644 --- a/drivers/pinctrl/qcom/pinctrl-apq8016.c +++ b/drivers/pinctrl/qcom/pinctrl-apq8016.c @@ -29,6 +29,7 @@ static const char * const msm_pinctrl_pins[] = { };
static const struct pinctrl_function msm_pinctrl_functions[] = {
{"gpio", 0},Please split this into a separate patch.
Okay I can do that.
{"blsp_uart1", 2}, {"blsp_uart2", 2},}; diff --git a/drivers/pinctrl/qcom/pinctrl-qcom.c b/drivers/pinctrl/qcom/pinctrl-qcom.c index ee0624df296..932be7c4840 100644 --- a/drivers/pinctrl/qcom/pinctrl-qcom.c +++ b/drivers/pinctrl/qcom/pinctrl-qcom.c @@ -29,15 +29,25 @@ struct msm_pinctrl_priv { #define GPIO_CONFIG_REG(priv, x) \ (qcom_pin_offset((priv)->data->pin_data.pin_offsets, x))
-#define TLMM_GPIO_PULL_MASK GENMASK(1, 0) -#define TLMM_FUNC_SEL_MASK GENMASK(5, 2) -#define TLMM_DRV_STRENGTH_MASK GENMASK(8, 6) -#define TLMM_GPIO_DISABLE BIT(9) +#define GPIO_IN_OUT_REG(priv, x) \
(GPIO_CONFIG_REG(priv, x) + 0x4)+#define TLMM_GPIO_PULL_MASK GENMASK(1, 0) +#define TLMM_FUNC_SEL_MASK GENMASK(5, 2) +#define TLMM_DRV_STRENGTH_MASK GENMASK(8, 6) +#define TLMM_GPIO_OUTPUT_MASK BIT(1) +#define TLMM_GPIO_OE_MASK BIT(9)
+/* GPIO_IN_OUT register shifts. */ +#define GPIO_IN 0 +#define GPIO_OUT 1
Are there two separate bits?
Yeah these are two separate bits. I can rename them as GPIO_IN_SHIFT and GPIO_OUT_SHIFT if that is more clear.
GPIO_IN is never used?
Okay I can drop it as it is very unlikely for the pinctrl driver to read GPIO value.
Maybe just define GPIO_OUT as BIT(0) instead?
static const struct pinconf_param msm_conf_params[] = { { "drive-strength", PIN_CONFIG_DRIVE_STRENGTH, 2 }, { "bias-disable", PIN_CONFIG_BIAS_DISABLE, 0 }, { "bias-pull-up", PIN_CONFIG_BIAS_PULL_UP, 3 },
{ "output-high", PIN_CONFIG_OUTPUT, 1, },{ "output-low", PIN_CONFIG_OUTPUT, 0, },};
static int msm_get_functions_count(struct udevice *dev) @@ -89,7 +99,7 @@ static int msm_pinmux_set(struct udevice *dev, unsigned int pin_selector, return 0;
clrsetbits_le32(priv->base + GPIO_CONFIG_REG(priv, pin_selector),
TLMM_FUNC_SEL_MASK | TLMM_GPIO_DISABLE,
TLMM_FUNC_SEL_MASK | TLMM_GPIO_OE_MASK, priv->data->get_function_mux(func_selector) << 2); return 0;} @@ -117,6 +127,12 @@ static int msm_pinconf_set(struct udevice *dev, unsigned int pin_selector, clrsetbits_le32(priv->base + GPIO_CONFIG_REG(priv, pin_selector), TLMM_GPIO_PULL_MASK, argument); break;
case PIN_CONFIG_OUTPUT:writel(argument << GPIO_OUT,Then this can be "argument ? GPIO_OUT : GPIO_IN" which feels much cleaner. Or even just !!argument if it's bit 0.
No, here GPIO_OUT is rather a shift for the output bit and "argument" is the value to be written. As above I can change it to "argument << GPIO_OUT_SHIFT" to be more clear.
-Sumit
priv->base + GPIO_IN_OUT_REG(priv, pin_selector));setbits_le32(priv->base + GPIO_CONFIG_REG(priv, pin_selector),TLMM_GPIO_OE_MASK);break; default: return 0; }-- // Caleb (they/them)