Topics

[RFC PATCH 4/6] gpio: add device config helpers


Vlad Dogaru <vlad.dogaru@...>
 

Change-Id: I8af573484934a02893a395bb0d19551b5c9d0291
Signed-off-by: Vlad Dogaru <vlad.dogaru(a)intel.com>
---
include/gpio.h | 30 ++++++++++++++++++++++++++++++
1 file changed, 30 insertions(+)

diff --git a/include/gpio.h b/include/gpio.h
index 57b3e22..cb1c4db 100644
--- a/include/gpio.h
+++ b/include/gpio.h
@@ -432,6 +432,36 @@ static inline int gpio_port_disable_callback(struct device *port)
return api->disable_callback(port, GPIO_ACCESS_BY_PORT, 0);
}

+struct gpio_pin_config {
+ /* In the future, devices will be selected by their handle, not
+ * name.
+ */
+ /* struct device *gpio_controller; */
+ char *gpio_controller;
+ uint32_t gpio_pin;
+};
+
+#define DECLARE_GPIO_PIN_CONFIG_IDX(idx) \
+ struct gpio_pin_config gpio_pin_ ##idx
+#define DECLARE_GPIO_PIN_CONFIG \
+ DECLARE_GPIO_PIN_CONFIG_IDX()
+
+#define GPIO_PIN_IDX(idx, controller, pin) \
+ .gpio_pin_ ##idx = { \
+ .gpio_controller = (controller),\
+ .gpio_pin = (pin), \
+ }
+#define GPIO_PIN(controller, pin) \
+ GPIO_PIN_IDX(, controller, pin)
+
+#define GET_GPIO_CONTROLLER_IDX(idx, conf) \
+ (conf)->gpio_pin_ ##idx.gpio_controller
+#define GET_GPIO_PIN_IDX(idx, conf) \
+ (conf)->gpio_pin_ ##idx.gpio_pin
+
+#define GET_GPIO_CONTROLLER(conf) GET_GPIO_CONTROLLER_IDX(, conf)
+#define GET_GPIO_PIN(conf) GET_GPIO_PIN_IDX(, conf)
+
#ifdef __cplusplus
}
#endif
--
1.9.1


Vlad Dogaru <vlad.dogaru@...>
 

On Mon, Apr 25, 2016 at 01:13:51PM +0200, Tomasz Bursztyka wrote:
Hi Vlad,

Change-Id: I8af573484934a02893a395bb0d19551b5c9d0291
Signed-off-by: Vlad Dogaru <vlad.dogaru(a)intel.com>
---
include/gpio.h | 30 ++++++++++++++++++++++++++++++
1 file changed, 30 insertions(+)

diff --git a/include/gpio.h b/include/gpio.h
index 57b3e22..cb1c4db 100644
--- a/include/gpio.h
+++ b/include/gpio.h
@@ -432,6 +432,36 @@ static inline int gpio_port_disable_callback(struct device *port)
return api->disable_callback(port, GPIO_ACCESS_BY_PORT, 0);
}
+struct gpio_pin_config {
+ /* In the future, devices will be selected by their handle, not
+ * name.
+ */
+ /* struct device *gpio_controller; */
+ char *gpio_controller;
+ uint32_t gpio_pin;
+};
I see a problem with that: it will fit only device that need to
configure 1 pin.
On cc2520 it requires many ones, and many complex devices will need
more than 1.
This structure does declare only one pin. But you can call the _IDX
macros multiple times:

struct mydev_config {
DECLARE_GPIO_PIN_CONFIG_IDX(0);
DECLARE_GPIO_PIN_CONFIG_IDX(1);
/* ... */
};

/* And when declaring the device: */
struct mydev_config mydev_0_config = {
GPIO_PIN_IDX(0, "GPIO_0", 12);
GPIO_PIN_IDX(1, "GPIO_0", 14);
/* ... */
};

+
+#define DECLARE_GPIO_PIN_CONFIG_IDX(idx) \
+ struct gpio_pin_config gpio_pin_ ##idx
+#define DECLARE_GPIO_PIN_CONFIG \
+ DECLARE_GPIO_PIN_CONFIG_IDX()
+
+#define GPIO_PIN_IDX(idx, controller, pin) \
+ .gpio_pin_ ##idx = { \
+ .gpio_controller = (controller),\
+ .gpio_pin = (pin), \
+ }
+#define GPIO_PIN(controller, pin) \
+ GPIO_PIN_IDX(, controller, pin)
+
+#define GET_GPIO_CONTROLLER_IDX(idx, conf) \
+ (conf)->gpio_pin_ ##idx.gpio_controller
+#define GET_GPIO_PIN_IDX(idx, conf) \
+ (conf)->gpio_pin_ ##idx.gpio_pin
+
+#define GET_GPIO_CONTROLLER(conf) GET_GPIO_CONTROLLER_IDX(, conf)
+#define GET_GPIO_PIN(conf) GET_GPIO_PIN_IDX(, conf)
And GPIO_ prefixed.

and documentation
Noted.


Tomasz Bursztyka
 

Hi Vlad,

Change-Id: I8af573484934a02893a395bb0d19551b5c9d0291
Signed-off-by: Vlad Dogaru <vlad.dogaru(a)intel.com>
---
include/gpio.h | 30 ++++++++++++++++++++++++++++++
1 file changed, 30 insertions(+)

diff --git a/include/gpio.h b/include/gpio.h
index 57b3e22..cb1c4db 100644
--- a/include/gpio.h
+++ b/include/gpio.h
@@ -432,6 +432,36 @@ static inline int gpio_port_disable_callback(struct device *port)
return api->disable_callback(port, GPIO_ACCESS_BY_PORT, 0);
}

