Hi Geert,

Thank you for the review.

On Thu, Oct 28, 2021 at 8:56 AM Geert Uytterhoeven [off-list ref] wrote:

Hi Prabhakar,

On Wed, Oct 27, 2021 at 3:45 PM Lad Prabhakar
[off-list ref] wrote:

quoted

Add support to get/set drive-strength and output-impedance-ohms
for the supported pins.

While at it also renamed the below macros to match the HW manual,
PIN_CFG_IOLH_SD0 -> PIN_CFG_IO_VMC_SD0
PIN_CFG_IOLH_SD1 -> PIN_CFG_IO_VMC_SD1
PIN_CFG_IOLH_QSPI -> PIN_CFG_IO_VMC_QSPI
PIN_CFG_IOLH_ETH0 -> PIN_CFG_IO_VMC_ETH0
PIN_CFG_IOLH_ETH1 -> PIN_CFG_IO_VMC_ETH1

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>

Thanks for the update!

quoted

---
RFC->v1
 * Renamed macros to match HW manual

You may want to split that off into a separate patch, as not all lines
changed are touched for other reasons.

Sure will do.

BTW, where do I find these "VMC" names in the HW manual?

I have shortened VOLTAGE MODE CONTROL to VMC from ETH_ch0/1, SD_ch0/1
and QSPI registers.

quoted

 * Added PIN_CFG_IOLH_A/B macros to differentiate Group A/B
 * Added helper function to read/rmw pin config
 * Included RB tags

quoted

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

quoted

+static u32 rzg2l_read_pin_config(void __iomem *addr,
+                                u8 bit, u32 mask)

The above fits on a single line.

quoted

+{
+       void __iomem *addr_adjust = addr;
+       u8 bit_adjust = bit;

No need for these, just operate on addr and bit directly.

will do.

quoted

+       u32 reg;
+
+       if (bit >= 4) {
+               bit_adjust -= 4;
+               addr_adjust += 4;
+       }
+
+       reg = readl(addr_adjust) & (mask << (bit_adjust * 8));
+       return (reg >> (bit_adjust * 8));
+}
+
+static void rzg2l_rmw_pin_config(void __iomem *addr,
+                                u8 bit, u32 mask, u32 val)
+{

The above fits on a single line.

will adjust that.

quoted

+       void __iomem *addr_adjust = addr;
+       u8 bit_adjust = bit;

No need for these, just operate on addr and bit directly.

OK.

quoted

+       u32 reg;
+
+       if (bit >= 4) {
+               bit_adjust -= 4;
+               addr_adjust += 4;
+       }
+
+       reg = readl(addr_adjust) & ~(mask << (bit_adjust * 8));
+
+       writel(reg | val, addr_adjust);

I think you should handle "val << (bit * 8)" here, instead of in
all callers.

Agreed.

quoted

+}

Please split the introduction of these helpers (and add conversion
of the existing PIN_CONFIG_INPUT_ENABLE handling) off into a separate
patch.

Agreed, will do.

quoted

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

+       case PIN_CONFIG_DRIVE_STRENGTH: {
+               static const unsigned int mA[4] = { 2, 4, 8, 12 };
+
+               if (!(cfg & PIN_CFG_IOLH_A))
+                       return -EINVAL;
+
+               spin_lock_irqsave(&pctrl->lock, flags);
+               addr = pctrl->base + IOLH(port);
+               reg = rzg2l_read_pin_config(addr, bit, IOLH_MASK);
+               arg = mA[reg];
+               spin_unlock_irqrestore(&pctrl->lock, flags);

Do you need the spinlock for reading?

Yes, to avoid any mishaps, I also looked at pinctrl.c which uses
spinlock while reading.

quoted

+               break;
+       }
+
+       case PIN_CONFIG_OUTPUT_IMPEDANCE_OHMS: {
+               static const unsigned int oi[4] = { 100, 66, 50, 33 };
+
+               if (!(cfg & PIN_CFG_IOLH_B))
+                       return -EINVAL;
+
+               spin_lock_irqsave(&pctrl->lock, flags);
+               addr = pctrl->base + IOLH(port);
+               reg = rzg2l_read_pin_config(addr, bit, IOLH_MASK);
+               arg = oi[reg];
+               spin_unlock_irqrestore(&pctrl->lock, flags);

Likewise.

quoted

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

quoted

@@ -564,6 +659,49 @@ static int rzg2l_pinctrl_pinconf_set(struct pinctrl_dev *pctldev,
                        spin_unlock_irqrestore(&pctrl->lock, flags);
                        break;
                }
+
+               case PIN_CONFIG_DRIVE_STRENGTH: {
+                       unsigned int arg = pinconf_to_config_argument(_configs[i]);
+                       static const unsigned int mA[4] = { 2, 4, 8, 12 };

Duplicate, move to file scope?

Ok will do.

quoted

+
+                       if (!(cfg & PIN_CFG_IOLH_A))
+                               return -EINVAL;
+
+                       for (i = 0; i < ARRAY_SIZE(mA); i++) {
+                               if (arg == mA[i])
+                                       break;
+                       }
+                       if (i >= ARRAY_SIZE(mA))
+                               return -EINVAL;
+
+                       spin_lock_irqsave(&pctrl->lock, flags);
+                       addr = pctrl->base + IOLH(port);
+                       rzg2l_rmw_pin_config(addr, bit, IOLH_MASK, (i << (bit * 8)));

Pass pctrl and offset instead of addr (also for rzg2l_read_pin_config,
for symmetry), and move locking into rzg2l_rmw_pin_config()?
Taking all of the above into account, that would become:

Agreed.

    rzg2l_rmw_pin_config(pctrl, IOLH(port), bit, IOLH_MASK, i);

That looks much cleaner.

quoted

+                       spin_unlock_irqrestore(&pctrl->lock, flags);
+                       break;
+               }
+
+               case PIN_CONFIG_OUTPUT_IMPEDANCE_OHMS: {
+                       unsigned int arg = pinconf_to_config_argument(_configs[i]);
+                       static const unsigned int oi[4] = { 100, 66, 50, 33 };

Duplicate, move to file scope?

Agreed.

quoted

+
+                       if (!(cfg & PIN_CFG_IOLH_B))
+                               return -EINVAL;
+
+                       for (i = 0; i < ARRAY_SIZE(oi); i++) {
+                               if (arg == oi[i])
+                                       break;
+                       }
+                       if (i >= ARRAY_SIZE(oi))
+                               return -EINVAL;
+
+                       spin_lock_irqsave(&pctrl->lock, flags);
+                       addr = pctrl->base + IOLH(port);
+                       rzg2l_rmw_pin_config(addr, bit, IOLH_MASK, (i << (bit * 8)));

Likewise.

OK.

quoted

+                       spin_unlock_irqrestore(&pctrl->lock, flags);
+                       break;
+               }
+
                default:
                        return -EOPNOTSUPP;
                }

The rest looks good to me!

Thanks, I'll post a v2 just with patches 3 and 4 only. Hope that is OK with you.

Cheers,
Prabhakar

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