gpio: make gpioc a child of gpiobus

[vexeduxr]

Summary

With gpioc being a direct child of the GPIO controller, it can't
allocate interrupts properly. It currently allocates interrupts using
it's parent dev (gpioX). This causes problems since the call never goes
through gpiobus. Instead, make gpioc a child of gpiobus and allocate
interrupts using our own dev. Also don't misuse pin->flags, it's not
meant to store the flags from sys/gpio.h

Reported by: Evgenii Ivanov <[e-mail]>

Test Plan

Tested with D26407

[mmel]

I like this step. But please give me one more day to re-analyze it.

sys/dev/gpio/gpiobus.c

391

Are you sure? That doesn't look right.

[vexeduxr]

sys/dev/gpio/gpiobus.c

391

We need gpioc to have access to all pins exposed by the controller, so pin 0 in the ivars points to pin 0 on the controller, pin 1 in the ivars points to pin 1 on the controller, etc. The GPIOBUS_PIN_* functions work through the ivars.

[vexeduxr]

Rebase.
Also remove NULL check for gpio_pin_release, it's not needed.

[vexeduxr]

sys/dev/gpio/gpioc.c

268–271

In D51932#1187749, @vexeduxr wrote:

Also remove NULL check for gpio_pin_release, it's not needed.

I feel like this warrants an explanation. So gpioc_release_pin_intr can only get called after a successful call from gpioc_allocate_pin_intr. If gpioc_allocate_pin_intr fails, this should never get called. So why the checks? Well gpioc_release_pin_intr can get called when re-configuring a pin's flags. If it fails, the pin doesn't get re-configured. If, for example, bus_teardown_intr were to succeed and bus_release_resource were to fail, then the user attempted to configure the pin again, bus_teardown_intr would have been called twice.

Since gpio_pin_release is the last (and it can't fail), there's no need to check if the pin was previously freed.

[mmel]

sys/dev/gpio/gpiobus.c

391

But why do you assume that the driver uses sequential pin numbering, starting from 1? The numbering depends on an external source and the gpio driver must respect it.
Some older FDTs for SoCs with multiple GPIOs used a scheme where the pins were numbered sequentially, without any relation to given GPIO.
(In fact, the number was the index of the pinctrl driver).

[vexeduxr]

sys/dev/gpio/gpiobus.c

391

Oh, I didn't know that.

[vexeduxr]

sys/dev/gpio/gpiobus.c

391

Hmm, does this mean that we can't even rely on GPIO_PIN_MAX to figure out how many pins we have?
It would make more sense to me if said drivers did some translation on the pin number passed to the other GPIO_PIN_* functions.

[mmel]

sys/dev/gpio/gpiobus.c

391

Oh, I didn't know that.

It is not necessary to implement it immediately, but we should at least have a plan B.

Hmm, does this mean that we can't even rely on GPIO_PIN_MAX to figure out how many pins we have?
It would make more sense to me if said drivers did some translation on the pin number passed to the other GPIO_PIN_* functions.

We can rely on that. The question is, however, what GPIO_PIN_MAX actually means. If it means the maximum number of pins, then the controller can return it, but it is almost unusable value in the gpiobus code.

If it means the number of implemented pins, then the controller can also easily report this, and the value is relevant for gpio. Only the name looks a little strange :)

[vexeduxr]

sys/dev/gpio/gpiobus.c

391

It is not necessary to implement it immediately, but we should at least have a plan B.

Yeah, I'm not sure what a good solution would be here.

[mmel]

We can introduce a new method, such as

METHOD  get_pin_list{
 device_t dev;
 device_t bus;
 uint32_t *pin_list;
};

which fills a pre-allocated array with the controller pin numbers, with a default implementation based on GPIO_PIN_MAX() (assuming it returns the number of controller pins - 1) and sequential numbering starting with pin number 0.
This way, we can obtain a list of pins for all 'standard' controllers without any changes to their code, but we retain the option to modify it for 'more creative' ones.

Consider this only as a first quick attempt to solve this puzzle.

[vexeduxr]

In D51932#1191073, @mmel wrote:

We can introduce a new method, such as

METHOD  get_pin_list{
 device_t dev;
 device_t bus;
 uint32_t *pin_list;
};

which fills a pre-allocated array with the controller pin numbers, with a default implementation based on GPIO_PIN_MAX() (assuming it returns the number of controller pins - 1) and sequential numbering starting with pin number 0.
This way, we can obtain a list of pins for all 'standard' controllers without any changes to their code, but we retain the option to modify it for 'more creative' ones.

Consider this only as a first quick attempt to solve this puzzle.

That sounds good :)
Although I don't see why we would need the gpiobus dev.

Just to make sure I'm understanding this correctly:
The pin numbers on a GPIO controller aren't guaranteed to start from zero or be ordered sequentially. So for example, a controller can only accept pin numbers 25 - 40 and would return EINVAL if you pass anything else?
And how do the GPIO_PIN_ACCESS_32 and GPIO_PIN_CONFIG_32 functions work on such controllers if they expect sequential pins? Or would that not be supported there?

Thanks.

[mmel]

In D51932#1191682, @vexeduxr wrote:

In D51932#1191073, @mmel wrote:

We can introduce a new method, such as

METHOD  get_pin_list{
 device_t dev;
 device_t bus;
 uint32_t *pin_list;
};

which fills a pre-allocated array with the controller pin numbers, with a default implementation based on GPIO_PIN_MAX() (assuming it returns the number of controller pins - 1) and sequential numbering starting with pin number 0.
This way, we can obtain a list of pins for all 'standard' controllers without any changes to their code, but we retain the option to modify it for 'more creative' ones.

Consider this only as a first quick attempt to solve this puzzle.

That sounds good :)
Although I don't see why we would need the gpiobus dev.

Right, it's not necessary. Please ignore it.

Just to make sure I'm understanding this correctly:
The pin numbers on a GPIO controller aren't guaranteed to start from zero or be ordered sequentially. So for example, a controller can only accept pin numbers 25 - 40 and would return EINVAL if you pass anything else?

Exactly.

And how do the GPIO_PIN_ACCESS_32 and GPIO_PIN_CONFIG_32 functions work on such controllers if they expect sequential pins? Or would that not be supported there?

If the controller has 32 sequential pins starting with the start pin, then it should work. Otherwise, it won't. I'm not thrilled about it, but I don't see any other consistent solution.

[vexeduxr]

Rebase and use GPIO_GET_PIN_LIST

[vexeduxr]

Better error handling in gpiobus_add_gpioc, call device_delete_child and gpiobus_free_ivars on failure.