Date   

Re: [RFC] NULL callbacks in driver API

Tomasz Bursztyka
 

Hi,

static inline uint32_t pinmux_pin_input_enable(struct device *dev,
uint32_t pin,
uint8_t func)
{
struct pinmux_driver_api *api;

api = (struct pinmux_driver_api *) dev->driver_api;
__ASSERT(api->input, "%s not implemented", __func__);
return api->input(dev, pin, func);
}
Something like that yes, though I'd put empty lines in between ;)

Tomasz


Re: [RFC] NULL callbacks in driver API

Maciek Borzecki <maciek.borzecki@...>
 

On Wed, Mar 9, 2016 at 11:00 AM, Tomasz Bursztyka
<tomasz.bursztyka(a)linux.intel.com> wrote:
Hi Maciek,

An obligatory example:

<---- code --------->
struct pinmux_driver_api {
pmux_set set;
pmux_get get;
pmux_pullup pullup;
pmux_input input;
};
...

static inline uint32_t pinmux_pin_input_enable(struct device *dev,
uint32_t pin,
uint8_t func)
{
struct pinmux_driver_api *api;

api = (struct pinmux_driver_api *) dev->driver_api;

/* proposed enhacement */
if (!api->input) {
return DEV_NO_SUPPORT;
}

Note that on many API, some calls have a mandatory support (let's say
spi_transceive),
so such test would not have to be generalized.

What about a different approach:

Thing is, Zephyr is an OS for embedded platform thus the developer is really
close to the hw he is using.
Thus, he should know what's available in terms of drivers and, finally, what
features these drivers expose.

So, instead of multiplying such test, which will take some bytes here and
there.
We could do this test through __ASSERT(). The user, while testing its app
could enable the assert checks
and verify he is not using unsupported features. And he could fix his code
accordingly, then disable the asserts.
If I understood correctly what you're suggesting, the example code
could look like this:

static inline uint32_t pinmux_pin_input_enable(struct device *dev,
uint32_t pin,
uint8_t func)
{
struct pinmux_driver_api *api;

api = (struct pinmux_driver_api *) dev->driver_api;
__ASSERT(api->input, "%s not implemented", __func__);
return api->input(dev, pin, func);
}

Cheers,
--
Maciek Borzecki


Re: [RFC] NULL callbacks in driver API

Jesus Sanchez-Palencia <jesus.sanchez-palencia@...>
 

Hi,


On Wed, 9 Mar 2016 11:00:49 +0100
Tomasz Bursztyka <tomasz.bursztyka(a)linux.intel.com> wrote:

Hi Maciek,

An obligatory example:

<---- code --------->
struct pinmux_driver_api {
pmux_set set;
pmux_get get;
pmux_pullup pullup;
pmux_input input;
};
...

static inline uint32_t pinmux_pin_input_enable(struct device *dev,
uint32_t pin,
uint8_t func)
{
struct pinmux_driver_api *api;

api = (struct pinmux_driver_api *) dev->driver_api;

/* proposed enhacement */
if (!api->input) {
return DEV_NO_SUPPORT;
}
There was a thread before [1] about differing between an API not supported and an API not
implemented for a given driver implementation. This would be done by returning DEV_NOT_IMPLEMENTED
instead of DEV_NO_SUPPORT. DEV_NOT_IMPLEMENTED hasn't been added to device.h yet, but the idea got
a green light back then from what I can tell.

This RFC would block that, so we need to keep this in mind and re-visit that discussion.


Note that on many API, some calls have a mandatory support (let's say
spi_transceive),
so such test would not have to be generalized.

What about a different approach:

Thing is, Zephyr is an OS for embedded platform thus the developer is
really close to the hw he is using.
Thus, he should know what's available in terms of drivers and, finally,
what features these drivers expose.

So, instead of multiplying such test, which will take some bytes here
and there.
We could do this test through __ASSERT(). The user, while testing its
app could enable the assert checks
and verify he is not using unsupported features. And he could fix his
code accordingly, then disable the asserts.
+1 for this to be considered.

Cheers,
jesus

[1]
https://lists.zephyrproject.org/archives/list/devel(a)lists.zephyrproject.org/thread/ONIGOHXRQWFFEAP2UBURZV7IOSFPQH5F/#ONIGOHXRQWFFEAP2UBURZV7IOSFPQH5F


Re: [RFC] NULL callbacks in driver API

Tomasz Bursztyka
 

Hi Maciek,

An obligatory example:

<---- code --------->
struct pinmux_driver_api {
pmux_set set;
pmux_get get;
pmux_pullup pullup;
pmux_input input;
};
...

static inline uint32_t pinmux_pin_input_enable(struct device *dev,
uint32_t pin,
uint8_t func)
{
struct pinmux_driver_api *api;

api = (struct pinmux_driver_api *) dev->driver_api;

/* proposed enhacement */
if (!api->input) {
return DEV_NO_SUPPORT;
}
Note that on many API, some calls have a mandatory support (let's say
spi_transceive),
so such test would not have to be generalized.

What about a different approach:

Thing is, Zephyr is an OS for embedded platform thus the developer is
really close to the hw he is using.
Thus, he should know what's available in terms of drivers and, finally,
what features these drivers expose.

So, instead of multiplying such test, which will take some bytes here
and there.
We could do this test through __ASSERT(). The user, while testing its
app could enable the assert checks
and verify he is not using unsupported features. And he could fix his
code accordingly, then disable the asserts.

Not sure this would fit all uses case though, I might miss something.

Tomasz


[RFC] NULL callbacks in driver API

Maciek Borzecki <maciek.borzecki@...>
 

Hi,

I'd like to make a proposal for enhancement of driver API
callbacks, but before jumping to code I'd like to get some feedback first.

Right now, whenever a call to an API is made, we directly jump to call
the driver's implementation. This is ok for the general case, when we
know that every driver will implement all callbacks.

However, this approach has some downsides in my opinion and might
become an issue once the number of drivers grows or the APIs become
larger.

First of all, in practice, not every driver will be able to implement all
callbacks. It is my understanding that the API will cover only a subset
of common functionality. However, as an example, the PWM driver for
STM32 may not have an applicable method of setting phase. In such case,
the driver would provide a dummy callback returning DEV_NO_SUPPORT.

Secondly, the current approach requires that whenever an API is
augmented with a new callback, someone must track down all driver
implementations and add this dummy callback.

Once the errno patches hit master, we'll need to make sure that
dummy callbacks (if present) return proper error code.

Given that, I'm proposing to add checks whether the driver implements
given callback in the public API calls. This has some benefits. Firstly,
we'll always have a default code path. Someone working on a driver
does not have add a dummy implementation.

Secondly, this will help to enforce consistent return values for a
case when a device lacks certain feature/functionality. Lack of
callback may return DEV_NO_SUPPORT, or ENOSYS/ENOTSUP.

We can rely on static *_driver_api struct allocation, and just fill in
the blanks. This is important when adding new API calls. IIRC Linux
does that a lot.

Lastly, I believe that this will give a minimal size reduction if one
happens to use drivers with dummy callbacks.

An obligatory example:

<---- code --------->
struct pinmux_driver_api {
pmux_set set;
pmux_get get;
pmux_pullup pullup;
pmux_input input;
};
...

static inline uint32_t pinmux_pin_input_enable(struct device *dev,
uint32_t pin,
uint8_t func)
{
struct pinmux_driver_api *api;

api = (struct pinmux_driver_api *) dev->driver_api;

/* proposed enhacement */
if (!api->input) {
return DEV_NO_SUPPORT;
}

return api->input(dev, pin, func);
}
<------ code ------------->

A similar thing is also a part of clock_control API review:
https://gerrit.zephyrproject.org/r/#/c/712/

Cheers,
--
Maciek Borzecki


Re: RFC: Remove dynamic interrupts / exceptions

Boie, Andrew P
 

On Tue, 2016-03-08 at 17:01 -0500, Benjamin Walsh wrote:
Hang on on this, I'm cleaning up a GDB server implementation that we
want to upstream, and it uses at least dynamic exceptions. I'll see
if I
can refactor it to use static ones instead.
Sure, let me know.
If it ends up you need a dynamic handler for one specific exception on
x86 we could treat it as a special case. irq_offload() is like this.

Andrew


Re: RFC: Remove dynamic interrupts / exceptions

Benjamin Walsh <benjamin.walsh@...>
 

Hang on on this, I'm cleaning up a GDB server implementation that we
want to upstream, and it uses at least dynamic exceptions. I'll see if I
can refactor it to use static ones instead.

On Tue, Mar 08, 2016 at 09:48:36PM +0000, Boie, Andrew P wrote:
Problem statement:
We currently have two separate implementations on how to connect
interrupts on all arches: static IRQs and dynamic IRQs. On x86 only we
also have static and dynamic APIs for connecting exception handlers.
Although static interrupts & exceptions impose some constraints on
developers, they save a nontrivial amount of RAM (generally 500b to 1K
depending on configuration) as certain data structures like the IDT do
not need to be writable and can remain in ROM. In the interest of
keeping the Zephyr codebase small and not having to maintain two
different ways of doing the same thing, I propose we deprecate and
eventually remove the dynamic implementations.

Details are below. In the discussion I only talk about interrupts, but
for x86 dynamic vs. static exceptions the same arguments apply. On ARC
and ARM all the exceptions are hard-coded to their handlers and there
is no API for them.

The static IRQ macro IRQ_CONNECT() imposes the constraint that all its
arguments be constants resolvable at build time. This includes the IRQ
line number and the context data pointer. This makes things a little
clumsy if you have a single driver that has multiple instances on the
system. Since you need to use build-time constants you cannot do
something like this:

int mydriver_init(struct device *dev)
{
    struct mydriver_config *config = dev->config->config_info;
    ...
    IRQ_CONNECT(config->irq, config->irq_prio, mydriver_isr, dev,      
                config->irq_flags);
    irq_enable(config->irq);
    ...
}

Instead, the idiom used in Zephyr drivers is to define a per-instance
config function and save a pointer to it in the config struct:

void mydriver_0_config_fn(void)
{
    IRQ_CONNECT(MYDRIVER_0_IRQ, MYDRIVER_0_PRI, mydriver_isr,
                DEVICE_GET(mydriver_0), MYDRIVER_0_FLAGS);
    irq_enable(MYDRIVER_0_IRQ);
}

struct mydriver_config mydriver_0_config {
    ...
    .config_func = mydriver_0_config_fn;
    ...
};

int mydriver_init(struct device *dev)
{
    struct mydriver_config
*config = dev->config->config_info;
    ...
    config->config_func();
   
 ...
}

IRQ_CONNECT is not actually a function; it does have a runtime
component (to program the interrupt controller) but it does linker or
assembly black magic to set up all the relevant data structures at
build time.

What about instances where the interrupt arguments are simply not known
at build time? I feel that for Zephyr this will never be a problem
given the classes of devices we need to support:

- PCI devices do announce what their interrupts are, but for
microcontrollers with PCI busses (like galileo) we already know this
information at build time anyway. Once you get to the point where the
hardware really does have hot-pluggable peripherals you might as well
run Linux on it; adding proper hotplug support to Zephyr is a much
larger thing than just having dynamic interrupts.

- Microkernel Task IRQs currently rely on dynamic interrupts, but this
is an implementation detail. We are considering removing these from
Zephyr as well, but if not, it's possible to refactor them to use
static.

- None of the drivers included in the Zephyr tree use dynamic
interrupts.

- A few of the footprint tests use dynamic interrupts, but again this
is an implementation detail.

The RAM savings come from a few areas which differ per arch.

x86:
- the IDT can remain in ROM. This is where most of the savings come
from.
- The _interrupt_vectors_allocated and dyn_irq_list data structures can
be omitted from RAM if no dynamic interrupts are used.
- The IRQ_CONNECT() macro as part of its job automatically generates
the assembly language stub. These stubs trampoline to common code which
fetches the context pointer associated with the interrupt; this is how
our ISRs can be passed parameters. For dynamic interrupts you need to
know how many dynamic IRQs you plan on using in advance and set
CONFIG_NUM_DYNAMIC_STUBS; if the number of calls to
irq_connect_dynamic() exceeds this you will crash.

ARC/ARM:
On these two arches both static and dynamic interrupts are tracked in
sw_isr_table, an array indexed by IRQ line with each element noting the
ISR and parameter for that IRQ. If no dynamic IRQs are used, this table
can remain in ROM.

In summary, I'm not currently seeing any scenarios where dynamic IRQs
are necessary, and would like to attach __attribute__((deprecated))
tags to their APIs and eventually remove them from the kernel.

Andrew


RFC: Remove dynamic interrupts / exceptions

Boie, Andrew P
 

Problem statement:
We currently have two separate implementations on how to connect
interrupts on all arches: static IRQs and dynamic IRQs. On x86 only we
also have static and dynamic APIs for connecting exception handlers.
Although static interrupts & exceptions impose some constraints on
developers, they save a nontrivial amount of RAM (generally 500b to 1K
depending on configuration) as certain data structures like the IDT do
not need to be writable and can remain in ROM. In the interest of
keeping the Zephyr codebase small and not having to maintain two
different ways of doing the same thing, I propose we deprecate and
eventually remove the dynamic implementations.

Details are below. In the discussion I only talk about interrupts, but
for x86 dynamic vs. static exceptions the same arguments apply. On ARC
and ARM all the exceptions are hard-coded to their handlers and there
is no API for them.

The static IRQ macro IRQ_CONNECT() imposes the constraint that all its
arguments be constants resolvable at build time. This includes the IRQ
line number and the context data pointer. This makes things a little
clumsy if you have a single driver that has multiple instances on the
system. Since you need to use build-time constants you cannot do
something like this:

int mydriver_init(struct device *dev)
{
    struct mydriver_config *config = dev->config->config_info;
    ...
    IRQ_CONNECT(config->irq, config->irq_prio, mydriver_isr, dev,      
                config->irq_flags);
    irq_enable(config->irq);
    ...
}

Instead, the idiom used in Zephyr drivers is to define a per-instance
config function and save a pointer to it in the config struct:

void mydriver_0_config_fn(void)
{
    IRQ_CONNECT(MYDRIVER_0_IRQ, MYDRIVER_0_PRI, mydriver_isr,
                DEVICE_GET(mydriver_0), MYDRIVER_0_FLAGS);
    irq_enable(MYDRIVER_0_IRQ);
}

struct mydriver_config mydriver_0_config {
    ...
    .config_func = mydriver_0_config_fn;
    ...
};

int mydriver_init(struct device *dev)
{
    struct mydriver_config
*config = dev->config->config_info;
    ...
    config->config_func();
   
 ...
}

IRQ_CONNECT is not actually a function; it does have a runtime
component (to program the interrupt controller) but it does linker or
assembly black magic to set up all the relevant data structures at
build time.

What about instances where the interrupt arguments are simply not known
at build time? I feel that for Zephyr this will never be a problem
given the classes of devices we need to support:

- PCI devices do announce what their interrupts are, but for
microcontrollers with PCI busses (like galileo) we already know this
information at build time anyway. Once you get to the point where the
hardware really does have hot-pluggable peripherals you might as well
run Linux on it; adding proper hotplug support to Zephyr is a much
larger thing than just having dynamic interrupts.

- Microkernel Task IRQs currently rely on dynamic interrupts, but this
is an implementation detail. We are considering removing these from
Zephyr as well, but if not, it's possible to refactor them to use
static.

- None of the drivers included in the Zephyr tree use dynamic
interrupts.

- A few of the footprint tests use dynamic interrupts, but again this
is an implementation detail.

The RAM savings come from a few areas which differ per arch.

x86:
- the IDT can remain in ROM. This is where most of the savings come
from.
- The _interrupt_vectors_allocated and dyn_irq_list data structures can
be omitted from RAM if no dynamic interrupts are used.
- The IRQ_CONNECT() macro as part of its job automatically generates
the assembly language stub. These stubs trampoline to common code which
fetches the context pointer associated with the interrupt; this is how
our ISRs can be passed parameters. For dynamic interrupts you need to
know how many dynamic IRQs you plan on using in advance and set
CONFIG_NUM_DYNAMIC_STUBS; if the number of calls to
irq_connect_dynamic() exceeds this you will crash.

ARC/ARM:
On these two arches both static and dynamic interrupts are tracked in
sw_isr_table, an array indexed by IRQ line with each element noting the
ISR and parameter for that IRQ. If no dynamic IRQs are used, this table
can remain in ROM.

In summary, I'm not currently seeing any scenarios where dynamic IRQs
are necessary, and would like to attach __attribute__((deprecated))
tags to their APIs and eventually remove them from the kernel.

Andrew


Re: SPI driver usage

Tomasz Bursztyka
 

Hi Corey,

You'll have to wait k64f spi driver gets into the tree first, unless you
want to try under review patch.

You won't have to tell anything about any pins. The driver manages all
that for you, hopefully.
You don't have to use the driver itself. You'll just have to grab the
right instance via device_get_binding()
and use it through the API described in include/spi.h

Tomasz

Hi,

Im looking to use the zephyr project spi driver for writing to the SD
card slot on the FRDM K64f board. So far I have found little to no
documentation on how to use the spi driver. For example how do I tell
it which pins to use for the MISO and MOSI and so forth?

Thanks for any help,

Corey


Re: [RFC] GPIO API changes

Johan Hedberg
 

Hi,

On Mon, Mar 07, 2016, Daniel Leung wrote:
Another thing is: it seems like that you expect the app developers
to statically allocate a bunch of this struct to have multiple callbacks.
This is, AFAIK, not a common practice when setting callbacks. Developers
may simply allocate a struct in stack (like inside a function) and
pass it to the function. This struct may go out of scope, and
the resulting errors and exceptions will be confusing to developers.
Could we do something to mitigate this?
I think the best we can do is to clearly document the life-time
expectation of the struct. I don't think there's really any other way
around it with asynchronous APIs that need to track more context than
just a simple function pointer. We've actually got several similar
approaches in the Bluetooth API as well.

Johan


SPI driver usage

Corey Williamson <corey.bailey.williamson@...>
 

Hi,

Im looking to use the zephyr project spi driver for writing to the SD card
slot on the FRDM K64f board. So far I have found little to no
documentation on how to use the spi driver. For example how do I tell it
which pins to use for the MISO and MOSI and so forth?

Thanks for any help,

Corey


Re: [RFC] GPIO API changes

Daniel Leung <daniel.leung@...>
 

On Mon, Mar 07, 2016 at 06:07:07PM +0200, Johan Hedberg wrote:
Hi Tomasz,

On Mon, Mar 07, 2016, Tomasz Bursztyka wrote:
-typedef void (*gpio_callback_t)(struct device *port, uint32_t pin);
+
+struct gpio_callback
+{
+ void (*handler)(struct device *port,
+ gpio_callback_t *cb,
+ uint32_t pins);
+ uint32_t pin_mask;
+
+ struct gpio_callback *_next;
+};
+
+typedef struct gpio_callback gpio_callback_t;
I realize this typedef is inherited from the original code, but do we
really need/want to enforce an opaque type here? The general preference
with the coding style (inherited from Linux and checkpatch even
complains about it) is to avoid typedefs whenever possible. I could e.g.
imagine a handler function wanting set/unset some pins in the pin_mask
when it gets called, in which case the struct couldn't be considered
completely opaque.
Another thing is: it seems like that you expect the app developers
to statically allocate a bunch of this struct to have multiple callbacks.
This is, AFAIK, not a common practice when setting callbacks. Developers
may simply allocate a struct in stack (like inside a function) and
pass it to the function. This struct may go out of scope, and
the resulting errors and exceptions will be confusing to developers.
Could we do something to mitigate this?


----------
Daniel Leung


Re: RFC: Counter driver API

Tseng, Kuo-Lang <kuo-lang.tseng@...>
 

-----Original Message-----
From: Kalowsky, Daniel
Sent: Monday, March 07, 2016 2:20 PM
To: Tseng, Kuo-Lang <kuo-lang.tseng(a)intel.com>; Guedes, Andre
<andre.guedes(a)intel.com>; Sanchez-Palencia, Jesus <jesus.sanchez-
palencia(a)intel.com>; Tomasz Bursztyka <tomasz.bursztyka(a)linux.intel.com>
Cc: devel(a)lists.zephyrproject.org
Subject: RE: [devel] Re: Re: Re: Re: Re: Re: RFC: Counter driver API

-----Original Message-----
From: Tseng, Kuo-Lang
Sent: Monday, March 7, 2016 1:32 PM
To: Kalowsky, Daniel <daniel.kalowsky(a)intel.com>; Guedes, Andre
<andre.guedes(a)intel.com>; Sanchez-Palencia, Jesus <jesus.sanchez-
palencia(a)intel.com>; Tomasz Bursztyka
<tomasz.bursztyka(a)linux.intel.com>
Cc: devel(a)lists.zephyrproject.org
Subject: RE: [devel] Re: Re: Re: Re: Re: Re: RFC: Counter driver API

Sure. Below are a summary of the API and changes. Please let me know
if anything else needs to be mentioned and I can add.
Thanks.

Did the decision on where this will live get decided as well? I don't see that in the
summary (unless that is implied by the counter driver framework statement below).
No, there was not any comment on the proposed location where this will live, i.e. /include/counter.h for the generic API and /drivers/counter directory for the drivers. The summary only consolidates the generic API set from feedbacks.

So if anyone has comment for the location, please do send and we can discuss and I can incorporate. For now since no comment around that, the generic API will be in include/counter.h and driver will be in drivers/counter.



The generic counter API will support 4 functions as summarized below.
Based on this, the change includes implementation of the following 3 parts:

1) A generic counter API - this implements the counter.h in a counter
driver framework.
2) Quark-specific counter drivers - implements the counter API for
AON counter and AON timer devices in Quark.
3) A sample application that demonstrates the use of the generic
counter API for counter usages.

