In the kernel there are basically two patterns to implement object
oriented code in C. You can either embedded a set of function pointers
in a struct along with other members or have a separate virtual function
table (vtable) structure that hold all the functions and only store a
pointer to that vtable on our particular object.
The struct gpio_chip uses the former approach, but I don't know if that
is a design decision or is just that this code predates the fact that
the separate structure pattern is now so popular. Since the having a
the operations on a different structure has a number of benefits:
- A clean separation between state (fields) and operations (functions).
- Size reduction of struct gpio_chip since will only hold one pointer.
- These functions are not supposed to change at runtime so the const
qualifier can be used to prevent pointers modification during execution.
- Similar drivers for a chip family can reuse their function vtable.
There is a drawback though which is that now two memory accesses are
needed to execute a GPIO operation since an additional level of
indirection is introduced but that should be minimized due temporal and
spatial memory locality.
So this is an RFC patch-set to add a virtual table to be used by
GPIO chip controllers and consist of the following patches:
Javier Martinez Canillas (5):
gpio: add a vtable to abstract GPIO controller operations
gpiolib: set gpio_chip operations on add using a gpio_chip_ops
gpio: omap: convert driver to use gpio_chip_ops
gpio: twl4030: convert driver to use gpio_chip_ops
gpio: switch to use struct struct gpio_chip_ops
drivers/gpio/gpio-omap.c | 19 ++++++++-----
drivers/gpio/gpio-twl4030.c | 10 +++++--
drivers/gpio/gpiolib.c | 64 ++++++++++++++++++++---------------------
include/linux/gpio/driver.h | 69 +++++++++++++++++++++++++++------------------
4 files changed, 93 insertions(+), 69 deletions(-)
The patch-set is not a complete one though since only the GPIO OMAP
and GPIO TWL4030 drivers have been converted so I could test it on
my platform (DM3730 OMAP IGEPv2 board).
But I preferred to send an early RFC than changing every single driver
before discussing if doing the split is worth it or not.
To not break git bisect-ability, I added some patches that are
transitional changes. If you have a better suggestion on how to
handle that please let me know.
Thanks a lot and best regards,
Javier
--
1.9.0
This is a transitional change to avoid breaking git bisect-ability
while converting GPIO controller drivers to set their operations by
using the newly introduced struct gpio_chip_ops virtual function table.
It should be removed once all GPIO chip drivers have been converted.
Signed-off-by: Javier Martinez Canillas <[email protected]>
---
drivers/gpio/gpiolib.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 761013f..f0cc93a 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1188,6 +1188,25 @@ int gpiochip_add(struct gpio_chip *chip)
goto fail;
}
+ /*
+ * REVISIT: this is a workaround to not break git bisectability by
+ * allowing GPIO controller drivers to set either either the function
+ * pointers embedded in struct gpio_chip or by using a gpio_chip_ops.
+ *
+ * Should be removed once all drivers are converted to set chip->ops.
+ */
+ if (chip->ops) {
+ chip->request = chip->ops->request;
+ chip->free = chip->ops->free;
+ chip->get_direction = chip->ops->get_direction;
+ chip->direction_input = chip->ops->direction_input;
+ chip->direction_output = chip->ops->direction_output;
+ chip->get = chip->ops->get;
+ chip->set = chip->ops->set;
+ chip->set_debounce = chip->ops->set_debounce;
+ chip->dbg_show = chip->ops->dbg_show;
+ }
+
spin_lock_irqsave(&gpio_lock, flags);
if (base < 0) {
--
1.9.0
All GPIO controller drivers have been migrated to use the struct
gpio_chip_ops virtual function table so the embeddeded function
pointers in struct gpio_chip can now be removed.
Signed-off-by: Javier Martinez Canillas <[email protected]>
---
drivers/gpio/gpiolib.c | 83 +++++++++++++++++----------------------------
include/linux/gpio/driver.h | 40 +++-------------------
2 files changed, 36 insertions(+), 87 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index f0cc93a..2808076 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -214,8 +214,8 @@ static int gpio_ensure_requested(struct gpio_desc *desc)
return -EIO;
}
desc_set_label(desc, "[auto]");
- /* caller must chip->request() w/o spinlock */
- if (chip->request)
+ /* caller must chip->ops->request() w/o spinlock */
+ if (chip->ops->request)
return 1;
}
return 0;
@@ -272,10 +272,10 @@ int gpiod_get_direction(const struct gpio_desc *desc)
chip = gpiod_to_chip(desc);
offset = gpio_chip_hwgpio(desc);
- if (!chip->get_direction)
+ if (!chip->ops->get_direction)
return status;
- status = chip->get_direction(chip, offset);
+ status = chip->ops->get_direction(chip, offset);
if (status > 0) {
/* GPIOF_DIR_IN, or other positive */
status = 1;
@@ -837,7 +837,7 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
goto fail_unlock;
}
- if (!desc->chip->direction_input || !desc->chip->direction_output)
+ if (!desc->chip->ops->direction_input || !desc->chip->ops->direction_output)
direction_may_change = false;
spin_unlock_irqrestore(&gpio_lock, flags);
@@ -1188,25 +1188,6 @@ int gpiochip_add(struct gpio_chip *chip)
goto fail;
}
- /*
- * REVISIT: this is a workaround to not break git bisectability by
- * allowing GPIO controller drivers to set either either the function
- * pointers embedded in struct gpio_chip or by using a gpio_chip_ops.
- *
- * Should be removed once all drivers are converted to set chip->ops.
- */
- if (chip->ops) {
- chip->request = chip->ops->request;
- chip->free = chip->ops->free;
- chip->get_direction = chip->ops->get_direction;
- chip->direction_input = chip->ops->direction_input;
- chip->direction_output = chip->ops->direction_output;
- chip->get = chip->ops->get;
- chip->set = chip->ops->set;
- chip->set_debounce = chip->ops->set_debounce;
- chip->dbg_show = chip->ops->dbg_show;
- }
-
spin_lock_irqsave(&gpio_lock, flags);
if (base < 0) {
@@ -1231,10 +1212,10 @@ int gpiochip_add(struct gpio_chip *chip)
* inputs (often with pullups enabled) so power
* usage is minimized. Linux code should set the
* gpio direction first thing; but until it does,
- * and in case chip->get_direction is not set,
+ * and in case chip->ops->get_direction is not set,
* we may expose the wrong direction in sysfs.
*/
- desc->flags = !chip->direction_input
+ desc->flags = !chip->ops->direction_input
? (1 << FLAG_IS_OUT)
: 0;
}
@@ -1711,10 +1692,10 @@ static int __gpiod_request(struct gpio_desc *desc, const char *label)
goto done;
}
- if (chip->request) {
- /* chip->request may sleep */
+ if (chip->ops->request) {
+ /* chip->ops->request may sleep */
spin_unlock_irqrestore(&gpio_lock, flags);
- status = chip->request(chip, gpio_chip_hwgpio(desc));
+ status = chip->ops->request(chip, gpio_chip_hwgpio(desc));
spin_lock_irqsave(&gpio_lock, flags);
if (status < 0) {
@@ -1723,8 +1704,8 @@ static int __gpiod_request(struct gpio_desc *desc, const char *label)
goto done;
}
}
- if (chip->get_direction) {
- /* chip->get_direction may sleep */
+ if (chip->ops->get_direction) {
+ /* chip->ops->get_direction may sleep */
spin_unlock_irqrestore(&gpio_lock, flags);
gpiod_get_direction(desc);
spin_lock_irqsave(&gpio_lock, flags);
@@ -1781,10 +1762,10 @@ static bool __gpiod_free(struct gpio_desc *desc)
chip = desc->chip;
if (chip && test_bit(FLAG_REQUESTED, &desc->flags)) {
- if (chip->free) {
+ if (chip->ops->free) {
spin_unlock_irqrestore(&gpio_lock, flags);
might_sleep_if(chip->can_sleep);
- chip->free(chip, gpio_chip_hwgpio(desc));
+ chip->ops->free(chip, gpio_chip_hwgpio(desc));
spin_lock_irqsave(&gpio_lock, flags);
}
desc_set_label(desc, NULL);
@@ -1989,7 +1970,7 @@ int gpiod_direction_input(struct gpio_desc *desc)
}
chip = desc->chip;
- if (!chip->get || !chip->direction_input) {
+ if (!chip->ops->get || !chip->ops->direction_input) {
gpiod_warn(desc,
"%s: missing get() or direction_input() operations\n",
__func__);
@@ -2010,7 +1991,7 @@ int gpiod_direction_input(struct gpio_desc *desc)
offset = gpio_chip_hwgpio(desc);
if (status) {
- status = chip->request(chip, offset);
+ status = chip->ops->request(chip, offset);
if (status < 0) {
gpiod_dbg(desc, "%s: chip request fail, %d\n",
__func__, status);
@@ -2021,7 +2002,7 @@ int gpiod_direction_input(struct gpio_desc *desc)
}
}
- status = chip->direction_input(chip, offset);
+ status = chip->ops->direction_input(chip, offset);
if (status == 0)
clear_bit(FLAG_IS_OUT, &desc->flags);
@@ -2060,7 +2041,7 @@ static int _gpiod_direction_output_raw(struct gpio_desc *desc, int value)
return gpiod_direction_input(desc);
chip = desc->chip;
- if (!chip->set || !chip->direction_output) {
+ if (!chip->ops->set || !chip->ops->direction_output) {
gpiod_warn(desc,
"%s: missing set() or direction_output() operations\n",
__func__);
@@ -2081,7 +2062,7 @@ static int _gpiod_direction_output_raw(struct gpio_desc *desc, int value)
offset = gpio_chip_hwgpio(desc);
if (status) {
- status = chip->request(chip, offset);
+ status = chip->ops->request(chip, offset);
if (status < 0) {
gpiod_dbg(desc, "%s: chip request fail, %d\n",
__func__, status);
@@ -2092,7 +2073,7 @@ static int _gpiod_direction_output_raw(struct gpio_desc *desc, int value)
}
}
- status = chip->direction_output(chip, offset, value);
+ status = chip->ops->direction_output(chip, offset, value);
if (status == 0)
set_bit(FLAG_IS_OUT, &desc->flags);
trace_gpio_value(desc_to_gpio(desc), 0, value);
@@ -2172,7 +2153,7 @@ int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)
}
chip = desc->chip;
- if (!chip->set || !chip->set_debounce) {
+ if (!chip->ops->set || !chip->ops->set_debounce) {
gpiod_dbg(desc,
"%s: missing set() or set_debounce() operations\n",
__func__);
@@ -2192,7 +2173,7 @@ int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)
might_sleep_if(chip->can_sleep);
offset = gpio_chip_hwgpio(desc);
- return chip->set_debounce(chip, offset, debounce);
+ return chip->ops->set_debounce(chip, offset, debounce);
fail:
spin_unlock_irqrestore(&gpio_lock, flags);
@@ -2245,7 +2226,7 @@ static bool _gpiod_get_raw_value(const struct gpio_desc *desc)
chip = desc->chip;
offset = gpio_chip_hwgpio(desc);
- value = chip->get ? chip->get(chip, offset) : false;
+ value = chip->ops->get ? chip->ops->get(chip, offset) : false;
trace_gpio_value(desc_to_gpio(desc), 1, value);
return value;
}
@@ -2308,11 +2289,11 @@ static void _gpio_set_open_drain_value(struct gpio_desc *desc, bool value)
int offset = gpio_chip_hwgpio(desc);
if (value) {
- err = chip->direction_input(chip, offset);
+ err = chip->ops->direction_input(chip, offset);
if (!err)
clear_bit(FLAG_IS_OUT, &desc->flags);
} else {
- err = chip->direction_output(chip, offset, 0);
+ err = chip->ops->direction_output(chip, offset, 0);
if (!err)
set_bit(FLAG_IS_OUT, &desc->flags);
}
@@ -2335,11 +2316,11 @@ static void _gpio_set_open_source_value(struct gpio_desc *desc, bool value)
int offset = gpio_chip_hwgpio(desc);
if (value) {
- err = chip->direction_output(chip, offset, 1);
+ err = chip->ops->direction_output(chip, offset, 1);
if (!err)
set_bit(FLAG_IS_OUT, &desc->flags);
} else {
- err = chip->direction_input(chip, offset);
+ err = chip->ops->direction_input(chip, offset);
if (!err)
clear_bit(FLAG_IS_OUT, &desc->flags);
}
@@ -2361,7 +2342,7 @@ static void _gpiod_set_raw_value(struct gpio_desc *desc, bool value)
else if (test_bit(FLAG_OPEN_SOURCE, &desc->flags))
_gpio_set_open_source_value(desc, value);
else
- chip->set(chip, gpio_chip_hwgpio(desc), value);
+ chip->ops->set(chip, gpio_chip_hwgpio(desc), value);
}
/**
@@ -2828,8 +2809,8 @@ static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip)
seq_printf(s, " gpio-%-3d (%-20.20s) %s %s %s",
gpio, gdesc->label,
is_out ? "out" : "in ",
- chip->get
- ? (chip->get(chip, i) ? "hi" : "lo")
+ chip->ops->get
+ ? (chip->ops->get(chip, i) ? "hi" : "lo")
: "? ",
is_irq ? "IRQ" : " ");
seq_printf(s, "\n");
@@ -2895,8 +2876,8 @@ static int gpiolib_seq_show(struct seq_file *s, void *v)
seq_printf(s, ", can sleep");
seq_printf(s, ":\n");
- if (chip->dbg_show)
- chip->dbg_show(s, chip);
+ if (chip->ops->dbg_show)
+ chip->ops->dbg_show(s, chip);
else
gpiolib_dbg_show(s, chip);
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 824cd32..326ad96 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -66,24 +66,8 @@ struct gpio_chip_ops {
* @dev: optional device providing the GPIOs
* @owner: helps prevent removal of modules exporting active GPIOs
* @list: links gpio_chips together for traversal
- * @request: optional hook for chip-specific activation, such as
- * enabling module power and clock; may sleep
- * @free: optional hook for chip-specific deactivation, such as
- * disabling module power and clock; may sleep
- * @get_direction: returns direction for signal "offset", 0=out, 1=in,
- * (same as GPIOF_DIR_XXX), or negative error
- * @direction_input: configures signal "offset" as input, or returns error
- * @direction_output: configures signal "offset" as output, or returns error
- * @get: returns value for signal "offset"; for output signals this
- * returns either the value actually sensed, or zero
- * @set: assigns output value for signal "offset"
- * @set_debounce: optional hook for setting debounce time for specified gpio in
- * interrupt triggered gpio chips
* @to_irq: optional hook supporting non-static gpio_to_irq() mappings;
* implementation may not sleep
- * @dbg_show: optional routine to show contents in debugfs; default code
- * will be used when this is omitted, but custom code can show extra
- * state (such as pullup/pulldown configuration).
* @base: identifies the first GPIO number handled by this chip; or, if
* negative during registration, requests dynamic ID allocation.
* @ngpio: the number of GPIOs handled by this controller; the last GPIO
@@ -116,29 +100,13 @@ struct gpio_chip {
struct module *owner;
struct list_head list;
- int (*request)(struct gpio_chip *chip,
- unsigned offset);
- void (*free)(struct gpio_chip *chip,
- unsigned offset);
- int (*get_direction)(struct gpio_chip *chip,
- unsigned offset);
- int (*direction_input)(struct gpio_chip *chip,
- unsigned offset);
- int (*direction_output)(struct gpio_chip *chip,
- unsigned offset, int value);
- int (*get)(struct gpio_chip *chip,
- unsigned offset);
- void (*set)(struct gpio_chip *chip,
- unsigned offset, int value);
- int (*set_debounce)(struct gpio_chip *chip,
- unsigned offset,
- unsigned debounce);
-
+ /*
+ * The to_irq function pointer is not in struct gpio_chip_ops
+ * because it can be modified inside the gpiolib.
+ */
int (*to_irq)(struct gpio_chip *chip,
unsigned offset);
- void (*dbg_show)(struct seq_file *s,
- struct gpio_chip *chip);
int base;
u16 ngpio;
struct gpio_desc *desc;
--
1.9.0
A common pattern to implement object oriented code in the kernel is
to use a separate structure to hold all the methods for a particular
kind of object and just have a pointer to this table rather than a
separate pointer for each method.
The alternate approach is to directly embedded function pointers in
the object but there are some advantages on using the former approach.
This patch adds a struct gpio_chip_ops to be set by GPIO chip controllers.
Signed-off-by: Javier Martinez Canillas <[email protected]>
---
include/linux/gpio/driver.h | 47 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 47 insertions(+)
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 1827b43..824cd32 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -16,6 +16,51 @@ struct seq_file;
#ifdef CONFIG_GPIOLIB
/**
+ * struct gpio_chip_ops - virtual function table for GPIO controller operations
+ * @request: optional hook for chip-specific activation, such as
+ * enabling module power and clock; may sleep
+ * @free: optional hook for chip-specific deactivation, such as
+ * disabling module power and clock; may sleep
+ * @get_direction: returns direction for signal "offset", 0=out, 1=in,
+ * (same as GPIOF_DIR_XXX), or negative error
+ * @direction_input: configures signal "offset" as input, or returns error
+ * @direction_output: configures signal "offset" as output, or returns error
+ * @get: returns value for signal "offset"; for output signals this
+ * returns either the value actually sensed, or zero
+ * @set: assigns output value for signal "offset"
+ * @set_debounce: optional hook for setting debounce time for specified gpio in
+ * interrupt triggered gpio chips
+ * @dbg_show: optional routine to show contents in debugfs; default code
+ * will be used when this is omitted, but custom code can show extra
+ * state (such as pullup/pulldown configuration).
+ *
+ * A gpio_chip_ops is used as a virtual function table for gpio_chip so GPIO
+ * drivers can define their custom methods as needed by its the GPIO controller.
+ */
+struct gpio_chip_ops {
+ int (*request)(struct gpio_chip *chip,
+ unsigned offset);
+ void (*free)(struct gpio_chip *chip,
+ unsigned offset);
+ int (*get_direction)(struct gpio_chip *chip,
+ unsigned offset);
+ int (*direction_input)(struct gpio_chip *chip,
+ unsigned offset);
+ int (*direction_output)(struct gpio_chip *chip,
+ unsigned offset, int value);
+ int (*get)(struct gpio_chip *chip,
+ unsigned offset);
+ void (*set)(struct gpio_chip *chip,
+ unsigned offset, int value);
+ int (*set_debounce)(struct gpio_chip *chip,
+ unsigned offset,
+ unsigned debounce);
+
+ void (*dbg_show)(struct seq_file *s,
+ struct gpio_chip *chip);
+};
+
+/**
* struct gpio_chip - abstract a GPIO controller
* @label: for diagnostics
* @dev: optional device providing the GPIOs
@@ -44,6 +89,7 @@ struct seq_file;
* @ngpio: the number of GPIOs handled by this controller; the last GPIO
* handled is (base + ngpio - 1).
* @desc: array of ngpio descriptors. Private.
+ * @ops: virtual table with GPIO controller operations.
* @names: if set, must be an array of strings to use as alternative
* names for the GPIOs in this chip. Any entry in the array
* may be NULL if there is no alias for the GPIO, however the
@@ -97,6 +143,7 @@ struct gpio_chip {
u16 ngpio;
struct gpio_desc *desc;
const char *const *names;
+ const struct gpio_chip_ops *ops;
bool can_sleep;
bool exported;
--
1.9.0
The GPIO controller operations has been split to be stored
on a separate struct gpio_chip_ops virtual function table.
Signed-off-by: Javier Martinez Canillas <[email protected]>
---
drivers/gpio/gpio-twl4030.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/gpio/gpio-twl4030.c b/drivers/gpio/gpio-twl4030.c
index 3ebb1a5..0d40bc0 100644
--- a/drivers/gpio/gpio-twl4030.c
+++ b/drivers/gpio/gpio-twl4030.c
@@ -386,15 +386,19 @@ static int twl_to_irq(struct gpio_chip *chip, unsigned offset)
: -EINVAL;
}
-static struct gpio_chip template_chip = {
- .label = "twl4030",
- .owner = THIS_MODULE,
+const struct gpio_chip_ops ops = {
.request = twl_request,
.free = twl_free,
.direction_input = twl_direction_in,
.get = twl_get,
.direction_output = twl_direction_out,
.set = twl_set,
+};
+
+static struct gpio_chip template_chip = {
+ .label = "twl4030",
+ .owner = THIS_MODULE,
+ .ops = &ops,
.to_irq = twl_to_irq,
.can_sleep = true,
};
--
1.9.0
The GPIO controller operations has been split to be stored
on a separate struct gpio_chip_ops virtual function table.
Signed-off-by: Javier Martinez Canillas <[email protected]>
---
drivers/gpio/gpio-omap.c | 19 ++++++++++++-------
1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 8cc9e91..50f0938 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -1072,6 +1072,16 @@ omap_mpuio_alloc_gc(struct gpio_bank *bank, unsigned int irq_start,
IRQ_NOREQUEST | IRQ_NOPROBE, 0);
}
+const struct gpio_chip_ops omap_gpio_ops = {
+ .request = omap_gpio_request,
+ .free = omap_gpio_free,
+ .direction_input = gpio_input,
+ .get = gpio_get,
+ .direction_output = gpio_output,
+ .set_debounce = gpio_debounce,
+ .set = gpio_set
+};
+
static int omap_gpio_chip_init(struct gpio_bank *bank)
{
int j;
@@ -1083,13 +1093,8 @@ static int omap_gpio_chip_init(struct gpio_bank *bank)
* REVISIT eventually switch from OMAP-specific gpio structs
* over to the generic ones
*/
- bank->chip.request = omap_gpio_request;
- bank->chip.free = omap_gpio_free;
- bank->chip.direction_input = gpio_input;
- bank->chip.get = gpio_get;
- bank->chip.direction_output = gpio_output;
- bank->chip.set_debounce = gpio_debounce;
- bank->chip.set = gpio_set;
+ bank->chip.ops = &omap_gpio_ops;
+
if (bank->is_mpuio) {
bank->chip.label = "mpuio";
if (bank->regs->wkup_en)
--
1.9.0
On Wed, Apr 9, 2014 at 3:20 AM, Javier Martinez Canillas
<[email protected]> wrote:
> In the kernel there are basically two patterns to implement object
> oriented code in C. You can either embedded a set of function pointers
s/embedded/embed
> in a struct along with other members or have a separate virtual function
> table (vtable) structure that hold all the functions and only store a
> pointer to that vtable on our particular object.
>
> The struct gpio_chip uses the former approach, but I don't know if that
> is a design decision or is just that this code predates the fact that
> the separate structure pattern is now so popular. Since the having a
> the operations on a different structure has a number of benefits:
"Since having the operations" maybe?
>
> - A clean separation between state (fields) and operations (functions).
> - Size reduction of struct gpio_chip since will only hold one pointer.
> - These functions are not supposed to change at runtime so the const
> qualifier can be used to prevent pointers modification during execution.
> - Similar drivers for a chip family can reuse their function vtable.
>
> There is a drawback though which is that now two memory accesses are
> needed to execute a GPIO operation since an additional level of
> indirection is introduced but that should be minimized due temporal and
> spatial memory locality.
I think I really do like this. Having ops in a separate structure is a
very common pattern in the kernel and makes things a lot cleaner. On
top of the advantages you listed, it also only requires a single
assignment in the driver's init function vs. a lot more today.
If no one complains about the additional memory access, I'd like to go
forward with this. I did much worse performance-hurting changes when
introducing gpiod, so I suppose it will be fine.
>
> So this is an RFC patch-set to add a virtual table to be used by
> GPIO chip controllers and consist of the following patches:
>
> Javier Martinez Canillas (5):
> gpio: add a vtable to abstract GPIO controller operations
> gpiolib: set gpio_chip operations on add using a gpio_chip_ops
> gpio: omap: convert driver to use gpio_chip_ops
> gpio: twl4030: convert driver to use gpio_chip_ops
> gpio: switch to use struct struct gpio_chip_ops
>
> drivers/gpio/gpio-omap.c | 19 ++++++++-----
> drivers/gpio/gpio-twl4030.c | 10 +++++--
> drivers/gpio/gpiolib.c | 64 ++++++++++++++++++++---------------------
> include/linux/gpio/driver.h | 69 +++++++++++++++++++++++++++------------------
> 4 files changed, 93 insertions(+), 69 deletions(-)
>
> The patch-set is not a complete one though since only the GPIO OMAP
> and GPIO TWL4030 drivers have been converted so I could test it on
> my platform (DM3730 OMAP IGEPv2 board).
>
> But I preferred to send an early RFC than changing every single driver
> before discussing if doing the split is worth it or not.
>
> To not break git bisect-ability, I added some patches that are
> transitional changes. If you have a better suggestion on how to
> handle that please let me know.
We will probably need that transition phase. We will also need to
switch every single driver to your new scheme, so please wait until we
hear from Linus before proceeding. :)
Thanks,
Alex.
Hello Alexandre,
Thanks a lot for your feedback.
On 04/10/2014 09:36 AM, Alexandre Courbot wrote:
> On Wed, Apr 9, 2014 at 3:20 AM, Javier Martinez Canillas
> <[email protected]> wrote:
>> In the kernel there are basically two patterns to implement object
>> oriented code in C. You can either embedded a set of function pointers
>
> s/embedded/embed
>
>> in a struct along with other members or have a separate virtual function
>> table (vtable) structure that hold all the functions and only store a
>> pointer to that vtable on our particular object.
>>
>> The struct gpio_chip uses the former approach, but I don't know if that
>> is a design decision or is just that this code predates the fact that
>> the separate structure pattern is now so popular. Since the having a
>> the operations on a different structure has a number of benefits:
>
> "Since having the operations" maybe?
>
Yes, since I'm not a native english speaker I sometimes miss some obvious
grammatical errors. I'll fix those when posting the final version with all the
drivers converted.
>>
>> - A clean separation between state (fields) and operations (functions).
>> - Size reduction of struct gpio_chip since will only hold one pointer.
>> - These functions are not supposed to change at runtime so the const
>> qualifier can be used to prevent pointers modification during execution.
>> - Similar drivers for a chip family can reuse their function vtable.
>>
>> There is a drawback though which is that now two memory accesses are
>> needed to execute a GPIO operation since an additional level of
>> indirection is introduced but that should be minimized due temporal and
>> spatial memory locality.
>
> I think I really do like this. Having ops in a separate structure is a
> very common pattern in the kernel and makes things a lot cleaner. On
> top of the advantages you listed, it also only requires a single
> assignment in the driver's init function vs. a lot more today.
>
> If no one complains about the additional memory access, I'd like to go
> forward with this. I did much worse performance-hurting changes when
> introducing gpiod, so I suppose it will be fine.
>
>>
>> So this is an RFC patch-set to add a virtual table to be used by
>> GPIO chip controllers and consist of the following patches:
>>
>> Javier Martinez Canillas (5):
>> gpio: add a vtable to abstract GPIO controller operations
>> gpiolib: set gpio_chip operations on add using a gpio_chip_ops
>> gpio: omap: convert driver to use gpio_chip_ops
>> gpio: twl4030: convert driver to use gpio_chip_ops
>> gpio: switch to use struct struct gpio_chip_ops
>>
>> drivers/gpio/gpio-omap.c | 19 ++++++++-----
>> drivers/gpio/gpio-twl4030.c | 10 +++++--
>> drivers/gpio/gpiolib.c | 64 ++++++++++++++++++++---------------------
>> include/linux/gpio/driver.h | 69 +++++++++++++++++++++++++++------------------
>> 4 files changed, 93 insertions(+), 69 deletions(-)
>>
>> The patch-set is not a complete one though since only the GPIO OMAP
>> and GPIO TWL4030 drivers have been converted so I could test it on
>> my platform (DM3730 OMAP IGEPv2 board).
>>
>> But I preferred to send an early RFC than changing every single driver
>> before discussing if doing the split is worth it or not.
>>
>> To not break git bisect-ability, I added some patches that are
>> transitional changes. If you have a better suggestion on how to
>> handle that please let me know.
>
> We will probably need that transition phase. We will also need to
> switch every single driver to your new scheme, so please wait until we
> hear from Linus before proceeding. :)
>
I'm glad you agree with the idea, let's see what Linus thinks about it.
> Thanks,
> Alex.
>
Best regards,
Javier
On Thu, 2014-04-10 at 11:34 +0200, Javier Martinez Canillas wrote:
> On 04/10/2014 09:36 AM, Alexandre Courbot wrote:
> >
> > "Since having the operations" maybe?
> >
>
> Yes, since I'm not a native english speaker I sometimes miss some obvious
> grammatical errors. I'll fix those when posting the final version with all the
> drivers converted.
JFYI: there is nice project called codespell [1].
[1] git://github.com/lucasdemarchi/codespell.git
--
Andy Shevchenko <[email protected]>
Intel Finland Oy
Hello Andy,
On Thu, Apr 10, 2014 at 1:00 PM, Andy Shevchenko
<[email protected]> wrote:
> On Thu, 2014-04-10 at 11:34 +0200, Javier Martinez Canillas wrote:
>> On 04/10/2014 09:36 AM, Alexandre Courbot wrote:
>> >
>> > "Since having the operations" maybe?
>> >
>>
>> Yes, since I'm not a native english speaker I sometimes miss some obvious
>> grammatical errors. I'll fix those when posting the final version with all the
>> drivers converted.
>
> JFYI: there is nice project called codespell [1].
>
> [1] git://github.com/lucasdemarchi/codespell.git
Interesting project, thanks a lot for the pointer.
Best regards,
Javier
>
> --
> Andy Shevchenko <[email protected]>
> Intel Finland Oy
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 8, 2014 at 8:20 PM, Javier Martinez Canillas
<[email protected]> wrote:
> So this is an RFC patch-set to add a virtual table to be used by
> GPIO chip controllers and consist of the following patches:
Overall I like this.
However I don't want to see any transitional phase. I prefer a BIG
fat patch converting everyone and its dog to the new vtable and
removing the old function pointers. This can be based on the HEAD
of my GPIO devel branch.
It may be a good idea to use coccinelle for this refactoring in order
not to miss any users.
Yours,
Linus Walleij
Hello Linus,
On Tue, Apr 22, 2014 at 1:36 PM, Linus Walleij <[email protected]> wrote:
> On Tue, Apr 8, 2014 at 8:20 PM, Javier Martinez Canillas
> <[email protected]> wrote:
>
>> So this is an RFC patch-set to add a virtual table to be used by
>> GPIO chip controllers and consist of the following patches:
>
> Overall I like this.
>
> However I don't want to see any transitional phase. I prefer a BIG
> fat patch converting everyone and its dog to the new vtable and
> removing the old function pointers. This can be based on the HEAD
> of my GPIO devel branch.
>
Ok, I was adding a commit per GPIO driver but the patch-set would have
been very big (~200 patches).
> It may be a good idea to use coccinelle for this refactoring in order
> not to miss any users.
>
Agreed, I was manually searching for users by using grep but I agree
that is much safer to use coccinelle for this. I don't have previous
experience writing coccinelle semantics patches though so it may take
more time than I thought but it is the perfect excuse to finally learn
how to do it :-)
Thanks a lot and best regards,
Javier
> Yours,
> Linus Walleij
> --
> To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 22 April 2014, Javier Martinez Canillas wrote:
> Hello Linus,
>
> On Tue, Apr 22, 2014 at 1:36 PM, Linus Walleij <[email protected]> wrote:
> > On Tue, Apr 8, 2014 at 8:20 PM, Javier Martinez Canillas
> > <[email protected]> wrote:
> >
> >> So this is an RFC patch-set to add a virtual table to be used by
> >> GPIO chip controllers and consist of the following patches:
> >
> > Overall I like this.
Agreed, it's a very good cleanup.
> > However I don't want to see any transitional phase. I prefer a BIG
> > fat patch converting everyone and its dog to the new vtable and
> > removing the old function pointers. This can be based on the HEAD
> > of my GPIO devel branch.
> >
>
> Ok, I was adding a commit per GPIO driver but the patch-set would have
> been very big (~200 patches).
>
> > It may be a good idea to use coccinelle for this refactoring in order
> > not to miss any users.
> >
>
> Agreed, I was manually searching for users by using grep but I agree
> that is much safer to use coccinelle for this. I don't have previous
> experience writing coccinelle semantics patches though so it may take
> more time than I thought but it is the perfect excuse to finally learn
> how to do it :-)
I'm not a big fan of doing this all at once, but it's not my call here.
Just one recommendation: if you can't do an obvious coccinelle patch
to do everything at once, use extra patches in the beginning to clean
up the code enough to make it work, then have the large patch fully
automated.
Arnd