Re: [PATCH v2 2/5] pinctrl: renesas: pinctrl-rzg2l: Add helper functions to read/write pin config
From: Geert Uytterhoeven <geert@linux-m68k.org>
Date: 2021-11-08 15:33:07
Also in:
linux-gpio, linux-renesas-soc, lkml
Hi Prabhakar, On Fri, Oct 29, 2021 at 2:44 PM Lad Prabhakar [off-list ref] 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!
quoted hunk ↗ jump to hunk
--- 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].
quoted hunk ↗ jump to hunk
+ #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;
+}
quoted hunk ↗ jump to hunk
@@ -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?
quoted hunk ↗ jump to hunk
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