On Thu, Dec 16, 2021 at 1:25 AM Rafał Miłecki [off-list ref] wrote:

From: Rafał Miłecki <rafal@milecki.pl>

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 [off-list ref]
+ */

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