mfd: tps65911-comparator: Fix an off by one bug

The tps_comparators[] array is used in two places. We only access the COMP1 (1) and COMP2 (2) elements. Unfortunately, we're accessing the wrong elements and also one element beyond the end of the array. There was supposed to be a zero element at the start of the array which is isn't accessed but makes the math work out nicely.

Fixes 6851ad3ab346 ("TPS65911: Comparator: Add comparator driver")

Signed-off-by Dan Carpenter <[email protected]>

---
I can't actually compile this code...

drivers/mfd/tps65911-comparator.c +1 -0

index c0789f81a1c5..35d7380f6fad 100644
@@ -42,6 +42,7 @@ struct comparator {
 };
 
 static struct comparator tps_comparators[] = {
+	{	.name = "COMP", },
 	{
 		.name = "COMP1",
 		.reg = TPS65911_VMBCH,

Quoted 5 lines · Dan Carpenter

The tps_comparators[] array is used in two places. We only access the COMP1 (1) and COMP2 (2) elements. Unfortunately, we're accessing the wrong elements and also one element beyond the end of the array. There was supposed to be a zero element at the start of the array which is isn't accessed but makes the math work out nicely.

I normally just apply patches from you, but this is a hack, right?

Quoted 8 lines · Lee Jones

Quoted 5 lines · Dan Carpenter

The tps_comparators[] array is used in two places. We only access the COMP1 (1) and COMP2 (2) elements. Unfortunately, we're accessing the wrong elements and also one element beyond the end of the array. There was supposed to be a zero element at the start of the array which is isn't accessed but makes the math work out nicely.

Quoted 2 lines

I normally just apply patches from you, but this is a hack, right?

I liked it, I thought it was nice. It uses 32 bytes but any fix was going to use *some* memory. I don't have strong feelings about it though, if you want to write a different patch I can do that instead.

Quoted 8 lines · Lee Jones

Quoted 5 lines · Dan Carpenter

The tps_comparators[] array is used in two places. We only access the COMP1 (1) and COMP2 (2) elements. Unfortunately, we're accessing the wrong elements and also one element beyond the end of the array. There was supposed to be a zero element at the start of the array which is isn't accessed but makes the math work out nicely.

Quoted 1 line

I normally just apply patches from you, but this is a hack, right?

I liked it, I thought it was nice. It uses 32 bytes but any fix was going to use *some* memory. I don't have strong feelings about it though, if you want to write a different patch I can do that instead.

The tps_comparators[] array is used in two places. We only access the COMP1 (1) and COMP2 (2) elements. Unfortunately, we're accessing the wrong elements and also one element beyond the end of the array. There was supposed to be a zero element at the start of the array which is isn't accessed but makes the math work out nicely.

Fixes 6851ad3ab346 ("TPS65911: Comparator: Add comparator driver")

Signed-off-by Dan Carpenter <[email protected]>

---
I can't actually compile this code...

Quoted 9 lines · Dan Carpenter

Quoted 8 lines · Lee Jones

Quoted 5 lines · Dan Carpenter

The tps_comparators[] array is used in two places. We only access the COMP1 (1) and COMP2 (2) elements. Unfortunately, we're accessing the wrong elements and also one element beyond the end of the array. There was supposed to be a zero element at the start of the array which is isn't accessed but makes the math work out nicely.

Quoted 5 lines

Quoted 1 line

I normally just apply patches from you, but this is a hack, right?

I liked it, I thought it was nice. It uses 32 bytes but any fix was going to use *some* memory. I don't have strong feelings about it though, if you want to write a different patch I can do that instead.

#define COMP                                    0
#define COMP1                                   1
#define COMP2                                   2

It's unclear what the defines mean, but if COMP really does exist (is there a datasheet for this device?) then your solution is a suitable one. However, if there is a COMP, then why isn't it used?

If it doesn't actually exist then this would be more appropriate change I think:

#define COMP1 0
#define COMP2 1

Quoted 28 lines · Lee Jones

Quoted 9 lines · Dan Carpenter

Quoted 8 lines · Lee Jones

Quoted 5 lines · Dan Carpenter

The tps_comparators[] array is used in two places. We only access the COMP1 (1) and COMP2 (2) elements. Unfortunately, we're accessing the wrong elements and also one element beyond the end of the array. There was supposed to be a zero element at the start of the array which is isn't accessed but makes the math work out nicely.

Quoted 5 lines

Quoted 1 line

I normally just apply patches from you, but this is a hack, right?

I liked it, I thought it was nice. It uses 32 bytes but any fix was going to use *some* memory. I don't have strong feelings about it though, if you want to write a different patch I can do that instead.

#define COMP                                    0
#define COMP1                                   1
#define COMP2                                   2

It's unclear what the defines mean, but if COMP really does exist (is there a datasheet for this device?) then your solution is a suitable one. However, if there is a COMP, then why isn't it used?

If it doesn't actually exist then this would be more appropriate change I think:

Quoted 3 lines

#define COMP1 0
#define COMP2 1

I hate to define something_one as zero... Let me send a different patch for this and see what you think.

The COMP1 and COMP2 elements are in 0 and 1 respectively so this code is accessing the wrong elements and one space beyond the end of the array. We should be using "id - 1" instead.

The "id" variable is never COMP (0) so that code can be removed.

Fixes 6851ad3ab346 ("TPS65911: Comparator: Add comparator driver")

Signed-off-by Dan Carpenter <[email protected]>

---
v2: we can fix the bug and save memory.

drivers/mfd/tps65911-comparator.c +2 -8

@@ -58,14 +57,11 @@ static struct comparator tps_comparators[] = {
 
 static int comp_threshold_set(struct tps65910 *tps65910, int id, int voltage)
 {
-	struct comparator tps_comp = tps_comparators[id];
+	struct comparator tps_comp = tps_comparators[id - 1];
 	int curr_voltage = 0;
 	int ret;
 	u8 index = 0, val;
 
-	if (id = COMP)
-		return 0;
-
 	while (curr_voltage < tps_comp.uV_max) {
 		curr_voltage = tps_comp.vsel_table[index];
 		if (curr_voltage >= voltage)

@@ -85,13 +81,10 @@ static int comp_threshold_set(struct tps65910 *tps65910, int id, int voltage)
 
 static int comp_threshold_get(struct tps65910 *tps65910, int id)
 {
-	struct comparator tps_comp = tps_comparators[id];
+	struct comparator tps_comp = tps_comparators[id - 1];
 	int ret;
 	u8 val;
 
-	if (id = COMP)
-		return 0;
-
 	ret = tps65910->read(tps65910, tps_comp.reg, 1, &val);
 	if (ret < 0)
 		return ret;

In 2012, we changed the tps65910 API and fixed most drivers but forgot to update this one.

Fixes 3f7e82759c69 ("mfd: Commonize tps65910 regmap access through header")

Signed-off-by Dan Carpenter <[email protected]>

I really don't want authorship credit for a patch that does:

#define COMP1 0
#define COMP2 1

It just makes me itch to look at it...

In 2012, we changed the tps65910 API and fixed most drivers but forgot to update this one.

Fixes 3f7e82759c69 ("mfd: Commonize tps65910 regmap access through header")

Signed-off-by Dan Carpenter <[email protected]>