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...
diff --git a/drivers/mfd/tps65911-comparator.c b/drivers/mfd/tps65911-comparator.c
index c0789f81a1c5..35d7380f6fad 100644
--- a/drivers/mfd/tps65911-comparator.c
+++ b/drivers/mfd/tps65911-comparator.c
@@ -42,6 +42,7 @@ struct comparator {
};
static struct comparator tps_comparators[] = {
+ { .name = "COMP", },
{
.name = "COMP1",
.reg = TPS65911_VMBCH,
On Thu, 19 Apr 2018, Dan Carpenter wrote:
> 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?
> Fixes: 6851ad3ab346 ("TPS65911: Comparator: Add comparator driver")
> Signed-off-by: Dan Carpenter <[email protected]>
> ---
> I can't actually compile this code...
>
> diff --git a/drivers/mfd/tps65911-comparator.c b/drivers/mfd/tps65911-comparator.c
> index c0789f81a1c5..35d7380f6fad 100644
> --- a/drivers/mfd/tps65911-comparator.c
> +++ b/drivers/mfd/tps65911-comparator.c
> @@ -42,6 +42,7 @@ struct comparator {
> };
>
> static struct comparator tps_comparators[] = {
> + { .name = "COMP", },
> {
> .name = "COMP1",
> .reg = TPS65911_VMBCH,
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Fri, Apr 20, 2018 at 09:09:43AM +0100, Lee Jones wrote:
> On Thu, 19 Apr 2018, Dan Carpenter wrote:
>
> > 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?
>
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.
regards,
dan carpenter
On Fri, Apr 20, 2018 at 11:21:50AM +0300, Dan Carpenter wrote:
> On Fri, Apr 20, 2018 at 09:09:43AM +0100, Lee Jones wrote:
> > On Thu, 19 Apr 2018, Dan Carpenter wrote:
> >
> > > 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?
> >
>
> 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.
"If you want *me* to write" I meant...
regards,
dan carpenter
On Fri, 20 Apr 2018, Dan Carpenter wrote:
> On Fri, Apr 20, 2018 at 09:09:43AM +0100, Lee Jones wrote:
> > On Thu, 19 Apr 2018, Dan Carpenter wrote:
> >
> > > 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?
>
> 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
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Thu, Apr 19, 2018 at 04:46:34PM +0300, Dan Carpenter wrote:
> 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...
I assumed that I couldn't compile this code because it was non-x86 or
something but actually it's just really out of date. We broke it in
2012 when we removed the ->write() function pointer.
commit 3f7e82759c692df473675ed06fb90b20f1f225c3
Author: Rhyland Klein <[email protected]>
Date: Tue May 8 11:42:38 2012 -0700
mfd: Commonize tps65910 regmap access through header
regards,
dan carpenter
On Fri, Apr 20, 2018 at 09:39:09AM +0100, Lee Jones wrote:
> On Fri, 20 Apr 2018, Dan Carpenter wrote:
>
> > On Fri, Apr 20, 2018 at 09:09:43AM +0100, Lee Jones wrote:
> > > On Thu, 19 Apr 2018, Dan Carpenter wrote:
> > >
> > > > 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?
> >
> > 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
>
I hate to define something_one as zero... Let me send a different
patch for this and see what you think.
regards,
dan carpenter
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.
diff --git a/drivers/mfd/tps65911-comparator.c b/drivers/mfd/tps65911-comparator.c
index c0789f81a1c5..887409c3938d 100644
--- a/drivers/mfd/tps65911-comparator.c
+++ b/drivers/mfd/tps65911-comparator.c
@@ -22,7 +22,6 @@
#include <linux/gpio.h>
#include <linux/mfd/tps65910.h>
-#define COMP 0
#define COMP1 1
#define COMP2 2
@@ -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]>
diff --git a/drivers/mfd/tps65911-comparator.c b/drivers/mfd/tps65911-comparator.c
index 887409c3938d..90cfdd1f88c6 100644
--- a/drivers/mfd/tps65911-comparator.c
+++ b/drivers/mfd/tps65911-comparator.c
@@ -74,7 +74,7 @@ static int comp_threshold_set(struct tps65910 *tps65910, int id, int voltage)
return -EINVAL;
val = index << 1;
- ret = tps65910->write(tps65910, tps_comp.reg, 1, &val);
+ ret = tps65910_reg_write(tps65910, tps_comp.reg, val);
return ret;
}
@@ -82,10 +82,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 - 1];
+ unsigned int val;
int ret;
- u8 val;
- ret = tps65910->read(tps65910, tps_comp.reg, 1, &val);
+ ret = tps65910_reg_read(tps65910, tps_comp.reg, &val);
if (ret < 0)
return ret;
On Fri, 20 Apr 2018, Dan Carpenter wrote:
> On Fri, Apr 20, 2018 at 09:39:09AM +0100, Lee Jones wrote:
> > On Fri, 20 Apr 2018, Dan Carpenter wrote:
> >
> > > On Fri, Apr 20, 2018 at 09:09:43AM +0100, Lee Jones wrote:
> > > > On Thu, 19 Apr 2018, Dan Carpenter wrote:
> > > >
> > > > > 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?
> > >
> > > 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
> >
>
> I hate to define something_one as zero... Let me send a different
> patch for this and see what you think.
I guess that's the call of the hardware engineer/datasheet author?
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Fri, 20 Apr 2018, Dan Carpenter wrote:
> 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.
Looks like it's as we feared:
http://www.ti.com/lit/ds/symlink/tps65911.pdf (page 52)
There are only 2 comparators 1 and 2.
> diff --git a/drivers/mfd/tps65911-comparator.c b/drivers/mfd/tps65911-comparator.c
> index c0789f81a1c5..887409c3938d 100644
> --- a/drivers/mfd/tps65911-comparator.c
> +++ b/drivers/mfd/tps65911-comparator.c
> @@ -22,7 +22,6 @@
> #include <linux/gpio.h>
> #include <linux/mfd/tps65910.h>
>
> -#define COMP 0
> #define COMP1 1
> #define COMP2 2
Sorry, but I think these defines should describe the indexes into the
supplied struct directly, especially as in this case it is their only
reason for being.
I completely agree with you that defining something that is named '1'
as '0' is not ideal, but IMHO it's better than jumping through hoops
using arithmetic in array indexes.
> @@ -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;
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
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...
regards,
dan carpenter
On 4/20/2018 5:21 AM, Dan Carpenter wrote:
> 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]>
>
LGTM
Acked-by: Rhyland Klein <[email protected]>
--
nvpublic
On Mon, 23 Apr 2018, Dan Carpenter wrote:
> 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...
As I say, I get where you're coming from, but it was the decision made
by the H/W engineers. Doesn't mean it's not the correct thing to do
from a S/W POV.
If you're not happy putting your SOB on the patch, I'll draft it and
sign it off. /me has no shame!
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Fri, 20 Apr 2018, Dan Carpenter wrote:
> 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]>
Applied, thanks.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog