Hi everyone,


On Thu, 3 Mar 2016 13:33:16 -0500
Benjamin Walsh <benjamin.walsh(a)windriver.com> wrote:

What we have on device.h currently is:

/* Common Error Codes devices can provide */
#define DEV_OK 0 /* No error */
#define DEV_FAIL 1 /* General operation failure */
#define DEV_INVALID_OP 2 /* Invalid operation */
#define DEV_INVALID_CONF 3 /* Invalid configuration */
#define DEV_USED 4 /* Device controller in use */
#define DEV_NO_ACCESS 5 /* Controller not accessible */
#define DEV_NO_SUPPORT 6 /* Device type not supported */
#define DEV_NOT_CONFIG 7 /* Device not configured */

DEV_OK = 0
DEV_FAIL = (??)

EFAULT

DEV_INVALID_OP = -EPERM or -ENOTSUP (??)

ESRCH

I think -ENOTSUP is a 1-1 match here:

ENOTSUP Operation not supported (POSIX.1)

After all the feedback so far (thanks, btw!), the updated list now looks like:

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

What we have on device.h currently is:

/* Common Error Codes devices can provide */
#define DEV_OK 0 /* No error */
#define DEV_FAIL 1 /* General operation failure */
#define DEV_INVALID_OP 2 /* Invalid operation */
#define DEV_INVALID_CONF 3 /* Invalid configuration */
#define DEV_USED 4 /* Device controller in use */
#define DEV_NO_ACCESS 5 /* Controller not accessible */
#define DEV_NO_SUPPORT 6 /* Device type not supported */
#define DEV_NOT_CONFIG 7 /* Device not configured */


I liked Dirk's suggestion on the previous thread, so I'm gonna propose 2 quick
steps:
a- let's try to map each of this to errno codes;
b- let's document when we should return each of them.

You probably have b) ready from the other thread as well, so let me give it a
try with a).
If we get this two right, then we end up with both the guidelines for
documenting and the
transition patch Dirk suggested. After he deprecates DEV_*, moving to errno
codes will be as easy
as a few seds. So:

This is proposal is very similar to what we had done originally for
the devices instead of using the DEV_ names.

DEV_OK = 0
DEV_FAIL = (??)

EFAULT

DEV_INVALID_OP = -EPERM or -ENOTSUP (??)

ESRCH

I think -ENOTSUP is a 1-1 match here:

ENOTSUP Operation not supported (POSIX.1)

DEV_INVALID_CONF = -EINVAL
DEV_USED = -EBUSY
DEV_NO_ACCESS = -EAGAIN
DEV_NO_SUPPORT = -ENODEV or -ENXIO (??)

ENOTSUP

DEV_NOT_CONFIG = (??)

EIO

Perhaps ENODEV would fit better since it device related.

DEV_NOT_IMPLEMENTED = -ENOSYS
*this last one is from your previous RFC.

On Thu, Mar 03, 2016 at 05:07:31PM +0100, Tomasz Bursztyka wrote:

Hi Daniel,

I am a bit late on that one. I fully agree on such addition.
1 comment below:

+/**
+ * @brief Define the application callback function signature for UART.
+ *
+ * @param port Device struct for the UART device.
+ */
+typedef void (*uart_irq_callback_t)(struct device *port);
+

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...)

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.

+/* For configuring IRQ on each individual UART device. Internal use only. */
+typedef void (*uart_irq_config_func_t)(struct device *port);
+
/** @brief UART device configuration.*/
struct uart_device_config {
/**
@@ -90,6 +100,10 @@ struct uart_device_config {
#ifdef CONFIG_PCI
struct pci_dev_info pci_dev;
#endif /* CONFIG_PCI */
+
+#ifdef CONFIG_UART_INTERRUPT_DRIVEN
+ uart_irq_config_func_t irq_config_func;
+#endif
};

/** @brief Driver API structure. */
@@ -145,6 +159,9 @@ struct uart_driver_api {
/** Interrupt driven input hook function */
int (*irq_input_hook)(struct device *dev, uint8_t byte);

+ /** Set the callback function */
+ void (*irq_callback_set)(struct device *dev, uart_irq_callback_t cb);
+
#endif

#ifdef CONFIG_UART_LINE_CTRL
@@ -516,6 +533,30 @@ static inline void uart_irq_input_hook_set(struct device *dev,
}
}

