Hi,

I sent a draft on gerrit:

https://gerrit.zephyrproject.org/r/#/c/914/

I kept enable/disable pin/port functions but the actual code in the driver
is slightly different.

So basically the real change are:
- gpio_callback_t is meant to be deprecated
- struct gpio_callback is the new way of providing a callback

- gpio_set_callback() is meant to be deprecated
- gpio_register_callback()/gpio_unregister_callback() are new ways of
handling callbacks

The thing I am not too found about is _gpio_compat_register_instance().
If there is another way, and seamless one even, would be nice.

Tomasz

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.

Rethinking about that, while doing the proper patch-set. You've got good
point.
I am making it useless if the user has not set a callback through
gpio_set_callback()
(old way of doing, for a unique callback on the whole port)

Thanks,

Tomasz

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

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

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

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

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

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)

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

Tomasz

On Mon, 07 Mar 2016 11:13:55 -0300, Vinicius Costa Gomes wrote:

Hi,

Sorry if I am a little too late.

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

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.

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?

Hi,

Sorry if I am a little too late.

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

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.

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)


[...]


Cheers,
--
Vinicius

static inline int gpio_dw_set_callback(struct device *port,
- gpio_callback_t callback)
+ gpio_callback_t callback,

When I say it's untested, it's for real ... should be *callback here.

/**
@@ -310,7 +336,6 @@ static inline int gpio_port_enable_callback(struct device *port)

api = (struct gpio_driver_api *) port->driver_api;
return api->enable_callback(port, GPIO_ACCESS_BY_PORT, 0);
-
}

With new api, this would call all callback handles, whatever is their
pin_mask.
(see _gpio_fire_callbacks())

Hi,

I quickly went through prototyping something, only to detail the RFC (so
it's not really tested etc...)
Also, it's not something that I am planning to send right away as it
breaks the former API.

See attached.

For the user data pointer, it's only a matter to use CONTAINER_OF() with
given cb pointer
as Johan noticed.

I put the generic callback list functions into a header in
drivers/gpio/, not sure it's would be the final place.

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