Re: [RFC PATCH 0/5] add gpio_chip_ops to hold GPIO operations

[Javier Martinez Canillas <javier.martinez@collabora.co.uk>]

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
> <javier.martinez@collabora.co.uk> 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