+
+/**
+ * @brief Set the IRQ callback function pointer.
+ *
+ * This sets up the callback for IRQ. When an IRQ is triggered,
+ * the specified function will be called.
+ *
+ * @param dev UART device structure.
+ * @param cb Pointer to the callback function.
+ *
+ * @return N/A
+ */
+static inline void uart_irq_callback_set(struct device *dev,
+ uart_irq_callback_t cb)

so it would have a void *user_data parameter as well

+{
+ struct uart_driver_api *api;
+
+ api = (struct uart_driver_api *)dev->driver_api;
+
+ if ((api != NULL) && (api->irq_callback_set != NULL)) {

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.

The UART drivers are done in a way where driver_api is NULL
if the driver init fails, hence the (api != NULL) check.


-----
Daniel Leung

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 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

Pawel 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

On Thu, Mar 3, 2016 at 5:00 PM, Tomasz Bursztyka
<tomasz.bursztyka(a)linux.intel.com> wrote:

Hi Maciek,

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.

Thanks for the comments. I'll wait for everyone to post theirs and try to
resolve the issues over the weekend.

Cheers,
--
Maciek Borzecki

Hi Daniel,

I am a bit late on that one. I fully agree on such addition.
1 comment below:

+/**
+ * @brief Define the application callback function signature for UART.
+ *
+ * @param port Device struct for the UART device.
+ */
+typedef void (*uart_irq_callback_t)(struct device *port);
+

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. */
+typedef void (*uart_irq_config_func_t)(struct device *port);
+
/** @brief UART device configuration.*/
struct uart_device_config {
/**
@@ -90,6 +100,10 @@ struct uart_device_config {
#ifdef CONFIG_PCI
struct pci_dev_info pci_dev;
#endif /* CONFIG_PCI */
+
+#ifdef CONFIG_UART_INTERRUPT_DRIVEN
+ uart_irq_config_func_t irq_config_func;
+#endif
};

/** @brief Driver API structure. */
@@ -145,6 +159,9 @@ struct uart_driver_api {
/** Interrupt driven input hook function */
int (*irq_input_hook)(struct device *dev, uint8_t byte);

+ /** Set the callback function */
+ void (*irq_callback_set)(struct device *dev, uart_irq_callback_t cb);
+
#endif

#ifdef CONFIG_UART_LINE_CTRL
@@ -516,6 +533,30 @@ static inline void uart_irq_input_hook_set(struct device *dev,
}
}

+
+/**
+ * @brief Set the IRQ callback function pointer.
+ *
+ * This sets up the callback for IRQ. When an IRQ is triggered,
+ * the specified function will be called.
+ *
+ * @param dev UART device structure.
+ * @param cb Pointer to the callback function.
+ *
+ * @return N/A
+ */
+static inline void uart_irq_callback_set(struct device *dev,
+ uart_irq_callback_t cb)

so it would have a void *user_data parameter as well

+{
+ struct uart_driver_api *api;
+
+ api = (struct uart_driver_api *)dev->driver_api;
+
+ if ((api != NULL) && (api->irq_callback_set != NULL)) {

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

Hi Maciek,

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

Hi!

On Thu, 2016-03-03 at 13:05 +0530, Himanshu Maurya wrote:

Hello zephyr team!!!

I am Himanshu, I am junior undergrad at Indian Institute of
Information Technology Design & Manufacturing Jabalpur .
I have been looking for an opportunity for working in an OS project
and Zephyr give is the perfect opportunity. 
Here's my profile for reference: in.linkedin.com/in/lordzuko 

Can you tell me where I can contribute to the project or some
prerequisites. 
Any help would be appreciated.

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

 
Hope to hear from you!!!

Cheers!!!!

Himanshu 
(lordzuko )

Hi Daniel,

On Wed, Mar 2, 2016 at 10:46 PM, Kalowsky, Daniel
<daniel.kalowsky(a)intel.com> wrote:

-----Original Message-----
From: Jesus Sanchez-Palencia [mailto:jesus.sanchez-palencia(a)intel.com]
Sent: Wednesday, March 2, 2016 11:03 AM
To: Guedes, Andre <andre.guedes(a)intel.com>
Cc: devel(a)lists.zephyrproject.org
Subject: [devel] Re: RFC: Use error codes from errno.h

Hi,

On Thu, 18 Feb 2016 17:47:56 -0200
Andre Guedes <andre.guedes(a)intel.com> wrote:

Hi all,

While we were discussing about adding a new error code for device.h (see
"[RFC] Add DEV_NOT_IMPLEMENTED error code" thread), we had an initial
agreement that it does make sense to use error codes from errno.h

instead

of the ones from include/device.h. Since this topic deserves its own RFC,
I'm sending this email so we can have a proper discussion.

Cool!
+1 from my side, mainly so we don't need to discuss the addition of new
error codes ever again :).

So the main points in favor of this change are 1) errno.h is a well-known
error convention which pretty much all developer is familiar with, 2)
errno.h codes address what we need, 3) no need to create new codes such
as DEV_NOT_IMPLEMENTED for instance, and 4) changing the current

