linux-kernel - Re: [PATCH 2/4] pinctrl: ingenic: add x1600 support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <C4FCA3DA-67C9-4553-8506-4EC5AC81E3D0@goldelico.com>
Date: Wed, 26 Feb 2025 21:09:39 +0100
From: "H. Nikolaus Schaller" <hns@...delico.com>
To: Paul Cercueil <paul@...pouillou.net>,
 Paul Boddie <paul@...die.org.uk>
Cc: Linus Walleij <linus.walleij@...aro.org>,
 Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>,
 Andreas Kemnade <andreas@...nade.info>,
 Tim Bysun <tim.bysun@...enic.com>,
 linux-gpio@...r.kernel.org,
 devicetree <devicetree@...r.kernel.org>,
 Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
 linux-mips@...r.kernel.org,
 Discussions about the Letux Kernel <letux-kernel@...nphoenux.org>
Subject: Re: [PATCH 2/4] pinctrl: ingenic: add x1600 support

Hi all,

> Am 26.02.2025 um 20:42 schrieb H. Nikolaus Schaller <hns@...delico.com>:
> 
> Hi Paul,
> 
>> Am 26.02.2025 um 19:43 schrieb Paul Cercueil <paul@...pouillou.net>:
>> 
>> Hi Nikolaus, and everyone involved,
>> 
>> Le mercredi 26 février 2025 à 18:16 +0100, H. Nikolaus Schaller a
>> écrit :
>>> From: Paul Boddie <paul@...die.org.uk>
>>> 
>>> Add support for the Lumissil/Ingenic X1600 SoC.
>>> 
>>> It uses shadow registers to commit changes to multiple pinctrl
>>> registers in parallel.
>>> 
>>> Define specific Chip ID, register offsets, pin tables etc.
>>> 
>>> Handling the unique X1600_GPIO_PU only for the x1600 but
>>> not for x1830 and above must be carefully taken into account.
>>> 
>>> Co-authored-by: Andreas Kemnade <andreas@...nade.info>
>>> Co-authored-by: H. Nikolaus Schaller <hns@...delico.com>
>>> Signed-off-by: Paul Boddie <paul@...die.org.uk>
>>> Signed-off-by: Andreas Kemnade <andreas@...nade.info>
>>> Signed-off-by: H. Nikolaus Schaller <hns@...delico.com>
>>> ---
>>> drivers/pinctrl/pinctrl-ingenic.c | 242
>>> +++++++++++++++++++++++++++++-
>>> 1 file changed, 240 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/drivers/pinctrl/pinctrl-ingenic.c
>>> b/drivers/pinctrl/pinctrl-ingenic.c
>>> index bc7ee54e062b5..dfdc89ece9b8a 100644
>>> --- a/drivers/pinctrl/pinctrl-ingenic.c
>>> +++ b/drivers/pinctrl/pinctrl-ingenic.c
>>> 

...

>>> +static int x1600_pwm_pwm0_pins[] = { 0x40, };
>>> +static int x1600_pwm_pwm1_pins[] = { 0x41, };
>>> +static int x1600_pwm_pwm2_pins[] = { 0x42, };
>>> +static int x1600_pwm_pwm3_pins[] = { 0x58, };
>>> +static int x1600_pwm_pwm4_pins[] = { 0x59, };
>>> +static int x1600_pwm_pwm5_pins[] = { 0x33, 0x5a, };
>>> +static int x1600_pwm_pwm6_pins[] = { 0x29, 0x34, };
>>> +static int x1600_pwm_pwm7_pins[] = { 0x2a, 0x35, };
>> 
>> Just a quick question about these ones: why are there 2 pins here? If
>> you have the PWM5/6/7 function on two different pins then you should
>> probably have two groups.
> 
> I think they are added through INGENIC_PIN_GROUP_FUNCS()
> to x1600_groups[].
> 
> So the pins list is different from pwm0 to 4 which
> uses INGENIC_PIN_GROUP().

I have now checked with the programming manual.

Yes, pwm5 to pwm7 can be pinmuxed to different pads,
while pwm0 to pwm4 have only one option.

E.g. pwm5: PC26 in function 1 or PB19 in function 2.

This is simular to what we have with uart2-data-a and
uart2-data-b.

So I tend to agree that we need different pin groups
("pwm5-a", "pwm5-b") and no need to use INGENIC_PIN_GROUP_FUNCS().

Maybe we didn't realize since we have not yet used PWM in
any x1600 based device.

...

>>> +static int x1600_pwm_pwm5_funcs[] = { 2, 1, };
>>> +static int x1600_pwm_pwm6_funcs[] = { 1, 2, };
>>> +static int x1600_pwm_pwm7_funcs[] = { 1, 2, };
>>> +
>>> +static const struct group_desc x1600_groups[] = {
>>> + INGENIC_PIN_GROUP("uart0-data", x1600_uart0_data, 0),
>>> + INGENIC_PIN_GROUP("uart0-hwflow", x1600_uart0_hwflow, 0),
>>> + INGENIC_PIN_GROUP("uart1-data", x1600_uart1_data, 1),
>>> + INGENIC_PIN_GROUP("uart1-hwflow", x1600_uart1_hwflow, 1),
>>> + INGENIC_PIN_GROUP("uart2-data-a", x1600_uart2_data_a, 2),
>>> + INGENIC_PIN_GROUP("uart2-data-b", x1600_uart2_data_b, 1),
>>> + INGENIC_PIN_GROUP("uart3-data-b", x1600_uart3_data_b, 0),
>>> + INGENIC_PIN_GROUP("uart3-data-d", x1600_uart3_data_d, 2),

...

>>> + INGENIC_PIN_GROUP("pwm0", x1600_pwm_pwm0, 0),
>>> + INGENIC_PIN_GROUP("pwm1", x1600_pwm_pwm1, 0),
>>> + INGENIC_PIN_GROUP("pwm2", x1600_pwm_pwm2, 0),
>>> + INGENIC_PIN_GROUP("pwm3", x1600_pwm_pwm3, 1),
>>> + INGENIC_PIN_GROUP("pwm4", x1600_pwm_pwm4, 1),
>>> + INGENIC_PIN_GROUP_FUNCS("pwm5", x1600_pwm_pwm5,
>>> x1600_pwm_pwm5_funcs),
>>> + INGENIC_PIN_GROUP_FUNCS("pwm6", x1600_pwm_pwm6,
>>> x1600_pwm_pwm6_funcs),
>>> + INGENIC_PIN_GROUP_FUNCS("pwm7", x1600_pwm_pwm7,
>>> x1600_pwm_pwm7_funcs),
>>> + INGENIC_PIN_GROUP("mac", x1600_mac, 1),
>>> +};
>>> +

If Paul Boddie agrees, I will add it to the V2.

BR and thanks,
Nikolaus