Re: [PATCH RFC 10/18] pinctrl: bcm2835: bcm7211: Add support for 7211 pull-up functionality

[Linus Walleij <linus.walleij@linaro.org>]

Hi Stefan!

Thanks for your patch!

On Wed, Jul 17, 2019 at 9:50 PM Stefan Wahren <wahrenst@gmx.net> wrote:

> From: Al Cooper <alcooperx@gmail.com>
>
> The 7211 has a new way of selecting the pull-up/pull-down setting
> for a GPIO pin. The registers used for the bcm2837, GP_PUD and
> GP_PUDCLKn0, are no longer connected. A new set of registers,
> GP_GPIO_PUP_PDN_CNTRL_REGx must be used. This commit will add
> a new compatible string "brcm,bcm7211-gpio" and the kernel
> driver will use it to select which method is used to select
> pull-up/pull-down.
>
> Signed-off-by: Al Cooper <alcooperx@gmail.com>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

(...)
>  /* argument: bcm2835_pinconf_pull */
>  #define BCM2835_PINCONF_PARAM_PULL     (PIN_CONFIG_END + 1)

Why this derivative of PULL?

What do you need that these standard configs and their parameters
doesn't already cover:

 * @PIN_CONFIG_BIAS_PULL_DOWN: the pin will be pulled down (usually with high
 *      impedance to GROUND). If the argument is != 0 pull-down is enabled,
 *      if it is 0, pull-down is total, i.e. the pin is connected to GROUND.
(...)
 * @PIN_CONFIG_BIAS_PULL_UP: the pin will be pulled up (usually with high
 *      impedance to VDD). If the argument is != 0 pull-up is enabled,
 *      if it is 0, pull-up is total, i.e. the pin is connected to VDD.

The argument isn't really defined but at one time I thought it should be
in ohms, but proprietary values are possible I guess.

 +static int bcm7211_pinconf_set(struct pinctrl_dev *pctldev,
> +                              unsigned int pin, unsigned long *configs,
> +                              unsigned int num_configs)
> +{
(...)
> +       for (i = 0; i < num_configs; i++) {
> +               param = pinconf_to_config_param(configs[i]);
> +               if (param != BCM2835_PINCONF_PARAM_PULL)
> +                       return -EINVAL;

This seems unnecessary, like some "hello I am here with magic value" instead
of just using the standard pull up/down configs and some ohm or custom
value.

> +               arg = pinconf_to_config_argument(configs[i]);
> +
> +               /* convert to 7211 value */
> +               switch (arg) {
> +               case PIN_CONFIG_BIAS_DISABLE:
> +                       arg = BCM7211_PINCONFIG_PULL_NONE;
> +                       break;
> +               case PIN_CONFIG_BIAS_PULL_DOWN:
> +                       arg = BCM7211_PINCONFIG_PULL_DOWN;
> +                       break;
> +               case PIN_CONFIG_BIAS_PULL_UP:
> +                       arg = BCM7211_PINCONFIG_PULL_UP;
> +                       break;
> +               }

This switch is fine, but lose the arg translation and just use the arg
as-is, I don't see the point.

The interpretation is anyway clear from:

> +               .compatible = "brcm,bcm7211-gpio",
> +               .data = &bcm7211_pinconf_ops,

This compatible string, and the variant of SoC is what you should
store in your state container instead of custom arguments, like

> +       match = of_match_node(bcm2835_pinctrl_match, pdev->dev.of_node);
> +       if (match) {
> +               bcm2835_pinctrl_desc.confops =
> +                       (const struct pinconf_ops *)match->data;
> +       }

Instead of storing the confops in the .data store just an enum for the SoC
and make the code adapt to which SoC it is. Storing different ops may make
sense but it seems to just be adding kludges here in the argument to
work around that later on the driver doesn't know which SoC it is
running on.

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel