Topics

[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


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


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


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


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