diff mbox series

[v2,2/5] pinctrl: renesas: pinctrl-rzg2l: Add helper functions to read/write pin config

Message ID 20211029124437.20721-3-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series RZ/G2L: pinctrl: Support to get/set drive-strength and output-impedance-ohms | expand

Commit Message

Prabhakar Oct. 29, 2021, 12:44 p.m. UTC 
Add helper functions to read/read modify write pin config.

Switch to use helper functions for pins supporting PIN_CONFIG_INPUT_ENABLE
capabilities.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/pinctrl/renesas/pinctrl-rzg2l.c | 78 +++++++++++++++++--------
 1 file changed, 54 insertions(+), 24 deletions(-)

Comments

Geert Uytterhoeven Nov. 8, 2021, 3:32 p.m. UTC | #1 
Hi Prabhakar,

On Fri, Oct 29, 2021 at 2:44 PM Lad Prabhakar
<prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> Add helper functions to read/read modify write pin config.
>
> Switch to use helper functions for pins supporting PIN_CONFIG_INPUT_ENABLE
> capabilities.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>

Thanks for your patch!

> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> @@ -91,6 +91,8 @@
>  #define SD_CH(n)               (0x3000 + (n) * 4)
>  #define QSPI                   (0x3008)
>
> +#define PORT_PIN_CFG_OFFSET    0x80

This definition belongs in [PATCH v2 5/5].

> +
>  #define PVDD_1800              1       /* I/O domain voltage <= 1.8V */
>  #define PVDD_3300              0       /* I/O domain voltage >= 3.3V */
>
> @@ -424,6 +426,52 @@ static int rzg2l_dt_node_to_map(struct pinctrl_dev *pctldev,
>         return ret;
>  }
>
> +static u32 rzg2l_read_pin_config(struct rzg2l_pinctrl *pctrl, bool port_pin,
> +                                u32 offset, u8 bit, u32 mask)
> +{
> +       void __iomem *addr = pctrl->base + offset;
> +       unsigned long flags;
> +       u32 reg;
> +
> +       if (port_pin)
> +               addr += PORT_PIN_CFG_OFFSET;

I'm wondering if it would be better to handle this in the caller,
by passing an adjusted offset?
Same for rzg2l_rmw_pin_config().

> +
> +       /* handle _L/_H for 32-bit register read/write */
> +       if (bit >= 4) {
> +               bit -= 4;
> +               addr += 4;
> +       }
> +
> +       spin_lock_irqsave(&pctrl->lock, flags);
> +       reg = readl(addr) & (mask << (bit * 8));

The masking is not needed here, as it is done below.

> +       spin_unlock_irqrestore(&pctrl->lock, flags);

I still think you don't need that spinlock here, as reading a MMIO
register is an atomic operation.

(/me fixes drivers/pinctrl/renesas/pinctrl.c you referred to before)

> +       reg = (reg >> (bit * 8)) & mask;
> +
> +       return reg;

return (reg >> (bit * 8)) & mask;

> +}

