[PATCH v2 2/6] pinctrl: airoha: add pin controller and gpio driver for AN7581 SoC

[David Lechner]

On 4/29/26 12:09 PM, Mikhail Kshevetskiy wrote:
> 
> On 4/28/26 23:42, David Lechner wrote:
>> On 4/28/26 10:34 AM, Mikhail Kshevetskiy wrote:
>>> The driver based on official linux airoha pinctrl and gpio driver with
>>> Matheus Sampaio Queiroga <srherobrine20 at gmail.com> changes.
>>> The changes:
>>>  * Separate code for each SoC and keep some of the functions in
>>>    common between them,
>>>  * Add pinctrl driver for EN7523 SoC.
>> These comments above seem more approiate for the cover letter.
>> I can keep the cover letter in the git history when I pick this up if
>> we think that is important.
> ok
>>> The original Matheus Sampaio Queiroga driver can be taken from the repo:
>>>   https://sirherobrine23.com.br/airoha_an7523/kernel/commits/branch/airoha_an7523_pinctrl
>>>
>>> This patch adds U-Boot pin controller and gpio driver for Airoha AN7581 SoC.
>>>
>>> Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy at iopsys.eu>
>>> ---
>>>  drivers/pinctrl/Kconfig                 |    1 +
>>>  drivers/pinctrl/Makefile                |    1 +
>>>  drivers/pinctrl/airoha/Kconfig          |   16 +
>>>  drivers/pinctrl/airoha/Makefile         |    5 +
>>>  drivers/pinctrl/airoha/airoha-common.h  |  513 +++++++++++
>>>  drivers/pinctrl/airoha/pinctrl-airoha.c |  691 +++++++++++++++
>>>  drivers/pinctrl/airoha/pinctrl-an7581.c | 1060 +++++++++++++++++++++++
>> Let's split out an7581 support into a separate patch too and have
>> this patch just be the core/shared code.
> ok
>>
>>>  7 files changed, 2287 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
>>>  create mode 100644 drivers/pinctrl/airoha/pinctrl-an7581.c
>>>
>>> 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/
>> Alphabetical order?
> Could you provide your vision or more details?
> What is the best sorting strategy?
> 
> Should it be sorted by CONFIG_* usage or by corresponding filename?
> Should be files and directories sorted repeatedly?

I was just expecting it to be added before broadcom like in the Kconfig.
I didn't look at this file beyond this context shown here though, so
maybe it is just a mess already and not sorted well.

In general though, sorting by CONFIG_* probably makes the most sense
since it is easy to mechanically sort it then.


>>> +//#define PINCTRL_GET_STATE
>>> +
>> Drop commented/dead code. If there is a good reason to keep
>> airoha_pinctrl_get_conf(), the make a comment there, otherwise
>> drop it.
>>
> This code allows reading current pinmux/pinconf settings out of hardware.
> It might be very useful for debugging reasons, so I prefer keep it.  

OK. FYI, I did something similar with mtk_pinconf_get() recently
so that it prints the current config in the `pinmux` command. Then
debugging can be done without modifying the source code.

>>> +int airoha_pinctrl_probe(struct udevice *dev)
>>> +{
>>> +	struct airoha_pinctrl *pinctrl = dev_get_priv(dev);
>>> +
>>> +	pinctrl->dev = dev;
>>> +	pinctrl->data = (struct airoha_pinctrl_match_data *)dev_get_driver_data(dev);
>> nit: casting isn't needed here (leaving it out makes it more readable)
> 
> Yes, it will be a bit better readable. Unfortunately dev_get_driver_data() returns ulong,
> but pinctrl->data is a pointer. The compiler might print warning.

Ah right, I forgot that. I usually do (void *) in that case since it
is shorter, but leaving it the way it is is fine too.