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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] `ASPEED_G7_SOC1_MUX_FUNC_MASK` only masks 3 bits for a 4-bit multi-function field.
- [Medium] Semantic mismatch: explicit bias pull direction requests (`PIN_CONFIG_BIAS_PULL_DOWN` / `PIN_CONFIG_BIAS_PULL_UP`) are blindly mapped to a generic enable.
- [Low] Unnecessary code duplication of `pinctrl_generic_get_group_pins()`.
--

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

pinctrl: aspeed: Add AST2700 SoC1 support

This commit adds support for the AST2700 SoC1 pin controller. It implements
pin multiplexing and configuration using static SoC data tables via the
generic pinctrl, pinmux, and pinconf frameworks.

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

[ ... ]

> +#define ASPEED_G7_SOC1_MUX_BASE          0x400
> +#define ASPEED_G7_SOC1_BIAS_BASE         0x480
> +#define ASPEED_G7_SOC1_DRV_BASE          0x4C0
> +#define ASPEED_G7_SOC1_PCIE_REG          0x908
> +#define ASPEED_G7_SOC1_USB_MODE_REG      0x3B0
> +#define ASPEED_G7_SOC1_SGMII_REG         0x47C
> +
> +#define ASPEED_G7_SOC1_MUX_FUNC_MASK 0x7
> +#define ASPEED_G7_SOC1_MUX_BITS_PER_PIN 4

Since ASPEED_G7_SOC1_MUX_BITS_PER_PIN is 4, should ASPEED_G7_SOC1_MUX_FUNC_MASK
be 0xF instead of 0x7? If the most significant bit (bit 3) is set by the
bootloader and is part of the mux configuration, the driver might fail to clear
it when updating the register via regmap_update_bits().

[ ... ]

> +static int aspeed_g7_soc1_pin_config_set(struct pinctrl_dev *pctldev,
> +					 unsigned int pin,
> +					 unsigned long *configs,
> +					 unsigned int num_configs)
> +{
> +	struct aspeed_g7_soc1_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
> +	struct aspeed_g7_field field;
> +	enum pin_config_param param;
> +	int i, ret, ds_idx;
> +	unsigned int val;
> +	u32 arg;
> +
> +	if (pin > AC24)
> +		return -EINVAL;
> +
> +	for (i = 0; i < num_configs; i++) {
> +		param = pinconf_to_config_param(configs[i]);
> +		arg = pinconf_to_config_argument(configs[i]);
> +
> +		switch (param) {
> +		case PIN_CONFIG_BIAS_PULL_DOWN:
> +		case PIN_CONFIG_BIAS_PULL_UP:
> +		case PIN_CONFIG_BIAS_DISABLE:
> +			field = aspeed_g7_soc1_bias_field_from_pin(pin);
> +			val = (param == PIN_CONFIG_BIAS_DISABLE) ? 1 : 0;
> +			break;

Is it safe to blindly map PIN_CONFIG_BIAS_PULL_DOWN and PIN_CONFIG_BIAS_PULL_UP
to a generic enable (val = 0)? As noted in aspeed_g7_soc1_pin_config_get(), the
hardware pull direction is fixed in silicon. If a device tree requests a
pull-down on a pin with a fixed pull-up, it will silently receive inverted
hardware logic. Should the driver reject explicit direction requests it cannot
guarantee and instead support PIN_CONFIG_BIAS_PULL_PIN_DEFAULT?

[ ... ]

> +static int aspeed_g7_soc1_get_group_pins(struct pinctrl_dev *pctldev,
> +					 unsigned int selector,
> +					 const unsigned int **pins,
> +					 unsigned int *npins)
> +{
> +	struct group_desc *group;
> +
> +	group = pinctrl_generic_get_group(pctldev, selector);
> +	if (!group)
> +		return -EINVAL;
> +
> +	if (!group->grp.npins)
> +		return -ENODEV;
> +
> +	*pins = group->grp.pins;
> +	*npins = group->grp.npins;
> +
> +	return 0;
> +}

Is there a reason to implement this local helper instead of directly using the
exported pinctrl_generic_get_group_pins()? The local function seems to
identically mimic the core function's behavior.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519-pinctrl-single-bit-v4-0-5fe568a8ffde@aspeedtech.com?part=3