The old patch (https://gerrit.zephyrproject.org/r/#/c/474/) will be
updated based on above three parts.

The generic counter API that has feedback incorporated:

/**
* Start the counter device. If the device is a 'count up' counter the
* counter initial value is set to zero. If it is a 'countdown'
counter
* the initial value is set to the maximum value supported by the device.
*
* @brief Start counter device in free running mode.
* @param dev Pointer to the device structure for the driver instance.
* @retval DEV_OK If successful.
* @retval DEV_* Code otherwise.
*/
int counter_start(struct device *dev);

/**
* @brief Stop counter device. If alarm was set, this function also clears
* the alarm setting.
* @param dev Pointer to the device structure for the driver instance.
*
* @retval DEV_OK If successful.
* @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.
*
* @brief Set an alarm.
* @param dev Pointer to the device structure for the driver instance.
* @param callback Pointer to the callback function. If this is NULL,
this function
* unsets the alarm.
* @param count Number of counter ticks.
* @param user_data pointer to user data
*
* @retval DEV_OK If successful.
* @retval DEV_INVALID_OP If the counter was not started yet.
* @retval DEV_NO_SUPPORT if the device doesn't support interrupt (e.g.
* free running counters).
*/
int counter_set_alarm(struct device *dev, counter_callback_t callback,
uint32_t count, void *user_data);

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



-----Original Message-----
From: Kalowsky, Daniel
Sent: Monday, March 07, 2016 12:58 PM
To: Tseng, Kuo-Lang <kuo-lang.tseng(a)intel.com>; Tseng, Kuo-Lang
<kuo- lang.tseng(a)intel.com>; Guedes, Andre <andre.guedes(a)intel.com>;
Sanchez-
Palencia, Jesus <jesus.sanchez-palencia(a)intel.com>; Tomasz Bursztyka
<tomasz.bursztyka(a)linux.intel.com>
Cc: devel(a)lists.zephyrproject.org
Subject: RE: [devel] Re: Re: Re: Re: Re: Re: RFC: Counter driver API

Before starting, send out a summary of what you're going to change
as a
final RFC
please.

-----Original Message-----
From: Tseng, Kuo-Lang [mailto:kuo-lang.tseng(a)intel.com]
Sent: Monday, March 7, 2016 12:13 PM
To: Tseng, Kuo-Lang <kuo-lang.tseng(a)intel.com>; Guedes, Andre
<andre.guedes(a)intel.com>; Sanchez-Palencia, Jesus <jesus.sanchez-
palencia(a)intel.com>; Tomasz Bursztyka
<tomasz.bursztyka(a)linux.intel.com>
Cc: devel(a)lists.zephyrproject.org
Subject: [devel] Re: Re: Re: Re: Re: Re: RFC: Counter driver API

Since this RFC has been quietly for a while and it seems we have
reached a good amount of feedback so we will implement it and
update the current patch.

-----Original Message-----
From: Tseng, Kuo-Lang [mailto:kuo-lang.tseng(a)intel.com]
Sent: Thursday, March 03, 2016 5:45 PM
To: Guedes, Andre <andre.guedes(a)intel.com>; Sanchez-Palencia,
Jesus
<jesus.sanchez-palencia(a)intel.com>; Tomasz Bursztyka
<tomasz.bursztyka(a)linux.intel.com>
Cc: devel(a)lists.zephyrproject.org
Subject: [devel] Re: Re: Re: Re: Re: RFC: Counter driver API

Hi Andre, Tomasz, Jesus,

Thanks for your feedbacks. I updated the API with these feedbacks.
Please
correct
or if I missed any part that needed to be reflected. The updated
API looks
like
below:

/**
* Start the counter device. If the device is a 'count up'
counter the
* counter initial value is set to zero. If it is a 'countdown'
counter
* the initial value is set to the maximum value supported by the device.
*
* @brief Start counter device in free running mode.
* @param dev Pointer to the device structure for the driver instance.
* @retval DEV_OK If successful.
* @retval DEV_* Code otherwise.
*/
int counter_start(struct device *dev);

/**
* @brief Stop counter device. If alarm was set, this function also clears
* the alarm setting.
* @param dev Pointer to the device structure for the driver instance.
*
* @retval DEV_OK If successful.
* @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.
*
* @brief Set an alarm.
* @param dev Pointer to the device structure for the driver instance.
* @param callback Pointer to the callback function. If this is
NULL, this
function
* unsets the alarm.
* @param count Number of counter ticks.
* @param user_data pointer to user data
*
* @retval DEV_OK If successful.
* @retval DEV_INVALID_OP If the counter was not started yet.
* @retval DEV_NO_SUPPORT if the device doesn't support
interrupt
(e.g.
* free running counters).
*/
int counter_set_alarm(struct device *dev, counter_callback_t
callback,
uint32_t
count, void *user_data);

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



-----Original Message-----
From: Andre Guedes [mailto:andre.guedes(a)intel.com]
Sent: Thursday, March 03, 2016 6:22 AM
To: Sanchez-Palencia, Jesus
<jesus.sanchez-palencia(a)intel.com>;
Tomasz
Bursztyka <tomasz.bursztyka(a)linux.intel.com>
Cc: devel(a)lists.zephyrproject.org
Subject: [devel] Re: Re: Re: Re: RFC: Counter driver API

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


Re: RFC: Counter driver API

Kalowsky, Daniel <daniel.kalowsky@...>
 

-----Original Message-----
From: Tseng, Kuo-Lang
Sent: Monday, March 7, 2016 1:32 PM
To: Kalowsky, Daniel <daniel.kalowsky(a)intel.com>; Guedes, Andre
<andre.guedes(a)intel.com>; Sanchez-Palencia, Jesus <jesus.sanchez-
palencia(a)intel.com>; Tomasz Bursztyka
<tomasz.bursztyka(a)linux.intel.com>
Cc: devel(a)lists.zephyrproject.org
Subject: RE: [devel] Re: Re: Re: Re: Re: Re: RFC: Counter driver API

Sure. Below are a summary of the API and changes. Please let me know if
anything else needs to be mentioned and I can add.
Thanks.

Did the decision on where this will live get decided as well? I don't see that in the summary (unless that is implied by the counter driver framework statement below).


The generic counter API will support 4 functions as summarized below. Based
on this, the change includes implementation of the following 3 parts:

1) A generic counter API - this implements the counter.h in a counter driver
framework.
2) Quark-specific counter drivers - implements the counter API for AON
counter and AON timer devices in Quark.
3) A sample application that demonstrates the use of the generic counter API
for counter usages.

