Re: [PATCH v2] gpio: vortex: add new GPIO device driver

[Marcos Del Sol Vives <marcos@orca.pet>]

El 11/07/2025 a las 16:53, Andy Shevchenko escribió:
>> 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.
> 
> $ git grep -lw '\.io_port[[:space:]]\+= true,'
> drivers/counter/104-quad-8.c
> drivers/gpio/gpio-104-dio-48e.c
> drivers/gpio/gpio-104-idi-48.c
> drivers/gpio/gpio-104-idio-16.c
> drivers/gpio/gpio-exar.c
> drivers/gpio/gpio-gpio-mm.c
> drivers/gpio/gpio-pci-idio-16.c
> drivers/gpio/gpio-pcie-idio-24.c
> drivers/gpio/gpio-ws16c48.c
> drivers/iio/addac/stx104.c
> drivers/iio/dac/cio-dac.c
> 
> Take a look.

I've already made a third version of the patch, using gpio-regmap this time.
This time I'm also using a Southbridge driver that pulls it as a platform
device, much like the rdc321x-southbridge.c does. It's not yet ready for
merging, but it's available for now at
https://github.com/socram8888/linux/tree/vortex-gpio

I have found a small issue though regarding gpio-regmap, and before making
a third version of the patch, I'd prefer to know the way to approach it.

The Vortex86 SoCs require the direction of the GPIO pin to be set before
writing the pin's value. Otherwise, writes to the data ports are ignored.

Currently gpio-regmap does it in the opposite order:

> static int gpio_regmap_direction_output(struct gpio_chip *chip,
> 					unsigned int offset, int value)
> {
>	gpio_regmap_set(chip, offset, value);
>
> 	return gpio_regmap_set_direction(chip, offset, true);
> }

(I have also noticed that it does not properly check the return value of
gpio_regmap_set, but that's another thing)

So there are IMO three different approaches:

1. Add a boolean flag that allows changing the behaviour. If set, invert
the order of operations. Else do as before.
2. Same, but with a "flags" bitfield, in case more flags need to be added
in the future.
3. Do an additional "gpio_regmap_set" after setting the direction. This
means no new fields need to be added to the structures but causes an extra
write that may not be needed on other drivers.

>> IRQ is only available for the first two ports out of the five available.
> Would  it be a problem to support them?

I cannot test that on my platform: as mentioned before, only ports 0 and 1
have IRQs, and in my mini PC I only have two pins available, and they're
both on port 4. 

Any code I'd write would be completely untested and IMHO sounds like a
terrible idea to have such code merged.

Thanks,
Marcos