Re: [PATCH v3 3/3] gpio: xilinx: Utilize generic bitmap_get_value and _set_value

On Fri, Mar 26, 2021 at 11:27 PM Andy Shevchenko
[off-list ref] wrote:

On Sat, Mar 6, 2021 at 4:08 PM Syed Nayyar Waris [off-list ref] wrote:

quoted

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.

...

quoted

+       u32 *const state = chip->gpio_state;
+       unsigned int *const width = chip->gpio_width;

quoted

+

Extra blank line.

quoted

+       DECLARE_BITMAP(old, 64);
+       DECLARE_BITMAP(new, 64);
+       DECLARE_BITMAP(changed, 64);

quoted

+       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

+       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?

quoted

+       spin_unlock(&chip->gpio_lock[1]);
+       spin_unlock_irqrestore(&chip->gpio_lock[0], flags);

--
With Best Regards,
Andy Shevchenko

Have removed the extra line and added comments. Regarding locking - I
see that now there is just a single lock available instead of 2 locks.
Have made necessary changes. Thanks
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/gpio/gpio-xilinx.c?id=37ef334680800263b32bb96a5156a4b47f0244a2

Regards

Syed Nayyar Waris

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel