Re: [PATCH v3 3/3] pinctrl: aspeed: Add AST2700 SoC1 support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Calling `devm_pinctrl_register()` before adding groups and functions breaks device tree pin hogs and exposes an initialization race.
- [High] The SGMII0 pin register offset and shift calculations are corrupted by preceding virtual pins in the enum.
- [High] `ASPEED_G7_SOC1_MUX_FUNC_MASK` is defined as 0x7 instead of 0xF, leaving the highest bit of the 4-bit mux field uncleared.
- [Medium] `aspeed_g7_soc1_pin_config_get()` violates the generic pinconf API by failing to return `-EINVAL` for inactive boolean parameters.
--

commit d166d24632d04188131a95c43f87c2fa9c723d2e
Author: Billy Tsai <billy_tsai@aspeedtech.com>

pinctrl: aspeed: Add AST2700 SoC1 support

This patch implements pin multiplexing and configuration for the AST2700
SoC1 SCU pinctrl block. It describes the packed-field register layout directly
in driver data and accesses the controller registers via regmap.

> diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed-g7-soc1.c b/drivers/pinctrl/aspeed/pinctrl-aspeed-g7-soc1.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed-g7-soc1.c

[ ... ]

> +#define ASPEED_G7_SOC1_MUX_FUNC_MASK 0x7
> +#define ASPEED_G7_SOC1_MUX_BITS_PER_PIN 4

Should this mask be 0xF to cover all 4 bits?

Since ASPEED_G7_SOC1_MUX_BITS_PER_PIN is 4, using 0x7 will only clear the
lower 3 bits when regmap_update_bits() is called. If the bootloader leaves
the 4th bit set, will it remain set and corrupt the pin multiplexing behavior?

[ ... ]

> +	AC25,
> +	AB25,
> +	AC24,
> +	PCIERC2_PERST,
> +	PORTC_MODE,
> +	PORTD_MODE,
> +	SGMII0,
> +};

Since SGMII0 is placed after the virtual pins in this enum, its index becomes
219 instead of 216. 

When aspeed_g7_soc1_pinmux_field_from_pin() calculates the shift using
(pin % 8) * 4, will this inflated offset skip 3 pin slots (12 bits) and
configure the wrong hardware bits for SGMII0? Would it be safer to move the
virtual pins to the end of the enum?

[ ... ]

> +static int aspeed_g7_soc1_pin_config_get(struct pinctrl_dev *pctldev,
> +					 unsigned int pin,
> +					 unsigned long *config)
> +{

[ ... ]

> +	val = (val_raw & (field.mask << field.shift)) >> field.shift;
> +	if (param == PIN_CONFIG_DRIVE_STRENGTH)
> +		val = (val + ASPEED_G7_SOC1_DRV_STRENGTH_HW_BASE) *
> +		      ASPEED_G7_SOC1_DRV_STRENGTH_STEP_MA;
> +	else if (param != PIN_CONFIG_BIAS_DISABLE)
> +		val = !val;
> +
> +	*config = pinconf_to_config_packed(param, val);
> +
> +	return 0;
> +}

Does this unconditionally return 0 (success) even if the queried boolean
parameter is not currently active?

The generic pinconf subsystem expects -EINVAL if a parameter like
PIN_CONFIG_BIAS_PULL_UP or PIN_CONFIG_BIAS_DISABLE is queried but not active
on the pin. Will returning 0 cause tools like debugfs to report mutually
exclusive states as active simultaneously?

[ ... ]

> +static int aspeed_g7_soc1_pinctrl_probe(struct platform_device *pdev)
> +{

[ ... ]

> +	pctl->pctl = devm_pinctrl_register(dev, &aspeed_g7_soc1_desc, pctl);
> +	if (IS_ERR(pctl->pctl)) {
> +		dev_err(dev, "Failed to register pinctrl\n");
> +		return PTR_ERR(pctl->pctl);
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(aspeed_g7_soc1_groups); i++) {
> +		const struct pingroup *grp = &aspeed_g7_soc1_groups[i];
> +
> +		ret = pinctrl_generic_add_group(pctl->pctl, grp->name, (unsigned int *)grp->pins,
> +						grp->npins, pctl);

Is it safe to call devm_pinctrl_register() before the groups and functions
are added to the radix trees?

devm_pinctrl_register() immediately exposes the device and parses device tree
pin hogs via pinctrl_enable(). Will any DT hogs defined for this controller
fail to apply because the internal radix trees are still empty? 

Should this use devm_pinctrl_register_and_init() instead, followed by an
explicit pinctrl_enable() after populating the groups and functions?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515-pinctrl-single-bit-v3-0-e97da4312104@aspeedtech.com?part=3