Re: [PATCH v3 4/5] pinctrl: add pinctrl/GPIO driver for Apple SoCs

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Sat, Oct 16, 2021 at 4:18 PM Joey Gouly <joey.gouly@xxxxxxx> wrote:

> This driver adds support for the pinctrl / GPIO hardware found
> on some Apple SoCs.
>
> Co-developed-by: Stan Skowronek <stan@xxxxxxxxxxxxx>
> Signed-off-by: Stan Skowronek <stan@xxxxxxxxxxxxx>
> Signed-off-by: Joey Gouly <joey.gouly@xxxxxxx>

Hi Joey, this looks really good. I started to write a reply but
noticed that all my comments are already covered by
Marc Z so I scrapped it: do what Marc says.

This is interesting:

> +#define REG_GPIOx_PULL_OFF   0
> +#define REG_GPIOx_PULL_DOWN  1
> +#define REG_GPIOx_PULL_UP_STRONG 2
> +#define REG_GPIOx_PULL_UP    3

Pull-up strong! Nice that you found these details. We don't have a
generic pinconf binding for that but possibly the bias-pull-up
argument can be used if we know how many ohms each is
(preferred).

But this is no problem for the moment because pin config is
for implementing later (I assume). I bet this is going to be important
to get right as well as it usually affects the power dissipation in
suspend-to-RAM.

> +static int apple_gpio_pinmux_enable(struct pinctrl_dev *pctldev, unsigned func,
> +                                   unsigned group)
(...)
> +       .set_mux = apple_gpio_pinmux_enable,

What about just naming it apple_gpio_pinmux_set()?

Next iteration should be good to go I guess!

Yours,
Linus Walleij