| Message ID | 1348780923-27428-1-git-send-email-stigge@antcom.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 23:22 Thu 27 Sep , Roland Stigge wrote: > The recurring task of providing simultaneous access to GPIO lines (especially > for bit banging protocols) needs an appropriate API. > > This patch adds a kernel internal "Block GPIO" API that enables simultaneous > access to several GPIOs in the same gpio_chip (bit mapped). Further, it adds a > sysfs interface (/sys/class/gpio/gpiochipXX/block). > > Signed-off-by: Roland Stigge <stigge@antcom.de> > > --- > NOTE: This is only useful if individual drivers implement the .get_block() and > .set_block() functions. I'm providing an example implementation for max730x > (see next patch), and can provide further driver patches after API review. > > Thanks in advance! > > Documentation/gpio.txt | 52 +++++++++++++++++++ > drivers/gpio/gpiolib.c | 121 +++++++++++++++++++++++++++++++++++++++++++++ > include/asm-generic/gpio.h | 7 ++ > include/linux/gpio.h | 24 ++++++++ > 4 files changed, 204 insertions(+) > > --- linux-2.6.orig/Documentation/gpio.txt > +++ linux-2.6/Documentation/gpio.txt > @@ -439,6 +439,51 @@ slower clock delays the rising edge of S > signaling rate accordingly. > > > +Block GPIO (optional) > +--------------------- > + > +The above described interface concentrates on handling single GPIOs. However, > +in applications where it is critical to set several GPIOs at once, this > +interface doesn't work well, e.g. bit-banging protocols via GPIO lines. > +Consider a GPIO controller that is connected via a slow I2C line. When > +switching two or more GPIOs one after another, there can be considerable time > +between those events. This is solved by an interface called Block GPIO: > + > +void gpio_get_block(unsigned int gpio, u8* values, size_t size); > +void gpio_set_block(unsigned int gpio, u8* set, u8* clr, size_t size); > + > +The function gpio_get_block() detects the current state of several GPIOs at > +once, practically by doing only one query at the hardware level (e.g. memory > +mapped or via bus transfers like I2C). There are some limits to this interface: > +A certain gpio_chip (see below) must be specified via the gpio parameter as the > +first GPIO in the gpio_chip group. The Block GPIO interface only supports > +simultaneous handling of GPIOs in the same gpio_chip group since different > +gpio_chips typically map to different GPIO hardware blocks. so basicaly you use a gpio numberthat you do not request, that is maybe requested. This is broken if you want to get or set block you need to pass the list of GPIO you want to control not some fancy magic Otherwise this will end be broken code. And how you can hope to describe this via DT Best Regards, J.
Hi! On 09/28/2012 04:47 AM, Jean-Christophe PLAGNIOL-VILLARD wrote: >> +Block GPIO (optional) >> +--------------------- >> + >> +The above described interface concentrates on handling single GPIOs. However, >> +in applications where it is critical to set several GPIOs at once, this >> +interface doesn't work well, e.g. bit-banging protocols via GPIO lines. >> +Consider a GPIO controller that is connected via a slow I2C line. When >> +switching two or more GPIOs one after another, there can be considerable time >> +between those events. This is solved by an interface called Block GPIO: >> + >> +void gpio_get_block(unsigned int gpio, u8* values, size_t size); >> +void gpio_set_block(unsigned int gpio, u8* set, u8* clr, size_t size); >> + >> +The function gpio_get_block() detects the current state of several GPIOs at >> +once, practically by doing only one query at the hardware level (e.g. memory >> +mapped or via bus transfers like I2C). There are some limits to this interface: >> +A certain gpio_chip (see below) must be specified via the gpio parameter as the >> +first GPIO in the gpio_chip group. The Block GPIO interface only supports >> +simultaneous handling of GPIOs in the same gpio_chip group since different >> +gpio_chips typically map to different GPIO hardware blocks. > so basicaly you use a gpio numberthat you do not request, that is maybe > requested. This is broken if you want to get or set block you need to pass the > list of GPIO you want to control not some fancy magic Right - will add checking for the request state of the respective GPIOs. The list of GPIOs to handle is defined by the offset (specified GPIO) and bitmapped list. If it looks more natural, I can change this to a list of ints specifying GPIOs directly. > And how you can hope to describe this via DT Haven't had planned that yet. Finally, this interface should just be another view on the GPIOs already requested / assigned. Or which additional info do you mean? Thanks for your notes! Roland
On 09:14 Fri 28 Sep , Roland Stigge wrote: > > Hi! > > On 09/28/2012 04:47 AM, Jean-Christophe PLAGNIOL-VILLARD wrote: > >> +Block GPIO (optional) > >> +--------------------- > >> + > >> +The above described interface concentrates on handling single GPIOs. However, > >> +in applications where it is critical to set several GPIOs at once, this > >> +interface doesn't work well, e.g. bit-banging protocols via GPIO lines. > >> +Consider a GPIO controller that is connected via a slow I2C line. When > >> +switching two or more GPIOs one after another, there can be considerable time > >> +between those events. This is solved by an interface called Block GPIO: > >> + > >> +void gpio_get_block(unsigned int gpio, u8* values, size_t size); > >> +void gpio_set_block(unsigned int gpio, u8* set, u8* clr, size_t size); > >> + > >> +The function gpio_get_block() detects the current state of several GPIOs at > >> +once, practically by doing only one query at the hardware level (e.g. memory > >> +mapped or via bus transfers like I2C). There are some limits to this interface: > >> +A certain gpio_chip (see below) must be specified via the gpio parameter as the > >> +first GPIO in the gpio_chip group. The Block GPIO interface only supports > >> +simultaneous handling of GPIOs in the same gpio_chip group since different > >> +gpio_chips typically map to different GPIO hardware blocks. > > so basicaly you use a gpio numberthat you do not request, that is maybe > > requested. This is broken if you want to get or set block you need to pass the > > list of GPIO you want to control not some fancy magic > > Right - will add checking for the request state of the respective GPIOs. > > The list of GPIOs to handle is defined by the offset (specified GPIO) > and bitmapped list. > > If it looks more natural, I can change this to a list of ints specifying > GPIOs directly. you pass the correctly information to the gpiolib as if you do not request the gpio the gpiolib will auto request the gpios so you api will be int gpio_get_block(unsigned int *gpios, u8* values, size_t size); return an error int gpio_set_block(unsigned int *gpios, u8* set, size_t size); so you do not care about the banks you work on the gpiolib framework will call each gpio_chip seperatly. If the set_block get_block is not availlable the gpiolib could fallback to get/set inside of the gpiolib that you call each bank with a bitmapped list is correct but not in the public gpiolib API > > > And how you can hope to describe this via DT > > Haven't had planned that yet. Finally, this interface should just be > another view on the GPIOs already requested / assigned. Or which > additional info do you mean? how do you plan to give the gpio base vai DT to the driver as via DT we just pass the list of GPIO to work on > > Thanks for your notes! > > Roland
Hi! On 09/28/2012 09:51 AM, Jean-Christophe PLAGNIOL-VILLARD wrote: >> Right - will add checking for the request state of the respective GPIOs. >> >> The list of GPIOs to handle is defined by the offset (specified GPIO) >> and bitmapped list. >> >> If it looks more natural, I can change this to a list of ints specifying >> GPIOs directly. > you pass the correctly information to the gpiolib > > as if you do not request the gpio the gpiolib will auto request the gpios > so you api will be > int gpio_get_block(unsigned int *gpios, u8* values, size_t size); > > return an error > > int gpio_set_block(unsigned int *gpios, u8* set, size_t size); > > so you do not care about the banks you work on the gpiolib framework will call > each gpio_chip seperatly. If the set_block get_block is not availlable the > gpiolib could fallback to get/set > > inside of the gpiolib that you call each bank with a bitmapped list is correct > but not in the public gpiolib API Good idea! Talking about the public API (your above gpio_set_block()): *gpios is a list of GPIOs, but set is still bitmapped (mapped onto the list specified in *gpios)? To prevent confusion about what the size argument means (number of gpios in *gpios _or_ number of bytes in the bitmap *set) - wouldn't it be clearer to have a "bool *set" and "bool *values" list? >>> And how you can hope to describe this via DT >> >> Haven't had planned that yet. Finally, this interface should just be >> another view on the GPIOs already requested / assigned. Or which >> additional info do you mean? > how do you plan to give the gpio base vai DT to the driver as via DT we just > pass the list of GPIO to work on Right. If I understand correctly, with the above discussed changes, this "GPIO base" issue is gone... Thanks for your feedback! Roland
On 10:51 Fri 28 Sep , Roland Stigge wrote: > Hi! > > On 09/28/2012 09:51 AM, Jean-Christophe PLAGNIOL-VILLARD wrote: > >> Right - will add checking for the request state of the respective GPIOs. > >> > >> The list of GPIOs to handle is defined by the offset (specified GPIO) > >> and bitmapped list. > >> > >> If it looks more natural, I can change this to a list of ints specifying > >> GPIOs directly. > > you pass the correctly information to the gpiolib > > > > as if you do not request the gpio the gpiolib will auto request the gpios > > so you api will be > > int gpio_get_block(unsigned int *gpios, u8* values, size_t size); > > > > return an error > > > > int gpio_set_block(unsigned int *gpios, u8* set, size_t size); > > > > so you do not care about the banks you work on the gpiolib framework will call > > each gpio_chip seperatly. If the set_block get_block is not availlable the > > gpiolib could fallback to get/set > > > > inside of the gpiolib that you call each bank with a bitmapped list is correct > > but not in the public gpiolib API > > Good idea! Talking about the public API (your above gpio_set_block()): > *gpios is a list of GPIOs, but set is still bitmapped (mapped onto the > list specified in *gpios)? To prevent confusion about what the size > argument means (number of gpios in *gpios _or_ number of bytes in the > bitmap *set) - wouldn't it be clearer to have a "bool *set" and "bool > *values" list? public API list of gpio as example gpios = {1, 33, 34, 55}; set = {1, 0, 0 ,1}; gpio_set_blocks(gpios, set, 4); private you do just provide the array related to the gpio_chip lets assume 4 bank with 32 gpio each gpio0 = {1}; set0 = {1}; gpio1 = {33, 34}; set1 = {0, 0}; gpio2 = {55}; set2 = {1}; set_blocks(gpio_chip0, gpio0, set0, 1); set_blocks(gpio_chip1, gpio1, set1, 2); set_blocks(gpio_chip2, gpio2, set2, 1); Best Regards, J.
On Thu, Sep 27, 2012 at 11:22 PM, Roland Stigge <stigge@antcom.de> wrote: Grant really need to look at this patch too... > The recurring task of providing simultaneous access to GPIO lines (especially > for bit banging protocols) needs an appropriate API. > > This patch adds a kernel internal "Block GPIO" API that enables simultaneous > access to several GPIOs in the same gpio_chip (bit mapped). Further, it adds a > sysfs interface (/sys/class/gpio/gpiochipXX/block). I've had others talk about the need for this in the past, I think it may have been Bill Gatliff who brought it up. I'm pretty sure it's a need for the industry so we really need something like this, thanks for working on it Roland. > Documentation/gpio.txt | 52 +++++++++++++++++++ > drivers/gpio/gpiolib.c | 121 +++++++++++++++++++++++++++++++++++++++++++++ > include/asm-generic/gpio.h | 7 ++ > include/linux/gpio.h | 24 ++++++++ > 4 files changed, 204 insertions(+) You probably want to patch Documentation/ABI/testing/sysfs-gpio as well. > @@ -686,6 +731,13 @@ read-only attributes: > > "ngpio" ... how many GPIOs this manges (N to N + ngpio - 1) > > + "block" ... get/set Block GPIO: > + * reads: space separated list of GPIO inputs of this chip that > + are set to 1, e.g. "83 85 87 99" > + * write: space separated list of GPIO outputs of this chip > + that are to be set or cleared, e.g. "80 -83 -85" (prefix > + "-" clears) This sort of breaks the sysfs convention of one value per file, does it not? It's not like I have some better idea, just we need to think about other possible solutions. The GPIO sysfs interface is not universally liked. What are the typical applications you have for this? Industrial control by bit-banging userspace processes? I've heard about people doing that kind of things and running these processes as real-time scheduled and then running e.g. factory lines and other scary stuff through the GPIO sysfs ... J-C:s comments are valid as well, will not repeat them. Yours, Linus Walleij
On 09/28/2012 11:08 AM, Jean-Christophe PLAGNIOL-VILLARD wrote: >> Good idea! Talking about the public API (your above gpio_set_block()): >> *gpios is a list of GPIOs, but set is still bitmapped (mapped onto the >> list specified in *gpios)? To prevent confusion about what the size >> argument means (number of gpios in *gpios _or_ number of bytes in the >> bitmap *set) - wouldn't it be clearer to have a "bool *set" and "bool >> *values" list? > public API list of gpio as example > > gpios = {1, 33, 34, 55}; > set = {1, 0, 0 ,1}; > > gpio_set_blocks(gpios, set, 4); > > private you do just provide the array related to the gpio_chip > lets assume 4 bank with 32 gpio each > > gpio0 = {1}; > set0 = {1}; > > gpio1 = {33, 34}; > set1 = {0, 0}; > > gpio2 = {55}; > set2 = {1}; > > set_blocks(gpio_chip0, gpio0, set0, 1); > set_blocks(gpio_chip1, gpio1, set1, 2); > set_blocks(gpio_chip2, gpio2, set2, 1); Good. For the internal driver API (gpio_chip), we even don't really need the first argument (gpio_chip) since we can infer it from the gpios. Will provide an update. Roland
On 09/28/2012 11:14 AM, Linus Walleij wrote: >> @@ -686,6 +731,13 @@ read-only attributes: >> >> "ngpio" ... how many GPIOs this manges (N to N + ngpio - 1) >> >> + "block" ... get/set Block GPIO: >> + * reads: space separated list of GPIO inputs of this chip that >> + are set to 1, e.g. "83 85 87 99" >> + * write: space separated list of GPIO outputs of this chip >> + that are to be set or cleared, e.g. "80 -83 -85" (prefix >> + "-" clears) > > This sort of breaks the sysfs convention of one value per file, > does it not? > > It's not like I have some better idea, just we need to think about > other possible solutions. > > The GPIO sysfs interface is not universally liked. What are the > typical applications you have for this? Industrial control by > bit-banging userspace processes? Yes, I had several projects in the past with the need of setting groups of GPIOs at once (typically, 8 bit busses via GPIO lines), so needed to provide some hacks. Don't want to do this over and over again. :-) Bit-banging in kernel and userspace. It's hard to do the one-value-per-file right for a several-gpios-at-once goal. :-) I originally had a one-value solution: A bit map, continuously hex coded, like in the original kernel API idea (e.g. 0x000F0A0010). Wasn't sure because it encodes GPIO numbers in a weird way. Strictly formally: Isn't a comma-separated list of a GPIO block (e.g. "80,81,85") a singe value in a sense? :-) Or other possibilities? Maybe some node in /proc? Or some kind of new character device node? Otherwise, I need to think about leaving out the sysfs for this purpose. Thanks in advance, Roland
On 11:08 Fri 28 Sep , Jean-Christophe PLAGNIOL-VILLARD wrote: > On 10:51 Fri 28 Sep , Roland Stigge wrote: > > Hi! > > > > On 09/28/2012 09:51 AM, Jean-Christophe PLAGNIOL-VILLARD wrote: > > >> Right - will add checking for the request state of the respective GPIOs. > > >> > > >> The list of GPIOs to handle is defined by the offset (specified GPIO) > > >> and bitmapped list. > > >> > > >> If it looks more natural, I can change this to a list of ints specifying > > >> GPIOs directly. > > > you pass the correctly information to the gpiolib > > > > > > as if you do not request the gpio the gpiolib will auto request the gpios > > > so you api will be > > > int gpio_get_block(unsigned int *gpios, u8* values, size_t size); > > > > > > return an error > > > > > > int gpio_set_block(unsigned int *gpios, u8* set, size_t size); > > > > > > so you do not care about the banks you work on the gpiolib framework will call > > > each gpio_chip seperatly. If the set_block get_block is not availlable the > > > gpiolib could fallback to get/set > > > > > > inside of the gpiolib that you call each bank with a bitmapped list is correct > > > but not in the public gpiolib API > > > > Good idea! Talking about the public API (your above gpio_set_block()): > > *gpios is a list of GPIOs, but set is still bitmapped (mapped onto the > > list specified in *gpios)? To prevent confusion about what the size > > argument means (number of gpios in *gpios _or_ number of bytes in the > > bitmap *set) - wouldn't it be clearer to have a "bool *set" and "bool > > *values" list? > public API list of gpio as example > > gpios = {1, 33, 34, 55}; > set = {1, 0, 0 ,1}; > > gpio_set_blocks(gpios, set, 4); > > private you do just provide the array related to the gpio_chip > lets assume 4 bank with 32 gpio each > > gpio0 = {1}; > set0 = {1}; > > gpio1 = {33, 34}; here should be pin0 = {1}; set0 = {1}; pin1 = {1, 2, 23}; set1 = {0, 0, 1}; set_blocks(gpio_chip0, pin0, set0, 1); set_blocks(gpio_chip1, pin1, set1, 3); You may need to add a prepare to do not do the conversion between array and gpio_chip array as I guess you will work on gpio block with multiple time acces Best Regards, J.
On 09/28/2012 12:28 PM, Jean-Christophe PLAGNIOL-VILLARD wrote: >> gpio0 = {1}; >> set0 = {1}; >> >> gpio1 = {33, 34}; > here should be > > pin0 = {1}; > set0 = {1}; > > pin1 = {1, 2, 23}; > set1 = {0, 0, 1}; > > set_blocks(gpio_chip0, pin0, set0, 1); > set_blocks(gpio_chip1, pin1, set1, 3); > > You may need to add a prepare to do not do the conversion between array and > gpio_chip array as I guess you will work on gpio block with multiple time > acces Maybe like this, for some struct block *? block = set_block_prepare(gc, pins, values, size); if (block) { set_block(gc, block); ... set_block_unprepare(gc, block); } Would mean that all supported drivers would need to implement those 3 new functions... Need to be careful about not introducing bloat... Thanks, Roland
On Fri, Sep 28, 2012 at 11:52 AM, Roland Stigge <stigge@antcom.de> wrote: > It's hard to do the one-value-per-file right for a several-gpios-at-once > goal. :-) That's true. This is one of the reasons I think GPIO chips should be /dev/gpioN device nodes and this business be handled by ioctl():s instead, it is harder from an ABI point of view but can do several things at once. > I originally had a one-value solution: A bit map, continuously > hex coded, like in the original kernel API idea (e.g. 0x000F0A0010). > Wasn't sure because it encodes GPIO numbers in a weird way. Yeah, well that is not so nice either. > Strictly formally: Isn't a comma-separated list of a GPIO block (e.g. > "80,81,85") a singe value in a sense? :-) I think it's called an array ... > Or other possibilities? Maybe > some node in /proc? Heaven's no. > Or some kind of new character device node? Yes, I don't know if we should create /dev/gpioN or /dev/pinctrlN for this, and add GPIO functions to pinctrl (while wrapping it in the gpiolib to aid migration) the latter would encourage users of the new ABI to switch to writing pinctrl drivers. > Otherwise, I need to think about leaving out the sysfs for this purpose. Do you really need it? Yours, Linus Walleij
Hi, On 09/28/2012 01:34 PM, Linus Walleij wrote: >> Or some kind of new character device node? > > Yes, I don't know if we should create /dev/gpioN or /dev/pinctrlN for > this, and add GPIO functions to pinctrl (while wrapping it in the > gpiolib to aid migration) the latter would encourage users of the > new ABI to switch to writing pinctrl drivers. Will have a look at this. >> Otherwise, I need to think about leaving out the sysfs for this purpose. > > Do you really need it? In more that half of the cases, I needed userspace access. So I'm quite interested in it. Thanks for your notes. Roland
On 13:32 Fri 28 Sep , Roland Stigge wrote: > On 09/28/2012 12:28 PM, Jean-Christophe PLAGNIOL-VILLARD wrote: > >> gpio0 = {1}; > >> set0 = {1}; > >> > >> gpio1 = {33, 34}; > > here should be > > > > pin0 = {1}; > > set0 = {1}; > > > > pin1 = {1, 2, 23}; > > set1 = {0, 0, 1}; > > > > set_blocks(gpio_chip0, pin0, set0, 1); > > set_blocks(gpio_chip1, pin1, set1, 3); > > > > You may need to add a prepare to do not do the conversion between array and > > gpio_chip array as I guess you will work on gpio block with multiple time > > acces > > Maybe like this, for some struct block *? > > block = set_block_prepare(gc, pins, values, size); > if (block) { > set_block(gc, block); > ... > set_block_unprepare(gc, block); > } > > Would mean that all supported drivers would need to implement those 3 > new functions... Need to be careful about not introducing bloat... the prepare is gpiolib specific, it will be a helper to conver a gpio list to a gpio block list I was thinking more block = gpio_block_prepare(pins, size); gpio_block_set_value(pin0, val); gpio_block_set_value(pin1, val); gpio_block_set_value(pin2, val); gpio_block_set(block); andfor get gpio_block_get(block) val = gpio_block_get_value(block, pin0); val = gpio_block_get_value(block, pin1); for the gpio driver ti's transparent Best Regards, J.
Hi, On 28/09/12 18:01, Jean-Christophe PLAGNIOL-VILLARD wrote: >> Maybe like this, for some struct block *? >> >> block = set_block_prepare(gc, pins, values, size); >> if (block) { >> set_block(gc, block); >> ... >> set_block_unprepare(gc, block); >> } >> >> Would mean that all supported drivers would need to implement those 3 >> new functions... Need to be careful about not introducing bloat... > the prepare is gpiolib specific, it will be a helper to conver a gpio list to > a gpio block list > > I was thinking more > > block = gpio_block_prepare(pins, size); > > gpio_block_set_value(pin0, val); > gpio_block_set_value(pin1, val); > gpio_block_set_value(pin2, val); > gpio_block_set(block); > > andfor get > > gpio_block_get(block) > val = gpio_block_get_value(block, pin0); > val = gpio_block_get_value(block, pin1); > > for the gpio driver ti's transparent Problem here is that it's only an intermediate format since hardware often needs special preparation of the data. But will evaluate what makes most sense. Thanks for your notes! Roland
On 20:32 Fri 28 Sep , Roland Stigge wrote: > Hi, > > On 28/09/12 18:01, Jean-Christophe PLAGNIOL-VILLARD wrote: > >>Maybe like this, for some struct block *? > >> > >>block = set_block_prepare(gc, pins, values, size); > >>if (block) { > >> set_block(gc, block); > >> ... > >> set_block_unprepare(gc, block); > >>} > >> > >>Would mean that all supported drivers would need to implement those 3 > >>new functions... Need to be careful about not introducing bloat... > >the prepare is gpiolib specific, it will be a helper to conver a gpio list to > >a gpio block list > > > >I was thinking more > > > >block = gpio_block_prepare(pins, size); > > > >gpio_block_set_value(pin0, val); > >gpio_block_set_value(pin1, val); > >gpio_block_set_value(pin2, val); > >gpio_block_set(block); > > > >andfor get > > > >gpio_block_get(block) > >val = gpio_block_get_value(block, pin0); > >val = gpio_block_get_value(block, pin1); > > > >for the gpio driver ti's transparent > > Problem here is that it's only an intermediate format since hardware > often needs special preparation of the data. > > But will evaluate what makes most sense. the key point here is to avoid to manipualte data each time we call gpio_block_set hardware specific will have to be handle at driver level Best Regards, J.
On Thu, Sep 27, 2012 at 11:22 PM, Roland Stigge <stigge@antcom.de> wrote: > The recurring task of providing simultaneous access to GPIO lines > (especially > for bit banging protocols) needs an appropriate API. > > This patch adds a kernel internal "Block GPIO" API that enables > simultaneous > access to several GPIOs in the same gpio_chip (bit mapped). Further, it > adds a > sysfs interface (/sys/class/gpio/gpiochipXX/block). > > Signed-off-by: Roland Stigge <stigge@antcom.de> > > --- > NOTE: This is only useful if individual drivers implement the .get_block() > and > .set_block() functions. I'm providing an example implementation for max730x > (see next patch), and can provide further driver patches after API review. > > Thanks in advance! > > Hi Roland, In our kernel tree we have similar code. If you like I can request permission to share. I can, however, already give you an update on the basic structure, perhaps it's useful now. For the first part, the drivers need to implement a the gpio interface for groups. gpio_set_multi, gpio_get_multi, gpio_direction_input_multi, gpio_direction_output_multi. Each of them gets a 'u32 mask'. Secondly, gpiolib gets some new code to handle groups: groups are requested via a list of gpio ids. Mind that order is respected: request( [1, 5, 2, 4] ) followed by a set(0x5) will translate to gpio_set_multi( 0x18 ). An opaque gpio_group struct is used to keep track. This means the gpiolib interface also has a u32 mask, but translation is done for the gpio-drivers. There is some code to request groups via device-tree (again respecting order) and there are also platform driver structures. gpiolib was also extended to export groups into sysfs, respecting policy (input, output, user-selectable) and to make softlinks to groups in other driver's subdir. (One driver we use this in is a power-sequencer with 2 gpios selecting a margining profile, this driver then has the gpio_group exported in it's sysfs dir as .../profile, allowing H/W engineers to select the profile without voltage glitches) There's also a separate driver, that does nothing more than exporting both individual pins and groups to userspace based on platform description or devicetree. This is probably less interesting for mainline, since we're abusing device-tree to do away with some init script that can do the same. The rationale behind a 32bit mask is that typical processors can at most set one processor-word worth of GPIOs at once and there are probably few chips with over 32GPIOs on a single gpio_chip anyway. Nevertheless, in the era of 64bit, it's definitely possible to go for u64 instead. Let me know if this looks like something usable to you. Regards, Stijn
On 29/09/12 21:57, Jean-Christophe PLAGNIOL-VILLARD wrote: >> Problem here is that it's only an intermediate format since hardware >> often needs special preparation of the data. >> >> But will evaluate what makes most sense. > the key point here is to avoid to manipualte data each time we call > gpio_block_set > > hardware specific will have to be handle at driver level Understand, thanks! I'm just trying to prevent overly complex API because: * In our discussed scheme, the driver still needs to convert the data bits * In practice, the block gpio API is especially useful for use on single gpio_chips (only there, a real simultaneous i/o is possible anyway) * Wouldn't introduce this kind of optimization in lack of measurable improvement * The actual i/o data bits still need handling, generating a bit CPU load anyway. Trying to provide as simple API as possible. Roland
On 30/09/12 11:35, Stijn Devriendt wrote: > In our kernel tree we have similar code. If you like I can request > permission > to share. I can, however, already give you an update on the basic > structure, perhaps > it's useful now. > > For the first part, the drivers need to implement a the gpio interface > for groups. > gpio_set_multi, gpio_get_multi, gpio_direction_input_multi, > gpio_direction_output_multi. Each of them gets a 'u32 mask'. > > Secondly, gpiolib gets some new code to handle groups: > groups are requested via a list of gpio ids. Mind that order is respected: > request( [1, 5, 2, 4] ) followed by a set(0x5) will translate to > gpio_set_multi( 0x18 ). An opaque gpio_group struct is used to keep track. > This means the gpiolib interface also has a u32 mask, but translation is > done for the gpio-drivers. > > There is some code to request groups via device-tree (again respecting > order) > and there are also platform driver structures. > > gpiolib was also extended to export groups into sysfs, respecting policy > (input, output, user-selectable) and to make softlinks to groups in other > driver's subdir. (One driver we use this in is a power-sequencer with 2 > gpios selecting a margining profile, this driver then has the gpio_group > exported in it's sysfs dir as .../profile, allowing H/W engineers to select > the profile without voltage glitches) > > There's also a separate driver, that does nothing more than exporting > both individual pins and groups to userspace based on platform description > or devicetree. This is probably less interesting for mainline, since we're > abusing device-tree to do away with some init script that can do the same. > > The rationale behind a 32bit mask is that typical processors can at most > set one processor-word worth of GPIOs at once and there are probably > few chips with over 32GPIOs on a single gpio_chip anyway. > Nevertheless, in the era of 64bit, it's definitely possible to go for > u64 instead. Hi Stijn, thank you for your notes! Besides what I discussed with JC and Linus, I find the unsigned int (i.e. u32 or u64, depending on the arch) quite appealing. It is a nice compromise between my general bit mapped data model (variable size *u8 array) and the bool *values list. Even maps easily onto a single sysfs entry for values, by abstracting a gpio list to an actual data word. What do others think? JC? Linus? I'm considering this (unsigned int data) a valid option. One question: How did you solve the one-value-per-file in the sysfs interface? Thanks in advance! Roland
On Sun, Sep 30, 2012 at 12:50 PM, Roland Stigge <stigge@antcom.de> wrote: > On 30/09/12 11:35, Stijn Devriendt wrote: >> In our kernel tree we have similar code. If you like I can request >> permission >> to share. I can, however, already give you an update on the basic >> structure, perhaps >> it's useful now. >> >> For the first part, the drivers need to implement a the gpio interface >> for groups. >> gpio_set_multi, gpio_get_multi, gpio_direction_input_multi, >> gpio_direction_output_multi. Each of them gets a 'u32 mask'. >> >> Secondly, gpiolib gets some new code to handle groups: >> groups are requested via a list of gpio ids. Mind that order is respected: >> request( [1, 5, 2, 4] ) followed by a set(0x5) will translate to >> gpio_set_multi( 0x18 ). An opaque gpio_group struct is used to keep track. >> This means the gpiolib interface also has a u32 mask, but translation is >> done for the gpio-drivers. >> >> There is some code to request groups via device-tree (again respecting >> order) >> and there are also platform driver structures. >> >> gpiolib was also extended to export groups into sysfs, respecting policy >> (input, output, user-selectable) and to make softlinks to groups in other >> driver's subdir. (One driver we use this in is a power-sequencer with 2 >> gpios selecting a margining profile, this driver then has the gpio_group >> exported in it's sysfs dir as .../profile, allowing H/W engineers to select >> the profile without voltage glitches) >> >> There's also a separate driver, that does nothing more than exporting >> both individual pins and groups to userspace based on platform description >> or devicetree. This is probably less interesting for mainline, since we're >> abusing device-tree to do away with some init script that can do the same. >> >> The rationale behind a 32bit mask is that typical processors can at most >> set one processor-word worth of GPIOs at once and there are probably >> few chips with over 32GPIOs on a single gpio_chip anyway. >> Nevertheless, in the era of 64bit, it's definitely possible to go for >> u64 instead. > > Hi Stijn, > > thank you for your notes! > > Besides what I discussed with JC and Linus, I find the unsigned int > (i.e. u32 or u64, depending on the arch) quite appealing. It is a nice > compromise between my general bit mapped data model (variable size *u8 > array) and the bool *values list. Even maps easily onto a single sysfs > entry for values, by abstracting a gpio list to an actual data word. > > What do others think? JC? Linus? I'm considering this (unsigned int > data) a valid option. > > One question: How did you solve the one-value-per-file in the sysfs > interface? > By exporting the group as a whole: /sys/.../gpiogroup248/value where value contains a decimal representing the group value. Again, this respects the ordering of the pins: Actual pins: 0x2D (b 0010 1101) Selected pins: 6 3 0 1 Readout: 6 (b 0 1 1 0) The export sysfs file does, however, accept multiple gpio IDs for groups. Not sure if this is a 'violation' per se... If the user stores a single value he gets a single pin, multiple (space-separated) values give him a group. Regards, Stijn
Hi Stijn, On 30/09/12 16:52, Stijn Devriendt wrote: >> One question: How did you solve the one-value-per-file in the sysfs >> interface? >> > By exporting the group as a whole: > /sys/.../gpiogroup248/value > where value contains a decimal representing the group value. > Again, this respects the ordering of the pins: > > Actual pins: 0x2D (b 0010 1101) > Selected pins: 6 3 0 1 > Readout: 6 (b 0 1 1 0) > > The export sysfs file does, however, accept multiple gpio IDs for groups. > Not sure if this is a 'violation' per se... If I understand correctly, it's a violation (single-value should hold for read and write). To solve it, I have the following in mind: /sys/.../gpiogroupXXX/ contains files "bit0" ... "bit31" which contain a gpio number each, empty if "unconnected". Roland
On Sun, Sep 30, 2012 at 12:34 PM, Roland Stigge <stigge@antcom.de> wrote: > On 29/09/12 21:57, Jean-Christophe PLAGNIOL-VILLARD wrote: >>> Problem here is that it's only an intermediate format since hardware >>> often needs special preparation of the data. >>> >>> But will evaluate what makes most sense. >> the key point here is to avoid to manipualte data each time we call >> gpio_block_set >> >> hardware specific will have to be handle at driver level > > Understand, thanks! I'm just trying to prevent overly complex API because: > > * In our discussed scheme, the driver still needs to convert the data bits The u32 mask scheme fits simple-gpio drivers (1 register for input/output, 1 for direction, e.g. mpc8xxx driver) pretty well. Consider that to get true synchronous output behavior of multiple pins, this is probably the only option anyway. For the cavium octeon driver (which doesn't seem to be upstream yet; sports a single register for readout, per-pin write register) you can only emulate group behavior by looping over the mask, without synchronous behavior. > * In practice, the block gpio API is especially useful for use on single > gpio_chips (only there, a real simultaneous i/o is possible anyway) > * Wouldn't introduce this kind of optimization in lack of measurable > improvement > * The actual i/o data bits still need handling, generating a bit CPU > load anyway. I believe it's not worth it to try and save some tens of CPU cycles, considering that GPIO pins might be on some slower bus anyway. Even then, simple-gpio drivers have 0 added overhead, because they can use the mask straight away. That means all processing is done in gpiolib's code and is limited to a loop of 32 iterations at most... For drivers where performance really is critical, you could probably precompute the needed values: sclmask = gpio_group_precompute(MY_I2C_SCL) sdamask = gpio_group_precompute(MY_I2C_SDA) gpio_group_set_direct(sclmask | sdamask) Regards, Stijn
On Sun, Sep 30, 2012 at 5:09 PM, Roland Stigge <stigge@antcom.de> wrote: > Hi Stijn, > > On 30/09/12 16:52, Stijn Devriendt wrote: >>> One question: How did you solve the one-value-per-file in the sysfs >>> interface? >>> >> By exporting the group as a whole: >> /sys/.../gpiogroup248/value >> where value contains a decimal representing the group value. >> Again, this respects the ordering of the pins: >> >> Actual pins: 0x2D (b 0010 1101) >> Selected pins: 6 3 0 1 >> Readout: 6 (b 0 1 1 0) >> >> The export sysfs file does, however, accept multiple gpio IDs for groups. >> Not sure if this is a 'violation' per se... > > If I understand correctly, it's a violation (single-value should hold > for read and write). > > To solve it, I have the following in mind: /sys/.../gpiogroupXXX/ > contains files "bit0" ... "bit31" which contain a gpio number each, > empty if "unconnected". Unfortunately that means you can't atomically create a group. It also creates a mess to keep ordering intact and to either keep the current pin state or override it at allocation-time. Rules are rules, but why make the interface overly complex when the multi-value file is saner, cleaner and simpler? I know I'm on the happy/corporate side of things and I can violate whatever rule I can if it gets our product shipping, but I'd still propose to have the multi-value approach, even in mainline. Regards, Stijn
On 30/09/12 17:19, Stijn Devriendt wrote: >> If I understand correctly, it's a violation (single-value should hold >> for read and write). >> >> To solve it, I have the following in mind: /sys/.../gpiogroupXXX/ >> contains files "bit0" ... "bit31" which contain a gpio number each, >> empty if "unconnected". > > Unfortunately that means you can't atomically create a group. I don't see a big advantage of having atomic create/request. Most important is set/get, isn't it? I assume the following usage pattern: * Create(request) - non atomic (maybe atomic but why not add GPIOs later?) * Set - atomic * Get - atomic * ... > It also creates a mess to keep ordering intact and to either > keep the current pin state or override it at allocation-time. Ordering should stay intact, and later add/delete operations could be possible. I meant bit0 ... bit31 in the gpio block as such: bit0 - "80" bit1 - "" (i.e. unconnected) bit2 - "85" bit3 - "2" ... bit31 - "" This scheme can support multiple gpio_chips, as discussed with Linus and JC, which of course can't always guarantee real simultaneous I/O but provide virtual I/O word access (32bit/64bit). > Rules are rules, but why make the interface overly complex when > the multi-value file is saner, cleaner and simpler? Simply because they won't (and probably shouldn't) accept it mainline. Roland
On Thu, Sep 27, 2012 at 11:22:02PM +0200, Roland Stigge wrote: > The recurring task of providing simultaneous access to GPIO lines (especially > for bit banging protocols) needs an appropriate API. > This patch adds a kernel internal "Block GPIO" API that enables simultaneous > access to several GPIOs in the same gpio_chip (bit mapped). Further, it adds a > sysfs interface (/sys/class/gpio/gpiochipXX/block). It would be very useful if this had the option of falling back to addressing the GPIOs one by one. There's some usages that aren't performance dependent and it'd make drivers able to run even if they're suboptimal.
On Sun, Sep 30, 2012 at 12:50 PM, Roland Stigge <stigge@antcom.de> wrote: > Besides what I discussed with JC and Linus, I find the unsigned int > (i.e. u32 or u64, depending on the arch) quite appealing. It is a nice > compromise between my general bit mapped data model (variable size *u8 > array) and the bool *values list. Even maps easily onto a single sysfs > entry for values, by abstracting a gpio list to an actual data word. > > What do others think? JC? Linus? I'm considering this (unsigned int > data) a valid option. I think we mostly use an unsigned long for such stuff as IRQ flags and ioctl() parameters in the kernel. In this case it has the upside that it will be 32bit on 32bit systems and 64bit on 64bit systems if I'm not mistaken. Yours, Linus Walleij
On Sun, Sep 30, 2012 at 5:46 PM, Roland Stigge <stigge@antcom.de> wrote: > On 30/09/12 17:19, Stijn Devriendt wrote: >> >> Rules are rules, but why make the interface overly complex when >> the multi-value file is saner, cleaner and simpler? > > Simply because they won't (and probably shouldn't) accept it mainline. We, including you and Stijn *are* the mainline ... ;-) The only reason I really dislike it is that the GPIO sysfs interface is scary as it is, so I don't want to add to it if we can instead push to reform it into something more sane. Linus Walleij
Hi, On 04/10/12 01:07, Linus Walleij wrote: >> What do others think? JC? Linus? I'm considering this (unsigned int >> data) a valid option. > > I think we mostly use an unsigned long for such stuff as IRQ flags > and ioctl() parameters in the kernel. > > In this case it has the upside that it will be 32bit on 32bit systems > and 64bit on 64bit systems if I'm not mistaken. Fine. Will try to prepare a patch tomorrow, including fallback to single GPIO handling (if the driver doesn't implement block operations) and omitting the sysfs interface in the first patch. Roland
--- linux-2.6.orig/Documentation/gpio.txt +++ linux-2.6/Documentation/gpio.txt @@ -439,6 +439,51 @@ slower clock delays the rising edge of S signaling rate accordingly. +Block GPIO (optional) +--------------------- + +The above described interface concentrates on handling single GPIOs. However, +in applications where it is critical to set several GPIOs at once, this +interface doesn't work well, e.g. bit-banging protocols via GPIO lines. +Consider a GPIO controller that is connected via a slow I2C line. When +switching two or more GPIOs one after another, there can be considerable time +between those events. This is solved by an interface called Block GPIO: + +void gpio_get_block(unsigned int gpio, u8* values, size_t size); +void gpio_set_block(unsigned int gpio, u8* set, u8* clr, size_t size); + +The function gpio_get_block() detects the current state of several GPIOs at +once, practically by doing only one query at the hardware level (e.g. memory +mapped or via bus transfers like I2C). There are some limits to this interface: +A certain gpio_chip (see below) must be specified via the gpio parameter as the +first GPIO in the gpio_chip group. The Block GPIO interface only supports +simultaneous handling of GPIOs in the same gpio_chip group since different +gpio_chips typically map to different GPIO hardware blocks. + +The values and size (in bytes) arguments specify a bit field of consecutive +values for the GPIOs in this gpio_chip group, relative to the specified GPIO. +E.g., when the gpio_chip group contains 16 GPIOs (80-95), size is 2 and values +points to an array of two bytes, the first of which contains the input values +of GPIOs 80-87 and the second one the values of GPIOs 88-95. + +Setting and clearing can be done via gpio_set_block(). Similar to the values +argument of gpio_get_block(), the arrays pointed to by set and clr contain bit +mapped lists of GPIOs to set and clear. This way, it is possible to +simultaneously set e.g. GPIOs 10 and 12 and clear GPIO 3, leaving the others in +the current state. The size argument refers to both set and clr which must be +sized equally. + +Another limit of this interface is that although gpio_get_block() and +gpio_set_block() are valid for all gpio_chips, they only work as expected where +the actual hardware really supports setting and clearing simultaneously. Some +GPIO hardware can only set simultaneously or clear simultaneously, but not set +and clear simultaneously. Further, the respective GPIO driver must implement +the .get_block() and .set_block() functions in their struct gpio_chip +efficiently. If they default to NULL, gpiolib uses .get() and .set() functions +as backup, which effectively leads to non-simultaneous GPIO handling. Please +check the actual GPIO driver you are using. + + What do these conventions omit? =============================== One of the biggest things these conventions omit is pin multiplexing, since @@ -686,6 +731,13 @@ read-only attributes: "ngpio" ... how many GPIOs this manges (N to N + ngpio - 1) + "block" ... get/set Block GPIO: + * reads: space separated list of GPIO inputs of this chip that + are set to 1, e.g. "83 85 87 99" + * write: space separated list of GPIO outputs of this chip + that are to be set or cleared, e.g. "80 -83 -85" (prefix + "-" clears) + Board documentation should in most cases cover what GPIOs are used for what purposes. However, those numbers are not always stable; GPIOs on a daughtercard might be different depending on the base board being used, --- linux-2.6.orig/drivers/gpio/gpiolib.c +++ linux-2.6/drivers/gpio/gpiolib.c @@ -589,10 +589,114 @@ static ssize_t chip_ngpio_show(struct de } static DEVICE_ATTR(ngpio, 0444, chip_ngpio_show, NULL); + +static ssize_t chip_block_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct gpio_chip *chip = dev_get_drvdata(dev); + size_t size = (chip->ngpio + 7) / 8; + u8 *bits; + int ret = 0; + int i, chars; + + bits = kzalloc(size, GFP_KERNEL); + + if (chip->get_block) { + chip->get_block(chip, bits, size); + } else { /* emulate as fallback */ + u8 byte = 0; + + for (i = 0; i < chip->ngpio; i++) { + byte |= gpio_get_value_cansleep(chip->base + i) << + (i & 7); + if ((i & 7) == 7 || i == chip->ngpio - 1) { + bits[i >> 3] = byte; + byte = 0; + } + } + } + + for (i = 0; i < chip->ngpio; i++) { + if (bits[i >> 3] & BIT(i & 7)) { + chars = sprintf(buf, "%s%d", ret ? " " : "", + chip->base + i); + buf += chars; + ret += chars; + } + } + + kfree(bits); + + ret += sprintf(buf, "\n"); + + return ret; +} + +static ssize_t chip_block_store(struct device *dev, + struct device_attribute *attr, const char *buf, + size_t size) +{ + struct gpio_chip *chip = dev_get_drvdata(dev); + u8 *set_bits; + u8 *clr_bits; + size_t bits_size = (chip->ngpio + 7) / 8; + int count = size; + int gpio; + + mutex_lock(&sysfs_lock); + + set_bits = kzalloc(bits_size, GFP_KERNEL); + clr_bits = kzalloc(bits_size, GFP_KERNEL); + + while (count >= 0) { + bool clear = buf[0] == '-' ? true : false; + if (sscanf(buf, "%d", &gpio) == 1) { + if (clear) + gpio = -gpio; + gpio -= chip->base; + if (gpio >= 0 && gpio < chip->ngpio) { + if (clear) + clr_bits[gpio >> 3] |= BIT(gpio & 7); + else + set_bits[gpio >> 3] |= BIT(gpio & 7); + } + } + + /* Find next token */ + while (count >= 0 && *buf != ' ') { + buf++; + count--; + } + buf++; + count--; + } + + if (chip->set_block) { + chip->set_block(chip, set_bits, clr_bits, bits_size); + } else if (chip->set) { /* emulate as fall back */ + int i; + + for (i = 0; i < chip->ngpio; i++) { + if (set_bits[i >> 3] & BIT(i & 7)) + chip->set(chip, i, 1); + if (clr_bits[i >> 3] & BIT(i & 7)) + chip->set(chip, i, 0); + } + } + kfree(set_bits); + kfree(clr_bits); + mutex_unlock(&sysfs_lock); + + return size; +} + +static DEVICE_ATTR(block, 0644, chip_block_show, chip_block_store); + static const struct attribute *gpiochip_attrs[] = { &dev_attr_base.attr, &dev_attr_label.attr, &dev_attr_ngpio.attr, + &dev_attr_block.attr, NULL, }; @@ -1599,6 +1703,14 @@ int __gpio_get_value(unsigned gpio) } EXPORT_SYMBOL_GPL(__gpio_get_value); +void __gpio_get_block(unsigned gpio, u8 *values, size_t size) +{ + struct gpio_chip *chip = gpio_to_chip(gpio); + + return chip->get_block ? chip->get_block(chip, values, size) : 0; +} +EXPORT_SYMBOL_GPL(__gpio_get_block); + /* * _gpio_set_open_drain_value() - Set the open drain gpio's value. * @gpio: Gpio whose state need to be set. @@ -1676,6 +1788,15 @@ void __gpio_set_value(unsigned gpio, int } EXPORT_SYMBOL_GPL(__gpio_set_value); +void __gpio_set_block(unsigned gpio, u8 *set, u8 *clr, size_t size) +{ + struct gpio_chip *chip = gpio_to_chip(gpio); + + if (chip->set_block) + chip->set_block(chip, set, clr, size); +} +EXPORT_SYMBOL_GPL(__gpio_set_block); + /** * __gpio_cansleep() - report whether gpio value access will sleep * @gpio: gpio in question --- linux-2.6.orig/include/asm-generic/gpio.h +++ linux-2.6/include/asm-generic/gpio.h @@ -105,6 +105,8 @@ struct gpio_chip { unsigned offset); int (*get)(struct gpio_chip *chip, unsigned offset); + void (*get_block)(struct gpio_chip *chip, + u8 *values, size_t size); int (*direction_output)(struct gpio_chip *chip, unsigned offset, int value); int (*set_debounce)(struct gpio_chip *chip, @@ -112,6 +114,8 @@ struct gpio_chip { void (*set)(struct gpio_chip *chip, unsigned offset, int value); + void (*set_block)(struct gpio_chip *chip, u8 *set, + u8 *clr, size_t size); int (*to_irq)(struct gpio_chip *chip, unsigned offset); @@ -171,6 +175,9 @@ extern void gpio_set_value_cansleep(unsi extern int __gpio_get_value(unsigned gpio); extern void __gpio_set_value(unsigned gpio, int value); +extern void __gpio_get_block(unsigned gpio, u8 *values, size_t size); +extern void __gpio_set_block(unsigned gpio, u8 *set, u8 *clr, size_t size); + extern int __gpio_cansleep(unsigned gpio); extern int __gpio_to_irq(unsigned gpio); --- linux-2.6.orig/include/linux/gpio.h +++ linux-2.6/include/linux/gpio.h @@ -57,6 +57,17 @@ static inline void gpio_set_value(unsign __gpio_set_value(gpio, value); } +static inline void gpio_get_block(unsigned int gpio, u8 *values, size_t size) +{ + return __gpio_get_block(gpio, values, size); +} + +static inline +void gpio_set_block(unsigned int gpio, u8 *set, u8 *clr, size_t size) +{ + __gpio_set_block(gpio, set, clr, size); +} + static inline int gpio_cansleep(unsigned int gpio) { return __gpio_cansleep(gpio); @@ -167,6 +178,19 @@ static inline void gpio_set_value(unsign { /* GPIO can never have been requested or set as output */ WARN_ON(1); +} + +static inline void gpio_get_block(unsigned gpio, u8 *values, size_t size) +{ + /* GPIO can never have been requested or set as {in,out}put */ + WARN_ON(1); + return 0; +} + +static inline void gpio_set_block(unsigned gpio, u8 *set, u8 *clr, size_t size) +{ + /* GPIO can never have been requested or set as output */ + WARN_ON(1); } static inline int gpio_cansleep(unsigned gpio)
The recurring task of providing simultaneous access to GPIO lines (especially for bit banging protocols) needs an appropriate API. This patch adds a kernel internal "Block GPIO" API that enables simultaneous access to several GPIOs in the same gpio_chip (bit mapped). Further, it adds a sysfs interface (/sys/class/gpio/gpiochipXX/block). Signed-off-by: Roland Stigge <stigge@antcom.de> --- NOTE: This is only useful if individual drivers implement the .get_block() and .set_block() functions. I'm providing an example implementation for max730x (see next patch), and can provide further driver patches after API review. Thanks in advance! Documentation/gpio.txt | 52 +++++++++++++++++++ drivers/gpio/gpiolib.c | 121 +++++++++++++++++++++++++++++++++++++++++++++ include/asm-generic/gpio.h | 7 ++ include/linux/gpio.h | 24 ++++++++ 4 files changed, 204 insertions(+)