Re: [PATCH 3/3] pinctrl: ingenic: Add X2600 pin groups and functions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] pinctrl: ingenic: x2600 pin groups omitted from
  x2600_functions table
- [Medium] pinctrl: ingenic: truncated pull bitmasks in x2600_pull_ups
  and pull_downs
- [Low] pinctrl: ingenic: empty x2600_lcd_none_pins breaks
  ingenic_pinconf_group_get()
--

commit faeb190e107ce4d769c2094c52cf1675b346a6a9
Author: H. Nikolaus Schaller <hns@goldelico.com>

pinctrl: ingenic: Add X2600 pin groups and functions

This commit adds pin groups, pinmux functions, pull-up/pull-down masks, and
register access ranges for the Lumissil X2600 SoC to the Ingenic pinctrl
driver.

> diff --git a/drivers/pinctrl/pinctrl-ingenic.c b/drivers/pinctrl/pinctrl-ingenic.c
> index 6794c662d2afe..557cd652ecc00 100644
> --- a/drivers/pinctrl/pinctrl-ingenic.c
> +++ b/drivers/pinctrl/pinctrl-ingenic.c
> @@ -3515,6 +3515,503 @@ static const struct ingenic_chip_info x2100_chip_info = {
>  	.access_table = &x2000_access_table,
>  };
>  
> +static const u32 x2600_pull_ups[5] = {
> +	0x000007ff, 0xffffffff, 0x7ffbfff, 0x0000ffff, 0x000ffffe,
> +};
> +
> +static const u32 x2600_pull_downs[5] = {
> +	0x000007ff, 0xffffffff, 0x01ffff1, 0x0001e000, 0x000fffff,
> +};
> +
> +static int x2600_can0_pins[] = { 0x47, 0x48, };
> +static int x2600_can1_pins[] = { 0x49, 0x4a, };
> +
> +static int x2600_cim_pins[] = {
> +	0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07,
> +	0x08, 0x09, 0x0a, 0x0b,
> +};

[Severity: Medium]
Are the Port C and Port A masks in x2600_pull_ups and x2600_pull_downs
truncated?

The Port C entries (0x7ffbfff and 0x01ffff1) have only 7 hex digits instead
of 8, leaving bits 27..30 clear so ingenic_pinconf_set() rejects pull
configuration with -EINVAL on PC27..PC30 (0x5b..0x5e, used by
x2600_msc1_1bit_c_pins, x2600_msc1_4bit_c_pins, and the ssi1-*-c25 groups).
Similarly, the Port A mask 0x000007ff only covers 11 pins (0x00..0x0a),
which causes group pull configuration on cim-data to fail with -EINVAL on
PA11 (0x0b).

[ ... ]

> +static int x2600_lcd_sync_pins[] = { 0x3c, 0x3d, 0x3e, 0x3f, };
> +
> +static int x2600_lcd_none_pins[] = { };

[Severity: Low]
Does defining x2600_lcd_none_pins as an empty array break
ingenic_pinconf_group_get() for the "lcd-none" group?

When npins is 0, the loop in ingenic_pinconf_group_get() does not execute
and the function returns 0 without updating *config:

drivers/pinctrl/pinctrl-ingenic.c:ingenic_pinconf_group_get() {
	...
	for (i = 0; i < npins; i++) {
		if (ingenic_pinconf_get(pctldev, pins[i], config))
			return -ENOTSUPP;
	...
	return 0;
}

Reading debugfs pinconf-groups then reports every generic pin_config_param
as active on "lcd-none", and selecting "lcd-none" in
ingenic_pinmux_set_mux() is a no-op that configures no pins.

[ ... ]

> +	/* I2S (Audio) & PCM */
> +	INGENIC_PIN_GROUP("i2s-clk-rx", x2600_i2s_clk_rx, 1),
> +	INGENIC_PIN_GROUP("i2s-sysclk", x2600_i2s_sysclk, 1),
> +	INGENIC_PIN_GROUP("i2s-clk-tx", x2600_i2s_clk_tx, 1),
> +	INGENIC_PIN_GROUP("i2s-data", x2600_i2s_data, 1),
> +	INGENIC_PIN_GROUP("pcm", x2600_pcm, 3),

[ ... ]

> +	INGENIC_PIN_GROUP("pwm10-c", x2600_pwm10_c, 0),
> +	INGENIC_PIN_GROUP("pwm10-e", x2600_pwm10_e, 2),
> +	INGENIC_PIN_GROUP("pwm11", x2600_pwm11, 0),
> +	INGENIC_PIN_GROUP("pwm12", x2600_pwm12, 0),
> +	INGENIC_PIN_GROUP("pwm13", x2600_pwm13, 0),
> +	INGENIC_PIN_GROUP("pwm14", x2600_pwm14, 0),
> +	INGENIC_PIN_GROUP("pwm15", x2600_pwm15, 0),
> +	INGENIC_PIN_GROUP("pwm-trig-c", x2600_pwm_trig_c, 1),
> +	INGENIC_PIN_GROUP("pwm-trig-d", x2600_pwm_trig_d, 1),

[ ... ]

> +	/* WAIT */
> +	INGENIC_PIN_GROUP("wait", x2600_wait, 3),
> +};

