Re: RFC: Counter driver API


Andre Guedes <andre.guedes@...>
 

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

Join devel@lists.zephyrproject.org to automatically receive all group messages.