gpio: pca953x: Rewrite ->get_multiple() function

The commit 96d7c7b3e654 ("gpio: gpio-pca953x, Add get_multiple function") basically did everything wrong from style and code reuse perspective, i.e.

- it didn't utilize existing PCA953x internal helpers
- it didn't utilize bitmap API
- it misses the point that ilog2(), besides that BANK_SFT is useless, can be used in macros

- it has indentation issues.

Quoted 3 lines · Andy Shevchenko

The commit 96d7c7b3e654 ("gpio: gpio-pca953x, Add get_multiple function") basically did everything wrong from style and code reuse perspective, i.e.

Hi Andy,

Well your version is certainly elegant and simple, and does better with code reuse. However there are a couple of other goals I had in mind.
First, the "lazy" approach of 96d7c7b3e654 is actually faster when user space sets up a 8-bit linehandle[1]146us (single regmap_read()) vs 172us (pca953x_read_regs()) which incidentally is what we do in our application. In lazily reading 1 byte at a time it is the fastest access for that, if user space is always setting up the linehandle for the whole chip pca953x_read_regs() would be faster. Seeing as get_multiple has been unimplemented for this chip until now perhaps our use case deserves some consideration?

That being said, the
pca953x_read_regs() is still far better than calling regmap_read() 8 times.

Second, your version does not work with a 5.2 kernel, bitmap_replace is not there yet and pca953x_read_regs() signature is different. So perhaps we'll all move on and no one will care about 5.2, but as that's what we are using that was the basis for the patch.

Have you
tested this with actual hardware?

I actually didn't do a proper test
just the timing of the pca953x_read_regs().

Quoted 4 lines

- it didn't utilize existing PCA953x internal helpers
- it didn't utilize bitmap API
- it misses the point that ilog2(), besides that BANK_SFT is useless, can be used in macros

Yes, I know ilog2() can be used in macros, I didn't think it was worth including the .h file just to calculate 3. Putting the ilog2(x) in the comments seemed to be common in other kernel sections, but maybe that was historic before the macro version? Either way is fine. The shift is not useless, without that you would go into the if statement for every bit, but you only want to do a regmap_read() for every byte.

Quoted 1 line

- it has indentation issues.

It passed checkpatch.pl, and any indentation fixes are fine with me.

[1] Tested using 16-bit max7312

Quoted 7 lines · Paul Thomas

Quoted 3 lines · Andy Shevchenko

The commit 96d7c7b3e654 ("gpio: gpio-pca953x, Add get_multiple function") basically did everything wrong from style and code reuse perspective, i.e.

Hi Andy,

Quoted 11 lines

Well your version is certainly elegant and simple, and does better with code reuse. However there are a couple of other goals I had in mind.
First, the "lazy" approach of 96d7c7b3e654 is actually faster when user space sets up a 8-bit linehandle[1]146us (single regmap_read()) vs 172us (pca953x_read_regs()) which incidentally is what we do in our application. In lazily reading 1 byte at a time it is the fastest access for that, if user space is always setting up the linehandle for the whole chip pca953x_read_regs() would be faster. Seeing as get_multiple has been unimplemented for this chip until now perhaps our use case deserves some consideration?

I understand completely your goal, but
- for I²C expanders timings is the last thing to look for (they are quite slow by nature), so, I really don't care about 16% speed up for one call; don't forget that we are in multi-task OS, where this can be easily interrupted and user will see the result quite after expected quick result

- the code maintenance has a priority over micro-optimization (this driver suffered many times of breakages because of some optimizations done)

- it breaks Uwe's approach to fix AI chips, after my patch Uwe's ones are applied cleanly

Quoted 20 lines · Andy Shevchenko

Quoted 7 lines · Paul Thomas

Quoted 3 lines · Andy Shevchenko

The commit 96d7c7b3e654 ("gpio: gpio-pca953x, Add get_multiple function") basically did everything wrong from style and code reuse perspective, i.e.

Hi Andy,

Quoted 11 lines

Well your version is certainly elegant and simple, and does better with code reuse. However there are a couple of other goals I had in mind.
First, the "lazy" approach of 96d7c7b3e654 is actually faster when user space sets up a 8-bit linehandle[1]146us (single regmap_read()) vs 172us (pca953x_read_regs()) which incidentally is what we do in our application. In lazily reading 1 byte at a time it is the fastest access for that, if user space is always setting up the linehandle for the whole chip pca953x_read_regs() would be faster. Seeing as get_multiple has been unimplemented for this chip until now perhaps our use case deserves some consideration?

