Messages in this thread | | | From | Geert Uytterhoeven <> | Date | Thu, 7 Oct 2021 19:23:31 +0200 | Subject | Re: [RFC PATCH 4/4] pinctrl: renesas: pinctrl-rzg2l: Add support to get/set drive-strength and output-impedance |
| |
Hi Prabhakar,
On Thu, Sep 30, 2021 at 2:17 PM Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > 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!
> --- 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?
> > #define RZG2L_MPXED_PIN_FUNCS (PIN_CFG_IOLH | \ > PIN_CFG_SR | \
> @@ -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
> + > + 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.
And after that, you can split it in two smaller separate cases for drive strength and output impedance.
> + 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
| |