Message-ID: <aHD40TD8MLug0C6b@black.fi.intel.com>
Date: Fri, 11 Jul 2025 14:43:13 +0300
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Marcos Del Sol Vives <marcos@...a.pet>,
	William Breathitt Gray <wbg@...nel.org>
Cc: linux-kernel@...r.kernel.org, Linus Walleij <linus.walleij@...aro.org>,
	Bartosz Golaszewski <brgl@...ev.pl>, linux-gpio@...r.kernel.org
Subject: Re: [PATCH v2] gpio: vortex: add new GPIO device driver

+Cc: William,
who is an expert in embedded industrial x86 platforms and might help with this.

Bart, thanks for Cc'ing me. I have tons of questions and comments regarding this.

On Wed, Jul 09, 2025 at 11:15:40AM +0200, Marcos Del Sol Vives wrote:
> Add a new simple GPIO device driver for Vortex86 lines of SoCs,
> implemented according to their programming reference manual [1].
> 
> This is required for detecting the status of the poweroff button and
> performing the poweroff sequence on ICOP eBox computers.
> 
> IRQs are not implemented as they are available for less than half the
> GPIO pins, and they are not the ones required for the poweroff stuff, so
> polling will be required anyway.

> [1]: http://www.dmp.com.tw/tech/DMP_Vortex86_Series_Software_Programming_Reference_091216.pdf
> 

Make this a Link tag by removing a blank line and converting above to

Link: http://www.dmp.com.tw/tech/DMP_Vortex86_Series_Software_Programming_Reference_091216.pdf [1]

> Signed-off-by: Marcos Del Sol Vives <marcos@...a.pet>

But the first question first. Is this not a dead code? I mean I have heard that
i486SX is not supported in Linux kernel as there is no more FP emu layer. Of
course it might be not a problem in some cases, but still. Second point is that
there is an ongoing (still ongoing?) discussion on removal i486 support completely.
Would it affect these SoCs? (According to the shared manual seems the answer is
"yes, it will".

...

> +	help
> +	  Driver to access the five 8-bit bidirectional GPIO ports present on
> +	  all DM&P Vortex86 SoCs.

...

> + *  Based on the it87xx GPIO driver by Diego Elio Pettenò

Why that driver can't be reused?

> + */

...

> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

Why? The driver should use dev_*() macros which will uniquely define the device
for what the message is printed.

...

> +#include <linux/types.h>
> +#include <linux/errno.h>
> +#include <linux/module.h>
> +#include <linux/ioport.h>
> +#include <linux/spinlock.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/platform_device.h>

This should be sorted alphabetically. Also follow IWYU principle.

...

> +static DEFINE_SPINLOCK(gpio_lock);

Global lock? Why?

...

> +static int vortex_gpio_get(struct gpio_chip *chip, unsigned int gpio_num)
> +{
> +	uint8_t port = gpio_num / GPIO_PER_PORT;
> +	uint8_t bit  = gpio_num % GPIO_PER_PORT;
> +	uint8_t val;

Use kernel types for kernel. Include types.h (you probably missed it anyway)
and then convert above to u8.

> +	val = inb(GPIO_DATA_BASE + port);

Better to use ioread8() / iowrite8() and "map" IO ports (see, for example,
how drivers/pinctrl/intel-pinctrl-lynxpoint.c does).

> +	return !!(val & (1 << bit));

BIT() from bits.h will help a lot.

> +}
> +
> +static int vortex_gpio_direction_in(struct gpio_chip *chip, unsigned int gpio_num)
> +{
> +	uint8_t port = gpio_num / GPIO_PER_PORT;
> +	uint8_t bit  = gpio_num % GPIO_PER_PORT;
> +	unsigned long flags;
> +	uint8_t dir;

All the same comments, plus...

> +	spin_lock_irqsave(&gpio_lock, flags);

...use APIs from cleanup.h, i.e. guard() in this case.

> +	dir = inb(GPIO_DIRECTION_BASE + port);
> +	dir &= ~(1 << bit); /* 0 = input */
> +	outb(dir, GPIO_DIRECTION_BASE + port);
> +
> +	spin_unlock_irqrestore(&gpio_lock, flags);
> +
> +	return 0;
> +}

...

So, the above is pretty much simple, why doesn't it use gpio-regmap with the
respective configuration? Moreover, SX and DX variants are differ since the
latter one may provide an IRQ chip, for which gpio-regmap also can be used,
i.o.w. with that done, it will be quite easy to support both.

...

> +static int vortex_gpio_probe(struct platform_device *pdev)
> +{
> +	/* Set up GPIO labels */
> +	for (int i = 0; i < GPIO_COUNT; i++) {
> +		sprintf(labels[i], "vortex_gp%u%u", i / 8, i % 8);
> +		labels_table[i] = &labels[i][0];

'&...[0]' is redundant.

> +	}

Why this can't be made static once?

> +	return devm_gpiochip_add_data(&pdev->dev, &gpio_chip, NULL);
> +}
> +
> +static struct platform_driver vortex_gpio_driver = {
> +	.driver = {
> +		.name = KBUILD_MODNAME,

> +		.owner = THIS_MODULE,

This field is not needed for ages (15+ years). Is this driver got dusted for this long?

> +	},
> +	.probe = vortex_gpio_probe,
> +};

...

> +static struct resource vortex_gpio_resources[] = {
> +	DEFINE_RES_IO_NAMED(GPIO_DATA_BASE, GPIO_PORTS, KBUILD_MODNAME " data"),
> +	DEFINE_RES_IO_NAMED(GPIO_DIRECTION_BASE, GPIO_PORTS, KBUILD_MODNAME " dir"),

Named resources? Why?

> +};

...

> +static int __init vortex_gpio_init(void)
> +{
> +	if (boot_cpu_data.x86_vendor != X86_VENDOR_VORTEX) {
> +		pr_err("Not a Vortex86 CPU, refusing to load\n");
> +		return -ENODEV;
> +	}
> +
> +	pdev = platform_create_bundle(&vortex_gpio_driver, vortex_gpio_probe,
> +			vortex_gpio_resources, ARRAY_SIZE(vortex_gpio_resources),
> +			NULL, 0);
> +	return PTR_ERR_OR_ZERO(pdev);
> +}

Oh my... Can you elaborate more on this ugly hack. Why do we need this at all?
What's wrong with the BIOS or other firmware that is provided?
(The documentation mentions BIOS, btw.)

Also, is this anyhow visible as a PCI device? Is it part of LPC (docs suggests
so for SX, but not so clear in DX diagram)?

...

On top of that the GPIO3 is marked as one with the pin muxing. Where is the driver
for it? Or what are the plans about it?

GPIO4 seems muxed with UART, so also subject to pin muxing.

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists