Re: [PATCH v6 12/12] pinctrl: Add pin controller driver for AAEON UP boards

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Tue, May 20, 2025 at 03:28:36PM +0200, Thomas Richard wrote:
> This enables the pin control support of the onboard FPGA on AAEON UP
> boards.
> 
> This FPGA acts as a level shifter between the Intel SoC pins and the pin
> header, and also as a mux or switch.
> 
> +---------+          +--------------+             +---+
>           |          |              |             |   |
>           | PWM0     |       \      |             | H |
>           |----------|------  \-----|-------------| E |
>           | I2C0_SDA |              |             | A |
> Intel SoC |----------|------\       |             | D |
>           | GPIO0    |       \------|-------------| E |
>           |----------|------        |             | R |
>           |          |     FPGA     |             |   |
> ----------+          +--------------+             +---+
> 
> For most of the pins, the FPGA opens/closes a switch to enable/disable
> the access to the SoC pin from a pin header.
> Each switch, has a direction flag that is set depending the status of the
> SoC pin.
> 
> For some other pins, the FPGA acts as a mux, and routes one pin (or the
> other one) to the header.
> 
> The driver also provides a GPIO chip. It requests SoC pins in GPIO mode,
> and drives them in tandem with FPGA pins (switch/mux direction).
> 
> This commit adds support only for UP Squared board.

...

> +static int upboard_pinctrl_pin_get_mode(struct pinctrl_dev *pctldev, unsigned int pin)
> +{
> +	struct upboard_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> +	struct upboard_pin *p = &pctrl->pins[pin];
> +	unsigned int val;
> +	int ret;
> +
> +	if (p->funcbit) {
> +		ret = regmap_field_read(p->funcbit, &val);
> +		if (ret)
> +			return ret;
> +		if (val)
> +			return UPBOARD_PIN_MODE_FUNCTION;
> +	}
> +
> +	ret = regmap_field_read(p->enbit, &val);
> +	if (ret)
> +		return ret;
> +	if (!val)
> +		return UPBOARD_PIN_MODE_DISABLED;
> +
> +	ret = regmap_field_read(p->dirbit, &val);
> +	if (ret)
> +		return ret;
> +
> +	return val ? UPBOARD_PIN_MODE_GPIO_IN : UPBOARD_PIN_MODE_GPIO_OUT;
> +}
> +
> +static void upboard_pinctrl_dbg_show(struct pinctrl_dev *pctldev, struct seq_file *s,
> +				     unsigned int offset)
> +{
> +	int ret;
> +
> +	ret = upboard_pinctrl_pin_get_mode(pctldev, offset);
> +	if (ret == UPBOARD_PIN_MODE_FUNCTION)
> +		seq_puts(s, "mode function ");
> +	else if (ret == UPBOARD_PIN_MODE_DISABLED)
> +		seq_puts(s, "HIGH-Z ");

> +	else
> +		seq_printf(s, "GPIO (%s) ", str_input_output(ret == UPBOARD_PIN_MODE_GPIO_IN));

Actually this should be

	else if (ret < 0)
		seq_printf(s, "N/A "); // or similar text
	else
		seq_printf(s, "GPIO (%s) ", str_input_output(ret == UPBOARD_PIN_MODE_GPIO_IN));

as the above may return a negative error code which is not listed here.

> +}

With the above being addressed,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

...

Thanks for doing this driver, eventually we will have it working
out-of-the-box. Do you have any plans for enabling HSI and SIM card
slot on UP Squared 7000?

-- 
With Best Regards,
Andy Shevchenko