Mon, May 22, 2023 at 06:31:14PM +0200, Esteban Blanc kirjoitti:
> TI TPS6594 PMIC has 11 GPIOs which can be used
> for different functions.
>
> This patch adds a pinctrl and GPIO drivers in
> order to use those functions.
...
> +#define FUNCTION(n, g, v) \
> + { \
> + .pinfunction = PINCTRL_PINFUNCTION((n), (g), ARRAY_SIZE(g)), \
> + .muxval = v, \
> + }
It seems you have used SPACEs before \, can you move to TABs?
...
> +static int tps6594_gpio_regmap_xlate(struct gpio_regmap *gpio,
> + unsigned int base, unsigned int offset,
> + unsigned int *reg, unsigned int *mask)
> +{
> + unsigned int line = offset % 8;
> + unsigned int stride = offset / 8;
> +
> + switch (base) {
> + case TPS6594_REG_GPIO1_CONF:
> + *reg = TPS6594_REG_GPIOX_CONF(offset);
> + *mask = TPS6594_BIT_GPIO_DIR;
> + break;
> + case TPS6594_REG_GPIO_IN_1:
> + case TPS6594_REG_GPIO_OUT_1:
> + *reg = base + stride;
> + *mask = BIT(line);
> + break;
> + default:
> + return -EINVAL;
> + }
> + return 0;
You can return directly instead of breaking.
> +}
> + pinctrl->pins = tps6594_pins;
> + pinctrl->pctl_dev =
> + devm_pinctrl_register(&pdev->dev, pctrl_desc, pinctrl);
With
struct device *dev = &pdev->dev;
the above becomes a single line. This may help other similar uses.
> + if (IS_ERR(pinctrl->pctl_dev)) {
> + return dev_err_probe(&pdev->dev, PTR_ERR(pinctrl->pctl_dev),
> + "Couldn't register pinctrl driver\n");
> + }
Also the {} can be dropped.
...
> + pinctrl->gpio_regmap = devm_gpio_regmap_register(&pdev->dev, &config);
> + if (IS_ERR(pinctrl->gpio_regmap)) {
> + return dev_err_probe(&pdev->dev, PTR_ERR(pinctrl->gpio_regmap),
> + "Couldn't register gpio_regmap driver\n");
> + }
Ditto.
...
> +static struct platform_driver tps6594_pinctrl_driver = {
> + .driver = { .name = "tps6594-pinctrl" },
Can you use more standard way of style here, i.e.
.driver = {
.name = "tps6594-pinctrl",
},
In case someone is going to add something there in the future it will become
just as cleaner as possible.
> + .probe = tps6594_pinctrl_probe,
> +};
> +
> +module_platform_driver(tps6594_pinctrl_driver);
Move the above blank line here.
> +MODULE_ALIAS("platform:tps6594-pinctrl");
> +MODULE_AUTHOR("Esteban Blanc <eblanc@xxxxxxxxxxxx>");
...
> +// Used to compute register address of GPIO1_CONF to GPIO11_CONF
This is good.
> -#define TPS6594_REG_GPIOX_CONF(gpio_inst) (0x31 + (gpio_inst))
> +#define TPS6594_REG_GPIOX_CONF(gpio_inst) (0x31 + (gpio_inst))
But why this?!
--
With Best Regards,
Andy Shevchenko
