[PATCH] pinctrl: mediatek: add airoha an7581/an7583 pinctrl driver

[David Lechner]

On 4/20/26 12:21 PM, Mikhail Kshevetskiy wrote:
> The patch adds pinctrl and gpio drivers for an7581/an7583 SoCs.
> The code is based on linux airoha pinctrl driver.
> 
> There are several '#if 0' .. '#endif' in the code. This was done

IMHO, `#if 0` is just noise and we should delete it.

> to make it easier to compare Linux and U-Boot pinctrl drivers.
> 
> Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy at iopsys.eu>
> ---
>  drivers/pinctrl/mediatek/Kconfig          |   22 +-
>  drivers/pinctrl/mediatek/Makefile         |    3 +
>  drivers/pinctrl/mediatek/pinctrl-airoha.c | 3297 +++++++++++++++++++++

Wowsers! This is a lot of code for one file, let alone one patch. hopefully
we can split it up. For example, move the SoC-specific tables to separate
files.

>  drivers/pinctrl/mediatek/pinctrl-airoha.h |   84 +
>  4 files changed, 3405 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/pinctrl/mediatek/pinctrl-airoha.c
>  create mode 100644 drivers/pinctrl/mediatek/pinctrl-airoha.h
> 
> diff --git a/drivers/pinctrl/mediatek/Kconfig b/drivers/pinctrl/mediatek/Kconfig
> index 8a588d17c4b..1ee44ff16d7 100644
> --- a/drivers/pinctrl/mediatek/Kconfig
> +++ b/drivers/pinctrl/mediatek/Kconfig
> @@ -1,4 +1,4 @@
> -if ARCH_MEDIATEK
> +if ARCH_MEDIATEK || ARCH_AIROHA

Why does this need to be mixed in with MediaTek? It doesn't look like
they actually share any code here.

>  
>  config PINCTRL_MTK
>  	depends on PINCTRL_GENERIC
> @@ -6,6 +6,26 @@ config PINCTRL_MTK
>  	select SYSCON
>  	bool
>  
> +endif
> +
> +if ARCH_AIROHA
> +
> +config PINCTRL_AIROHA
> +	bool "Airoha an7581/an7583 SoC pinctrl driver"

As above, to keep binary size down, it would be better to have each platform
individually enabled.

> +	select PINCTRL_MTK
> +	select PINCTRL_FULL
> +	select PINMUX
> +	select PINCONF
> +	help
> +	  Say yes here to support pin control on the Airoha SoC.
> +	  This also provides an interface to the GPIO pins not used by other
> +	  peripherals supporting inputs, outputs, configuring pull-up/pull-down
> +	  and interrupts on input changes.
> +
> +endif
> +
> +if ARCH_MEDIATEK
> +
>  config PINCTRL_MT7622
>  	bool "MT7622 SoC pinctrl driver"
>  	select PINCTRL_MTK
> diff --git a/drivers/pinctrl/mediatek/Makefile b/drivers/pinctrl/mediatek/Makefile
> index b9116c073ea..e3f245bb9d1 100644
> --- a/drivers/pinctrl/mediatek/Makefile
> +++ b/drivers/pinctrl/mediatek/Makefile
> @@ -1,8 +1,11 @@
>  # SPDX-License-Identifier: GPL-2.0
>  # Core
> +ifneq (CONFIG_ARCH_MEDIATEK,)

As above, I don't think this needs to be in the mediatek folder.
If it really does though, I don't think we need the ifneq here
since obj-$(CONFIG_PINCTRL_MTK) should already do the right thing.

>  obj-$(CONFIG_PINCTRL_MTK) += pinctrl-mtk-common.o
> +endif
>  
>  # SoC Drivers
> +obj-$(CONFIG_PINCTRL_AIROHA) += pinctrl-airoha.o
>  obj-$(CONFIG_PINCTRL_MT7622) += pinctrl-mt7622.o
>  obj-$(CONFIG_PINCTRL_MT7623) += pinctrl-mt7623.o
>  obj-$(CONFIG_PINCTRL_MT7629) += pinctrl-mt7629.o
> diff --git a/drivers/pinctrl/mediatek/pinctrl-airoha.c b/drivers/pinctrl/mediatek/pinctrl-airoha.c
> new file mode 100644
> index 00000000000..0bce5bf97dc
> --- /dev/null
> +++ b/drivers/pinctrl/mediatek/pinctrl-airoha.c
> @@ -0,0 +1,3297 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Author: Lorenzo Bianconi <lorenzo at kernel.org>
> + * Author: Benjamin Larsson <benjamin.larsson at genexis.eu>
> + * Author: Markus Gothe <markus.gothe at genexis.eu>
> + *
> + * Adapted for U-Boot by Mikhail Kshevetskiy <mikhail.kshevetskiy at iopsys.eu>
> + */
> +
> +#include <config.h>
> +#include <dm.h>
> +#include <dm/device_compat.h>
> +#include <dm/device-internal.h>
> +#include <dm/devres.h>
> +#include <dm/lists.h>
> +#include <dm/pinctrl.h>
> +#include <dm/root.h>
> +#include <errno.h>
> +#include <asm/io.h>
> +#include <asm-generic/gpio.h>
> +
> +#include <dt-bindings/pinctrl/mt65xx.h>
> +#include <linux/bitfield.h>
> +#include <syscon.h>
> +#include <regmap.h>
> +#include <asm/arch/scu-regmap.h>
> +
> +#include "pinctrl-airoha.h"
> +
> +#define PINCTRL_GET_STATE

