On Tue, Feb 27, 2024 at 03:55:26PM +0100, Théo Lebrun wrote:
> Add the Mobileye EyeQ5 pin controller driver. It might grow to add later
> support of other platforms from Mobileye. It belongs to a syscon region
> called OLB.
>
> Existing pins and their function live statically in the driver code
> rather than in the devicetree, see compatible match data.
...
> +config PINCTRL_EYEQ5
> + bool "Mobileye EyeQ5 pinctrl driver"
Can't be a module?
> + depends on OF
It's even not needed for this software as far as I can tell from the code.
> + depends on MACH_EYEQ5 || COMPILE_TEST
> + select PINMUX
> + select GENERIC_PINCONF
> + select MFD_SYSCON
> + default MACH_EYEQ5
> + help
> + Pin controller driver for the Mobileye EyeQ5 platform. It does both
> + pin config & pin muxing. It does not handle GPIO.
> +
> + Pin muxing supports two functions for each pin: first is GPIO, second
> + is pin-dependent. Pin config is about bias & drive strength.
...
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/pinctrl/pinconf-generic.h>
> +#include <linux/pinctrl/pinconf.h>
> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/pinctrl/pinmux.h>
> +#include <linux/platform_device.h>
> +#include <linux/seq_file.h>
Semi-random list of the inclusions. Please, fix it.
While doing that, group out pinctrl/* ones as it's done in other drivers.
> +#include "core.h"
> +#include "pinctrl-utils.h"
...
> +struct eq5p_function {
> + const char *name;
> + const char * const *groups;
> + unsigned int ngroups;
> +};
We have struct pinfunction, use it instead.
...
> +static const char * const gpio_groups[] = {
> + /* Bank A */
> + "PA0", "PA1", "PA2", "PA3", "PA4", "PA5", "PA6", "PA7", "PA8", "PA9",
> + "PA10", "PA11", "PA12", "PA13", "PA14", "PA15", "PA16", "PA17", "PA18",
> + "PA19", "PA20", "PA21", "PA22", "PA23", "PA24", "PA25", "PA26", "PA27",
> + "PA28",
For all arrays like this, please split them on 4/8/10/16 items per line as it's
much easier to count and refer by index when reading the code.
> + /* Bank B */
> + "PB0", "PB1", "PB2", "PB3", "PB4", "PB5", "PB6", "PB7", "PB8", "PB9",
> + "PB10", "PB11", "PB12", "PB13", "PB14", "PB15", "PB16", "PB17", "PB18",
> + "PB19", "PB20", "PB21", "PB22",
> +};
...
> +#define FUNCTION(a, b) { .name = a, .groups = b, .ngroups = ARRAY_SIZE(b) }
Use PINCTRL_PINFUNCTION() instead.
...
> +static bool eq5p_test_bit(const struct eq5p_pinctrl *pctrl,
> + enum eq5p_bank bank, enum eq5p_regs reg, int offset)
> +{
> + u32 val = readl(pctrl->base + eq5p_regs[bank][reg]);
> + if (WARN_ON(offset > 31))
> + return false;
When this condition can be true?
> + return (val & BIT(offset)) != 0;
> +}
...
> +static int eq5p_pinconf_get(struct pinctrl_dev *pctldev, unsigned int pin,
> + unsigned long *config);
Can't you avoid forward declarations?
...
> + if (!eq5p_test_bit(pctrl, bank, EQ5P_IOCR, offset)) {
What's wrong with positive conditional?
> + } else {
> + }
...
> +static const struct pinctrl_ops eq5p_pinctrl_ops = {
> + .get_groups_count = eq5p_pinctrl_get_groups_count,
> + .get_group_name = eq5p_pinctrl_get_group_name,
> + .get_group_pins = eq5p_pinctrl_get_group_pins,
> + .pin_dbg_show = eq5p_pinctrl_pin_dbg_show,
> + .dt_node_to_map = pinconf_generic_dt_node_to_map_pin,
> + .dt_free_map = pinctrl_utils_free_map,
ifdef is missing for these... But the question is, isn't these a default when
OF is in use?
> +};
...
> + dev_dbg(pctldev->dev, "%s: func=%s group=%s\n", __func__, func_name,
> + group_name);
Drop __func__ from all debug messages. With Dynamic Debug enabled (which is
often the case) we can do it at run-time).
...
> + mask = BIT(offset);
> + val = is_gpio ? 0 : U32_MAX;
I think you meant something else (semantically) than U32_MAX.
Perhaps GENMASK(31, 0)?
...
> +static int eq5p_pinconf_get(struct pinctrl_dev *pctldev, unsigned int pin,
> + unsigned long *config)
> +{
> + enum pin_config_param param = pinconf_to_config_param(*config);
> + struct eq5p_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> + unsigned int offset = eq5p_pin_to_offset(pin);
> + enum eq5p_bank bank = eq5p_pin_to_bank(pin);
> + u32 val_ds, arg = 0;
What's arg assignment for?
> + bool pd, pu;
> +
> + pd = eq5p_test_bit(pctrl, bank, EQ5P_PD, offset);
> + pu = eq5p_test_bit(pctrl, bank, EQ5P_PU, offset);
> +
> + switch (param) {
> + case PIN_CONFIG_BIAS_DISABLE:
> + arg = !(pd || pu);
> + break;
> + case PIN_CONFIG_BIAS_PULL_DOWN:
> + arg = pd;
> + break;
> + case PIN_CONFIG_BIAS_PULL_UP:
> + arg = pu;
> + break;
> + case PIN_CONFIG_DRIVE_STRENGTH:
> + offset *= 2; /* two bits per pin */
> + if (offset >= 32) {
> + val_ds = readl(pctrl->base + eq5p_regs[bank][EQ5P_DS_HIGH]);
> + offset -= 32;
> + } else {
> + val_ds = readl(pctrl->base + eq5p_regs[bank][EQ5P_DS_LOW]);
> + }
I'm wondering why you can't use your helpers before multiplication?
> + arg = (val_ds >> offset) & 0b11;
GENMASK(1, 0)
> + break;
> + default:
> + return -ENOTSUPP;
> + }
> +
> + *config = pinconf_to_config_packed(param, arg);
> + return 0;
> +}
...
> +static int eq5p_pinconf_set_drive_strength(struct pinctrl_dev *pctldev,
> + unsigned int pin, u32 arg)
> +{
> + struct eq5p_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> + unsigned int offset = eq5p_pin_to_offset(pin);
> + enum eq5p_bank bank = eq5p_pin_to_bank(pin);
> + unsigned int reg;
> + u32 mask, val;
> +
> + if (arg > 3) {
Magic number.
> + dev_err(pctldev->dev, "Unsupported drive strength: %u\n", arg);
> + return -EINVAL;
> + }
> +
> + offset *= 2; /* two bits per pin */
> +
> + if (offset >= 32) {
> + reg = EQ5P_DS_HIGH;
> + offset -= 32;
> + } else {
> + reg = EQ5P_DS_LOW;
> + }
> + mask = 0b11 << offset;
> + val = arg << offset;
> + eq5p_update_bits(pctrl, bank, reg, mask, val);
Similar comments as per previous function.
> + return 0;
> +}
...
> +static const struct of_device_id eq5p_match[] = {
> + { .compatible = "mobileye,eyeq5-pinctrl" },
> + {},
No comma in the terminator entry.
> +};
No MODULE_DEVICE_TABLE()?
> +static struct platform_driver eq5p_driver = {
> + .driver = {
> + .name = "eyeq5-pinctrl",
> + .of_match_table = eq5p_match,
> + },
> + .probe = eq5p_probe,
> +};
> +
Unneeded blank line.
> +builtin_platform_driver(eq5p_driver);
--
With Best Regards,
Andy Shevchenko
