Re: [PATCH v4 2/2] pinctrl: pinctrl-mlxbf: Add pinctrl driver support

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Fri, Feb 17, 2023 at 11:27 PM Asmaa Mnebhi <asmaa@xxxxxxxxxx> wrote:
>
> NVIDIA BlueField-3 SoC has a few pins that can be used as GPIOs
> or take the default hardware functionality. Add a driver for
> the pinmuxing.

pin muxing

...

> +++ b/drivers/pinctrl/pinctrl-mlxbf.c

Wondering if it would be better to match the GPIO driver naming
schema, i.e. by adding 3. In this case the additional explanation in
Kconfig help won't be necessary.

...

> +#define MLXBF_GPIO0_FW_CONTROL_SET   0
> +#define MLXBF_GPIO0_FW_CONTROL_CLEAR 0x14
> +#define MLXBF_GPIO1_FW_CONTROL_SET   0x80
> +#define MLXBF_GPIO1_FW_CONTROL_CLEAR 0x94

Unclear if these are commands or register offsets. If they are of the
same type (semantically), make them fixed width, e.g., 0x00.

...

> +enum {
> +       MLXBF_GPIO_HW_MODE,
> +       MLXBF_GPIO_SW_MODE

I would leave a comma here as it might be extended in the future.

> +};

...

> +static const char * const mlxbf_pinctrl_single_group_names[] = {
> +       "gpio0", "gpio1",  "gpio2",  "gpio3",  "gpio4",  "gpio5",  "gpio6",
> +       "gpio7", "gpio8",  "gpio9",  "gpio10", "gpio11", "gpio12", "gpio13",
> +       "gpio14", "gpio15", "gpio16", "gpio17", "gpio18", "gpio19", "gpio20",
> +       "gpio21", "gpio22", "gpio23", "gpio24", "gpio25", "gpio26", "gpio27",
> +       "gpio28", "gpio29", "gpio30", "gpio31", "gpio32", "gpio33", "gpio34",
> +       "gpio35", "gpio36", "gpio37", "gpio38", "gpio39", "gpio40", "gpio41",
> +       "gpio42", "gpio43", "gpio44", "gpio45", "gpio46", "gpio47", "gpio48",
> +       "gpio49", "gpio50", "gpio51", "gpio52", "gpio53", "gpio54", "gpio55"

Ditto.
Can you group by 8?

> +};
> +
> +/* Set of pin numbers for single-pin groups */
> +static const unsigned int mlxbf_pinctrl_single_group_pins[] = {
> +       0,  1,  2,  3,  4,  5,  6,
> +       7,  8,  9, 10, 11, 12, 13,
> +       14, 15, 16, 17, 18, 19, 20,
> +       21, 22, 23, 24, 25, 26, 27,
> +       28, 29, 30, 31, 32, 33, 34,
> +       35, 36, 37, 38, 39, 40, 41,
> +       42, 43, 44, 45, 46, 47, 48,
> +       49, 50, 51, 52, 53, 54, 55,

Group by 8 which is the more natural length of subarray per line.

Is it just 1:1 to the index? If so, why do you need this table at all?

> +};

...

> +static const struct {
> +       const char *name;
> +       const char * const *group_names;

Use this instead
https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git/tree/include/linux/pinctrl/pinctrl.h?h=devel#n215
and this
https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git/tree/include/linux/pinctrl/pinctrl.h?h=devel#n222

> +} mlxbf_pmx_funcs[] = {

> +};

...

> +{
> +       struct mlxbf_pinctrl *priv = pinctrl_dev_get_drvdata(pctldev);
> +
> +       /* disable GPIO functionality by giving control back to hardware */
> +       if (offset < MLXBF_NGPIOS_GPIO0)
> +               writel(BIT(offset), priv->base + MLXBF_GPIO0_FW_CONTROL_CLEAR);
> +       else
> +               writel(BIT(offset % MLXBF_NGPIOS_GPIO0), priv->base + MLXBF_GPIO1_FW_CONTROL_CLEAR);

> +

Redundant blank line.

> +}

...

> +static_assert(ARRAY_SIZE(mlxbf_pinctrl_single_group_names) ==
> +             ARRAY_SIZE(mlxbf_pinctrl_single_group_pins));

I would put it on a single line, but it's up to you.

...

> +       struct resource *res;

Useless?

...

> +       /* This resource is shared so use devm_ioremap */

Can you elaborate on who actually requests the region? And why is it
not _this_ driver?

> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       if (!res)
> +               return -ENODEV;

...

> +       ret = devm_pinctrl_register_and_init(priv->dev,

Is the priv->dev different from dev?

> +                                            &mlxbf_pin_desc,
> +                                            priv,
> +                                            &priv->pctl);
> +       if (ret)
> +               return dev_err_probe(dev, ret, "Failed to register pinctrl\n");

...

> +       pinctrl_add_gpio_range(priv->pctl, &mlxbf_pinctrl_gpio_ranges[0]);
> +       pinctrl_add_gpio_range(priv->pctl, &mlxbf_pinctrl_gpio_ranges[1]);

pinctrl_add_gpio_ranges() ?

--
With Best Regards,
Andy Shevchenko