drivers

to use errno.h codes instead of DEV_* is a feasible task.

I would say it's more about 3) and 4) really, as I've seen surprisingly often
how 1) and 2) are not
always true. I've seen recently people getting confused about what to return
and when, myself
included. In the end it doesn't matter from each .h file you get the errors
from, as long as you
have clear guidelines for them, in my opinion.

The initial discussion was about using errno.h codes at the driver's layer
but I think we can expand it to the whole system. Actually, errno.h codes
are already used in net/bluetooth and net/ip.

+1 ! But I would propose that we first get this right for the device driver APIs.

What we have on device.h currently is:

/* Common Error Codes devices can provide */
#define DEV_OK 0 /* No error */
#define DEV_FAIL 1 /* General operation failure */
#define DEV_INVALID_OP 2 /* Invalid operation */
#define DEV_INVALID_CONF 3 /* Invalid configuration */
#define DEV_USED 4 /* Device controller in use */
#define DEV_NO_ACCESS 5 /* Controller not accessible */
#define DEV_NO_SUPPORT 6 /* Device type not supported */
#define DEV_NOT_CONFIG 7 /* Device not configured */


I liked Dirk's suggestion on the previous thread, so I'm gonna propose 2 quick
steps:
a- let's try to map each of this to errno codes;
b- let's document when we should return each of them.

You probably have b) ready from the other thread as well, so let me give it a
try with a).
If we get this two right, then we end up with both the guidelines for
documenting and the
transition patch Dirk suggested. After he deprecates DEV_*, moving to errno
codes will be as easy
as a few seds. So:

This is proposal is very similar to what we had done originally for the devices instead of using the DEV_ names.

DEV_OK = 0
DEV_FAIL = (??)

EFAULT

DEV_INVALID_OP = -EPERM or -ENOTSUP (??)

ESRCH

DEV_INVALID_CONF = -EINVAL
DEV_USED = -EBUSY
DEV_NO_ACCESS = -EAGAIN
DEV_NO_SUPPORT = -ENODEV or -ENXIO (??)

ENOTSUP

DEV_NOT_CONFIG = (??)

EIO

Perhaps ENODEV would fit better since it device related.

DEV_NOT_IMPLEMENTED = -ENOSYS
*this last one is from your previous RFC.


How bad does that look?!

--
Luiz Augusto von Dentz

Hi Daniel,

Quoting Daniel Leung (2016-03-02 22:35:24)

This is actually a nice improvement on the UART API since it moves the
burden of registering the ISR from the user layer to the driver layer,
which is the right place, IMO.

Actually, the UART API is very low level when we compare it with other
APIs such as SPI and I2C. For instance, the UART API exposes functions
to manipulate the FIFOs and check for interrupt flags. Is there any plan
to add higher level APIs such as "transmit the characters from this buffer
and call this callback once the transmission finishes" or "read 10
characters into this buffer and call this callback once it is done"?

This is something I have thought about, but there is no concrete plan.

Is there anything particular you are looking for? Use cases would be
a good start. This helps me to understand what needs to be done.

No, I don't have anything in particular. Probably the bluetooth drivers
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

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

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

Hi Andre,

* @brief Start counter device.
* @param dev Pointer to the device structure for the driver instance.
* @retval DEV_OK If successful.
* @retval DEV_* Code otherwise.

Looks like we are moving to Posix error codes, so it would be wise to do
it here as
well. (better now than after the API is upstream). Apply that to all.

Yes, we'll move to errno codes any time soon. I'd prefer we go with the
current error code convention (DEV_*) at the moment and fix all drivers
at once in the 'errno patchset'.

