On Tue, Mar 18, 2025 at 05:26:19PM +0100, Mathieu Dubois-Briand wrote:
> Add driver for Maxim Integrated MAX7360 pinctrl on the PORT pins. Pins
> can be used either for GPIO, PWM or rotary encoder functionalities.
...
> + help
> + Say Y here to enable Pin control support for Maxim MAX7360 keypad
s/Pin/pin/
> + controller.
> + This keypad controller has 8 GPIO pins that work as GPIO as well as
"...that may work as GPIO, or PWM, or..."
> + PWM or rotary encoder alternate modes.
...
+ array_size.h
+ dev_printk.h
+ device/devres.h // currently only in Linux Next
+ err.h
> +#include <linux/init.h>
> +#include <linux/mfd/max7360.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/pinctrl/pinconf-generic.h>
> +#include <linux/pinctrl/pinmux.h>
We usually move this group of inclusions...
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
...to somewhere here.
> +#include "core.h"
> +#include "pinmux.h"
...
> +static const struct pingroup max7360_groups[] = {
> + PINCTRL_PINGROUP("PORT0", port0_pins, ARRAY_SIZE(port0_pins)),
> + PINCTRL_PINGROUP("PORT1", port1_pins, ARRAY_SIZE(port1_pins)),
> + PINCTRL_PINGROUP("PORT2", port2_pins, ARRAY_SIZE(port2_pins)),
> + PINCTRL_PINGROUP("PORT3", port3_pins, ARRAY_SIZE(port3_pins)),
> + PINCTRL_PINGROUP("PORT4", port4_pins, ARRAY_SIZE(port4_pins)),
> + PINCTRL_PINGROUP("PORT5", port5_pins, ARRAY_SIZE(port5_pins)),
> + PINCTRL_PINGROUP("PORT6", port6_pins, ARRAY_SIZE(port6_pins)),
> + PINCTRL_PINGROUP("PORT7", port7_pins, ARRAY_SIZE(port7_pins)),
> + PINCTRL_PINGROUP("ROTARY", rotary_pins, ARRAY_SIZE(rotary_pins))
Leave trailing comma. Helps in the future in case of expansion.
> +};
...
> +static const char * const simple_groups[] = {"PORT0", "PORT1", "PORT2", "PORT3",
> + "PORT4", "PORT5", "PORT6", "PORT7"};
It's better to read when split as
static const char * const simple_groups[] = {
"PORT0", "PORT1", "PORT2", "PORT3",
"PORT4", "PORT5", "PORT6", "PORT7",
};
(also note the trailing comma).
…
> +static const char * const rotary_groups[] = {"ROTARY"};
Add spaces inside {}.
...
> +#define MAX7360_PINCTRL_FN_ROTARY 2
> +static const struct pinfunction max7360_functions[] = {
> + PINCTRL_PINFUNCTION("gpio", simple_groups, ARRAY_SIZE(simple_groups)),
> + PINCTRL_PINFUNCTION("pwm", simple_groups, ARRAY_SIZE(simple_groups)),
> + [MAX7360_PINCTRL_FN_ROTARY] = PINCTRL_PINFUNCTION("rotary", rotary_groups,
> + ARRAY_SIZE(rotary_groups)),
Please make them all look the same, if indexed, than add indices to all.
> +};
...
> +static int max7360_set_mux(struct pinctrl_dev *pctldev, unsigned int selector,
> + unsigned int group)
> +{
> + struct regmap *regmap;
> + int ret = 0;
Variable is not needed, just return directly.
> + int val;
> +
> + /*
> + * GPIO and PWM functions are the same: we only need to handle the
> + * rotary encoder function, on pins 6 and 7.
> + */
> + if (max7360_groups[group].pins[0] >= 6) {
> + if (selector == MAX7360_PINCTRL_FN_ROTARY)
> + val = MAX7360_GPIO_CFG_RTR_EN;
> + else
> + val = 0;
> +
> + regmap = dev_get_regmap(pctldev->dev, NULL);
> + ret = regmap_write_bits(regmap, MAX7360_REG_GPIOCFG, MAX7360_GPIO_CFG_RTR_EN, val);
> + }
> +
> + return ret;
> +}
...
> +static int max7360_pinctrl_probe(struct platform_device *pdev)
> +{
With
struct device *dev = &pdev->dev;
the below will look better.
> + struct regmap *regmap;
> + struct pinctrl_desc *pd;
> + struct max7360_pinctrl *chip;
> +
> + chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
> + if (!chip)
> + return -ENOMEM;
> +
> + regmap = dev_get_regmap(pdev->dev.parent, NULL);
> + if (!regmap)
> + dev_err_probe(&pdev->dev, -ENODEV, "Could not get parent regmap\n");
Make it first check, in such a case you don't even need to allocate memory for
peanuts.
> + pd = &chip->pinctrl_desc;
> +
> + pd->pctlops = &max7360_pinctrl_ops;
> + pd->pmxops = &max7360_pmxops;
> + pd->name = dev_name(&pdev->dev);
> + pd->pins = max7360_pins;
> + pd->npins = MAX7360_MAX_GPIO;
> + pd->owner = THIS_MODULE;
> +
> + chip->pctldev = devm_pinctrl_register(pdev->dev.parent, pd, chip);
> + if (IS_ERR(chip->pctldev))
> + return dev_err_probe(&pdev->dev, PTR_ERR(chip->pctldev),
> + "can't register controller\n");
> +
> + return 0;
> +}
--
With Best Regards,
Andy Shevchenko
