diff mbox series

[v2,4/5] pinctrl: qcom: Add support for driving GPIO pins output

Message ID 20240311111027.44577-5-sumit.garg@linaro.org
State New
Headers show
Series Add SE HMBSC board support | expand

Commit Message

Sumit Garg March 11, 2024, 11:10 a.m. UTC
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(-)

Comments

Caleb Connolly March 11, 2024, 12:37 p.m. UTC | #1
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;
>  	}
Sumit Garg March 12, 2024, 8:12 a.m. UTC | #2
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 mbox series

Patch

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;
 	}