On 5/7/26 01:31, David Lechner wrote:
> 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?
Good questions. Probably Markus Gothe or Benjamin Larsson are better
maintainers candidates.
Markus Gothe, Benjamin Larsson what do you think?
>
>> 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.
will fix.
>
>> 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.
will fix
>
>> 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?
No special ordering. Just a kind of ecstatic.
Will fix a bit.
>> +
>> +#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 ().
nice catch, will fix
>
>> + 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()?
will fix
>
>> +
>> + 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()?
will fix
>
>> + 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()?
The bug caused by linux driver porting, will fix.
>> + 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.
will fix
>> + 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.
will fix
>
>> +{
>> + 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.
yeah, will create 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.
>
will fix
