Hi Prabhakar,
On Fri, Oct 29, 2021 at 2:44 PM Lad Prabhakar
<prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> Reviewed-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxx
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