+struct gpio_pin_config {
+ /* In the future, devices will be selected by their handle, not
+ * name.
+ */
+ /* struct device *gpio_controller; */
+ char *gpio_controller;
+ uint32_t gpio_pin;
+};
I see a problem with that: it will fit only device that need to
configure 1 pin.
On cc2520 it requires many ones, and many complex devices will need more
than 1.

+
+#define DECLARE_GPIO_PIN_CONFIG_IDX(idx) \
+ struct gpio_pin_config gpio_pin_ ##idx
+#define DECLARE_GPIO_PIN_CONFIG \
+ DECLARE_GPIO_PIN_CONFIG_IDX()
+
+#define GPIO_PIN_IDX(idx, controller, pin) \
+ .gpio_pin_ ##idx = { \
+ .gpio_controller = (controller),\
+ .gpio_pin = (pin), \
+ }
+#define GPIO_PIN(controller, pin) \
+ GPIO_PIN_IDX(, controller, pin)
+
+#define GET_GPIO_CONTROLLER_IDX(idx, conf) \
+ (conf)->gpio_pin_ ##idx.gpio_controller
+#define GET_GPIO_PIN_IDX(idx, conf) \
+ (conf)->gpio_pin_ ##idx.gpio_pin
+
+#define GET_GPIO_CONTROLLER(conf) GET_GPIO_CONTROLLER_IDX(, conf)
+#define GET_GPIO_PIN(conf) GET_GPIO_PIN_IDX(, conf)
And GPIO_ prefixed.

and documentation


Tomasz


Vlad Dogaru <vlad.dogaru@...>
 

On Mon, Apr 25, 2016 at 01:33:33PM +0200, Tomasz Bursztyka wrote:
Hi Vlad,
+struct gpio_pin_config {
+ /* In the future, devices will be selected by their handle, not
+ * name.
+ */
+ /* struct device *gpio_controller; */
+ char *gpio_controller;
+ uint32_t gpio_pin;
+};
I see a problem with that: it will fit only device that need to
configure 1 pin.
On cc2520 it requires many ones, and many complex devices will need
more than 1.
This structure does declare only one pin. But you can call the _IDX
macros multiple times:

struct mydev_config {
DECLARE_GPIO_PIN_CONFIG_IDX(0);
DECLARE_GPIO_PIN_CONFIG_IDX(1);
/* ... */
};

/* And when declaring the device: */
struct mydev_config mydev_0_config = {
GPIO_PIN_IDX(0, "GPIO_0", 12);
GPIO_PIN_IDX(1, "GPIO_0", 14);
/* ... */
};
This will bloat things up by repeating controller's name pointer
(which is most likely be the same).
Maybe a variable array of pins would be better then.

I don't have an obvious solution for that.
I'll take a stab at that in the next version, assuming we're comfortable
with the assumption that a device will never have GPIO pins tied to
different controllers.


Tomasz Bursztyka
 

Hi Vlad,
+struct gpio_pin_config {
+ /* In the future, devices will be selected by their handle, not
+ * name.
+ */
+ /* struct device *gpio_controller; */
+ char *gpio_controller;
+ uint32_t gpio_pin;
+};
I see a problem with that: it will fit only device that need to
configure 1 pin.
On cc2520 it requires many ones, and many complex devices will need
more than 1.
This structure does declare only one pin. But you can call the _IDX
macros multiple times:

struct mydev_config {
DECLARE_GPIO_PIN_CONFIG_IDX(0);
DECLARE_GPIO_PIN_CONFIG_IDX(1);
/* ... */
};

/* And when declaring the device: */
struct mydev_config mydev_0_config = {
GPIO_PIN_IDX(0, "GPIO_0", 12);
GPIO_PIN_IDX(1, "GPIO_0", 14);
/* ... */
};
This will bloat things up by repeating controller's name pointer (which
is most likely be the same).
Maybe a variable array of pins would be better then.

I don't have an obvious solution for that.

Tomasz


Tomasz Bursztyka
 

Hi Vlad,


+struct gpio_pin_config {
+ /* In the future, devices will be selected by their handle, not
+ * name.
+ */
+ /* struct device *gpio_controller; */
+ char *gpio_controller;
+ uint32_t gpio_pin;
+};
I see a problem with that: it will fit only device that need to
configure 1 pin.
On cc2520 it requires many ones, and many complex devices will need
more than 1.
This structure does declare only one pin. But you can call the _IDX
macros multiple times:

struct mydev_config {
DECLARE_GPIO_PIN_CONFIG_IDX(0);
DECLARE_GPIO_PIN_CONFIG_IDX(1);
/* ... */
};

/* And when declaring the device: */
struct mydev_config mydev_0_config = {
GPIO_PIN_IDX(0, "GPIO_0", 12);
GPIO_PIN_IDX(1, "GPIO_0", 14);
/* ... */
};
This will bloat things up by repeating controller's name pointer
(which is most likely be the same).
Maybe a variable array of pins would be better then.

I don't have an obvious solution for that.
I'll take a stab at that in the next version, assuming we're comfortable
with the assumption that a device will never have GPIO pins tied to
different controllers.
It might and it's not going to be a rare case.
Again, about cc2520 it uses 2 GPIO controller on quark_se_devboard (it's
board specific obviously)
Actually, take a look at how we addressed this issue in
boards/quark_se_dev_board/board.<h/c>
It's far from being perfect and it's not generic, but it may help.

Tomasz