Quoted 5 lines

I understand completely your goal, but
- for I²C expanders timings is the last thing to look for (they are quite slow by nature), so, I really don't care about 16% speed up for one call; don't forget that we are in multi-task OS, where this can be easily interrupted and user will see the result quite after expected quick result

I didn't do any timing analysis and while I understand your argumentation, I'm not sure to agree. I noticed while debugging the problem that then resulted in my fix that gpioctl uses the .set_multiple callback. I told my customer to use gpioctl instead of /sys/class/gpio because it performs better just to notice that when using gpioctl to set a single GPIO this transfers five bytes instead of only two.

Having said that I think the sane approach is to optimize .{g,s}et_multiple to reduce the read/write size to the smallest bulk size possible that covers all bits that have their corresponding bit set in mask.

Quoted 2 lines

- the code maintenance has a priority over micro-optimization (this driver suffered many times of breakages because of some optimizations done)

ack here. Some parts of the driver were harder to grasp than necessary.

Quoted 2 lines

- it breaks Uwe's approach to fix AI chips, after my patch Uwe's ones are applied cleanly

I didn't check, is 96d7c7b3e654 broken for some chips?

I will add my suggested optimisation to my todo list for evaluation. If I think it is still nice and maintainable I'll send a patch. Until I have looked into this (or someone else did) I'm in favour of applying Andy's patch.

Quoted 21 lines · Uwe Kleine-König

Quoted 20 lines · Andy Shevchenko

Quoted 18 lines · Paul Thomas

Quoted 3 lines · Andy Shevchenko

The commit 96d7c7b3e654 ("gpio: gpio-pca953x, Add get_multiple function") basically did everything wrong from style and code reuse perspective, i.e.

Hi Andy,

Well your version is certainly elegant and simple, and does better with code reuse. However there are a couple of other goals I had in mind.
First, the "lazy" approach of 96d7c7b3e654 is actually faster when user space sets up a 8-bit linehandle[1]146us (single regmap_read()) vs 172us (pca953x_read_regs()) which incidentally is what we do in our application. In lazily reading 1 byte at a time it is the fastest access for that, if user space is always setting up the linehandle for the whole chip pca953x_read_regs() would be faster. Seeing as get_multiple has been unimplemented for this chip until now perhaps our use case deserves some consideration?

Quoted 5 lines

Quoted 5 lines

I understand completely your goal, but
- for I²C expanders timings is the last thing to look for (they are quite slow by nature), so, I really don't care about 16% speed up for one call; don't forget that we are in multi-task OS, where this can be easily interrupted and user will see the result quite after expected quick result

Quoted 8 lines

I didn't do any timing analysis and while I understand your argumentation, I'm not sure to agree. I noticed while debugging the problem that then resulted in my fix that gpioctl uses the .set_multiple callback. I told my customer to use gpioctl instead of /sys/class/gpio because it performs better just to notice that when using gpioctl to set a single GPIO this transfers five bytes instead of only two.

Quoted 7 lines

Having said that I think the sane approach is to optimize .{g,s}et_multiple to reduce the read/write size to the smallest bulk size possible that covers all bits that have their corresponding bit set in mask.

Quoted 2 lines

- the code maintenance has a priority over micro-optimization (this driver suffered many times of breakages because of some optimizations done)

Quoted 7 lines

ack here. Some parts of the driver were harder to grasp than necessary.

Quoted 2 lines

- it breaks Uwe's approach to fix AI chips, after my patch Uwe's ones are applied cleanly

I didn't check, is 96d7c7b3e654 broken for some chips?

I was referring to another call to recalc address with additional parameters, which your second patch actually gets rid of.

Quoted 5 lines · Andy Shevchenko

Quoted 4 lines · Uwe Kleine-König

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Paul Thomas

On Mon, Apr 20, 2020 at 1:27 PM Andy Shevchenko
<[email protected]> wrote:

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

I understand completely your goal, but
- for I²C expanders timings is the last thing to look for (they are quite slow by nature), so, I really don't care about 16% speed up for one call; don't forget that we are in multi-task OS, where this can be easily interrupted and user will see the result quite after expected quick result

Sure, it's not a HUGE deal, but this will get worse for 5 bank chips. Also, 26us is not insignificant, with the preempt-rt kernel we're using that can be more than the max interrupt latency.

Quoted 12 lines

Quoted 12 lines

