lkml.org 
[lkml]   [2021]   [Nov]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v2 2/5] pinctrl: renesas: pinctrl-rzg2l: Add helper functions to read/write pin config
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

\
 
 \ /
  Last update: 2021-11-08 16:33    [W:0.594 / U:24.656 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site