[ ... ]

> +static const struct pinfunction x2600_functions[] = {
> +	INGENIC_PIN_FUNCTION("can0", x2600_can0),
> +	INGENIC_PIN_FUNCTION("can1", x2600_can1),
> +	INGENIC_PIN_FUNCTION("cim", x2600_cim),
> +	INGENIC_PIN_FUNCTION("exclk", x2600_clk),
> +	INGENIC_PIN_FUNCTION("dmic", x2600_dmic),
> +	INGENIC_PIN_FUNCTION("i2c0", x2600_i2c0),
> +	INGENIC_PIN_FUNCTION("i2c1", x2600_i2c1),
> +	INGENIC_PIN_FUNCTION("i2c2", x2600_i2c2),
> +	INGENIC_PIN_FUNCTION("i2c3", x2600_i2c3),
> +	INGENIC_PIN_FUNCTION("i2s", x2600_i2s),
> +	INGENIC_PIN_FUNCTION("lcd", x2600_lcd),
> +	INGENIC_PIN_FUNCTION("mac", x2600_mac),
> +	INGENIC_PIN_FUNCTION("mmc0", x2600_msc0),
> +	INGENIC_PIN_FUNCTION("mmc1", x2600_msc1),
> +	INGENIC_PIN_FUNCTION("otg", x2600_otg),
> +	INGENIC_PIN_FUNCTION("pwm0", x2600_pwm0),
> +	INGENIC_PIN_FUNCTION("pwm1", x2600_pwm1),
> +	INGENIC_PIN_FUNCTION("pwm2", x2600_pwm2),
> +	INGENIC_PIN_FUNCTION("pwm3", x2600_pwm3),
> +	INGENIC_PIN_FUNCTION("pwm4", x2600_pwm4),
> +	INGENIC_PIN_FUNCTION("pwm5", x2600_pwm5),
> +	INGENIC_PIN_FUNCTION("pwm6", x2600_pwm6),
> +	INGENIC_PIN_FUNCTION("pwm7", x2600_pwm7),
> +	INGENIC_PIN_FUNCTION("pwm8", x2600_pwm8),
> +	INGENIC_PIN_FUNCTION("pwm9", x2600_pwm9),
> +	INGENIC_PIN_FUNCTION("pwm10", x2600_pwm10),
> +	INGENIC_PIN_FUNCTION("sfc", x2600_sfc),
> +	INGENIC_PIN_FUNCTION("slcd", x2600_slcd),
> +	INGENIC_PIN_FUNCTION("ssi0", x2600_ssi0),
> +	INGENIC_PIN_FUNCTION("ssi1", x2600_ssi1),
> +	INGENIC_PIN_FUNCTION("ssi-slv", x2600_ssi_slv),
> +	INGENIC_PIN_FUNCTION("uart0", x2600_uart0),
> +	INGENIC_PIN_FUNCTION("uart1", x2600_uart1),
> +	INGENIC_PIN_FUNCTION("uart2", x2600_uart2),
> +	INGENIC_PIN_FUNCTION("uart3", x2600_uart3),
> +	INGENIC_PIN_FUNCTION("uart4", x2600_uart4),
> +	INGENIC_PIN_FUNCTION("uart5", x2600_uart5),
> +	INGENIC_PIN_FUNCTION("uart6", x2600_uart6),
> +	INGENIC_PIN_FUNCTION("uart7", x2600_uart7),
> +	INGENIC_PIN_FUNCTION("tcu", x2600_tcu),
> +};

[Severity: Medium]
Were the "pcm", "pwm11" through "pwm15", "pwm-trig-c", "pwm-trig-d", and
"wait" pin groups left out of x2600_functions?

These nine groups are defined in x2600_groups, but they have no
corresponding group arrays or INGENIC_PIN_FUNCTION() entries in
x2600_functions, so pinmux_map_to_setting() will reject any pinctrl mapping
trying to mux them with -EINVAL.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1794557697.git.hns@goldelico.com?part=3