Re: [PATCH v3 3/3] gpio: xilinx: Utilize generic bitmap_get_value and _set_value
flat view
From: Andy Shevchenko <hidden>
Date: 2021-03-26 17:58:42
Also in:
linux-arm-kernel, linux-gpio, linux-pm, lkml
On Sat, Mar 6, 2021 at 4:08 PM Syed Nayyar Waris [off-list ref] wrote:
This patch reimplements the xgpio_set_multiple() function in drivers/gpio/gpio-xilinx.c to use the new generic functions: bitmap_get_value() and bitmap_set_value(). The code is now simpler to read and understand. Moreover, instead of looping for each bit in xgpio_set_multiple() function, now we can check each channel at a time and save cycles.
...
+ u32 *const state = chip->gpio_state; + unsigned int *const width = chip->gpio_width;
+
Extra blank line.
quoted hunk ↗ jump to hunk
+ DECLARE_BITMAP(old, 64); + DECLARE_BITMAP(new, 64); + DECLARE_BITMAP(changed, 64);
+ spin_lock_irqsave(&chip->gpio_lock[0], flags); + spin_lock(&chip->gpio_lock[1]);
I understand why this is done at the top of the function in the original code. I do not understand why you put some operations under spin lock. Have you checked what each of these spin locks protects? Please check and try to lock as minimum as possible.
quoted hunk ↗ jump to hunk
+ bitmap_set_value(old, 64, state[0], width[0], 0); + bitmap_set_value(old, 64, state[1], width[1], width[0]); + bitmap_replace(new, old, bits, mask, gc->ngpio); + + bitmap_set_value(old, 64, state[0], 32, 0); + bitmap_set_value(old, 64, state[1], 32, 32); + state[0] = bitmap_get_value(new, 0, width[0]); + state[1] = bitmap_get_value(new, width[0], width[1]); + bitmap_set_value(new, 64, state[0], 32, 0); + bitmap_set_value(new, 64, state[1], 32, 32); + bitmap_xor(changed, old, new, 64);
Original code and this is cryptic. Can you add a few comments explaining what is going on here?
+ spin_unlock(&chip->gpio_lock[1]); + spin_unlock_irqrestore(&chip->gpio_lock[0], flags);
-- With Best Regards, Andy Shevchenko