On Saturday 07 June 2014 22:24:52 Alexandre Courbot wrote:
> On Tue, Jun 3, 2014 at 12:28 AM, Rojhalat Ibrahim <[email protected]> wrote:
> > Introduce new functions gpiod_set_array & gpiod_set_raw_array to the
> > consumer
> > interface which allow setting multiple outputs with just one function call.
> > Also add an optional set_multiple function to the driver interface. Without
> > an
> > implementation of that function in the chip driver outputs are set
> > sequentially.
> >
> > Implementing the set_multiple function in a chip driver allows for:
> > - Improved performance for certain use cases. The original motivation for
> > this
> > was the task of configuring an FPGA. In that specific case, where 9 GPIO
> > lines have to be set many times, configuration time goes down from 48 s to
> > 20 s when using the new function.
> > - Simultaneous glitch-free setting of multiple pins on any kind of parallel
> > bus attached to GPIOs provided they all reside on the same chip and bank.
> >
> > Limitations:
> > Performance is only improved for normal high-low outputs. Open drain and
> > open source outputs are always set separately from each other. Those kinds
> > of outputs could probably be accelerated in a similar way if we could
> > forgo the error checking when setting GPIO directions.
> >
> > Signed-off-by: Rojhalat Ibrahim <[email protected]>
> > ---
> > Change log:
> > v4: - add gpiod_set_array function for setting logical values
> > - change interface of the set_multiple driver function to use
> > unsigned long as type for the bit fields
> > - use generic bitops (which also use unsigned long for bit fields)
> > - do not use ARCH_NR_GPIOS any more
> > v3: - add documentation
> > - change commit message
> > v2: - use descriptor interface
> > - allow arbitrary groups of GPIOs spanning multiple chips
> >
> > Documentation/gpio/consumer.txt | 23 +++++
> > drivers/gpio/gpiolib.c | 180
> > ++++++++++++++++++++++++++++++++++++++++
> > include/linux/gpio/consumer.h | 38 +++++++++
> > include/linux/gpio/driver.h | 4 +
> > 4 files changed, 245 insertions(+)
> >
> > diff --git a/Documentation/gpio/consumer.txt
> > b/Documentation/gpio/consumer.txt
> > index 09854fe..ba02b84 100644
> > --- a/Documentation/gpio/consumer.txt
> > +++ b/Documentation/gpio/consumer.txt
> > @@ -163,6 +163,29 @@ The active-low state of a GPIO can also be queried
> > using the following call:
> > Note that these functions should only be used with great moderation ; a
> > driver
> > should not have to care about the physical line level.
> >
> > +Set multiple GPIO outputs with a single function call
> > +-----------------------------------------------------
> > +The following functions set the output values of an array of GPIOs:
> > +
> > + void gpiod_set_array(unsigned int array_size,
> > + struct gpio_desc **desc_array,
> > + int *value_array)
> > + void gpiod_set_raw_array(unsigned int array_size,
> > + struct gpio_desc **desc_array,
> > + int *value_array)
> > + void gpiod_set_array_cansleep(unsigned int array_size,
> > + struct gpio_desc **desc_array,
> > + int *value_array)
> > + void gpiod_set_raw_array_cansleep(unsigned int array_size,
> > + struct gpio_desc **desc_array,
> > + int *value_array)
> > +
> > +The array can be an arbitrary set of GPIOs. The functions will try to set
> > +GPIOs belonging to the same bank or chip simultaneously if supported by the
> > +corresponding chip driver. In that case a significantly improved
> > performance
> > +can be expected. If simultaneous setting is not possible the GPIOs will be
> > set
> > +sequentially.
> > +
> > GPIOs mapped to IRQs
> > --------------------
> > GPIO lines can quite often be used as IRQs. You can get the IRQ number
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index f48817d..a2be195 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -2345,6 +2345,84 @@ static void _gpiod_set_raw_value(struct gpio_desc
> > *desc, bool value)
> > chip->set(chip, gpio_chip_hwgpio(desc), value);
> > }
> >
> > +/*
> > + * set multiple outputs on the same chip;
> > + * use the chip's set_multiple function if available;
> > + * otherwise set the outputs sequentially;
> > + * @mask: bit mask array; one bit per output; BITS_PER_LONG bits per word
> > + * defines which outputs are to be changed
> > + * @bits: bit value array; one bit per output; BITS_PER_LONG bits per word
> > + * defines the values the outputs specified by mask are to be set to
> > + */
> > +static void gpio_chip_set_multiple(struct gpio_chip *chip,
> > + unsigned long *mask, unsigned long *bits)
> > +{
> > + if (chip->set_multiple) {
> > + chip->set_multiple(chip, mask, bits);
> > + } else {
> > + int i;
> > + for (i = 0; i < chip->ngpio; i++) {
> > + if (mask[BIT_WORD(i)] == 0) {
> > + /* no more set bits in this mask word;
> > + * skip ahead to the next word */
> > + i = (BIT_WORD(i) + 1) * BITS_PER_LONG - 1;
> > + continue;
> > + }
> > + /* set outputs if the corresponding mask bit is set
> > */
> > + if (__test_and_clear_bit(i, mask)) {
> > + chip->set(chip, i, test_bit(i, bits));
>
> Shouldn't this be
>
> chip->set(chip, i, test_bit(i, bits[BIT_WORD(i)]);
>
> ?
>
No. The test_bit function already handles this correctly. Here's the function
from include/asm-generic/bitops/non-atomic.h:
static inline int test_bit(int nr, const volatile unsigned long *addr)
{
return 1UL & (addr[BIT_WORD(nr)] >> (nr & (BITS_PER_LONG-1)));
}
I'll post a new revision of the patch to deal with your other comments.
Thanks for reviewing this.
Rojhalat
--
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
