[PATCH linux dev-4.13 v2 2/2] pinctrl: npcm: add NPCM7xx pin control driver

[Joel Stanley]

Hi Tomer,

On 17 May 2018 at 22:54, Tomer Maimon <tmaimon77 at gmail.com> wrote:
> Add Nuvoton BMC NPCM7xx pin controller driver.
>
> The NPCM7XX Pin Controller multi-function routed
> through the multiplexing block, Each pin supports
> GPIO functionality (GPIOx) and multiple functions
> that directly connect the pin to different hardware
> blocks.
>
> Signed-off-by: Tomer Maimon <tmaimon77 at gmail.com>

I've applied this to dev-4.13 with some minor fixes. How is the
upstream submission of this one going?

> ---
>  drivers/pinctrl/Kconfig                   |    1 +
>  drivers/pinctrl/Makefile                  |    2 +
>  drivers/pinctrl/nuvoton/Kconfig           |   12 +
>  drivers/pinctrl/nuvoton/Makefile          |    1 +
>  drivers/pinctrl/nuvoton/pinctrl-npcm7xx.c | 2133 +++++++++++++++++++++++++++++
>  5 files changed, 2149 insertions(+)
>  create mode 100644 drivers/pinctrl/nuvoton/Kconfig
>  create mode 100644 drivers/pinctrl/nuvoton/Makefile
>  create mode 100644 drivers/pinctrl/nuvoton/pinctrl-npcm7xx.c
>
> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
> index e14b46c7b37f..25b7c1383631 100644
> --- a/drivers/pinctrl/Kconfig
> +++ b/drivers/pinctrl/Kconfig
> @@ -351,6 +351,7 @@ source "drivers/pinctrl/uniphier/Kconfig"
>  source "drivers/pinctrl/vt8500/Kconfig"
>  source "drivers/pinctrl/mediatek/Kconfig"
>  source "drivers/pinctrl/zte/Kconfig"
> +source "drivers/pinctrl/nuvoton/Kconfig"
>
>  config PINCTRL_XWAY
>         bool
> diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
> index 2bc641d62400..925d99b05e47 100644
> --- a/drivers/pinctrl/Makefile
> +++ b/drivers/pinctrl/Makefile
> @@ -62,3 +62,5 @@ obj-$(CONFIG_PINCTRL_UNIPHIER)        += uniphier/
>  obj-$(CONFIG_ARCH_VT8500)      += vt8500/
>  obj-$(CONFIG_PINCTRL_MTK)      += mediatek/
>  obj-$(CONFIG_PINCTRL_ZX)       += zte/
> +obj-$(CONFIG_ARCH_NPCM7XX)     += nuvoton/
> +
> diff --git a/drivers/pinctrl/nuvoton/Kconfig b/drivers/pinctrl/nuvoton/Kconfig
> new file mode 100644
> index 000000000000..e174a83e8be2
> --- /dev/null
> +++ b/drivers/pinctrl/nuvoton/Kconfig
> @@ -0,0 +1,12 @@
> +config PINCTRL_NPCM7XX
> +       bool "Pinctrl driver for Nuvoton NPCM7XX"
> +       depends on (ARCH_NPCM7XX || COMPILE_TEST) && OF
> +       depends on MFD_SYSCON

This needed to be select MFD_SYSCON, or else I got Kconfig warnings.


> +       select GPIOLIB
> +       select GPIOLIB_IRQCHIP
> +       select PINMUX
> +       select PINCONF
> +       select GENERIC_PINCONF
> +       help
> +         Say Y here to enable pin controller and GPIO support
> +         for Nuvoton NPCM7xx SoCs.

> --- /dev/null
> +++ b/drivers/pinctrl/nuvoton/pinctrl-npcm7xx.c

> +
> +#define DRV_DATE    "2018-05-14"
> +#define DRV_VERSION "2.0.0"

I don't think this is necessary.


> +/* Get slew rate of pin (high/low) */
> +static int npcm_get_slew_rate(struct NPCM_GPIO *bank,
> +                             struct regmap *gcr_regmap, unsigned int pin)
> +{
> +       u32 val;
> +       int gpio = (pin % bank->gc.ngpio);
> +
> +       if (pincfg[pin].flag & SLEW)
> +               return gpio_bitop(bank, opGETBIT, gpio, NPCM_GP_N_OSRC);
> +       /* LPC Slew rate in SRCNT register */
> +       if (pincfg[pin].flag & SLEWLPC)
> +       {
> +               regmap_read(gcr_regmap, NPCM7XX_GCR_SRCNT, &val);
> +               return !!(val & SRCNT_ESPI);
> +       }
> +       return -EINVAL;
> +}
> +
> +/* Get drive strength for a pin, if supported */
> +static int npcm_get_drive_strength(struct pinctrl_dev *pctldev,
> +                                  unsigned int pin)
> +{
> +       struct NPCM7xx_pinctrl *npcm = pinctrl_dev_get_drvdata(pctldev);
> +       struct NPCM_GPIO *bank = &npcm->gpio_bank[pin/GPIO_PER_BANK];
> +       int gpio = (pin % bank->gc.ngpio);
> +       u32 val, ds = 0;
> +       int flg;
> +
> +       flg = pincfg[pin].flag;
> +       if (flg & DRIVE_STRENGTH_MASK) {
> +               /* Get standard reading */
> +               val = gpio_bitop(bank, opGETBIT, gpio, NPCM_GP_N_ODSC);
> +               ds = val ? DSHI(flg) : DSLO(flg);
> +       }
> +       dev_dbg(bank->gc.parent, " pin %d strength %d = %d\n", pin, val, ds);

I got a warning here:


drivers/pinctrl/nuvoton/pinctrl-npcm7xx.c: In function ‘npcm_config_get’:
./include/linux/dynamic_debug.h:134:3: warning: ‘val’ may be used
uninitialized in this function [-Wmaybe-uninitialized]
   __dynamic_dev_dbg(&descriptor, dev, fmt, \
   ^~~~~~~~~~~~~~~~~
drivers/pinctrl/nuvoton/pinctrl-npcm7xx.c:1560:6: note: ‘val’ was declared here
  u32 val, ds = 0;
      ^~~

It might make sense to move the dev_dbg up into the if ().


> +       return ds;

In the case where DRIVE_STRENGTH_MASK is not supported, should you
instead return EINVAL like you do for the slew rate above?


> +}
> +
> +/* Set drive strength for a pin, if supported */
> +static int npcm_set_drive_strength(struct NPCM7xx_pinctrl *npcm,
> +                                  unsigned int pin, int nval)
> +{
> +       int v;
> +       struct NPCM_GPIO *bank = &npcm->gpio_bank[pin/GPIO_PER_BANK];
> +       int gpio = (pin % bank->gc.ngpio);
> +
> +       v = (pincfg[pin].flag & DRIVE_STRENGTH_MASK);
> +       if (!nval || !v)
> +               return 0;
> +       if (DSLO(v) == nval) {
> +               dev_dbg(bank->gc.parent, " setting pin %d to low strength "
> +                                        "[%d]\n", pin, nval);
> +               gpio_bitop(bank, opCLRBIT, gpio, NPCM_GP_N_ODSC);
> +               return 1;
> +       } else if (DSHI(v) == nval) {
> +               dev_dbg(bank->gc.parent, " setting pin %d to high strength "
> +                                        "[%d]\n", pin, nval);
> +               gpio_bitop(bank, opSETBIT, gpio, NPCM_GP_N_ODSC);
> +               return 1;
> +       }
> +       return 0;
> +}
>

Cheers,

Joel