I didn't do any timing analysis and while I understand your argumentation, I'm not sure to agree. I noticed while debugging the problem that then resulted in my fix that gpioctl uses the .set_multiple callback. I told my customer to use gpioctl instead of /sys/class/gpio because it performs better just to notice that when using gpioctl to set a single GPIO this transfers five bytes instead of only two.

Having said that I think the sane approach is to optimize .{g,s}et_multiple to reduce the read/write size to the smallest bulk size possible that covers all bits that have their corresponding bit set in mask.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

- the code maintenance has a priority over micro-optimization (this driver suffered many times of breakages because of some optimizations done)

Yeah, I appreciate that maintainability needs to be a big priority, I'm wondering if comments & syntax could be improved so that the general idea of 96d7c7b3e654 is clear and maintainable. It is just walking through mask, and whenever it gets to a new byte it reads it from the hardware.

Quoted 7 lines

Quoted 6 lines

ack here. Some parts of the driver were harder to grasp than necessary.

Quoted 2 lines

- it breaks Uwe's approach to fix AI chips, after my patch Uwe's ones are applied cleanly

I didn't check, is 96d7c7b3e654 broken for some chips?

Quoted 2 lines

I was referring to another call to recalc address with additional parameters, which your second patch actually gets rid of.

If it's just the call to pca953x_recalc_addr() that caused the conflict with Uwe's work with 96d7c7b3e654, we can just remove the last two arguments so it matches what pca953x_gpio_get_value() is doing.

The commit 96d7c7b3e654 ("gpio: gpio-pca953x, Add get_multiple function") basically did everything wrong from style and code reuse perspective, i.e.
- it didn't utilize existing PCA953x internal helpers
- it didn't utilize bitmap API
- it misses the point that ilog2(), besides that BANK_SFT is useless, can be used in macros
- it has indentation issues.

Rewrite the function completely.

Cc Paul Thomas <[email protected]>

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/gpio/gpio-pca953x.c | 41 ++++++++++---------------------------
 1 file changed, 11 insertions(+), 30 deletions(-)

drivers/gpio/gpio-pca953x.c +11 -30

index 60ae18e4b5f5a..41be681ae77c2 100644
@@ -115,7 +115,6 @@ MODULE_DEVICE_TABLE(acpi, pca953x_acpi_ids);
 
 #define MAX_BANK 5
 #define BANK_SZ 8
-#define BANK_SFT 3 /* ilog2(BANK_SZ) */
 #define MAX_LINE	(MAX_BANK * BANK_SZ)
 
 #define NBANK(chip) DIV_ROUND_UP(chip->gpio_chip.ngpio, BANK_SZ)

