diff mbox series

[v2,3/5] pinctrl: renesas: pinctrl-rzg2l: Add support to get/set pin config for GPIO port pins

Message ID 20211029124437.20721-4-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 support to get/set pin config for GPIO port pins.

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 | 33 +++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

Comments

Geert Uytterhoeven Nov. 8, 2021, 3:35 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 support to get/set pin config for GPIO port pins.
>
> 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

> @@ -495,6 +512,14 @@ static int rzg2l_pinctrl_pinconf_get(struct pinctrl_dev *pctldev,
>                 port = RZG2L_SINGLE_PIN_GET_PORT(*pin_data);
>                 cfg = RZG2L_SINGLE_PIN_GET_CFGS(*pin_data);
>                 bit = RZG2L_SINGLE_PIN_GET_BIT(*pin_data);
> +       } else {
> +               cfg = RZG2L_GPIO_PORT_GET_CFGS(*pin_data);
> +               port = RZG2L_PIN_ID_TO_PORT(_pin);
> +               bit = RZG2L_PIN_ID_TO_PIN(_pin);
> +               port_pin = true;

Instead of setting this flag, perhaps port should be adjusted?
Then rzg2l_r{ead,mw}_pin_config() don't have to care about that
anymore.

> +
> +               if (rzg2l_validate_gpio_pin(pctrl, *pin_data, port, bit))
> +                       return -EINVAL;
>         }
>
>         switch (param) {
> @@ -557,6 +582,14 @@ static int rzg2l_pinctrl_pinconf_set(struct pinctrl_dev *pctldev,
>                 port = RZG2L_SINGLE_PIN_GET_PORT(*pin_data);
>                 cfg = RZG2L_SINGLE_PIN_GET_CFGS(*pin_data);
>                 bit = RZG2L_SINGLE_PIN_GET_BIT(*pin_data);
> +       } else {
> +               cfg = RZG2L_GPIO_PORT_GET_CFGS(*pin_data);
> +               port = RZG2L_PIN_ID_TO_PORT(_pin);
> +               bit = RZG2L_PIN_ID_TO_PIN(_pin);
> +               port_pin = true;

Likewise.

> +
> +               if (rzg2l_validate_gpio_pin(pctrl, *pin_data, port, bit))
> +                       return -EINVAL;
>         }
>
>         for (i = 0; i < num_configs; i++) {

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
Lad, Prabhakar Nov. 9, 2021, 2:31 p.m. UTC | #2 
Hi Geert,

Thank you for the review.

On Mon, Nov 8, 2021 at 3:36 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Fri, Oct 29, 2021 at 2:44 PM Lad Prabhakar
> <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> > Add support to get/set pin config for GPIO port pins.
> >
> > 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
>
> > @@ -495,6 +512,14 @@ static int rzg2l_pinctrl_pinconf_get(struct pinctrl_dev *pctldev,
> >                 port = RZG2L_SINGLE_PIN_GET_PORT(*pin_data);
> >                 cfg = RZG2L_SINGLE_PIN_GET_CFGS(*pin_data);
> >                 bit = RZG2L_SINGLE_PIN_GET_BIT(*pin_data);
> > +       } else {
> > +               cfg = RZG2L_GPIO_PORT_GET_CFGS(*pin_data);
> > +               port = RZG2L_PIN_ID_TO_PORT(_pin);
> > +               bit = RZG2L_PIN_ID_TO_PIN(_pin);
> > +               port_pin = true;
>
> Instead of setting this flag, perhaps port should be adjusted?

Something like below?

#define RZG2L_PORT_START_OFFSET 0x10

port = RZG2L_PIN_ID_TO_PORT_pin) + RZG2L_PORT_START_OFFSET;
rzg2l_validate_gpio_pin(pctrl, *pin_data, port - RZG2L_PORT_START_OFFSET, bit)

and rename port -> port_offset in rzg2l_pinctrl_pinconf_get/set Or
would you prefer to change the RZG2L_PIN_ID_TO_PORT macro and adjust
the entire file?

> Then rzg2l_r{ead,mw}_pin_config() don't have to care about that
> anymore.
>
Agreed.

Cheers,
Prabhakar

> > +
> > +               if (rzg2l_validate_gpio_pin(pctrl, *pin_data, port, bit))
> > +                       return -EINVAL;
> >         }
> >
> >         switch (param) {
> > @@ -557,6 +582,14 @@ static int rzg2l_pinctrl_pinconf_set(struct pinctrl_dev *pctldev,
> >                 port = RZG2L_SINGLE_PIN_GET_PORT(*pin_data);
> >                 cfg = RZG2L_SINGLE_PIN_GET_CFGS(*pin_data);
> >                 bit = RZG2L_SINGLE_PIN_GET_BIT(*pin_data);
> > +       } else {
> > +               cfg = RZG2L_GPIO_PORT_GET_CFGS(*pin_data);
> > +               port = RZG2L_PIN_ID_TO_PORT(_pin);
> > +               bit = RZG2L_PIN_ID_TO_PIN(_pin);
> > +               port_pin = true;
>
> Likewise.
>
> > +
> > +               if (rzg2l_validate_gpio_pin(pctrl, *pin_data, port, bit))
> > +                       return -EINVAL;
> >         }
> >
> >         for (i = 0; i < num_configs; i++) {
>
> 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
Geert Uytterhoeven Nov. 9, 2021, 3 p.m. UTC | #3 
Hi Prabhakar,

On Tue, Nov 9, 2021 at 3:31 PM Lad, Prabhakar
<prabhakar.csengg@gmail.com> wrote:
> On Mon, Nov 8, 2021 at 3:36 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Fri, Oct 29, 2021 at 2:44 PM Lad Prabhakar
> > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> > > Add support to get/set pin config for GPIO port pins.
> > >
> > > 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
> >
> > > @@ -495,6 +512,14 @@ static int rzg2l_pinctrl_pinconf_get(struct pinctrl_dev *pctldev,
> > >                 port = RZG2L_SINGLE_PIN_GET_PORT(*pin_data);
> > >                 cfg = RZG2L_SINGLE_PIN_GET_CFGS(*pin_data);
> > >                 bit = RZG2L_SINGLE_PIN_GET_BIT(*pin_data);
> > > +       } else {
> > > +               cfg = RZG2L_GPIO_PORT_GET_CFGS(*pin_data);
> > > +               port = RZG2L_PIN_ID_TO_PORT(_pin);
> > > +               bit = RZG2L_PIN_ID_TO_PIN(_pin);
> > > +               port_pin = true;
> >
> > Instead of setting this flag, perhaps port should be adjusted?
>
> Something like below?
>
> #define RZG2L_PORT_START_OFFSET 0x10
>
> port = RZG2L_PIN_ID_TO_PORT_pin) + RZG2L_PORT_START_OFFSET;
> rzg2l_validate_gpio_pin(pctrl, *pin_data, port - RZG2L_PORT_START_OFFSET, bit)

Or adjust port after the call to rzg2l_validate_gpio_pin(), to avoid adding
the offset first, and subtracting it again for calling the latter?

> and rename port -> port_offset in rzg2l_pinctrl_pinconf_get/set

That makes sense.  Currently "port" has two meanings: it can mean
either the GPIO port index, or the global register index covering both
single function pin groups and GPIO port indices.
RZG2L_SINGLE_PIN_GET_PORT() returns the latter.
RZG2L_PIN_ID_TO_PORT() returns the former, thus needing an extra offset
to convert to the global register index.

> Or
> would you prefer to change the RZG2L_PIN_ID_TO_PORT macro and adjust
> the entire file?

Changing RZG2L_PIN_ID_TO_PORT() would imply changing all macros
accessing GPIO registers, and is thus quite intrusive.

> > Then rzg2l_r{ead,mw}_pin_config() don't have to care about that
> > anymore.
> >
> Agreed.

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
Lad, Prabhakar Nov. 9, 2021, 3:24 p.m. UTC | #4 
Hi Geert,

On Tue, Nov 9, 2021 at 3:00 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Tue, Nov 9, 2021 at 3:31 PM Lad, Prabhakar
> <prabhakar.csengg@gmail.com> wrote:
> > On Mon, Nov 8, 2021 at 3:36 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Fri, Oct 29, 2021 at 2:44 PM Lad Prabhakar
> > > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> > > > Add support to get/set pin config for GPIO port pins.
> > > >
> > > > 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
> > >
> > > > @@ -495,6 +512,14 @@ static int rzg2l_pinctrl_pinconf_get(struct pinctrl_dev *pctldev,
> > > >                 port = RZG2L_SINGLE_PIN_GET_PORT(*pin_data);
> > > >                 cfg = RZG2L_SINGLE_PIN_GET_CFGS(*pin_data);
> > > >                 bit = RZG2L_SINGLE_PIN_GET_BIT(*pin_data);
> > > > +       } else {
> > > > +               cfg = RZG2L_GPIO_PORT_GET_CFGS(*pin_data);
> > > > +               port = RZG2L_PIN_ID_TO_PORT(_pin);
> > > > +               bit = RZG2L_PIN_ID_TO_PIN(_pin);
> > > > +               port_pin = true;
> > >
> > > Instead of setting this flag, perhaps port should be adjusted?
> >
> > Something like below?
> >
> > #define RZG2L_PORT_START_OFFSET 0x10
> >
> > port = RZG2L_PIN_ID_TO_PORT_pin) + RZG2L_PORT_START_OFFSET;
> > rzg2l_validate_gpio_pin(pctrl, *pin_data, port - RZG2L_PORT_START_OFFSET, bit)
>
> Or adjust port after the call to rzg2l_validate_gpio_pin(), to avoid adding
> the offset first, and subtracting it again for calling the latter?
>
> > and rename port -> port_offset in rzg2l_pinctrl_pinconf_get/set
>
> That makes sense.  Currently "port" has two meanings: it can mean
> either the GPIO port index, or the global register index covering both
> single function pin groups and GPIO port indices.
> RZG2L_SINGLE_PIN_GET_PORT() returns the latter.
> RZG2L_PIN_ID_TO_PORT() returns the former, thus needing an extra offset
> to convert to the global register index.
>
for symmetry will rename the below:
RZG2L_SINGLE_PIN_GET_PORT -> RZG2L_SINGLE_PIN_GET_PORT_OFFSET

Introduce a new macros:
#define RZG2L_PORT_START_OFFSET 0x10
#define RZG2L_PIN_ID_TO_PORT_OFFSET(id) (((id) / RZG2L_PINS_PER_PORT)
+ RZG2L_PORT_START_OFFSET)

And use the above two in rzg2l_pinctrl_pinconf_get/set along with
renaming  port -> port_offset

And for rzg2l_validate_gpio_pin() will use below instead:
rzg2l_validate_gpio_pin(pctrl, *pin_data, RZG2L_PIN_ID_TO_PORT(_pin), bit);

> > Or
> > would you prefer to change the RZG2L_PIN_ID_TO_PORT macro and adjust
> > the entire file?
>
> Changing RZG2L_PIN_ID_TO_PORT() would imply changing all macros
> accessing GPIO registers, and is thus quite intrusive.
>
Agreed, I will drop this option.

Cheers,
Prabhakar
> > > Then rzg2l_r{ead,mw}_pin_config() don't have to care about that
> > > anymore.
> > >
> > Agreed.
>
> 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 f294ae7b8b5a..bc34c63bbb36 100644
--- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
+++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
@@ -426,6 +426,23 @@  static int rzg2l_dt_node_to_map(struct pinctrl_dev *pctldev,
 	return ret;
 }
 
+static int rzg2l_validate_gpio_pin(struct rzg2l_pinctrl *pctrl,
+				   u32 cfg, u32 port, u8 bit)
+{
+	u8 pincount = RZG2L_GPIO_PORT_GET_PINCNT(cfg);
+	u32 port_index = RZG2L_GPIO_PORT_GET_INDEX(cfg);
+	u32 data;
+
+	if (bit >= pincount || port >= pctrl->data->n_port_pins)
+		return -EINVAL;
+
+	data = pctrl->data->port_pin_configs[port];
+	if (port_index != RZG2L_GPIO_PORT_GET_INDEX(data))
+		return -EINVAL;
+
+	return 0;
+}
+
 static u32 rzg2l_read_pin_config(struct rzg2l_pinctrl *pctrl, bool port_pin,
 				 u32 offset, u8 bit, u32 mask)
 {
@@ -495,6 +512,14 @@  static int rzg2l_pinctrl_pinconf_get(struct pinctrl_dev *pctldev,
 		port = RZG2L_SINGLE_PIN_GET_PORT(*pin_data);
 		cfg = RZG2L_SINGLE_PIN_GET_CFGS(*pin_data);
 		bit = RZG2L_SINGLE_PIN_GET_BIT(*pin_data);
+	} else {
+		cfg = RZG2L_GPIO_PORT_GET_CFGS(*pin_data);
+		port = RZG2L_PIN_ID_TO_PORT(_pin);
+		bit = RZG2L_PIN_ID_TO_PIN(_pin);
+		port_pin = true;
+
+		if (rzg2l_validate_gpio_pin(pctrl, *pin_data, port, bit))
+			return -EINVAL;
 	}
 
 	switch (param) {
@@ -557,6 +582,14 @@  static int rzg2l_pinctrl_pinconf_set(struct pinctrl_dev *pctldev,
 		port = RZG2L_SINGLE_PIN_GET_PORT(*pin_data);
 		cfg = RZG2L_SINGLE_PIN_GET_CFGS(*pin_data);
 		bit = RZG2L_SINGLE_PIN_GET_BIT(*pin_data);
+	} else {
+		cfg = RZG2L_GPIO_PORT_GET_CFGS(*pin_data);
+		port = RZG2L_PIN_ID_TO_PORT(_pin);
+		bit = RZG2L_PIN_ID_TO_PIN(_pin);
+		port_pin = true;
+
+		if (rzg2l_validate_gpio_pin(pctrl, *pin_data, port, bit))
+			return -EINVAL;
 	}
 
 	for (i = 0; i < num_configs; i++) {