The old patch (https://gerrit.zephyrproject.org/r/#/c/474/) will be updated
based on above three parts.

The generic counter API that has feedback incorporated:

/**
* Start the counter device. If the device is a 'count up' counter the
* counter initial value is set to zero. If it is a 'countdown' counter
* the initial value is set to the maximum value supported by the device.
*
* @brief Start counter device in free running mode.
* @param dev Pointer to the device structure for the driver instance.
* @retval DEV_OK If successful.
* @retval DEV_* Code otherwise.
*/
int counter_start(struct device *dev);

/**
* @brief Stop counter device. If alarm was set, this function also clears
* the alarm setting.
* @param dev Pointer to the device structure for the driver instance.
*
* @retval DEV_OK If successful.
* @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.
*
* @brief Set an alarm.
* @param dev Pointer to the device structure for the driver instance.
* @param callback Pointer to the callback function. If this is NULL, this
function
* unsets the alarm.
* @param count Number of counter ticks.
* @param user_data pointer to user data
*
* @retval DEV_OK If successful.
* @retval DEV_INVALID_OP If the counter was not started yet.
* @retval DEV_NO_SUPPORT if the device doesn't support interrupt (e.g.
* free running counters).
*/
int counter_set_alarm(struct device *dev, counter_callback_t callback,
uint32_t count, void *user_data);

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



-----Original Message-----
From: Kalowsky, Daniel
Sent: Monday, March 07, 2016 12:58 PM
To: Tseng, Kuo-Lang <kuo-lang.tseng(a)intel.com>; Tseng, Kuo-Lang <kuo-
lang.tseng(a)intel.com>; Guedes, Andre <andre.guedes(a)intel.com>;
Sanchez-
Palencia, Jesus <jesus.sanchez-palencia(a)intel.com>; Tomasz Bursztyka
<tomasz.bursztyka(a)linux.intel.com>
Cc: devel(a)lists.zephyrproject.org
Subject: RE: [devel] Re: Re: Re: Re: Re: Re: RFC: Counter driver API

Before starting, send out a summary of what you're going to change as a
final RFC
please.

-----Original Message-----
From: Tseng, Kuo-Lang [mailto:kuo-lang.tseng(a)intel.com]
Sent: Monday, March 7, 2016 12:13 PM
To: Tseng, Kuo-Lang <kuo-lang.tseng(a)intel.com>; Guedes, Andre
<andre.guedes(a)intel.com>; Sanchez-Palencia, Jesus <jesus.sanchez-
palencia(a)intel.com>; Tomasz Bursztyka
<tomasz.bursztyka(a)linux.intel.com>
Cc: devel(a)lists.zephyrproject.org
Subject: [devel] Re: Re: Re: Re: Re: Re: RFC: Counter driver API

Since this RFC has been quietly for a while and it seems we have
reached a good amount of feedback so we will implement it and update
the current patch.

-----Original Message-----
From: Tseng, Kuo-Lang [mailto:kuo-lang.tseng(a)intel.com]
Sent: Thursday, March 03, 2016 5:45 PM
To: Guedes, Andre <andre.guedes(a)intel.com>; Sanchez-Palencia,
Jesus
<jesus.sanchez-palencia(a)intel.com>; Tomasz Bursztyka
<tomasz.bursztyka(a)linux.intel.com>
Cc: devel(a)lists.zephyrproject.org
Subject: [devel] Re: Re: Re: Re: Re: RFC: Counter driver API

Hi Andre, Tomasz, Jesus,

Thanks for your feedbacks. I updated the API with these feedbacks.
Please
correct
or if I missed any part that needed to be reflected. The updated API
looks
like
below:

/**
* Start the counter device. If the device is a 'count up' counter
the
* counter initial value is set to zero. If it is a 'countdown'
counter
* the initial value is set to the maximum value supported by the device.
*
* @brief Start counter device in free running mode.
* @param dev Pointer to the device structure for the driver instance.
* @retval DEV_OK If successful.
* @retval DEV_* Code otherwise.
*/
int counter_start(struct device *dev);

/**
* @brief Stop counter device. If alarm was set, this function also clears
* the alarm setting.
* @param dev Pointer to the device structure for the driver instance.
*
* @retval DEV_OK If successful.
* @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.
*
* @brief Set an alarm.
* @param dev Pointer to the device structure for the driver instance.
* @param callback Pointer to the callback function. If this is
NULL, this
function
* unsets the alarm.
* @param count Number of counter ticks.
* @param user_data pointer to user data
*
* @retval DEV_OK If successful.
* @retval DEV_INVALID_OP If the counter was not started yet.
* @retval DEV_NO_SUPPORT if the device doesn't support interrupt
(e.g.
* free running counters).
*/
int counter_set_alarm(struct device *dev, counter_callback_t
callback,
uint32_t
count, void *user_data);

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



-----Original Message-----
From: Andre Guedes [mailto:andre.guedes(a)intel.com]
Sent: Thursday, March 03, 2016 6:22 AM
To: Sanchez-Palencia, Jesus <jesus.sanchez-palencia(a)intel.com>;
Tomasz
Bursztyka <tomasz.bursztyka(a)linux.intel.com>
Cc: devel(a)lists.zephyrproject.org
Subject: [devel] Re: Re: Re: Re: RFC: Counter driver API

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


Re: RFC: Counter driver API

Tseng, Kuo-Lang <kuo-lang.tseng@...>
 

Sure. Below are a summary of the API and changes. Please let me know if anything else needs to be mentioned and I can add.

The generic counter API will support 4 functions as summarized below. Based on this, the change includes implementation of the following 3 parts:

1) A generic counter API - this implements the counter.h in a counter driver framework.
2) Quark-specific counter drivers - implements the counter API for AON counter and AON timer devices in Quark.
3) A sample application that demonstrates the use of the generic counter API for counter usages.

