On 5/4/26 8:03 AM, Mikhail Kshevetskiy wrote:
> This patch introduce shared Airoha pinctrl code.
>
> Signed-off-by: Mikhail Kshevetskiy <[email protected]>
> ---
> drivers/pinctrl/Kconfig | 1 +
> drivers/pinctrl/Makefile | 1 +
> drivers/pinctrl/airoha/Kconfig | 11 +
> drivers/pinctrl/airoha/Makefile | 3 +
> drivers/pinctrl/airoha/airoha-common.h | 512 ++++++++++++++
> drivers/pinctrl/airoha/pinctrl-airoha.c | 843 ++++++++++++++++++++++++
> 6 files changed, 1371 insertions(+)
> create mode 100644 drivers/pinctrl/airoha/Kconfig
> create mode 100644 drivers/pinctrl/airoha/Makefile
> create mode 100644 drivers/pinctrl/airoha/airoha-common.h
> create mode 100644 drivers/pinctrl/airoha/pinctrl-airoha.c
I don't remember if I asked before...
Should we add a MAINTAINERS entry for this new directory?
>
> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
> index 578edbf8168..46a95a1ab6b 100644
> --- a/drivers/pinctrl/Kconfig
> +++ b/drivers/pinctrl/Kconfig
> @@ -405,6 +405,7 @@ config SPL_PINCTRL_ZYNQMP
>
> endif
>
> +source "drivers/pinctrl/airoha/Kconfig"
> source "drivers/pinctrl/broadcom/Kconfig"
> source "drivers/pinctrl/exynos/Kconfig"
> source "drivers/pinctrl/intel/Kconfig"
> diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
> index 29fb9b484d0..b03e838ab39 100644
> --- a/drivers/pinctrl/Makefile
> +++ b/drivers/pinctrl/Makefile
> @@ -24,6 +24,7 @@ obj-$(CONFIG_PINCTRL_PIC32) += pinctrl_pic32.o
> obj-$(CONFIG_PINCTRL_EXYNOS) += exynos/
> obj-$(CONFIG_PINCTRL_K210) += pinctrl-k210.o
> obj-$(CONFIG_PINCTRL_MESON) += meson/
> +obj-$(CONFIG_PINCTRL_AIROHA) += airoha/
I still think this could be better placed. It looks like there is
at leaset some attempt to put these in alphabetical order.
> obj-$(CONFIG_PINCTRL_MTK) += mediatek/
> obj-$(CONFIG_PINCTRL_MSCC) += mscc/
> obj-$(CONFIG_ARCH_MVEBU) += mvebu/
> diff --git a/drivers/pinctrl/airoha/Kconfig b/drivers/pinctrl/airoha/Kconfig
> new file mode 100644
> index 00000000000..eb87afbb374
> --- /dev/null
> +++ b/drivers/pinctrl/airoha/Kconfig
> @@ -0,0 +1,11 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +config PINCTRL_AIROHA
> + depends on ARCH_AIROHA
> + select PINCTRL_FULL
> + select PINCTRL_GENERIC
> + select PINMUX
> + select PINCONF
> + select REGMAP
> + select SYSCON
> + bool
> diff --git a/drivers/pinctrl/airoha/Makefile b/drivers/pinctrl/airoha/Makefile
> new file mode 100644
> index 00000000000..a25b744dd7a
> --- /dev/null
> +++ b/drivers/pinctrl/airoha/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +obj-$(CONFIG_PINCTRL_AIROHA) += pinctrl-airoha.o
> diff --git a/drivers/pinctrl/airoha/airoha-common.h
> b/drivers/pinctrl/airoha/airoha-common.h
> new file mode 100644
> index 00000000000..af4964bf25d
> --- /dev/null
> +++ b/drivers/pinctrl/airoha/airoha-common.h
> @@ -0,0 +1,512 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef __AIROHA_COMMON_HEADER__
> +#define __AIROHA_COMMON_HEADER__
> +
...
> +static const u32 gpio_data_regs[] = {
> + REG_GPIO_DATA,
> + REG_GPIO_DATA1
> +};
> +
> +static const u32 gpio_out_regs[] = {
> + REG_GPIO_OE,
> + REG_GPIO_OE1
> +};
> +
> +static const u32 gpio_dir_regs[] = {
> + REG_GPIO_CTRL,
> + REG_GPIO_CTRL1,
> + REG_GPIO_CTRL2,
> + REG_GPIO_CTRL3
> +};
> +
> +static const u32 irq_status_regs[] = {
> + REG_GPIO_INT,
> + REG_GPIO_INT1
> +};
> +
> +static const u32 irq_level_regs[] = {
> + REG_GPIO_INT_LEVEL,
> + REG_GPIO_INT_LEVEL1,
> + REG_GPIO_INT_LEVEL2,
> + REG_GPIO_INT_LEVEL3
> +};
> +
> +static const u32 irq_edge_regs[] = {
> + REG_GPIO_INT_EDGE,
> + REG_GPIO_INT_EDGE1,
> + REG_GPIO_INT_EDGE2,
> + REG_GPIO_INT_EDGE3
> +};
Putting these arrays in the header isn't a good idea. Each compile
unit (C file) that includes this header will have them repeated. It
looks like they are only used in pinctrl-arioha.c anyway, so they
should should just be moved there.
> diff --git a/drivers/pinctrl/airoha/pinctrl-airoha.c
> b/drivers/pinctrl/airoha/pinctrl-airoha.c
> new file mode 100644
> index 00000000000..11830c44612
> --- /dev/null
> +++ b/drivers/pinctrl/airoha/pinctrl-airoha.c
> @@ -0,0 +1,843 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Author: Lorenzo Bianconi <[email protected]>
> + * Author: Benjamin Larsson <[email protected]>
> + * Author: Markus Gothe <[email protected]>
> + * Author: Mikhail Kshevetskiy <[email protected]>
> + */
> +#include <dm.h>
> +#include <dm/device_compat.h>
> +#include <dm/device-internal.h>
> +#include <dm/lists.h>
> +#include <dm/ofnode.h>
> +#include <asm-generic/gpio.h>
> +#include <dt-bindings/pinctrl/mt65xx.h>
> +#include <regmap.h>
> +
> +#include <syscon.h>
> +#include <asm/arch/scu-regmap.h>
Not sure I understand the organization of the includes
above. Why the blank line between regmap and syscon? Why
not everything in alphabetical order?
> +
> +#include "airoha-common.h"
> +
> +
> +static int pin_in_group(unsigned int pin, const struct pingroup *grp)
> +{
> + for (int i = 0; i < grp->npins; i++) {
> + if (grp->pins[i] == pin)
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> +static int pin_to_gpio(struct airoha_pinctrl *pinctrl, unsigned int pin)
> +{
> + struct airoha_pinctrl_match_data *data = pinctrl->data;
> +
> + if ((pin < data->gpio_offs) ||
> + (pin > data->gpio_offs + data->gpio_pin_cnt))
Should this be >= for the upper bound check?
Also has unnecessary ().
> + return -EINVAL;
> +
> + return pin - data->gpio_offs;
> +}
> +
> +static int airoha_pinctrl_get_conf(struct airoha_pinctrl *pinctrl,
> + enum airoha_pinctrl_confs_type conf_type,
> + int pin, u32 *val)
> +{
> + const struct airoha_pinctrl_confs_info *confs_info;
> + const struct airoha_pinctrl_reg *reg;
> +
> + confs_info = &pinctrl->data->confs_info[conf_type];
> +
> + reg = airoha_pinctrl_get_conf_reg(confs_info->confs,
> + confs_info->num_confs,
> + pin);
> + if (!reg)
> + return -EINVAL;
> +
> + if (regmap_read(pinctrl->chip_scu, reg->offset, val))
> + return -EINVAL;
> +
> + *val = (*val & reg->mask) >> __ffs(reg->mask);
field_get()?
> +
> + return 0;
> +}
> +
> +static int airoha_pinctrl_set_conf(struct airoha_pinctrl *pinctrl,
> + enum airoha_pinctrl_confs_type conf_type,
> + int pin, u32 val)
> +{
> + const struct airoha_pinctrl_confs_info *confs_info;
> + const struct airoha_pinctrl_reg *reg = NULL;
> +
> + confs_info = &pinctrl->data->confs_info[conf_type];
> +
> + reg = airoha_pinctrl_get_conf_reg(confs_info->confs,
> + confs_info->num_confs,
> + pin);
> + if (!reg)
> + return -EINVAL;
> +
> + if (regmap_update_bits(pinctrl->chip_scu, reg->offset, reg->mask,
> + val << __ffs(reg->mask)))
field_prep()?
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static int airoha_pinconf_get_direction(struct airoha_pinctrl *pinctrl, u32
> p)
> +{
> + int gpio;
> +
> + gpio = pin_to_gpio(pinctrl, p);
> + if (gpio < 0)
> + return gpio;
> +
> + return airoha_gpio_get_direction(pinctrl, gpio);
> +}
> +
> +static int airoha_pinconf_get(struct airoha_pinctrl *pinctrl,
> + unsigned int pin, unsigned long *config)
> +{
> + enum pin_config_param param = pinconf_to_config_param(*config);
> + u32 arg;
> +
> + switch (param) {
> + case PIN_CONFIG_BIAS_PULL_DOWN:
> + case PIN_CONFIG_BIAS_DISABLE:
> + case PIN_CONFIG_BIAS_PULL_UP: {
> + u32 pull_up, pull_down;
> +
> + if (airoha_pinctrl_get_pullup_conf(pinctrl, pin, &pull_up) ||
> + airoha_pinctrl_get_pulldown_conf(pinctrl, pin, &pull_down))
> + return -EINVAL;
> +
> + if (param == PIN_CONFIG_BIAS_PULL_UP &&
> + !(pull_up && !pull_down))
> + return -EINVAL;
> + else if (param == PIN_CONFIG_BIAS_PULL_DOWN &&
> + !(pull_down && !pull_up))
> + return -EINVAL;
> + else if (pull_up || pull_down)
> + return -EINVAL;
> +
> + arg = 1;
> + break;
> + }
> + case PIN_CONFIG_DRIVE_STRENGTH: {
> + u32 e2, e4;
> +
> + if (airoha_pinctrl_get_drive_e2_conf(pinctrl, pin, &e2) ||
> + airoha_pinctrl_get_drive_e4_conf(pinctrl, pin, &e4))
> + return -EINVAL;
> +
> + arg = e4 << 1 | e2;
> + break;
> + }
> + case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> + if (airoha_pinctrl_get_pcie_rst_od_conf(pinctrl, pin, &arg))
> + return -EINVAL;
> + break;
> + case PIN_CONFIG_OUTPUT_ENABLE:
> + case PIN_CONFIG_INPUT_ENABLE:
> + arg = airoha_pinconf_get_direction(pinctrl, pin);
> + if (arg != param)
How does this work? It looks like airoha_pinconf_get_direction()
returns enum gpio_func_t or negative error code.
Should we check for error first before comparing?
Do we need some kind of conversion between enum gpio_func_t and
enum pin_config_param in airoha_pinconf_get_direction()?
> + return -EINVAL;
> +
> + arg = 1;
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> +
> + *config = pinconf_to_config_packed(param, arg);
> +
> + return 0;
> +}
> +
> +static int airoha_pinconf_set_pin_value(struct airoha_pinctrl *pinctrl,
> + unsigned int p, bool value)
> +{
> + int gpio;
> +
> + gpio = pin_to_gpio(pinctrl, p);
> + if (gpio < 0)
> + return gpio;
> +
> + return airoha_gpio_set(pinctrl, gpio, value);
> +}
> +
> +static int airoha_pinconf_set(struct airoha_pinctrl *pinctrl,
> + unsigned int pin, unsigned long *configs,
> + unsigned int num_configs)
> +{
> + int i;
> +
> + for (i = 0; i < num_configs; i++) {
> + u32 param = pinconf_to_config_param(configs[i]);
> + u32 arg = pinconf_to_config_argument(configs[i]);
> +
In all of the cases below, I would expect that we should be checking
the return value for error. Or add a comment on why it is safe to ignore
the return value if that is the case.
> + switch (param) {
> + case PIN_CONFIG_BIAS_DISABLE:
> + airoha_pinctrl_set_pulldown_conf(pinctrl, pin, 0);
> + airoha_pinctrl_set_pullup_conf(pinctrl, pin, 0);
> + break;
> + case PIN_CONFIG_BIAS_PULL_UP:
> + airoha_pinctrl_set_pulldown_conf(pinctrl, pin, 0);
> + airoha_pinctrl_set_pullup_conf(pinctrl, pin, 1);
> + break;
> + case PIN_CONFIG_BIAS_PULL_DOWN:
> + airoha_pinctrl_set_pulldown_conf(pinctrl, pin, 1);
> + airoha_pinctrl_set_pullup_conf(pinctrl, pin, 0);
> + break;
> + case PIN_CONFIG_DRIVE_STRENGTH: {
> + u32 e2 = 0, e4 = 0;
> +
> + switch (arg) {
> + case MTK_DRIVE_2mA:
> + break;
> + case MTK_DRIVE_4mA:
> + e2 = 1;
> + break;
> + case MTK_DRIVE_6mA:
> + e4 = 1;
> + break;
> + case MTK_DRIVE_8mA:
> + e2 = 1;
> + e4 = 1;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + airoha_pinctrl_set_drive_e2_conf(pinctrl, pin, e2);
> + airoha_pinctrl_set_drive_e4_conf(pinctrl, pin, e4);
> + break;
> + }
> + case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> + airoha_pinctrl_set_pcie_rst_od_conf(pinctrl, pin,
> !!arg);
> + break;
> + case PIN_CONFIG_OUTPUT_ENABLE:
> + case PIN_CONFIG_INPUT_ENABLE:
> + case PIN_CONFIG_OUTPUT: {
> + bool input = param == PIN_CONFIG_INPUT_ENABLE;
> + int err;
> +
> + err = airoha_pinmux_set_direction(pinctrl, pin, input);
> + if (err)
> + return err;
> +
> + if (param == PIN_CONFIG_OUTPUT) {
> + err = airoha_pinconf_set_pin_value(pinctrl,
> + pin, !!arg);
> + if (err)
> + return err;
> + }
> +
> + break;
> + }
> + default:
> + return -EOPNOTSUPP;
> + }
> + }
> +
> + return 0;
> +}
> +
> +/**************************
> + * pinctrl driver interface
> + **************************/
> +static int airoha_get_pins_count(struct udevice *dev)
> +{
> + struct airoha_pinctrl *pinctrl = dev_get_priv(dev);
> +
> + return pinctrl->data->num_pins;
> +}
> +
> +static const char *airoha_get_pin_name(struct udevice *dev, unsigned
> selector)
Inconsitent use of unsigned. Preferred is unsigned int.
> +{
> + struct airoha_pinctrl *pinctrl = dev_get_priv(dev);
> +
> + return pinctrl->data->pins[selector].name;
> +}
> +
> +static int airoha_pinconf_set_handler(struct udevice *dev, unsigned
> pin_selector,
> + unsigned param, unsigned argument)
> +{
> + struct airoha_pinctrl *pinctrl = dev_get_priv(dev);
> + unsigned long configs[1] = { pinconf_to_config_packed(param, argument)
> };
> + unsigned int pin = pinctrl->data->pins[pin_selector].number;
> +
> + dev_dbg(dev, "enabling %s=%d property for pin %s\n",
> + airoha_pinconf_params[param].property, argument,
This will result in out of bounds access. param is not the index but rather
a field in the struct in the array.
Easiset would be to just print numeric value of param, otherwise we need a
lookup function.
> + airoha_get_pin_name(dev, pin_selector));
> +
> + return airoha_pinconf_set(pinctrl, pin, configs,
> + ARRAY_SIZE(configs));
> +}
> +
> +static int airoha_pinconf_group_set_handler(struct udevice *dev, unsigned
> group_selector,
> + unsigned param, unsigned argument)
> +{
> + struct airoha_pinctrl *pinctrl = dev_get_priv(dev);
> + unsigned long configs[1] = { pinconf_to_config_packed(param, argument)
> };
> +
> + dev_dbg(dev, "enabling %s=%d property for pin group %s\n",
> + airoha_pinconf_params[param].property, argument,
ditto
> + airoha_get_group_name(dev, group_selector));
> +
> + return airoha_pinconf_group_set(pinctrl, group_selector,
> + configs, ARRAY_SIZE(configs));
> +}
> +
> +static int airoha_get_pin_muxing(struct udevice *dev, unsigned int selector,
> + char *buf, int size)
> +{
> + struct airoha_pinctrl *pinctrl = dev_get_priv(dev);
> + struct airoha_pinctrl_match_data *data = pinctrl->data;
> + const char *name, *type;
> + int ret, gpio, found = 0;
> + unsigned long config;
> + unsigned int param, pin;
> + u32 val;
> +
> + pin = data->pins[selector].number;
> + for (int i = 0; i < data->num_grps; i++) {
> + if (!pin_in_group(pin, &data->grps[i]))
> + continue;
> +
> + name = data->grps[i].name;
> + for (int j = 0; j < data->num_funcs; j++) {
> + if (!func_grp_active(pinctrl, &data->funcs[j], name))
> + continue;
> +
> + ret = snprintf(buf, size, "%s(%s)",
> + data->funcs[j].desc.name, name);
Should use scnprintf(). snprintf() returns the number that would have been
written, which can be larger than number actually written.
Same applies elsewhere in this patch.
