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
