| Message ID | 6ca4bf0ed930c057bdbcd07bab34ed0a474dd78d.1521301345.git.vilhelm.gray@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Mar 17, 2018 at 5:50 PM, William Breathitt Gray <vilhelm.gray@gmail.com> wrote: > The ACCES I/O PCI-IDIO-16 series of devices provides 16 > optically-isolated digital inputs accessed via two 8-bit ports. Since > eight input lines are acquired on a single port input read, the > PCI-IDIO-16 GPIO driver may improve multiple input reads by utilizing a > get_multiple callback. This patch implements the > idio_16_gpio_get_multiple function which serves as the respective > get_multiple callback. > +static int idio_16_gpio_get_multiple(struct gpio_chip *chip, > + unsigned long *mask, unsigned long *bits) > +{ > + struct idio_16_gpio *const idio16gpio = gpiochip_get_data(chip); > + size_t i; > + const unsigned int gpio_reg_size = 8; > + unsigned int bits_offset; > + size_t word_index; > + unsigned int word_offset; > + unsigned long word_mask; > + const unsigned long port_mask = GENMASK(gpio_reg_size, 0); gpio_reg_size - 1? Though I would prefer not to have that variable at all, just use 8 or 7 respectively. > + unsigned long port_state; > + u8 __iomem ports[] = { > + idio16gpio->reg->out0_7, idio16gpio->reg->out8_15, > + idio16gpio->reg->in0_7, idio16gpio->reg->in8_15 I would leave comma even here. > + }; > + /* get bits are evaluated a gpio port register at a time */ > + for (i = 0; i < ARRAY_SIZE(ports); i++) { > + /* gpio offset in bits array */ > + bits_offset = i * gpio_reg_size; > + > + /* word index for bits array */ > + word_index = BIT_WORD(bits_offset); > + > + /* gpio offset within current word of bits array */ > + word_offset = bits_offset % BITS_PER_LONG; > + > + /* mask of get bits for current gpio within current word */ > + word_mask = mask[word_index] & (port_mask << word_offset); > + if (!word_mask) { > + /* no get bits in this port so skip to next one */ > + continue; > + } > + > + /* read bits from current gpio port */ > + port_state = ioread8(ports + i); > + > + /* store acquired bits at respective bits array offset */ > + bits[word_index] |= port_state << word_offset; > + } I would propose to do other way around, i.e. read all ports to the bitmap array and call bitmap_and() after. Further optimization can be something like introduction of generic bitmap_copy_uXX_off(unsigned long *dst, u8 src, unsigned int offset); It can be done using macros, though it's another story not quite related to the topic. > +}
On Wed, Mar 21, 2018 at 07:45:29PM +0200, Andy Shevchenko wrote: >On Sat, Mar 17, 2018 at 5:50 PM, William Breathitt Gray ><vilhelm.gray@gmail.com> wrote: >> The ACCES I/O PCI-IDIO-16 series of devices provides 16 >> optically-isolated digital inputs accessed via two 8-bit ports. Since >> eight input lines are acquired on a single port input read, the >> PCI-IDIO-16 GPIO driver may improve multiple input reads by utilizing a >> get_multiple callback. This patch implements the >> idio_16_gpio_get_multiple function which serves as the respective >> get_multiple callback. > >> +static int idio_16_gpio_get_multiple(struct gpio_chip *chip, >> + unsigned long *mask, unsigned long *bits) >> +{ >> + struct idio_16_gpio *const idio16gpio = gpiochip_get_data(chip); >> + size_t i; >> + const unsigned int gpio_reg_size = 8; >> + unsigned int bits_offset; >> + size_t word_index; >> + unsigned int word_offset; >> + unsigned long word_mask; > >> + const unsigned long port_mask = GENMASK(gpio_reg_size, 0); > >gpio_reg_size - 1? Oops, looks like I made an off-by-one error here so I'll make sure to fix that up. >Though I would prefer not to have that variable at all, just use 8 or >7 respectively. This device is simple enough that throughout this function I could inline gpio_reg_size and port_mask to 8 and 0xFF respectively, but I would like to keep the code generic enough for reuse in other drivers. In addition, I believe the variable names help keep the intention of the code clear, so I'll stick with dedicated const variables for now if there are no other objections. > >> + unsigned long port_state; > >> + u8 __iomem ports[] = { >> + idio16gpio->reg->out0_7, idio16gpio->reg->out8_15, > >> + idio16gpio->reg->in0_7, idio16gpio->reg->in8_15 > >I would leave comma even here. Will do. > >> + }; > >> + /* get bits are evaluated a gpio port register at a time */ >> + for (i = 0; i < ARRAY_SIZE(ports); i++) { >> + /* gpio offset in bits array */ >> + bits_offset = i * gpio_reg_size; >> + >> + /* word index for bits array */ >> + word_index = BIT_WORD(bits_offset); >> + >> + /* gpio offset within current word of bits array */ >> + word_offset = bits_offset % BITS_PER_LONG; >> + >> + /* mask of get bits for current gpio within current word */ >> + word_mask = mask[word_index] & (port_mask << word_offset); >> + if (!word_mask) { >> + /* no get bits in this port so skip to next one */ >> + continue; >> + } >> + >> + /* read bits from current gpio port */ >> + port_state = ioread8(ports + i); >> + >> + /* store acquired bits at respective bits array offset */ >> + bits[word_index] |= port_state << word_offset; >> + } > >I would propose to do other way around, i.e. >read all ports to the bitmap array and call bitmap_and() after. > >Further optimization can be something like introduction of generic > >bitmap_copy_uXX_off(unsigned long *dst, u8 src, unsigned int offset); > >It can be done using macros, though it's another story not quite >related to the topic. Port I/O is significantly more costly to perform than the bitmask evaluations for each port. Despite the increased complexity of the loop logic, I believe the latency improvements of skipping unnecessary I/O port reads are worth the trouble. I do like the idea of a bitmap_copy_uXX_off macro as that could be quite useful in general. Even if not for this particular patchset, I would be interested in seeing that functionality added to the bitmap API. Perhaps I might implement it as a standlone patch when I have some free time. William Breathitt Gray > >> +} > >-- >With Best Regards, >Andy Shevchenko -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/gpio/gpio-pci-idio-16.c b/drivers/gpio/gpio-pci-idio-16.c index 57d1b7fbf07b..f273cf776cac 100644 --- a/drivers/gpio/gpio-pci-idio-16.c +++ b/drivers/gpio/gpio-pci-idio-16.c @@ -11,6 +11,7 @@ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU * General Public License for more details. */ +#include <linux/bitmap.h> #include <linux/bitops.h> #include <linux/device.h> #include <linux/errno.h> @@ -103,6 +104,54 @@ static int idio_16_gpio_get(struct gpio_chip *chip, unsigned int offset) return !!(ioread8(&idio16gpio->reg->in8_15) & (mask >> 24)); } +static int idio_16_gpio_get_multiple(struct gpio_chip *chip, + unsigned long *mask, unsigned long *bits) +{ + struct idio_16_gpio *const idio16gpio = gpiochip_get_data(chip); + size_t i; + const unsigned int gpio_reg_size = 8; + unsigned int bits_offset; + size_t word_index; + unsigned int word_offset; + unsigned long word_mask; + const unsigned long port_mask = GENMASK(gpio_reg_size, 0); + unsigned long port_state; + u8 __iomem ports[] = { + idio16gpio->reg->out0_7, idio16gpio->reg->out8_15, + idio16gpio->reg->in0_7, idio16gpio->reg->in8_15 + }; + + /* clear bits array to a clean slate */ + bitmap_zero(bits, chip->ngpio); + + /* get bits are evaluated a gpio port register at a time */ + for (i = 0; i < ARRAY_SIZE(ports); i++) { + /* gpio offset in bits array */ + bits_offset = i * gpio_reg_size; + + /* word index for bits array */ + word_index = BIT_WORD(bits_offset); + + /* gpio offset within current word of bits array */ + word_offset = bits_offset % BITS_PER_LONG; + + /* mask of get bits for current gpio within current word */ + word_mask = mask[word_index] & (port_mask << word_offset); + if (!word_mask) { + /* no get bits in this port so skip to next one */ + continue; + } + + /* read bits from current gpio port */ + port_state = ioread8(ports + i); + + /* store acquired bits at respective bits array offset */ + bits[word_index] |= port_state << word_offset; + } + + return 0; +} + static void idio_16_gpio_set(struct gpio_chip *chip, unsigned int offset, int value) { @@ -299,6 +348,7 @@ static int idio_16_probe(struct pci_dev *pdev, const struct pci_device_id *id) idio16gpio->chip.direction_input = idio_16_gpio_direction_input; idio16gpio->chip.direction_output = idio_16_gpio_direction_output; idio16gpio->chip.get = idio_16_gpio_get; + idio16gpio->chip.get_multiple = idio_16_gpio_get_multiple; idio16gpio->chip.set = idio_16_gpio_set; idio16gpio->chip.set_multiple = idio_16_gpio_set_multiple;
The ACCES I/O PCI-IDIO-16 series of devices provides 16 optically-isolated digital inputs accessed via two 8-bit ports. Since eight input lines are acquired on a single port input read, the PCI-IDIO-16 GPIO driver may improve multiple input reads by utilizing a get_multiple callback. This patch implements the idio_16_gpio_get_multiple function which serves as the respective get_multiple callback. Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com> --- drivers/gpio/gpio-pci-idio-16.c | 50 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+)