From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 4383 invoked by alias); 7 Apr 2013 20:33:01 -0000 Mailing-List: contact ecos-patches-help@ecos.sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Post: List-Help: , Sender: ecos-patches-owner@ecos.sourceware.org Received: (qmail 4371 invoked by uid 89); 7 Apr 2013 20:33:00 -0000 X-Spam-SWARE-Status: No, score=-2.7 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.1 Received: from hagrid.ecoscentric.com (HELO mail.ecoscentric.com) (212.13.207.197) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Sun, 07 Apr 2013 20:32:58 +0000 Received: from localhost (hagrid.ecoscentric.com [127.0.0.1]) by mail.ecoscentric.com (Postfix) with ESMTP id D16524680007 for ; Sun, 7 Apr 2013 21:32:55 +0100 (BST) Received: from mail.ecoscentric.com ([127.0.0.1]) by localhost (hagrid.ecoscentric.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id VJPP9WfevVWA; Sun, 7 Apr 2013 21:32:46 +0100 (BST) From: bugzilla-daemon@bugs.ecos.sourceware.org To: ecos-patches@ecos.sourceware.org Subject: [Bug 1001787] GPIO Interrupt Support for Kinetis Date: Sun, 07 Apr 2013 20:33:00 -0000 X-Bugzilla-Reason: CC X-Bugzilla-Type: changed X-Bugzilla-Watch-Reason: None X-Bugzilla-Product: eCos X-Bugzilla-Component: Patches and contributions X-Bugzilla-Keywords: X-Bugzilla-Severity: enhancement X-Bugzilla-Who: mjones@linear.com X-Bugzilla-Status: UNCONFIRMED X-Bugzilla-Priority: low X-Bugzilla-Assigned-To: ilijak@siva.com.mk X-Bugzilla-Target-Milestone: --- X-Bugzilla-Changed-Fields: Message-ID: In-Reply-To: References: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit X-Bugzilla-URL: http://bugs.ecos.sourceware.org/ Auto-Submitted: auto-generated MIME-Version: 1.0 X-SW-Source: 2013-04/txt/msg00037.txt.bz2 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 --- 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.