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

["H. Nikolaus Schaller" <hns@xxxxxxxxxxxxx>]

Hi all,

> Am 26.02.2025 um 20:42 schrieb H. Nikolaus Schaller <hns@xxxxxxxxxxxxx>:
> 
> Hi Paul,
> 
>> Am 26.02.2025 um 19:43 schrieb Paul Cercueil <paul@xxxxxxxxxxxxxxx>:
>> 
>> Hi Nikolaus, and everyone involved,
>> 
>> Le mercredi 26 février 2025 à 18:16 +0100, H. Nikolaus Schaller a
>> écrit :
>>> From: Paul Boddie <paul@xxxxxxxxxxxxx>
>>> 
>>> 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@xxxxxxxxxxxx>
>>> Co-authored-by: H. Nikolaus Schaller <hns@xxxxxxxxxxxxx>
>>> Signed-off-by: Paul Boddie <paul@xxxxxxxxxxxxx>
>>> Signed-off-by: Andreas Kemnade <andreas@xxxxxxxxxxxx>
>>> Signed-off-by: H. Nikolaus Schaller <hns@xxxxxxxxxxxxx>
>>> ---
>>> 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