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,

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

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