[Bug 1001787] GPIO Interrupt Support for Kinetis

[bugzilla-daemon at bugs dot ecos dot sourceware dot org]

Please do not reply to this email, use the link below.

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001787

--- Comment #7 from Mike Jones <mjones@linear.com> ---
Ilija,

I will start comparing the existing STM32 API and the patch. Then I will
discuss my reason I made the API in the patch the way it is. Then I will
propose a solution that should satisfy two viewpoints, one the driver
writer/expert, and the other an application writer wanting simple/just-works.

STM32 has three functions:

__externC void hal_stm32_gpio_set( cyg_uint32 pin );
__externC void hal_stm32_gpio_out( cyg_uint32 pin, int val );
__externC void hal_stm32_gpio_in ( cyg_uint32 pin, int *val );

_set causes the driver to "setup" the pin. It calculates a port and bit from a
single number, sets speed, open-drain, etc. The "pin" is a bunch of bit fields,
so there is no structure to help the end user other than to use macros to set
the fields.

_out and _in let you set values on a single pin.

Now if you are a new user, you are immediately thrown into reverse engineering
of the code to figure out how to use it. You have to hunt down the file, and
look through the all the macros until you find the right ones to use and figure
out that "pin" is not a single digit. This is an aspect of GPIO macros I don't
particularly think helps application writers. I don't use the I2C interface
that way. I have generic C code that hopefully will work with all I2C
peripherals on all targets.

As for the API, there are no functions for setting words, which sometimes I
need because I need pins to change together from a single register write. So
this API is missing some capability I need.

Nonetheless, no intent or malice toward the STM32 author. My patch is no better
and was a hack to make it work the same day I needed it to work.

The Kinetis API in the patch is:

__externC void hal_kinetis_gpio_interrupt_acknowledge(cyg_uint32 port,
cyg_uint32 pin);
__externC void hal_kinetis_gpio_set_pin_in(cyg_uint32 port, cyg_uint32 pin);
__externC void hal_kinetis_gpio_set_pin_out(cyg_uint32 port, cyg_uint32 pin);
__externC void hal_kinetis_gpio_set_pin(cyg_uint32 port, cyg_uint32 pin);
__externC void hal_kinetis_gpio_clear_pin(cyg_uint32 port, cyg_uint32 pin);
__externC void hal_kinetis_gpio_get_pin(cyg_uint32 port, cyg_uint32 pin,
cyg_uint32* val);

__externC void hal_kinetis_gpio_put(cyg_uint32 port, cyg_uint32 val);
__externC void hal_kinetis_gpio_get(cyg_uint32 port, cyg_uint32* val);
__externC void hal_kinetis_gpio_clear(cyg_uint32 port, cyg_uint32* val);

__externC void hal_kinetis_gpio_setup_port(cyg_uint32 port, cyg_uint32 pin,
cyg_uint8 mode, cyg_uint8 mux, cyg_uint8 config);

These all take a port and pin. I can use these with no knowledge of macros. The
datasheet and my schematic has ports and pins written on them. I don't need to
look in any code files to know what to do (setup is an exception).

_interrupt_acknowledge is an artifact of the Kinetis unique interrupt
architecture. I think it is ugly because I can't use the standard interrupt
acknowledge. It only got my job done.

_set_pin_in and _set_pin_out set the direction of the pin. I find these names a
bit easy to confuse with _set_pin and I don't like them.

_set_pin, _clear_pin, and get_pin are all for setting and getting single pin
values.

_put, _get, _clear are for setting and getting all pins on a port. These are
also somewhat confusing and might be better as _put_port, etc.

_setup_port sets the attributes and this is ugly because it is Kinetis
specific, and you have to look up the proper values in the datasheet and
recognize the names of the variables in the data sheet.

So the first question I have is whether we should have a pin descriptor, as
asked? As an application writer, I don't see the value in it. I don't care if
my code configuring a port is a little slower. I only execute once, except
perhaps direction. I would prefer a universal API in C that is obvious without
sorting through a big file of macros trying to learn how to use the port. 

When I set pins/ports, I typically look at a schematic that has port/pin
numbers on it. Why would I want to setup a pin descriptor rather than just say
what I want the code to do? There is no value to me in carrying around a
descriptor full of information I don't care about. Even a moniker based API
hides this data behind the API. So please hide it from me. I don't care. I just
want it to work.

On the the other hand, not everyone may think of GPIO as a peripheral like I2C
where some conformity to a higher level API is important. Or perhaps the
expectation is that GPIO coding is really internal driver coding supporting
some other function with API. I have seen bit banged I2C like that. There is an
I2C C API with GPIO bit banging behind it.

I guess my perspective is that there are designs where there are small numbers
of GPIO pins that control things and don't require a "driver" to cover up the
code. For example, a baseboard controller might have some control pins that
turn supplies on or off and enable and disable loads. I see no reason to make a
driver for that. It is very application specific. I don't want my application
code to look like driver code.

So I am attempting to recognize there may be two views on this topic that
really reflect two different personas. A driver writer/orientation, and a
application writer or inexperienced newbie trying to make it work under high
schedule pressure. Enough pressure that if it does not work in a few days, eCos
is booted off the project, justified or not.

If I follow my own stated philosophy, the proposed API could be better and
generic. Perhaps:

// Kinetis specific
__externC void hal_kinetis_gpio_setup_port(cyg_uint32 port, cyg_uint32 pin,
cyg_uint8 mode, cyg_uint8 mux, 
__externC void hal_kinetis_gpio_interrupt_acknowledge(cyg_uint32 port,
cyg_uint32 pin);

// Generic

__externC void hal_gpio_set_pin_dir_in(cyg_uint32 port, cyg_uint32 pin);
__externC void hal_gpio_set_pin_dir_out(cyg_uint32 port, cyg_uint32 pin);

__externC void hal_gpio_set_pin(cyg_uint32 port, cyg_uint32 pin);
__externC void hal_gpio_clear_pin(cyg_uint32 port, cyg_uint32 pin);
__externC void hal_gpio_get_pin(cyg_uint32 port, cyg_uint32 pin, cyg_uint32*
val);

__externC void hal_gpio_port_set(cyg_uint32 port, cyg_uint32 val);
__externC void hal_gpio_port_clear(cyg_uint32 port, cyg_uint32 val);
__externC void hal_gpio_port_get(cyg_uint32 port, cyg_uint32* val);

I have tried here to create some naming symmetry between pin and port versions.
The direction is now more obvious. _setup_port might be better for the CDL and
have the kinetis API for times it has to be dynamic. These settings don't
change often. Interrupt acknowledge is just a problem. I have no idea how to
handle it in a more universal way.

So how to reconcile these two personas?

I want to propose the following:

The GPIO should support two programming models. One API for driver writers, and
one for application writers. The driver API should use macros and pin
descriptors, and be as consistent between targets as is reasonable without any
sacrifice of performance. The application API should be based on C, reflect
what one sees on datasheets and schematics, be universal, and supported by all
targets over time. Anyone that wants speed can always choose to use the macros
in their application and invest the learning time.

I think this implies the following:

1) The STM32 C API and the Kinetis C API need to be worked on so they match and
are universal.
2) The pin descriptor needs to be removed from the STM32 C API and used with
equivalent macros instead.
3) The STM32 and Kinetis macros need to evolve toward consistency over time.

In the short run, I think it is easier to agree on a universal C API and add it
to both platforms.

Ilija, have I convince you that a descriptor free C API has value?

-- 
You are receiving this mail because:
You are on the CC list for the bug.