Re: RFC: Use error codes from errno.h
Andre Guedes <andre.guedes@...>
Hi all,
Quoting Jesus Sanchez-Palencia (2016-03-02 16:03:22) Yes, this is the idea.The initial discussion was about using errno.h codes at the driver's layer+1 ! But I would propose that we first get this right for the device driver APIs. DEV_OK = 0Back then, I reviewed the error codes usages and this was what I had in mind: DEV_OK = 0 DEV_FAIL = -EIO /* Input/output error */ DEV_INVALID_OP = -EINVAL /* Invalid argument */ DEV_INVALID_CONF = -EINVAL /* Invalid argument */ DEV_USED = -EBUSY /* Device or resource busy */ DEV_NO_ACCESS = -EACCES /* Permission denied */ DEV_NO_SUPPORT = -ENOTSUP /* Operation not supported */ DEV_NOT_CONFIG = -EPERM /* Operation not permitted */ DEV_NOT_IMPLEMENTED -ENOSYS /* Function not implemented */ The comments besides the error codes come from 'man errno'. Regards, Andre
|
|
Re: RFC: Use error codes from errno.h
Jesus Sanchez-Palencia <jesus.sanchez-palencia@...>
Hi everyone,
On Thu, 3 Mar 2016 13:33:16 -0500 Benjamin Walsh <benjamin.walsh(a)windriver.com> wrote: After all the feedback so far (thanks, btw!), the updated list now looks like:I think -ENOTSUP is a 1-1 match here:What we have on device.h currently is: DEV_OK = 0 DEV_FAIL = -EFAULT DEV_INVALID_OP = -EPERM or -ENOTSUP or -ESRCH DEV_INVALID_CONF = -EINVAL DEV_USED = -EBUSY DEV_NO_ACCESS = -EACCES DEV_NO_SUPPORT = -ENODEV or -ENXIO or -ENOTSUP DEV_NOT_CONFIG = -EIO or -ENODEV DEV_NOT_IMPLEMENTED = -ENOSYS Which leaves us with only 3 left to be decided. By comparing again the original description with the ones from errno.h, what about: DEV_INVALID_OP = -ENOTSUP DEV_NO_SUPPORT = -ENODEV DEV_NOT_CONFIG = -EIO any better? thanks, jesus
|
|
Re: RFC: Use error codes from errno.h
Benjamin Walsh <benjamin.walsh@...>
I think -ENOTSUP is a 1-1 match here:What we have on device.h currently is:This is proposal is very similar to what we had done originally for ENOTSUP Operation not supported (POSIX.1) Perhaps ENODEV would fit better since it device related.DEV_INVALID_CONF = -EINVALENOTSUP
|
|
Re: [RFC] uart: add ISR callback mechanism for UART drivers
Daniel Leung <daniel.leung@...>
On Thu, Mar 03, 2016 at 05:07:31PM +0100, Tomasz Bursztyka wrote:
Hi Daniel,Do you have a particular use case in mind, w.r.t. UART, that requires user data? At this point, the patch is to simply move the IRQ setup code into the UART drivers. This is not a callback in generic terms, but just a an ISR calling another ISR. Since the first ISR does not have any additional user data, the second ISR should not be dependent on having additional information. The UART drivers are done in a way where driver_api is NULL+/* For configuring IRQ on each individual UART device. Internal use only. */so it would have a void *user_data parameter as well if the driver init fails, hence the (api != NULL) check. ----- Daniel Leung
|
|
Re: STM32F103x port
Maciek Borzecki <maciek.borzecki@...>
On Thu, Mar 3, 2016 at 3:43 PM, Maciek Borzecki
<maciek.borzecki(a)gmail.com> wrote: I'm sure all of you already got their emails with incoming reviewPawel spotted that the linker script was missing. This would prevent people from actually building the code. I've pushed an update for https://gerrit.zephyrproject.org/r/645 and the rest got automatically rebased. The problem was caused by filter masks in gitignore, what has been addressed in this review: https://gerrit.zephyrproject.org/r/668 gitignore: make sure that SOC specific linker scripts stay visible No other changes were made to the patches. If you're looking at, or have posted comments to the previous version, have no worries, they all apply to the current patchset. Sorry for not catching that earlier. Cheers, -- Maciek Borzecki
|
|
Re: STM32F103x port
Maciek Borzecki <maciek.borzecki@...>
On Thu, Mar 3, 2016 at 5:00 PM, Tomasz Bursztyka
<tomasz.bursztyka(a)linux.intel.com> wrote: Hi Maciek,Thanks for the comments. I'll wait for everyone to post theirs and try to resolve the issues over the weekend. Cheers, -- Maciek Borzecki
|
|
Re: [RFC] uart: add ISR callback mechanism for UART drivers
Tomasz Bursztyka
Hi Daniel,
I am a bit late on that one. I fully agree on such addition. 1 comment below: +/**Could be wise to add a void *user_data private pointer through the callback. It's something we are missing not only here (gpio etc...) +/* For configuring IRQ on each individual UART device. Internal use only. */so it would have a void *user_data parameter as well +{I guess we do this api != NULL check because of the UART_INTERRUPT_DRIVEN option. I guess at some point we will be only interrupt driven anyway. Tomasz
|
|
Re: STM32F103x port
Tomasz Bursztyka
Hi Maciek,
toggle quoted messageShow quoted text
Thanks for the patches, they are very nicely sliced down. It's quite easy to follow. Just be aware of the code style we apply. Basically the 2 main errors are the missing {} (even for one liners if, yes, and the while loop as well) and the variable being declared in the middle of the statement block: it's always at the beginning. It's minor changes. scripts/checkpatch.pl is nice so check before hand. Tomasz
I'm sure all of you already got their emails with incoming review
|
|
Re: Newbie wants to contribute
Perez Hernandez, Javier B <javier.b.perez.hernandez@...>
Hi!
On Thu, 2016-03-03 at 13:05 +0530, Himanshu Maurya wrote: Hello zephyr team!!!You can start by reading our documentation: https://www.zephyrproject.org/doc/collaboration/collaboration.html Any question, just ask in users(a)lists.zephyrproject.org or in IRC #zephyrproject. BTW, please use plain text in emails. Welcome! Javier B. Perez Â
|
|
Re: RFC: Use error codes from errno.h
Luiz Augusto von Dentz
Hi Daniel,
On Wed, Mar 2, 2016 at 10:46 PM, Kalowsky, Daniel <daniel.kalowsky(a)intel.com> wrote: Perhaps ENODEV would fit better since it device related.-----Original Message-----This is proposal is very similar to what we had done originally for the devices instead of using the DEV_ names. DEV_NOT_IMPLEMENTED = -ENOSYS -- Luiz Augusto von Dentz
|
|
Re: [RFC v2] uart: add ISR callback mechanism for UART drivers
Andre Guedes <andre.guedes@...>
Hi Daniel,
Quoting Daniel Leung (2016-03-02 22:35:24) No, I don't have anything in particular. Probably the bluetooth driversThis is actually a nice improvement on the UART API since it moves theThis is something I have thought about, but there is no concrete plan. and the console_uart driver are a good source of use cases. For instance, they all have their own implementation of a mechanism read data asynchronously. An API such as uart_read(dev, callback, buf, len) might be useful. Regards, Andre
|
|
[RFC] Sensor API
Vlad Dogaru <vlad.dogaru@...>
Hi everyone,
I have uploaded a new iteration of the sensor API patches to Gerrit, you can find them at [1]. We hope to address some of the concerns regarding memory consumption of sensor drivers. For the moment, I have only converted one driver to the new infrastructure, as I would like to get early feedback on the direction the API is evolving. The major change since the previous version is the handling of sensor triggers. Previously, we operated under the assumption that each driver that supported interrupts would create its own fiber to which it would defer bus traffic (since it can't touch I2C or SPI in an ISR). In this new iteration, the user is given a choice via Kconfig of the following three approaches: (1) Driver does not support triggering. No fiber is created. (2) Driver supports triggering, but uses a system-wide fiber to defer bus traffic. Multiple drivers can choose this approach, meaning they only pay the cost of the one fiber. (3) Driver supports triggering and creates its own fiber for bus traffic. This ensures the best response time, but uses more memory if multiple drivers choose to go with this option. The last patch of the series [2] is an example of how to add support for cases (2) and (3) if the initial driver only supports triggerless operation. There are more drivers available in the sensors topic on Gerrit [3], but they have not been converted to this newest iteration of the API. If the response to this RFC is positive, we will convert them to the three option approach detailed above. Finally, please be aware that this is still an RFC; at the very least, the final API will need documentation for standard units for each type of channel and magnetometer channel types. [1] https://gerrit.zephyrproject.org/r/487 https://gerrit.zephyrproject.org/r/488 https://gerrit.zephyrproject.org/r/489 https://gerrit.zephyrproject.org/r/490 [2] https://gerrit.zephyrproject.org/r/541 [3] https://gerrit.zephyrproject.org/r/#/q/topic:sensors Regards, Vlad
|
|
Re: STM32F103x port
Maciek Borzecki <maciek.borzecki@...>
I'm sure all of you already got their emails with incoming review
request. For others who might be interested as well, I've posted a series of patches to gerrit: https://gerrit.zephyrproject.org/r/645 st_stm32/stm32f1: introduce STM32F1x SoC family https://gerrit.zephyrproject.org/r/646 clock_control/Kconfig: Quark should depend on CLOCK_CONTROL https://gerrit.zephyrproject.org/r/647 clock_control/stm32f10x: introduce new driver for STM32F10x RCC https://gerrit.zephyrproject.org/r/648 soc/stm32f1: add GPIO registers mapping https://gerrit.zephyrproject.org/r/649 pinmux/stm32f10x: add driver for STM32F1 pinmux https://gerrit.zephyrproject.org/r/650 serial/stm32f10x: add new driver for STM32F10x UART https://gerrit.zephyrproject.org/r/651 gpio/stm32f10x: add new driver for STM32F10x GPIO https://gerrit.zephyrproject.org/r/652 boards/stm32_mini_a15: add new board https://gerrit.zephyrproject.org/r/653 samples/disco: add 'disco' sample program -- Maciek Borzecki
|
|
Re: RFC: Counter driver API
Tomasz Bursztyka
Hi Andre,
I missed the idea that DEV_* will anyway be mapped to errno, so keepYes, we'll move to errno codes any time soon. I'd prefer we go with the* @brief Start counter device.Looks like we are moving to Posix error codes, so it would be wise to do DEV_* yes. Actually, follow Jesus's comment. counter_set_alarm() should not starThis is a better comment./**Set an alarm callback. If the counter was not started yet, this will do the counter. If you are using 1 alarm at all time, then don't add counter_unset_alarm()What about (let me know if I am wrong, hw feature wise) resetting an alarm?API-wise, I think it makes sense we have the counter_unset_alarm. I'm just User could just use counter_set_alarm() with a NULL pointer as a callback Tomasz
|
|
Re: RFC: Counter driver API
Andre Guedes <andre.guedes@...>
Hi Jesus,
Quoting Jesus Sanchez-Palencia (2016-03-03 09:52:43) I don't think DEV_NO_SUPPORT will be ever returned by the counter_start APIProbably better here to:/** since this is a very basic API and all counter must support it. Anyways, I think we should list in the documentation all the return codes a given API can return instead of simply saying DEV_*. +1.Looks like we are moving to Posix error codes, so it would be wise to doIt's still not clear if the change will go straight to Posix errors or I'm fine with we have 2 API calls (counter_start and counter_set_alarm)./**Set an alarm callback. If the counter was not started yet, this will do Regards, Andre
|
|
Re: RFC: Counter driver API
Andre Guedes <andre.guedes@...>
Hi Tomasz,
Quoting Tomasz Bursztyka (2016-03-03 05:24:12) Yes, we'll move to errno codes any time soon. I'd prefer we go with the/**Looks like we are moving to Posix error codes, so it would be wise to do current error code convention (DEV_*) at the moment and fix all drivers at once in the 'errno patchset'. This is a better comment./**Set an alarm callback. If the counter was not started yet, this will do What about (let me know if I am wrong, hw feature wise) resetting an alarm?API-wise, I think it makes sense we have the counter_unset_alarm. I'm just wondering if the counter_stop API does cover the use case. I mean, counter_stop stops the counter so the alarm won't be fired. Now question is: would it be enough to have 1 alarm set a time, or couldATM, I'd say we should go with one alarm set a time. For such functionality you described, I think the user probably wants to use a system-wide timer, such as the system timer, instead. Bare in mind that this is a driver API and some platforms may not have any counter available. +1. Yes, this is a good idea indeed.*Sound fine, but please create a typedef for the callback, a bit more Thanks for your feedback, Andre
|
|
Re: RFC: Counter driver API
Jesus Sanchez-Palencia <jesus.sanchez-palencia@...>
Aloha,
On Thu, 3 Mar 2016 09:24:12 +0100 Tomasz Bursztyka <tomasz.bursztyka(a)linux.intel.com> wrote: Hi Andre,Probably better here to:/** @retval DEV_NO_SUPPORT if the device doesn't support starting the counter (e.g. free running counters). It's still not clear if the change will go straight to Posix errors or if the transition through DEV_* will happen first. We can't mix both. Probably better here to:*/ @retval DEV_NO_SUPPORT if the device doesn't support stopping the counter (e.g. free running counters). */Set an alarm callback. If the counter was not started yet, this will do In general having an API that does 2 things is not a good idea. An API called 'counter_set_alarm' should do only that, IMO. I'd rather have 2 API calls for that (set and start), but if we really want it to do both, then maybe better calling it counter_init_alarm(..., int count); ?! thanks, jesus
|
|
Re: [RFC] GPIO API changes
Tomasz Bursztyka
Hi Johan,
I fully agree on that. It saves a pointer in case the user does not want1) Let's add a user data pointer parameter to the callback:A perhaps slightly more compact solution (and a pattern I see a log e.g. to access any private data, and thus leaves space for adding pin mask in the struct. Tomasz
|
|
Re: [RFC] GPIO API changes
Hi Tomasz,
On Thu, Mar 03, 2016, Tomasz Bursztyka wrote: 1) Let's add a user data pointer parameter to the callback:A perhaps slightly more compact solution (and a pattern I see a log e.g. in Linux) is to take advantage of CONTAINER_OF. You'd first drop user_data from the gpio_callback and pass the original struct to the callback: struct gpio_callback { void (*func)(struct device *port, struct gpio_callback *cb, uint32_t pin); struct gpio_callback *_next; } Then users of the API could just use this struct directly, or if they need "user data" they can define their own structs that embed the gpio one: struct my_struct { ... struct gpio_callback cb; ... }; void my_func(struct device *port, struct gpio_callback *cb, uint32_t pins) { struct my_struct *data = CONTAINER_OF(cb, struct my_struct, cb); } { struct my_struct s; GPIO_INIT_CB(&s.cb, my_func); gpio_set_callback(port, &s.cb); } Johan
|
|
[RFC] GPIO API changes
Tomasz Bursztyka
Hi,
I would like to propose some changes to the public GPIO API. Addressing major 2 issues which I faced while writing some code using the API. And an third one, related to the consistency of the API. API issues: ========== 1) the callback might need to know about the context: Let's say you have a driver which sets a gpio callback. As soon as there is 2 instances of this driver, it won't be able to tell which instance does that callback call belongs to (unless keeping book about which gpio port/pin relates to which inner instance which is ugly) 2) 2+ different sub-system might need to set a callback: One sub-system might be interested to get notified when pin X generated an interrupt, when another would be interested by pin Y. 3) Currently, you can set either a callback for the whole port or per-pin. The major difference is found in how callback is called: -> port callback: the pin parameter of the callback is a bitfield, each bit telling which pin generated and int. -> pin callback: the pin parameter is the pin number and not anymore a bit in a bitfield. API changes: =========== 1) Let's add a user data pointer parameter to the callback: typedef void (*gpio_callback_t)(struct device *port, uint32_t pin, void *user_data); gpio_set_callback() would then require a void *user_data parameter as well. 2) The callback would not be only a function, but a structure in order to be able to manage a simple linked-list of callbacks. Johan had a good input on that one, as he and bt team have done that in the BT stack. Instead of a function pointer you would pass a pointer of: struct gpio_callback { void (*func)(struct device *port, uint32_t pin, void *user_data); void *user_data; struct gpio_callback *_next; } This avoid to manage any memory slots in the driver. All would be statically allowed. Also, being a generic list, the code could be centralized in a private header in drivers/gpio thus we would not have to re-implement such management in every driver. 3) I would like to normalize that: uint32_t pin would becomes uint32_t pins and it would be always a bitfield. Then instead of: -> port cb: if (pin & BIT(the_pin_to_look_up)) { ... } and -> pin cb: if (pin == the_pin_to_look_up) { ... } We would end up with a unique way of handling such parameter in the callback: if (pin & BIT(the_pin_to_look_up)) { ... } Another idea: ============ Having multiple callback, I see one issue: it's up to the callback to filter out which pins it's interested in. I mean, when setting a callback nothing tells which pins it's meant for. Once you enabled pin X, Y or Z to generate interrupts, all callbacks will be called whatever pin is generating an interrupt. If you have a callback interested by pin X and Y but not Z, it will be called for the 3 of them. It's not a big issue on the callback side since it will most likely filter on X or Y this way: if (pins & BIT(pin_X) { ... } else if (pins & BIT(pin_y)) { ... } So we could add a pin mask in the struct gpio_callback: uint32_t pin_mask. That one would be used in gpio driver ISR handler to filter which callback is relevant to call. I see 2 advantages: - less callback calls, only relevant ones (gpio int are then handled in a quicker way also) - it would be a tiny bit more simple to filter out pins in the callback: instead of the last "else if", you would just get a "else". It's not much but still. The drawback is it bloats a bit more the struct gpio_callback. However, on complex hw setup, with different sub-system being interested by the same gpio port and different pins, this could make thing performing better. The less we stay in an ISR context, the better. From API functions point of view, I want to keep most of it as it is, only parameters will change a bit. For instance I still want a unique gpio_set_callback() function. I still think it's relevant to keep gpio_pin_<enable/disable>_callback() no matter what pins are the callback interested in. Same for gpi_port_enable_callback(). Finally, only the gpio_callback_t type will change. I still have to figure out how to keep the old API behavior, probably through Kconfig option. It would be either/or, not both. Or should it enable old behavior always? Tomasz
|
|