| Message ID | 20240311111027.44577-5-sumit.garg@linaro.org |
|---|---|
| State | Superseded |
| Delegated to: | Caleb Connolly |
| Headers | show |
| Series | Add SE HMBSC board support | expand |
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 >,< > > 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. > {"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? GPIO_IN is never used? 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. > + 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; > }
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)
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}, {"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 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, + 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; }
Add support for driving the GPIO pins as output low or high. 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(-)