Message-ID: <CACRpkdYQ2PO0iysd4L7Qzu6UR1ysHhsUWK6HWeL8rJ_SRqkHYA@mail.gmail.com>
Date: Wed, 19 Nov 2025 13:08:26 +0100
From: Linus Walleij <linus.walleij@...aro.org>
To: Conor Dooley <conor@...nel.org>
Cc: Conor Dooley <conor.dooley@...rochip.com>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, linux-kernel@...r.kernel.org, 
	linux-gpio@...r.kernel.org, devicetree@...r.kernel.org, 
	Valentina.FernandezAlanis@...rochip.com, Bartosz Golaszewski <brgl@...ev.pl>
Subject: Re: [RFC v1 2/4] pinctrl: add polarfire soc mssio pinctrl driver

Hi Conor,

took a quick look at this!

On Wed, Nov 12, 2025 at 3:33 PM Conor Dooley <conor@...nel.org> wrote:

> +#include "linux/dev_printk.h"

Weird but it's RFC so OK :)

> +#define MPFS_PINCTRL_LOCKDOWN (PIN_CONFIG_END + 1)
> +#define MPFS_PINCTRL_CLAMP_DIODE (PIN_CONFIG_END + 2)
> +#define MPFS_PINCTRL_IBUFMD (PIN_CONFIG_END + 3)

Yeah this should work for custom props.

> +struct mpfs_pinctrl_drive_strength {
> +       u8 ma;
> +       u8 val;
> +};
> +
> +static struct mpfs_pinctrl_drive_strength mpfs_pinctrl_drive_strengths[8] = {
> +       { 2,   2 },
> +       { 4,   3 },
> +       { 6,   4 },
> +       { 8,   5 },
> +       { 10,  6 },
> +       { 12,  7 },
> +       { 16, 10 },
> +       { 20, 12 },
> +};

I would probably assign field explicitly with C99 syntax, but no
hard requirement.

{ .ma = 2, .val = 2 }

BTW you can see clearly that each setting activates
another driver stage in the silicon, each totempole giving
2 mA.

> +static const struct pinconf_generic_params mpfs_pinctrl_custom_bindings[] = {
> +       { "microchip,bank-lockdown", MPFS_PINCTRL_LOCKDOWN, 1 },
> +       { "microchip,clamp-diode", MPFS_PINCTRL_CLAMP_DIODE, 1 },
> +       { "microchip,ibufmd", MPFS_PINCTRL_IBUFMD, 0x0 },
> +};

I take it these have proper documentation in the DT bindings, so users know
exactly what they do.

> +static int mpfs_pinctrl_pin_to_iocfg_reg(unsigned int pin)
> +{
> +       u32 reg = MPFS_PINCTRL_IOCFG01_REG;
> +
> +       if (pin >= MPFS_PINCTRL_BANK2_START)
> +               reg += MPFS_PINCTRL_INTER_BANK_GAP;
> +
> +       // 2 pins per 32-bit register
> +       reg += (pin / 2) * 0x4;

This is a nice comment, easy to follow the code with small helpful
things like this.

> +static int mpfs_pinctrl_dt_node_to_map(struct pinctrl_dev *pctrl_dev, struct device_node *np,
> +                                      struct pinctrl_map **maps, unsigned int *num_maps)

I saw in the cover letter that you wanted this to use more generic helpers.

If you see room for improvement of the generic code, do not hesitate.
Doing a new driver is the only time when you actually have all these
details in your head and can create good helpers.

> +       //TODO @Linus, it correct to group these 3? There's no control over voltage.
> +       case PIN_CONFIG_INPUT_SCHMITT:
> +       case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
> +       case PIN_CONFIG_INPUT_SCHMITT_UV:

Consider not supporting some like PIN_CONFIG_INPUT_SCHMITT_UV
in the bindings if they don't make any sense, and let it just return error
if someone tries to do that.

Isn't PIN_CONFIG_INPUT_SCHMITT_ENABLE the only one that
makes sense for this hardware?

> +static int mpfs_pinctrl_pinconf_generate_config(struct mpfs_pinctrl *pctrl, unsigned int pin,
> +                                               unsigned long *configs, unsigned int num_configs,
> +                                               u32 *value)
(...)
> +               case PIN_CONFIG_BIAS_PULL_DOWN:
> +                       //TODO always start from val == 0, there's no reason to ever actually
> +                       // clear anything AFAICT. @Linus, does the driver need to check mutual
> +                       // exclusion on these, or can I drop the clearing?
> +                       val &= ~MPFS_PINCTRL_PULL_MASK;
> +                       val |= MPFS_PINCTRL_WPD;
> +                       break;

I was about to say that the core checks that you don't enable pull up
and pull down
at the same time, but apparently that was just a dream I had.

The gpiolib however contains this in gpiod_configure_flags():

        if (((lflags & GPIO_PULL_UP) && (lflags & GPIO_PULL_DOWN)) ||
            ((lflags & GPIO_PULL_UP) && (lflags & GPIO_PULL_DISABLE)) ||
            ((lflags & GPIO_PULL_DOWN) && (lflags & GPIO_PULL_DISABLE))) {
                gpiod_err(desc,
                          "multiple pull-up, pull-down or pull-disable
enabled, invalid configuration\n");
                return -EINVAL;
        }

So there is a precedent for checking this.

So if you patch pinconf-generic.c to disallow this that'd be great, I think
it makes most sense to do this in the core.

> +               case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
> +                       if (!arg)
> +                               break;
> +                       fallthrough;
> +               case PIN_CONFIG_INPUT_SCHMITT:
> +               case PIN_CONFIG_INPUT_SCHMITT_UV:
> +                       //TODO Is it enabled regardless of register setting, or must it
> +                       // be set for lower voltage IO? Docs are missing, MSS Configurator
> +                       // is not clear. Leaning towards the latter.
> +                       //bank_voltage = mpfs_pinctrl_pin_to_bank_voltage(pctrl, pin);
> +                       //if (bank_voltage < MPFS_PINCTRL_LVCMOS25 && !arg) {
> +                       //      dev_err(pctrl->dev,
> +                       //              "schmitt always enabled for 1.2, 1.5 and 1.8 volt io\n");
> +                       //      return -EINVAL;
> +                       //}
> +                       val |= MPFS_PINCTRL_ENHYST;
> +                       break;

See above.

I hope this helps!

Yours,
Linus Walleij

Powered by blists - more mailing lists