Hi Geert,

Thank you for the review.

On Thu, Oct 7, 2021 at 6:23 PM Geert Uytterhoeven [off-list ref] wrote:

Hi Prabhakar,

On Thu, Sep 30, 2021 at 2:17 PM Lad Prabhakar
[off-list ref] wrote:

quoted

Add support to get/set drive-strength and output-impedance of the pins.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Thanks for your patch!

quoted

--- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
+++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c

@@ -47,6 +47,7 @@
 #define PIN_CFG_FILONOFF               BIT(9)
 #define PIN_CFG_FILNUM                 BIT(10)
 #define PIN_CFG_FILCLKSEL              BIT(11)
+#define PIN_CFG_GROUP_B                        BIT(12)

Perhaps it would be easier to have separate PIN_CFG_IOLH_A and
PIN_CFG_IOLH_B flags, instead of a PIN_CFG_IOLH flag and a
PIN_CFG_GROUP_B modifier flag?

Agreed will do that.

quoted

 #define RZG2L_MPXED_PIN_FUNCS          (PIN_CFG_IOLH | \
                                         PIN_CFG_SR | \

quoted

@@ -484,6 +513,38 @@ static int rzg2l_pinctrl_pinconf_get(struct pinctrl_dev *pctldev,
                break;
        }

+       case PIN_CONFIG_OUTPUT_IMPEDANCE:
+       case PIN_CONFIG_DRIVE_STRENGTH: {
+               unsigned int mA[4] = { 2, 4, 8, 12 };
+               unsigned int oi[4] = { 100, 66, 50, 33 };

static const

agreed.

quoted

+
+               if (param == PIN_CONFIG_DRIVE_STRENGTH) {
+                       if (!(cfg & PIN_CFG_IOLH) || groupb_pin)
+                               return -EINVAL;
+               } else {
+                       if (!(cfg & PIN_CFG_IOLH) || !groupb_pin)
+                               return -EINVAL;
+               }
+
+               spin_lock_irqsave(&pctrl->lock, flags);
+
+               /* handle _L/_H for 32-bit register read/write */
+               addr = pctrl->base + IOLH(port);
+               if (bit >= 4) {
+                       bit -= 4;
+                       addr += 4;
+               }
+
+               reg = readl(addr) & (IOLH_MASK << (bit * 8));
+               reg = reg >> (bit * 8);
+               if (param == PIN_CONFIG_DRIVE_STRENGTH)
+                       arg = mA[reg];
+               else
+                       arg = oi[reg];
+               spin_unlock_irqrestore(&pctrl->lock, flags);

I think you've reached the point where it starts to make sense to
have helper functions to read and modify these sub-register fields
that may be located into the current or next register.

Ok will add helpers to read and rmw.

And after that, you can split it in two smaller separate cases for
drive strength and output impedance.

Agreed.

Cheers,
Prabhakar

quoted

+               break;
+       }
+
        default:
                return -ENOTSUPP;
        }

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