The old patch (https://gerrit.zephyrproject.org/r/#/c/474/) will be updated based on above three parts.

The generic counter API that has feedback incorporated:

/**
* Start the counter device. If the device is a 'count up' counter the
* counter initial value is set to zero. If it is a 'countdown' counter
* the initial value is set to the maximum value supported by the device.
*
* @brief Start counter device in free running mode.
* @param dev Pointer to the device structure for the driver instance.
* @retval DEV_OK If successful.
* @retval DEV_* Code otherwise.
*/
int counter_start(struct device *dev);

/**
* @brief Stop counter device. If alarm was set, this function also clears
* the alarm setting.
* @param dev Pointer to the device structure for the driver instance.
*
* @retval DEV_OK If successful.
* @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.
*
* @brief Set an alarm.
* @param dev Pointer to the device structure for the driver instance.
* @param callback Pointer to the callback function. If this is NULL, this function
* unsets the alarm.
* @param count Number of counter ticks.
* @param user_data pointer to user data
*
* @retval DEV_OK If successful.
* @retval DEV_INVALID_OP If the counter was not started yet.
* @retval DEV_NO_SUPPORT if the device doesn't support interrupt (e.g.
* free running counters).
*/
int counter_set_alarm(struct device *dev, counter_callback_t callback, uint32_t count, void *user_data);

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

-----Original Message-----
From: Kalowsky, Daniel
Sent: Monday, March 07, 2016 12:58 PM
To: Tseng, Kuo-Lang <kuo-lang.tseng(a)intel.com>; Tseng, Kuo-Lang <kuo-
lang.tseng(a)intel.com>; Guedes, Andre <andre.guedes(a)intel.com>; Sanchez-
Palencia, Jesus <jesus.sanchez-palencia(a)intel.com>; Tomasz Bursztyka
<tomasz.bursztyka(a)linux.intel.com>
Cc: devel(a)lists.zephyrproject.org
Subject: RE: [devel] Re: Re: Re: Re: Re: Re: RFC: Counter driver API

Before starting, send out a summary of what you're going to change as a final RFC
please.

-----Original Message-----
From: Tseng, Kuo-Lang [mailto:kuo-lang.tseng(a)intel.com]
Sent: Monday, March 7, 2016 12:13 PM
To: Tseng, Kuo-Lang <kuo-lang.tseng(a)intel.com>; Guedes, Andre
<andre.guedes(a)intel.com>; Sanchez-Palencia, Jesus <jesus.sanchez-
palencia(a)intel.com>; Tomasz Bursztyka
<tomasz.bursztyka(a)linux.intel.com>
Cc: devel(a)lists.zephyrproject.org
Subject: [devel] Re: Re: Re: Re: Re: Re: RFC: Counter driver API

Since this RFC has been quietly for a while and it seems we have
reached a good amount of feedback so we will implement it and update
the current patch.

-----Original Message-----
From: Tseng, Kuo-Lang [mailto:kuo-lang.tseng(a)intel.com]
Sent: Thursday, March 03, 2016 5:45 PM
To: Guedes, Andre <andre.guedes(a)intel.com>; Sanchez-Palencia, Jesus
<jesus.sanchez-palencia(a)intel.com>; Tomasz Bursztyka
<tomasz.bursztyka(a)linux.intel.com>
Cc: devel(a)lists.zephyrproject.org
Subject: [devel] Re: Re: Re: Re: Re: RFC: Counter driver API

Hi Andre, Tomasz, Jesus,

Thanks for your feedbacks. I updated the API with these feedbacks.
Please
correct
or if I missed any part that needed to be reflected. The updated API
looks
like
below:

/**
* Start the counter device. If the device is a 'count up' counter
the
* counter initial value is set to zero. If it is a 'countdown'
counter
* the initial value is set to the maximum value supported by the device.
*
* @brief Start counter device in free running mode.
* @param dev Pointer to the device structure for the driver instance.
* @retval DEV_OK If successful.
* @retval DEV_* Code otherwise.
*/
int counter_start(struct device *dev);

/**
* @brief Stop counter device. If alarm was set, this function also clears
* the alarm setting.
* @param dev Pointer to the device structure for the driver instance.
*
* @retval DEV_OK If successful.
* @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.
*
* @brief Set an alarm.
* @param dev Pointer to the device structure for the driver instance.
* @param callback Pointer to the callback function. If this is
NULL, this
function
* unsets the alarm.
* @param count Number of counter ticks.
* @param user_data pointer to user data
*
* @retval DEV_OK If successful.
* @retval DEV_INVALID_OP If the counter was not started yet.
* @retval DEV_NO_SUPPORT if the device doesn't support interrupt (e.g.
* free running counters).
*/
int counter_set_alarm(struct device *dev, counter_callback_t
callback,
uint32_t
count, void *user_data);

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



-----Original Message-----
From: Andre Guedes [mailto:andre.guedes(a)intel.com]
Sent: Thursday, March 03, 2016 6:22 AM
To: Sanchez-Palencia, Jesus <jesus.sanchez-palencia(a)intel.com>;
Tomasz
Bursztyka <tomasz.bursztyka(a)linux.intel.com>
Cc: devel(a)lists.zephyrproject.org
Subject: [devel] Re: Re: Re: Re: RFC: Counter driver API

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


Re: RFC: Counter driver API

Kalowsky, Daniel <daniel.kalowsky@...>
 

Before starting, send out a summary of what you're going to change as a final RFC please.

-----Original Message-----
From: Tseng, Kuo-Lang [mailto:kuo-lang.tseng(a)intel.com]
Sent: Monday, March 7, 2016 12:13 PM
To: Tseng, Kuo-Lang <kuo-lang.tseng(a)intel.com>; Guedes, Andre
<andre.guedes(a)intel.com>; Sanchez-Palencia, Jesus <jesus.sanchez-
palencia(a)intel.com>; Tomasz Bursztyka
<tomasz.bursztyka(a)linux.intel.com>
Cc: devel(a)lists.zephyrproject.org
Subject: [devel] Re: Re: Re: Re: Re: Re: RFC: Counter driver API

Since this RFC has been quietly for a while and it seems we have reached
a good amount of feedback so we will implement it and update the current
patch.

-----Original Message-----
From: Tseng, Kuo-Lang [mailto:kuo-lang.tseng(a)intel.com]
Sent: Thursday, March 03, 2016 5:45 PM
To: Guedes, Andre <andre.guedes(a)intel.com>; Sanchez-Palencia, Jesus
<jesus.sanchez-palencia(a)intel.com>; Tomasz Bursztyka
<tomasz.bursztyka(a)linux.intel.com>
Cc: devel(a)lists.zephyrproject.org
Subject: [devel] Re: Re: Re: Re: Re: RFC: Counter driver API

Hi Andre, Tomasz, Jesus,

Thanks for your feedbacks. I updated the API with these feedbacks. Please
correct
or if I missed any part that needed to be reflected. The updated API looks
like
below:

/**
* Start the counter device. If the device is a 'count up' counter the
* counter initial value is set to zero. If it is a 'countdown' counter
* the initial value is set to the maximum value supported by the device.
*
* @brief Start counter device in free running mode.
* @param dev Pointer to the device structure for the driver instance.
* @retval DEV_OK If successful.
* @retval DEV_* Code otherwise.
*/
int counter_start(struct device *dev);

/**
* @brief Stop counter device. If alarm was set, this function also clears
* the alarm setting.
* @param dev Pointer to the device structure for the driver instance.
*
* @retval DEV_OK If successful.
* @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.
*
* @brief Set an alarm.
* @param dev Pointer to the device structure for the driver instance.
* @param callback Pointer to the callback function. If this is NULL, this
function
* unsets the alarm.
* @param count Number of counter ticks.
* @param user_data pointer to user data
*
* @retval DEV_OK If successful.
* @retval DEV_INVALID_OP If the counter was not started yet.
* @retval DEV_NO_SUPPORT if the device doesn't support interrupt (e.g.
* free running counters).
*/
int counter_set_alarm(struct device *dev, counter_callback_t callback,
uint32_t
count, void *user_data);

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



-----Original Message-----
From: Andre Guedes [mailto:andre.guedes(a)intel.com]
Sent: Thursday, March 03, 2016 6:22 AM
To: Sanchez-Palencia, Jesus <jesus.sanchez-palencia(a)intel.com>;
Tomasz
Bursztyka <tomasz.bursztyka(a)linux.intel.com>
Cc: devel(a)lists.zephyrproject.org
Subject: [devel] Re: Re: Re: Re: RFC: Counter driver API

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


Re: RFC: Counter driver API

Tseng, Kuo-Lang <kuo-lang.tseng@...>
 

Since this RFC has been quietly for a while and it seems we have reached
a good amount of feedback so we will implement it and update the current
patch.

-----Original Message-----
From: Tseng, Kuo-Lang [mailto:kuo-lang.tseng(a)intel.com]
Sent: Thursday, March 03, 2016 5:45 PM
To: Guedes, Andre <andre.guedes(a)intel.com>; Sanchez-Palencia, Jesus
<jesus.sanchez-palencia(a)intel.com>; Tomasz Bursztyka
<tomasz.bursztyka(a)linux.intel.com>
Cc: devel(a)lists.zephyrproject.org
Subject: [devel] Re: Re: Re: Re: Re: RFC: Counter driver API

Hi Andre, Tomasz, Jesus,

Thanks for your feedbacks. I updated the API with these feedbacks. Please correct
or if I missed any part that needed to be reflected. The updated API looks like
below:

/**
* Start the counter device. If the device is a 'count up' counter the
* counter initial value is set to zero. If it is a 'countdown' counter
* the initial value is set to the maximum value supported by the device.
*
* @brief Start counter device in free running mode.
* @param dev Pointer to the device structure for the driver instance.
* @retval DEV_OK If successful.
* @retval DEV_* Code otherwise.
*/
int counter_start(struct device *dev);

/**
* @brief Stop counter device. If alarm was set, this function also clears
* the alarm setting.
* @param dev Pointer to the device structure for the driver instance.
*
* @retval DEV_OK If successful.
* @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.
*
* @brief Set an alarm.
* @param dev Pointer to the device structure for the driver instance.
* @param callback Pointer to the callback function. If this is NULL, this function
* unsets the alarm.
* @param count Number of counter ticks.
* @param user_data pointer to user data
*
* @retval DEV_OK If successful.
* @retval DEV_INVALID_OP If the counter was not started yet.
* @retval DEV_NO_SUPPORT if the device doesn't support interrupt (e.g.
* free running counters).
*/
int counter_set_alarm(struct device *dev, counter_callback_t callback, uint32_t
count, void *user_data);

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



-----Original Message-----
From: Andre Guedes [mailto:andre.guedes(a)intel.com]
Sent: Thursday, March 03, 2016 6:22 AM
To: Sanchez-Palencia, Jesus <jesus.sanchez-palencia(a)intel.com>; Tomasz
Bursztyka <tomasz.bursztyka(a)linux.intel.com>
Cc: devel(a)lists.zephyrproject.org
Subject: [devel] Re: Re: Re: Re: RFC: Counter driver API

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


Re: [RFC] GPIO API changes

Johan Hedberg
 

Hi Tomasz,

On Mon, Mar 07, 2016, Tomasz Bursztyka wrote:
-typedef void (*gpio_callback_t)(struct device *port, uint32_t pin);
+
+struct gpio_callback
+{
+ void (*handler)(struct device *port,
+ gpio_callback_t *cb,
+ uint32_t pins);
+ uint32_t pin_mask;
+
+ struct gpio_callback *_next;
+};
+
+typedef struct gpio_callback gpio_callback_t;
I realize this typedef is inherited from the original code, but do we
really need/want to enforce an opaque type here? The general preference
with the coding style (inherited from Linux and checkpatch even
complains about it) is to avoid typedefs whenever possible. I could e.g.
imagine a handler function wanting set/unset some pins in the pin_mask
when it gets called, in which case the struct couldn't be considered
completely opaque.

Johan


Re: [RFC] GPIO API changes

Vinicius Costa Gomes
 

Tomasz Bursztyka <tomasz.bursztyka(a)linux.intel.com> writes:

Hi Vinicius,

Another issue of the current API is the confusion caused by
'gpio_port_enable_callback()' and 'gpio_pin_enable_callback()'.

With the changes proposed later in this thread, you could have a
unified call:
'gpio_enable_callback(struct device *port, uint32_t pinmask)' (or something like it)
gpio_port_callback() make the callback called, whatever pins is
triggering the interrupt and enabled or not (callback wise).
So they are different (documentation could be better though)
I am just wondering that in the "old" API '_port_enable_callback()' was
a way to have the callback called with the pins expressed in a bitmask,
now the same behaviour can be achieved by running
'_pin_enable_callback(port, 0xffffffff)'. Just saying that, with the new
API, '_port_enable_callback()' adds little value.


gpio_dw.c implementation is awkward however: I don't think it should by
default enable the int on pin 0.

Tomasz

Cheers,
--
Vinicius


Re: [RFC] GPIO API changes

Tomasz Bursztyka
 

Hi Iván,


Another issue of the current API is the confusion caused by
'gpio_port_enable_callback()' and 'gpio_pin_enable_callback()'.

With the changes proposed later in this thread, you could have a
unified call:
'gpio_enable_callback(struct device *port, uint32_t pinmask)' (or something like it)
Consider there's a set/unset function in those changes, do we need to
enable? Can't we infer from the callbacks the user sets?
It's 2 different features:
- one (un)install a callback function (and the pins it's interested in)
- one enable or disable the interrupt trigger (and keeps track of it) of
one pin.

You might want to inhibate a pin to raise a cb, without removing the
callback that would be used for other pins.

For instance

gpio_set_callback(my_cb) /* interested by pin x and y*/
gpio_enable_callback(pin_y)

and:
my_cb(pins) {
if (pins & pin_y) {
gpio_disable_callback(pin_y)
gpio_enable_callback(pin_x)
} else {
gpio_disable_callback(pin_x)
gpio_enable_callback(pin_y)
}
}

Something like that could be needed.

Tomasz

7761 - 7780 of 8046