I missed the idea that DEV_* will anyway be mapped to errno, so keep
DEV_* yes.

/**
* Set an alarm callback. If the counter was not started yet, this
* function starts and set the alarm.

Set an alarm callback. If the counter was not started yet, this will do
it automatically
(no need to call counter_start()).

This is a better comment.

Actually, follow Jesus's comment. counter_set_alarm() should not star
the counter.

What about (let me know if I am wrong, hw feature wise) resetting an alarm?
Currently, it set set it, and nothing cannot stop it.

So we may want a counter_unset_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 could
it be
interesting to link alarms? (so different subsystem could use set an alarm)

ATM, 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.

If you are using 1 alarm at all time, then don't add counter_unset_alarm()
User could just use counter_set_alarm() with a NULL pointer as a callback

Tomasz

Hi Jesus,

Quoting Jesus Sanchez-Palencia (2016-03-03 09:52:43)

/**
* Start the counter device. If the device is a 'count up' counter the
* counter initial value is set to zero. It it is a 'countdown' counter
* the initial value is set to the maximum value supported by the device.
*
* @brief Start counter device.
* @param dev Pointer to the device structure for the driver instance.
* @retval DEV_OK If successful.
* @retval DEV_* Code otherwise.

Probably better here to:
@retval DEV_NO_SUPPORT if the device doesn't support starting the
counter (e.g. free running counters).

I don't think DEV_NO_SUPPORT will be ever returned by the counter_start API
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_*.

Looks like we are moving to Posix error codes, so it would be wise to do
it here as
well. (better now than after the API is upstream). Apply that to all.

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.

+1.

/**
* Set an alarm callback. If the counter was not started yet, this
* function starts and set the alarm.

Set an alarm callback. If the counter was not started yet, this will do
it automatically
(no need to call counter_start()).

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); ?!

I'm fine with we have 2 API calls (counter_start and counter_set_alarm).

Regards,

Andre

Hi Tomasz,

Quoting Tomasz Bursztyka (2016-03-03 05:24:12)

/**
* Start the counter device. If the device is a 'count up' counter the
* counter initial value is set to zero. It it is a 'countdown' counter
* the initial value is set to the maximum value supported by the device.
*
* @brief Start counter device.
* @param dev Pointer to the device structure for the driver instance.
* @retval DEV_OK If successful.
* @retval DEV_* Code otherwise.

Looks like we are moving to Posix error codes, so it would be wise to do
it here as
well. (better now than after the API is upstream). Apply that to all.

Yes, we'll move to errno codes any time soon. I'd prefer we go with the
current error code convention (DEV_*) at the moment and fix all drivers
at once in the 'errno patchset'.

/**
* Set an alarm callback. If the counter was not started yet, this
* function starts and set the alarm.

Set an alarm callback. If the counter was not started yet, this will do
it automatically
(no need to call counter_start()).

This is a better comment.

What about (let me know if I am wrong, hw feature wise) resetting an alarm?
Currently, it set set it, and nothing cannot stop it.

So we may want a counter_unset_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 could
it be
interesting to link alarms? (so different subsystem could use set an alarm)

ATM, 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.

*
* @brief Set an alarm callback.
* @param dev Pointer to the device structure for the driver instance.
* @param callback Pointer to the callback function.
* @param count Number of counter ticks.
*
* @retval DEV_OK If successful.
* @retval DEV_NO_SUPPORT if the device doesn't support interrupt (e.g.
* free running counters).
*/
int counter_set_alarm(struct device *dev, void (*callback)(void *data), int count);

Sound fine, but please create a typedef for the callback, a bit more
developed:

typedef void (*counter_callback_t)(struct device *dev, void *user_data)

Then:
- make count as an uint32_t
- and add a void *user_data pointer as well.

+1. Yes, this is a good idea indeed.

Thanks for your feedback,

Andre

Aloha,


On Thu, 3 Mar 2016 09:24:12 +0100
Tomasz Bursztyka <tomasz.bursztyka(a)linux.intel.com> wrote:

Hi Andre,

/**
* Start the counter device. If the device is a 'count up' counter the
* counter initial value is set to zero. It it is a 'countdown' counter
* the initial value is set to the maximum value supported by the device.
*
* @brief Start counter device.
* @param dev Pointer to the device structure for the driver instance.
* @retval DEV_OK If successful.
* @retval DEV_* Code otherwise.

Probably better here to:
@retval DEV_NO_SUPPORT if the device doesn't support starting the
counter (e.g. free running counters).

Looks like we are moving to Posix error codes, so it would be wise to do
it here as
well. (better now than after the API is upstream). Apply that to all.

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.

*/
int counter_start(struct device *dev);

