Re: [PATCH 2/2] pinctrl: bcm: add driver for BCM4908 pinmux

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Thu, Dec 16, 2021 at 1:25 AM Rafał Miłecki <zajec5@xxxxxxxxx> wrote:
>
> From: Rafał Miłecki <rafal@xxxxxxxxxx>
>
> BCM4908 has its own pins layout so it needs a custom binding and a Linux
> driver.

...

> +config PINCTRL_BCM4908
> +       bool "Broadcom BCM4908 pinmux driver"

Why not tristate?

> +       depends on OF && (ARCH_BCM4908 || COMPILE_TEST)

Is there really dependency to OF?

> +       select PINMUX
> +       select PINCONF
> +       select GENERIC_PINCONF
> +       select GENERIC_PINCTRL_GROUPS
> +       select GENERIC_PINMUX_FUNCTIONS
> +       default ARCH_BCM4908
> +       help
> +         Say Y here to enable driver for BCM4908 family SoCs with integrated
> +         pin controller.

With a module available it's good to mention its name here.

...

> +/*
> + * Copyright (C) 2021 Rafał Miłecki <rafal@xxxxxxxxxx>
> + */

One line?

...

> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/module.h>

> +#include <linux/of.h>
> +#include <linux/of_device.h>

No evidence of use of these headers.
But missed mod_devicetable.h.

> +#include <linux/pinctrl/pinconf-generic.h>
> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/pinctrl/pinmux.h>

Can you move this group...

> +#include <linux/platform_device.h>
> +#include <linux/slab.h>

...here?

> +#include "../core.h"
> +#include "../pinmux.h"

...

> +#define TEST_PORT_BLOCK_EN_LSB                 0x00
> +#define TEST_PORT_BLOCK_DATA_MSB               0x04
> +#define TEST_PORT_BLOCK_DATA_LSB               0x08
> +#define  TEST_PORT_LSB_PINMUX_DATA_SHIFT       12
> +#define TEST_PORT_COMMAND                      0x0c
> +#define  TEST_PORT_CMD_LOAD_MUX_REG            0x00000021

The prefix of all above doesn't match the module name.

...

> +struct bcm4908_pinctrl_grp {
> +       const char *name;
> +       const struct bcm4908_pinctrl_pin_setup *pins;
> +       const unsigned int num_pins;
> +};

Why not to (re)use struct group_desc?

...

> +       for (i = 0; i < group->num_pins; i++) {
> +               u32 lsb = 0;
> +
> +               lsb |= group->pins[i].number;
> +               lsb |= group->pins[i].function << TEST_PORT_LSB_PINMUX_DATA_SHIFT;
> +
> +               writel(0x0, bcm4908_pinctrl->base + TEST_PORT_BLOCK_DATA_MSB);
> +               writel(lsb, bcm4908_pinctrl->base + TEST_PORT_BLOCK_DATA_LSB);
> +               writel(TEST_PORT_CMD_LOAD_MUX_REG, bcm4908_pinctrl->base + TEST_PORT_COMMAND);
> +       }

No serialization for IO, is it okay?

...

> +       bcm4908_pinctrl->base = devm_platform_ioremap_resource(pdev, 0);
> +       if (IS_ERR(bcm4908_pinctrl->base)) {
> +               dev_err(dev, "Failed to map pinctrl regs\n");
> +               return PTR_ERR(bcm4908_pinctrl->base);

Besides of dev_err_probe(), why you duplicate message that already
printed by core?

> +       }

...

> +       /* Set pinctrl properties */
> +

Here and everywhere else, please drop redundant blank line.

...

> +       pins = devm_kcalloc(dev, BCM4908_NUM_PINS,
> +                           sizeof(struct pinctrl_pin_desc), GFP_KERNEL);

Please, use sizeof(*foo) form. Then put it on one line.

> +       if (!pins)
> +               return -ENOMEM;

...

> +       for (i = 0; i < BCM4908_NUM_PINS; i++) {
> +               pins[i].number = i;
> +               pins[i].name = devm_kasprintf(dev, GFP_KERNEL, "pin%d", i);
> +               if (!pins[i].name)
> +                       return -ENOMEM;
> +       }

Can you use the kasprintf_strarray() to make the style unified, please?

...

> +       bcm4908_pinctrl->pctldev = devm_pinctrl_register(dev, pctldesc, bcm4908_pinctrl);
> +       if (IS_ERR(bcm4908_pinctrl->pctldev)) {
> +               dev_err(dev, "Failed to register pinctrl\n");
> +               return PTR_ERR(bcm4908_pinctrl->pctldev);

return dev_err_probe(...);

> +       }

...

> +static struct platform_driver bcm4908_pinctrl_driver = {
> +       .probe = bcm4908_pinctrl_probe,
> +       .driver = {
> +               .name = "bcm4908-pinctrl",
> +               .of_match_table = bcm4908_pinctrl_of_match_table,
> +       },
> +};

> +

No need.

> +module_platform_driver(bcm4908_pinctrl_driver);


-- 
With Best Regards,
Andy Shevchenko