@@ -469,38 +468,20 @@ static int pca953x_gpio_get_direction(struct gpio_chip *gc, unsigned off)
 }
 
 static int pca953x_gpio_get_multiple(struct gpio_chip *gc,
-				      unsigned long *mask, unsigned long *bits)
+				     unsigned long *mask, unsigned long *bits)
 {
 	struct pca953x_chip *chip = gpiochip_get_data(gc);
-	unsigned int reg_val;
-	int offset, value, i, ret = 0;
-	u8 inreg;
+	DECLARE_BITMAP(reg_val, MAX_LINE);
+	int ret;
 
-	/* Force offset outside the range of i so that
-	 * at least the first relevant register is read
-	 */
-	offset = gc->ngpio;
-	for_each_set_bit(i, mask, gc->ngpio) {
-		/* whenever i goes into a new bank update inreg
-		 * and read the register
-		 */
-		if ((offset >> BANK_SFT) != (i >> BANK_SFT)) {
-			offset = i;
-			inreg = pca953x_recalc_addr(chip, chip->regs->input,
-						    offset, true, false);
-			mutex_lock(&chip->i2c_lock);
-			ret = regmap_read(chip->regmap, inreg, &reg_val);
-			mutex_unlock(&chip->i2c_lock);
-			if (ret < 0)
-				return ret;
-		}
-		/* reg_val is relative to the last read byte,
-		 * so only shift the relative bits
-		 */
-		value = (reg_val >> (i % 8)) & 0x01;
-		__assign_bit(i, bits, value);
-	}
-	return ret;
+	mutex_lock(&chip->i2c_lock);
+	ret = pca953x_read_regs(chip, chip->regs->input, reg_val);
+	mutex_unlock(&chip->i2c_lock);
+	if (ret)
+		return ret;
+
+	bitmap_replace(bits, bits, reg_val, mask, gc->ngpio);
+	return 0;
 }
 
 static void pca953x_gpio_set_multiple(struct gpio_chip *gc,

Signature · 1 line

2.26.1

Quoted 8 lines · Andy Shevchenko

The commit 96d7c7b3e654 ("gpio: gpio-pca953x, Add get_multiple function") basically did everything wrong from style and code reuse perspective, i.e.
- it didn't utilize existing PCA953x internal helpers
- it didn't utilize bitmap API
- it misses the point that ilog2(), besides that BANK_SFT is useless, can be used in macros
- it has indentation issues.

Quoted 1 line

Rewrite the function completely.

Bart, Linus, please, consider this series to be applied, because it has Uwe's fixes. We may still discuss the approach with ->get_multiple(), though.

For the record, should some of us volunteer to be a reviewer for this driver. It's awful that almost every release we get something either ugly or broken in it. Uwe, would you like to be a designated reviewer (I would also support)?

Quoted 8 lines

The commit 96d7c7b3e654 ("gpio: gpio-pca953x, Add get_multiple function") basically did everything wrong from style and code reuse perspective, i.e.
- it didn't utilize existing PCA953x internal helpers
- it didn't utilize bitmap API
- it misses the point that ilog2(), besides that BANK_SFT is useless, can be used in macros
- it has indentation issues.

Quoted 1 line

Rewrite the function completely.

Bart, Linus, please, consider this series to be applied, because it has Uwe's fixes. We may still discuss the approach with ->get_multiple(), though.

For the record, should some of us volunteer to be a reviewer for this driver. It's awful that almost every release we get something either ugly or broken in it. Uwe, would you like to be a designated reviewer (I would also support)?

wt., 21 kwi 2020 o 15:03 Andy Shevchenko
<[email protected]> napisał(a):

Quoted 12 lines · Andy Shevchenko

Quoted 9 lines

The commit 96d7c7b3e654 ("gpio: gpio-pca953x, Add get_multiple function") basically did everything wrong from style and code reuse perspective, i.e.
- it didn't utilize existing PCA953x internal helpers
- it didn't utilize bitmap API
- it misses the point that ilog2(), besides that BANK_SFT is useless, can be used in macros
- it has indentation issues.

Rewrite the function completely.

Quoted 3 lines

Bart, Linus, please, consider this series to be applied, because it has Uwe's fixes. We may still discuss the approach with ->get_multiple(), though.

Personally I like these patches - if Linus doesn't object in the coming days I'll pick them up into my tree as a follow-up to Paul's patch.

I would like that Uwe also agrees that we can merge these three and use as a base?

Quoted 3 lines · Bartosz Golaszewski

wt., 21 kwi 2020 o 15:03 Andy Shevchenko
<[email protected]> napisał(a):

Quoted 1 line · Andy Shevchenko

I don't mind trying to put in code to optimize use cases when accessing single bytes here either. But I'd like there to be explicit comments in the code saying why these optimizations are there. Can we do those on top of this (hopefully) known working base?

Quoted 5 lines · Linus Walleij

Quoted 3 lines · Bartosz Golaszewski

wt., 21 kwi 2020 o 15:03 Andy Shevchenko
<[email protected]> napisał(a):

Quoted 1 line · Andy Shevchenko

...

Quoted 5 lines

I don't mind trying to put in code to optimize use cases when accessing single bytes here either. But I'd like there to be explicit comments in the code saying why these optimizations are there. Can we do those on top of this (hopefully) known working base?

As I pointed out to Paul, the optimization like he proposed is not bad thing per se, the implementation is. On top of that I suggested to take a look to IRQ status bits, which presumable will leverage from this optimization as well.

So, After applying this series it would be matter of change one line in the ->get_multiple() to replace read_regs() with optimized version or so along with IRQ bits changes.

Quoted 8 lines · Andy Shevchenko

Quoted 5 lines · Linus Walleij

Quoted 3 lines · Bartosz Golaszewski

wt., 21 kwi 2020 o 15:03 Andy Shevchenko
<[email protected]> napisał(a):

Quoted 1 line · Andy Shevchenko

Quoted 7 lines

...

Quoted 5 lines

I don't mind trying to put in code to optimize use cases when accessing single bytes here either. But I'd like there to be explicit comments in the code saying why these optimizations are there. Can we do those on top of this (hopefully) known working base?

For the record my original get_multiple patch was a known working base.

Quoted 8 lines

As I pointed out to Paul, the optimization like he proposed is not bad thing per se, the implementation is. On top of that I suggested to take a look to IRQ status bits, which presumable will leverage from this optimization as well.

So, After applying this series it would be matter of change one line in the ->get_multiple() to replace read_regs() with optimized version or so along with IRQ bits changes.

This new function would then at least need to be called with mask as an additional argument right? Then the bitmap_replace() will set everything regardless of if it was read, this is fine I suppose because it doesn't matter if it's setting bits outside of mask. You just have two loops one in the new function and one in bitmap_replace().

If this is what people would like to see I can work
on it. I did look into the sparse reads and it seems as though regmap has a gather_write, but not a gather_read and gather_write isn't in regmap-i2c.c anyway.

Quoted 28 lines · Paul Thomas

Quoted 8 lines · Andy Shevchenko

Quoted 5 lines · Linus Walleij

Quoted 3 lines · Bartosz Golaszewski

wt., 21 kwi 2020 o 15:03 Andy Shevchenko
<[email protected]> napisał(a):

Quoted 1 line · Andy Shevchenko

Quoted 7 lines

...

Quoted 5 lines

I don't mind trying to put in code to optimize use cases when accessing single bytes here either. But I'd like there to be explicit comments in the code saying why these optimizations are there. Can we do those on top of this (hopefully) known working base?

For the record my original get_multiple patch was a known working base.

Quoted 8 lines

As I pointed out to Paul, the optimization like he proposed is not bad thing per se, the implementation is. On top of that I suggested to take a look to IRQ status bits, which presumable will leverage from this optimization as well.

So, After applying this series it would be matter of change one line in the ->get_multiple() to replace read_regs() with optimized version or so along with IRQ bits changes.

Quoted 6 lines

This new function would then at least need to be called with mask as an additional argument right? Then the bitmap_replace() will set everything regardless of if it was read, this is fine I suppose because it doesn't matter if it's setting bits outside of mask. You just have two loops one in the new function and one in bitmap_replace().

Note, on 64-bit architectures there is no loop in bitmap ops (since 40 <= 64 in this case). On 32-bit it might be (only for 40 case, which I think less present in the wild than the rest). And bitmap ops are optimized over longs, so, it's pretty much fast (esp. in comparison to I²C IO).

Quoted 4 lines

If this is what people would like to see I can work
on it. I did look into the sparse reads and it seems as though regmap has a gather_write, but not a gather_read and gather_write isn't in regmap-i2c.c anyway.

Yes, I think this is the right way to go.

--
With Best Regards,
Andy Shevchenko

Quoted 42 lines · Andy Shevchenko

Quoted 28 lines · Paul Thomas

Quoted 8 lines · Andy Shevchenko

Quoted 5 lines · Linus Walleij

Quoted 3 lines · Bartosz Golaszewski

wt., 21 kwi 2020 o 15:03 Andy Shevchenko
<[email protected]> napisał(a):

Quoted 1 line · Andy Shevchenko

Quoted 7 lines

...

Quoted 5 lines

I don't mind trying to put in code to optimize use cases when accessing single bytes here either. But I'd like there to be explicit comments in the code saying why these optimizations are there. Can we do those on top of this (hopefully) known working base?

For the record my original get_multiple patch was a known working base.

Quoted 8 lines

As I pointed out to Paul, the optimization like he proposed is not bad thing per se, the implementation is. On top of that I suggested to take a look to IRQ status bits, which presumable will leverage from this optimization as well.

So, After applying this series it would be matter of change one line in the ->get_multiple() to replace read_regs() with optimized version or so along with IRQ bits changes.

Quoted 6 lines

This new function would then at least need to be called with mask as an additional argument right? Then the bitmap_replace() will set everything regardless of if it was read, this is fine I suppose because it doesn't matter if it's setting bits outside of mask. You just have two loops one in the new function and one in bitmap_replace().

Note, on 64-bit architectures there is no loop in bitmap ops (since 40 <= 64 in this case). On 32-bit it might be (only for 40 case, which I think less present in the wild than the rest). And bitmap ops are optimized over longs, so, it's pretty much fast (esp. in comparison to I²C IO).

Quoted 6 lines

Quoted 4 lines

If this is what people would like to see I can work
on it. I did look into the sparse reads and it seems as though regmap has a gather_write, but not a gather_read and gather_write isn't in regmap-i2c.c anyway.

Yes, I think this is the right way to go.

OK, sounds good.