> @@ -432,10 +480,11 @@ static int rzg2l_pinctrl_pinconf_get(struct pinctrl_dev *pctldev,
>         enum pin_config_param param = pinconf_to_config_param(*config);
>         const struct pinctrl_pin_desc *pin = &pctrl->desc.pins[_pin];
>         unsigned int *pin_data = pin->drv_data;
> +       bool port_pin = false;

Do you really need this?

>         unsigned int arg = 0;
>         unsigned long flags;
>         void __iomem *addr;
> -       u32 port = 0, reg;
> +       u32 port = 0;
>         u32 cfg = 0;
>         u8 bit = 0;
>
> @@ -452,17 +501,8 @@ static int rzg2l_pinctrl_pinconf_get(struct pinctrl_dev *pctldev,
>         case PIN_CONFIG_INPUT_ENABLE:
>                 if (!(cfg & PIN_CFG_IEN))
>                         return -EINVAL;
> -               spin_lock_irqsave(&pctrl->lock, flags);
> -               /* handle _L/_H for 32-bit register read/write */
> -               addr = pctrl->base + IEN(port);
> -               if (bit >= 4) {
> -                       bit -= 4;
> -                       addr += 4;
> -               }
>
> -               reg = readl(addr) & (IEN_MASK << (bit * 8));
> -               arg = (reg >> (bit * 8)) & 0x1;
> -               spin_unlock_irqrestore(&pctrl->lock, flags);
> +               arg = rzg2l_read_pin_config(pctrl, port_pin, IEN(port), bit, IEN_MASK);

port_pin is always false here, as PIN_CFG_IEN is only ever set for
dedicated pins.

Same comments for rzg2l_pinctrl_pinconf_set().

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
diff mbox series

Patch

diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
index 20b2af889ca9..f294ae7b8b5a 100644
--- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
+++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
@@ -91,6 +91,8 @@ 
 #define SD_CH(n)		(0x3000 + (n) * 4)
 #define QSPI			(0x3008)
 
+#define PORT_PIN_CFG_OFFSET	0x80
+
 #define PVDD_1800		1	/* I/O domain voltage <= 1.8V */
 #define PVDD_3300		0	/* I/O domain voltage >= 3.3V */
 
@@ -424,6 +426,52 @@  static int rzg2l_dt_node_to_map(struct pinctrl_dev *pctldev,
 	return ret;
 }
 
+static u32 rzg2l_read_pin_config(struct rzg2l_pinctrl *pctrl, bool port_pin,
+				 u32 offset, u8 bit, u32 mask)
+{
+	void __iomem *addr = pctrl->base + offset;
+	unsigned long flags;
+	u32 reg;
+
+	if (port_pin)
+		addr += PORT_PIN_CFG_OFFSET;
+
+	/* handle _L/_H for 32-bit register read/write */
+	if (bit >= 4) {
+		bit -= 4;
+		addr += 4;
+	}
+
+	spin_lock_irqsave(&pctrl->lock, flags);
+	reg = readl(addr) & (mask << (bit * 8));
+	spin_unlock_irqrestore(&pctrl->lock, flags);
+	reg = (reg >> (bit * 8)) & mask;
+
+	return reg;
+}
+
+static void rzg2l_rmw_pin_config(struct rzg2l_pinctrl *pctrl, bool port_pin,
+				 u32 offset, u8 bit, u32 mask, u32 val)
+{
+	void __iomem *addr = pctrl->base + offset;
+	unsigned long flags;
+	u32 reg;
+
+	if (port_pin)
+		addr += PORT_PIN_CFG_OFFSET;
+
+	/* handle _L/_H for 32-bit register read/write */
+	if (bit >= 4) {
+		bit -= 4;
+		addr += 4;
+	}
+
+	spin_lock_irqsave(&pctrl->lock, flags);
+	reg = readl(addr) & ~(mask << (bit * 8));
+	writel(reg | (val << (bit * 8)), addr);
+	spin_unlock_irqrestore(&pctrl->lock, flags);
+}
+
 static int rzg2l_pinctrl_pinconf_get(struct pinctrl_dev *pctldev,
 				     unsigned int _pin,
 				     unsigned long *config)
@@ -432,10 +480,11 @@  static int rzg2l_pinctrl_pinconf_get(struct pinctrl_dev *pctldev,
 	enum pin_config_param param = pinconf_to_config_param(*config);
 	const struct pinctrl_pin_desc *pin = &pctrl->desc.pins[_pin];
 	unsigned int *pin_data = pin->drv_data;
+	bool port_pin = false;
 	unsigned int arg = 0;
 	unsigned long flags;
 	void __iomem *addr;
-	u32 port = 0, reg;
+	u32 port = 0;
 	u32 cfg = 0;
 	u8 bit = 0;
 
@@ -452,17 +501,8 @@  static int rzg2l_pinctrl_pinconf_get(struct pinctrl_dev *pctldev,
 	case PIN_CONFIG_INPUT_ENABLE:
 		if (!(cfg & PIN_CFG_IEN))
 			return -EINVAL;
-		spin_lock_irqsave(&pctrl->lock, flags);
-		/* handle _L/_H for 32-bit register read/write */
-		addr = pctrl->base + IEN(port);
-		if (bit >= 4) {
-			bit -= 4;
-			addr += 4;
-		}
 
-		reg = readl(addr) & (IEN_MASK << (bit * 8));
-		arg = (reg >> (bit * 8)) & 0x1;
-		spin_unlock_irqrestore(&pctrl->lock, flags);
+		arg = rzg2l_read_pin_config(pctrl, port_pin, IEN(port), bit, IEN_MASK);
 		break;
 
 	case PIN_CONFIG_POWER_SOURCE: {
@@ -502,10 +542,11 @@  static int rzg2l_pinctrl_pinconf_set(struct pinctrl_dev *pctldev,
 	const struct pinctrl_pin_desc *pin = &pctrl->desc.pins[_pin];
 	unsigned int *pin_data = pin->drv_data;
 	enum pin_config_param param;
+	bool port_pin = false;
 	unsigned long flags;
 	void __iomem *addr;
-	u32 port = 0, reg;
 	unsigned int i;
+	u32 port = 0;
 	u32 cfg = 0;
 	u8 bit = 0;
 
@@ -524,21 +565,10 @@  static int rzg2l_pinctrl_pinconf_set(struct pinctrl_dev *pctldev,
 		case PIN_CONFIG_INPUT_ENABLE: {
 			unsigned int arg =
 					pinconf_to_config_argument(_configs[i]);
-
 			if (!(cfg & PIN_CFG_IEN))
 				return -EINVAL;
 
-			/* handle _L/_H for 32-bit register read/write */
-			addr = pctrl->base + IEN(port);
-			if (bit >= 4) {
-				bit -= 4;
-				addr += 4;
-			}
-
-			spin_lock_irqsave(&pctrl->lock, flags);
-			reg = readl(addr) & ~(IEN_MASK << (bit * 8));
-			writel(reg | (arg << (bit * 8)), addr);
-			spin_unlock_irqrestore(&pctrl->lock, flags);
+			rzg2l_rmw_pin_config(pctrl, port_pin, IEN(port), bit, IEN_MASK, !!arg);
 			break;
 		}