It seems like all of these names that start with PINCTRL_ would have a high
chance of conflicting with generic pinctrl stuff in the future. I suggest
to add AIROHA_ or airoha_ prefix to all identifiers to avoid this. 

> +
> +#define PINCTRL_PIN_GROUP(id, table)					\
> +	PINCTRL_PINGROUP(id, table##_pins, ARRAY_SIZE(table##_pins))
> +
> +#define PINCTRL_FUNC_DESC(id, table)					\
> +	{								\
> +		.desc = PINCTRL_PINFUNCTION(id, table##_groups,		\
> +					    ARRAY_SIZE(table##_groups)),\
> +		.groups = table##_func_group,				\
> +		.group_size = ARRAY_SIZE(table##_func_group),		\
> +	}
> +
> +#define PINCTRL_CONF_DESC(p, offset, mask)				\
> +	{								\
> +		.pin = p,						\
> +		.reg = { offset, mask },				\
> +	}
> +

...

> diff --git a/drivers/pinctrl/mediatek/pinctrl-airoha.h b/drivers/pinctrl/mediatek/pinctrl-airoha.h
> new file mode 100644
> index 00000000000..972a4fb171e
> --- /dev/null
> +++ b/drivers/pinctrl/mediatek/pinctrl-airoha.h
> @@ -0,0 +1,84 @@

If we split this up and things defined in this file are used in multiple files,
then keeping this header is fine. If there is anything that is used in just one
file though, I would move it to that .c file.

> +/**
> + * struct pinfunction - Description about a function
> + * @name: Name of the function
> + * @groups: An array of groups for this function
> + * @ngroups: Number of groups in @groups
> + * @flags: Additional pin function flags
> + */
> +struct pinfunction {
> +	const char *name;
> +	const char * const *groups;
> +	size_t ngroups;
> +};

As above, everything in this file should have a airoha_ or AIROHA_
namespace prefix.

> +
> +/* Convenience macro to define a single named pinfunction */
> +#define PINCTRL_PINFUNCTION(_name, _groups, _ngroups)	\
> +(struct pinfunction) {					\
> +		.name = (_name),			\
> +		.groups = (_groups),			\
> +		.ngroups = (_ngroups),			\
> +	}
> +
> +/**
> + * struct pingroup - provides information on pingroup
> + * @name: a name for pingroup
> + * @pins: an array of pins in the pingroup
> + * @npins: number of pins in the pingroup
> + */
> +struct pingroup {
> +	const char *name;
> +	const unsigned int *pins;
> +	size_t npins;
> +};
> +
> +/* Convenience macro to define a single named or anonymous pingroup */
> +#define PINCTRL_PINGROUP(_name, _pins, _npins)	\
> +(struct pingroup) {				\
> +	.name = _name,				\
> +	.pins = _pins,				\
> +	.npins = _npins,			\
> +}
> +
> +/**
> + * struct pinctrl_pin_desc - boards/machines provide information on their
> + * pins, pads or other muxable units in this struct
> + * @number: unique pin number from the global pin number space
> + * @name: a name for this pin
> + * @drv_data: driver-defined per-pin data. pinctrl core does not touch this
> + */
> +struct pinctrl_pin_desc {
> +	unsigned int number;
> +	const char *name;
> +	void *drv_data;
> +};
> +
> +/* Convenience macro to define a single named or anonymous pin descriptor */
> +#define PINCTRL_PIN(a, b) { .number = a, .name = b }
> +
> +/*
> + * Helpful configuration macro to be used in tables etc.
> + */
> +#define PIN_CONF_PACKED(p, a) ((a << 8) | ((unsigned long) p & 0xffUL))
> +
> +/*
> + * The following inlines stuffs a configuration parameter and data value
> + * into and out of an unsigned long argument, as used by the generic pin config
> + * system. We put the parameter in the lower 8 bits and the argument in the
> + * upper 24 bits.
> + */
> +
> +static inline enum pin_config_param pinconf_to_config_param(unsigned long config)
> +{
> +	return (enum pin_config_param) (config & 0xffUL);
> +}
> +
> +static inline u32 pinconf_to_config_argument(unsigned long config)
> +{
> +	return (u32) ((config >> 8) & 0xffffffUL);
> +}
> +
> +static inline unsigned long pinconf_to_config_packed(enum pin_config_param param,
> +			     u32 argument)
> +{
> +	return PIN_CONF_PACKED(param, argument);
> +}

If some of these are meant to be generic functions or macros used by any driver,
then they should be moved to a more generic header file.