Message-ID: <99b67e0f-783a-4ac0-971f-07cf1544a651@orca.pet>
Date: Fri, 11 Jul 2025 15:52:39 +0200
From: Marcos Del Sol Vives <marcos@...a.pet>
To: Andy Shevchenko <andriy.shevchenko@...el.com>,
 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

El 11/07/2025 a las 13:43, Andy Shevchenko escribió:
>> + *  Based on the it87xx GPIO driver by Diego Elio Pettenò
> 
> Why that driver can't be reused?
> 

The driver uses a completely different port address and operation. It is
based in the sense I used it as an example of how a I/O mapped GPIO driver
should work, but nothing else.

>> +#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.

The error thrown if the module is attempted to load on a non-Vortex
processor happens before the platform device is created.

>> +static DEFINE_SPINLOCK(gpio_lock);
> 
> Global lock? Why?

Becase at most there'll be one GPIO device of this kind loaded, so it didn't
make sense to me to create a dynamically-allocated private structure of data
for a single global lock.

> 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.

Again, I am not an expert on the Linux kernel, but I did not see any code
or examples using neither gpio-mmio nor gpio-regmap for I/O-mapped registers.

IRQ is only available for the first two ports out of the five available.
As said in the comment, for shutting down the machine, port 3 is required
so I'm gonna need to poll anyway.

>> +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?

It absolutely can.

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

I saw the field on the documentation as well as on the IT87 driver I was
using as a reference, so I kept it.

>> +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?

So they appear with the proper name in /proc/iomem. That's the only reason:

0000-0cf7 : PCI Bus 0000:00
  0000-001f : dma1
  0020-0021 : pic1
  0040-0043 : timer0
  0050-0053 : timer1
  0060-0060 : keyboard
  0061-0061 : PNP0800:00
  0064-0064 : keyboard
  0070-0071 : rtc0
  0078-007c : gpio_vortex-data
  0080-008f : dma page reg
  0098-009c : gpio_vortex-dir
  00a0-00a1 : pic2
  00c0-00df : dma2
  00f0-00ff : PNP0C04:00
...

>> +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)?

The device is available at a hardcoded address for all Vortex86 devices, but
it is not part of any device in particular, and I don't see any good way
to enable it on all Vortex devices other than checking the CPU vendor.

These are the PCI devices on my DX3:

00:00.0 Host bridge [0600]: RDC Semiconductor, Inc. R6023 Host Bridge [17f3:6023] (rev 02)
00:01.0 PCI bridge [0604]: RDC Semiconductor, Inc. PCI/PCI-X to PCI-E Bridge [17f3:1031] (rev 01)
00:02.0 PCI bridge [0604]: RDC Semiconductor, Inc. PCI/PCI-X to PCI-E Bridge [17f3:1031] (rev 01)
00:07.0 ISA bridge [0601]: RDC Semiconductor, Inc. R6035 ISA Bridge [17f3:6035] (rev 01)
00:07.1 ISA bridge [0601]: RDC Semiconductor, Inc. R6035 ISA Bridge [17f3:6035] (rev 01)
00:0a.0 USB controller [0c03]: RDC Semiconductor, Inc. R6060 USB 1.1 Controller [17f3:6060] (rev 14)
00:0a.1 USB controller [0c03]: RDC Semiconductor, Inc. R6061 USB 2.0 Controller [17f3:6061] (rev 08)
00:0c.0 IDE interface [0101]: RDC Semiconductor, Inc. R1012 IDE Controller [17f3:1012] (rev 02)
00:0d.0 VGA compatible controller [0300]: RDC Semiconductor, Inc. RDC M2015 VGA-compatible graphics adapter [17f3:2015]
00:0e.0 Audio device [0403]: RDC Semiconductor, Inc. R3010 HD Audio Controller [17f3:3010] (rev 01)
01:00.0 Ethernet controller [0200]: Realtek Semiconductor Co., Ltd. RTL8111/8168/8411 PCI Express Gigabit Ethernet Controller [10ec:8168] (rev 07)

There's no multipurpose device that actually claims ownership of these I/O
ports according to the PCI's configuration section.

In fact the Vortex86MX-based EBOX-3350MX mini PC has a different host bridge
(R6021) and different ISA bridges (R6031), but has the very same GPIO.

And in "dmidecode" I just see a lot of lies (this device certainly does NOT
have fans or a parallel port) plus "To Be Filled By O.E.M." fields. Also
matching on BIOS does not seem a good idea since there are other industrial
machines that may not be using or reporting the same BIOS versions.

> 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.

The documentation does not cover how to use those UARTs and the pins default
to I/O, so I would say they're not a problem for now.

Ultimately, as mentioned, the goal is implementing a correct power off
sequence for ICOP EBOX machines and DM&P evaluation boards, which require
manually polling for the power off button, then setting an output pin
low to shut off power to the machine.

Powered by blists - more mailing lists