/**
* @brief Stop counter device.
* @param dev Pointer to the device structure for the driver instance.
*
* @retval DEV_OK If successful.
* @retval DEV_* Code otherwise.

Probably better here to:
@retval DEV_NO_SUPPORT if the device doesn't support stopping the
counter (e.g. free running counters).

*/
int counter_stop(struct device *dev);

/**
* @brief Read current counter value
* @param dev Pointer to the device structure for the driver instance.
*
* @return 32-bit value
*/
uint32_t counter_read(struct device *dev);

/**
* Set an alarm callback. If the counter was not started yet, this
* function starts and set the alarm.

Set an alarm callback. If the counter was not started yet, this will do
it automatically
(no need to call counter_start()).

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

What about (let me know if I am wrong, hw feature wise) resetting an alarm?
Currently, it set set it, and nothing cannot stop it.

So we may want a counter_unset_alarm() ?

Now question is: would it be enough to have 1 alarm set a time, or could
it be
interesting to link alarms? (so different subsystem could use set an alarm)

*
* @brief Set an alarm callback.
* @param dev Pointer to the device structure for the driver instance.
* @param callback Pointer to the callback function.
* @param count Number of counter ticks.
*
* @retval DEV_OK If successful.
* @retval DEV_NO_SUPPORT if the device doesn't support interrupt (e.g.
* free running counters).
*/
int counter_set_alarm(struct device *dev, void (*callback)(void *data), int count);

Sound fine, but please create a typedef for the callback, a bit more
developed:

typedef void (*counter_callback_t)(struct device *dev, void *user_data)

Then:
- make count as an uint32_t
- and add a void *user_data pointer as well.

Tomasz

Hi Johan,

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.

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);
}

I fully agree on that. It saves a pointer in case the user does not want
to access any private data,
and thus leaves space for adding pin mask in the struct.

Tomasz

Hi Tomasz,

On Thu, Mar 03, 2016, Tomasz Bursztyka wrote:

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.

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

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

Hi Andre,

/**
* Start the counter device. If the device is a 'count up' counter the
* counter initial value is set to zero. It it is a 'countdown' counter
* the initial value is set to the maximum value supported by the device.
*
* @brief Start counter device.
* @param dev Pointer to the device structure for the driver instance.
* @retval DEV_OK If successful.
* @retval DEV_* Code otherwise.

Looks like we are moving to Posix error codes, so it would be wise to do
it here as
well. (better now than after the API is upstream). Apply that to all.

*/
int counter_start(struct device *dev);

/**
* @brief Stop counter device.
* @param dev Pointer to the device structure for the driver instance.
*
* @retval DEV_OK If successful.
* @retval DEV_* Code otherwise.
*/
int counter_stop(struct device *dev);

/**
* @brief Read current counter value
* @param dev Pointer to the device structure for the driver instance.
*
* @return 32-bit value
*/
uint32_t counter_read(struct device *dev);

/**
* Set an alarm callback. If the counter was not started yet, this
* function starts and set the alarm.

Set an alarm callback. If the counter was not started yet, this will do
it automatically
(no need to call counter_start()).

What about (let me know if I am wrong, hw feature wise) resetting an alarm?
Currently, it set set it, and nothing cannot stop it.

So we may want a counter_unset_alarm() ?

Now question is: would it be enough to have 1 alarm set a time, or could
it be
interesting to link alarms? (so different subsystem could use set an alarm)

*
* @brief Set an alarm callback.
* @param dev Pointer to the device structure for the driver instance.
* @param callback Pointer to the callback function.
* @param count Number of counter ticks.
*
* @retval DEV_OK If successful.
* @retval DEV_NO_SUPPORT if the device doesn't support interrupt (e.g.
* free running counters).
*/
int counter_set_alarm(struct device *dev, void (*callback)(void *data), int count);

Sound fine, but please create a typedef for the callback, a bit more
developed:

typedef void (*counter_callback_t)(struct device *dev, void *user_data)

Then:
- make count as an uint32_t
- and add a void *user_data pointer as well.

Tomasz