* [PATCH 1/1] mfd: tps65911-comparator: Fix an off by one bug
@ 2018-05-01 9:45 ` Lee Jones
0 siblings, 0 replies; 10+ messages in thread
From: Lee Jones @ 2018-05-01 9:45 UTC (permalink / raw)
To: dan.carpenter; +Cc: linux-arm-kernel, linux-kernel, Lee Jones
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.
The "id" variable is never COMP (0) so that code can be removed.
Fixes: 6851ad3ab346 ("TPS65911: Comparator: Add comparator driver")
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
History:
Dan was the originator of this patch and the author of the commit log,
but produced 2 code solutions which I wasn't happy with. The first
submission [0] introduced a COMP device, which after a quick check of
the datasheet [1] appeared to be fictitious. A subsequent submission
[2] conducted arithmetic in array indexes.
It is my belief that the correct solution is to roll which the
situation the hardware engineers presented us with and define COMP1
at position 0 and COMP2 at position 1 such that we can use the
simplest code possible to select them.
Dan wasn't happy to put his name to this, which I completely
understand. Calling SOMETHING1 0 (zero) is a little unnatural.
However, since I have no shame, I offered to submit it.
[0] https://lkml.org/lkml/2018/4/19/449
[1] http://www.ti.com/lit/ds/symlink/tps65911.pdf (page 52)
[2] https://lkml.org/lkml/2018/4/20/204
drivers/mfd/tps65911-comparator.c | 11 ++---------
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/drivers/mfd/tps65911-comparator.c b/drivers/mfd/tps65911-comparator.c
index d223857fb4ad..33591767fb9b 100644
--- a/drivers/mfd/tps65911-comparator.c
+++ b/drivers/mfd/tps65911-comparator.c
@@ -22,9 +22,8 @@
#include <linux/gpio.h>
#include <linux/mfd/tps65910.h>
-#define COMP 0
-#define COMP1 1
-#define COMP2 2
+#define COMP1 0
+#define COMP2 1
/* Comparator 1 voltage selection table in millivolts */
static const u16 COMP_VSEL_TABLE[] = {
@@ -63,9 +62,6 @@ static int comp_threshold_set(struct tps65910 *tps65910, int id, int voltage)
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)
@@ -89,9 +85,6 @@ static int comp_threshold_get(struct tps65910 *tps65910, int id)
unsigned int val;
int ret;
- if (id == COMP)
- return 0;
-
ret = tps65910_reg_read(tps65910, tps_comp.reg, &val);
if (ret < 0)
return ret;
--
2.17.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 1/1] mfd: tps65911-comparator: Fix an off by one bug
@ 2018-05-01 9:45 ` Lee Jones
0 siblings, 0 replies; 10+ messages in thread
From: Lee Jones @ 2018-05-01 9:45 UTC (permalink / raw)
To: linux-arm-kernel
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.
The "id" variable is never COMP (0) so that code can be removed.
Fixes: 6851ad3ab346 ("TPS65911: Comparator: Add comparator driver")
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
History:
Dan was the originator of this patch and the author of the commit log,
but produced 2 code solutions which I wasn't happy with. The first
submission [0] introduced a COMP device, which after a quick check of
the datasheet [1] appeared to be fictitious. A subsequent submission
[2] conducted arithmetic in array indexes.
It is my belief that the correct solution is to roll which the
situation the hardware engineers presented us with and define COMP1
at position 0 and COMP2 at position 1 such that we can use the
simplest code possible to select them.
Dan wasn't happy to put his name to this, which I completely
understand. Calling SOMETHING1 0 (zero) is a little unnatural.
However, since I have no shame, I offered to submit it.
[0] https://lkml.org/lkml/2018/4/19/449
[1] http://www.ti.com/lit/ds/symlink/tps65911.pdf (page 52)
[2] https://lkml.org/lkml/2018/4/20/204
drivers/mfd/tps65911-comparator.c | 11 ++---------
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/drivers/mfd/tps65911-comparator.c b/drivers/mfd/tps65911-comparator.c
index d223857fb4ad..33591767fb9b 100644
--- a/drivers/mfd/tps65911-comparator.c
+++ b/drivers/mfd/tps65911-comparator.c
@@ -22,9 +22,8 @@
#include <linux/gpio.h>
#include <linux/mfd/tps65910.h>
-#define COMP 0
-#define COMP1 1
-#define COMP2 2
+#define COMP1 0
+#define COMP2 1
/* Comparator 1 voltage selection table in millivolts */
static const u16 COMP_VSEL_TABLE[] = {
@@ -63,9 +62,6 @@ static int comp_threshold_set(struct tps65910 *tps65910, int id, int voltage)
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)
@@ -89,9 +85,6 @@ static int comp_threshold_get(struct tps65910 *tps65910, int id)
unsigned int val;
int ret;
- if (id == COMP)
- return 0;
-
ret = tps65910_reg_read(tps65910, tps_comp.reg, &val);
if (ret < 0)
return ret;
--
2.17.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 1/1] mfd: tps65911-comparator: Fix an off by one bug
2018-05-01 9:45 ` Lee Jones
@ 2018-05-01 12:41 ` Robin Murphy
-1 siblings, 0 replies; 10+ messages in thread
From: Robin Murphy @ 2018-05-01 12:41 UTC (permalink / raw)
To: Lee Jones, dan.carpenter; +Cc: linux-kernel, linux-arm-kernel
On 01/05/18 10:45, Lee Jones 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.
>
> The "id" variable is never COMP (0) so that code can be removed.
>
> Fixes: 6851ad3ab346 ("TPS65911: Comparator: Add comparator driver")
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>
> History:
>
> Dan was the originator of this patch and the author of the commit log,
> but produced 2 code solutions which I wasn't happy with. The first
> submission [0] introduced a COMP device, which after a quick check of
> the datasheet [1] appeared to be fictitious. A subsequent submission
> [2] conducted arithmetic in array indexes.
>
> It is my belief that the correct solution is to roll which the
> situation the hardware engineers presented us with and define COMP1
> at position 0 and COMP2 at position 1 such that we can use the
> simplest code possible to select them.
>
> Dan wasn't happy to put his name to this, which I completely
> understand. Calling SOMETHING1 0 (zero) is a little unnatural.
>
> However, since I have no shame, I offered to submit it.
As an idly-curious passer-by, this looks perfectly reasonable to me - I
don't see why a mapping between names and indices should have to be
artificially constrained just because the names happen to contain
numerals. If it's really that abhorrent, then I guess they could be
named something like COMPn_ID for even more clarity.
That said, now that I've gone and looked, the whole business seems
ridiculously over-engineered. AFAICS it would be infinitely simpler to
just pass the register address directly where id is currently passed,
statically define UV_MAX, and get rid of the otherwise-pointless struct
comparator entirely. The current abstraction doesn't look like it could
actually scale to support different chips without major surgery anyway.
Robin.
> [0] https://lkml.org/lkml/2018/4/19/449
> [1] http://www.ti.com/lit/ds/symlink/tps65911.pdf (page 52)
> [2] https://lkml.org/lkml/2018/4/20/204
>
> drivers/mfd/tps65911-comparator.c | 11 ++---------
> 1 file changed, 2 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/mfd/tps65911-comparator.c b/drivers/mfd/tps65911-comparator.c
> index d223857fb4ad..33591767fb9b 100644
> --- a/drivers/mfd/tps65911-comparator.c
> +++ b/drivers/mfd/tps65911-comparator.c
> @@ -22,9 +22,8 @@
> #include <linux/gpio.h>
> #include <linux/mfd/tps65910.h>
>
> -#define COMP 0
> -#define COMP1 1
> -#define COMP2 2
> +#define COMP1 0
> +#define COMP2 1
>
> /* Comparator 1 voltage selection table in millivolts */
> static const u16 COMP_VSEL_TABLE[] = {
> @@ -63,9 +62,6 @@ static int comp_threshold_set(struct tps65910 *tps65910, int id, int voltage)
> 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)
> @@ -89,9 +85,6 @@ static int comp_threshold_get(struct tps65910 *tps65910, int id)
> unsigned int val;
> int ret;
>
> - if (id == COMP)
> - return 0;
> -
> ret = tps65910_reg_read(tps65910, tps_comp.reg, &val);
> if (ret < 0)
> return ret;
>
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH 1/1] mfd: tps65911-comparator: Fix an off by one bug
@ 2018-05-01 12:41 ` Robin Murphy
0 siblings, 0 replies; 10+ messages in thread
From: Robin Murphy @ 2018-05-01 12:41 UTC (permalink / raw)
To: linux-arm-kernel
On 01/05/18 10:45, Lee Jones 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.
>
> The "id" variable is never COMP (0) so that code can be removed.
>
> Fixes: 6851ad3ab346 ("TPS65911: Comparator: Add comparator driver")
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>
> History:
>
> Dan was the originator of this patch and the author of the commit log,
> but produced 2 code solutions which I wasn't happy with. The first
> submission [0] introduced a COMP device, which after a quick check of
> the datasheet [1] appeared to be fictitious. A subsequent submission
> [2] conducted arithmetic in array indexes.
>
> It is my belief that the correct solution is to roll which the
> situation the hardware engineers presented us with and define COMP1
> at position 0 and COMP2 at position 1 such that we can use the
> simplest code possible to select them.
>
> Dan wasn't happy to put his name to this, which I completely
> understand. Calling SOMETHING1 0 (zero) is a little unnatural.
>
> However, since I have no shame, I offered to submit it.
As an idly-curious passer-by, this looks perfectly reasonable to me - I
don't see why a mapping between names and indices should have to be
artificially constrained just because the names happen to contain
numerals. If it's really that abhorrent, then I guess they could be
named something like COMPn_ID for even more clarity.
That said, now that I've gone and looked, the whole business seems
ridiculously over-engineered. AFAICS it would be infinitely simpler to
just pass the register address directly where id is currently passed,
statically define UV_MAX, and get rid of the otherwise-pointless struct
comparator entirely. The current abstraction doesn't look like it could
actually scale to support different chips without major surgery anyway.
Robin.
> [0] https://lkml.org/lkml/2018/4/19/449
> [1] http://www.ti.com/lit/ds/symlink/tps65911.pdf (page 52)
> [2] https://lkml.org/lkml/2018/4/20/204
>
> drivers/mfd/tps65911-comparator.c | 11 ++---------
> 1 file changed, 2 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/mfd/tps65911-comparator.c b/drivers/mfd/tps65911-comparator.c
> index d223857fb4ad..33591767fb9b 100644
> --- a/drivers/mfd/tps65911-comparator.c
> +++ b/drivers/mfd/tps65911-comparator.c
> @@ -22,9 +22,8 @@
> #include <linux/gpio.h>
> #include <linux/mfd/tps65910.h>
>
> -#define COMP 0
> -#define COMP1 1
> -#define COMP2 2
> +#define COMP1 0
> +#define COMP2 1
>
> /* Comparator 1 voltage selection table in millivolts */
> static const u16 COMP_VSEL_TABLE[] = {
> @@ -63,9 +62,6 @@ static int comp_threshold_set(struct tps65910 *tps65910, int id, int voltage)
> 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)
> @@ -89,9 +85,6 @@ static int comp_threshold_get(struct tps65910 *tps65910, int id)
> unsigned int val;
> int ret;
>
> - if (id == COMP)
> - return 0;
> -
> ret = tps65910_reg_read(tps65910, tps_comp.reg, &val);
> if (ret < 0)
> return ret;
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1/1] mfd: tps65911-comparator: Fix an off by one bug
2018-05-01 12:41 ` Robin Murphy
@ 2018-05-01 13:10 ` Dan Carpenter
-1 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2018-05-01 13:10 UTC (permalink / raw)
To: Robin Murphy; +Cc: Lee Jones, linux-kernel, linux-arm-kernel
The background is that no one noticed that this driver stopped being
buildable in 2012 until I fixed it last week. This is the third
proposed patch to fix the off by one. All three patches fix the bug,
but Lee refused to accept my style and I refused to use Lee's style...
Absolutely *no way*! LOL. But really neither of us take it personally
and we've met in person before and collaborate over email and get along.
This thread has been pretty amusing for me, but let's move on. :P
regards,
dan carpenter
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/1] mfd: tps65911-comparator: Fix an off by one bug
@ 2018-05-01 13:10 ` Dan Carpenter
0 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2018-05-01 13:10 UTC (permalink / raw)
To: linux-arm-kernel
The background is that no one noticed that this driver stopped being
buildable in 2012 until I fixed it last week. This is the third
proposed patch to fix the off by one. All three patches fix the bug,
but Lee refused to accept my style and I refused to use Lee's style...
Absolutely *no way*! LOL. But really neither of us take it personally
and we've met in person before and collaborate over email and get along.
This thread has been pretty amusing for me, but let's move on. :P
regards,
dan carpenter
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] mfd: tps65911-comparator: Fix an off by one bug
2018-05-01 13:10 ` Dan Carpenter
@ 2018-05-01 13:24 ` Lee Jones
-1 siblings, 0 replies; 10+ messages in thread
From: Lee Jones @ 2018-05-01 13:24 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Robin Murphy, linux-kernel, linux-arm-kernel
On Tue, 01 May 2018, Dan Carpenter wrote:
> The background is that no one noticed that this driver stopped being
> buildable in 2012 until I fixed it last week. This is the third
> proposed patch to fix the off by one. All three patches fix the bug,
> but Lee refused to accept my style and I refused to use Lee's style...
> Absolutely *no way*! LOL. But really neither of us take it personally
> and we've met in person before and collaborate over email and get along.
>
> This thread has been pretty amusing for me, but let's move on. :P
+999
Plan is to let it sit for a week or so. Give some more idle
passers-by to have their say. Then I'll merge it regardless (/me
jokes).
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/1] mfd: tps65911-comparator: Fix an off by one bug
@ 2018-05-01 13:24 ` Lee Jones
0 siblings, 0 replies; 10+ messages in thread
From: Lee Jones @ 2018-05-01 13:24 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, 01 May 2018, Dan Carpenter wrote:
> The background is that no one noticed that this driver stopped being
> buildable in 2012 until I fixed it last week. This is the third
> proposed patch to fix the off by one. All three patches fix the bug,
> but Lee refused to accept my style and I refused to use Lee's style...
> Absolutely *no way*! LOL. But really neither of us take it personally
> and we've met in person before and collaborate over email and get along.
>
> This thread has been pretty amusing for me, but let's move on. :P
+999
Plan is to let it sit for a week or so. Give some more idle
passers-by to have their say. Then I'll merge it regardless (/me
jokes).
--
Lee Jones [???]
Linaro Services Technical Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] mfd: tps65911-comparator: Fix an off by one bug
2018-05-01 12:41 ` Robin Murphy
@ 2018-05-01 13:22 ` Lee Jones
-1 siblings, 0 replies; 10+ messages in thread
From: Lee Jones @ 2018-05-01 13:22 UTC (permalink / raw)
To: Robin Murphy; +Cc: dan.carpenter, linux-kernel, linux-arm-kernel
On Tue, 01 May 2018, Robin Murphy wrote:
> On 01/05/18 10:45, Lee Jones 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.
> >
> > The "id" variable is never COMP (0) so that code can be removed.
> >
> > Fixes: 6851ad3ab346 ("TPS65911: Comparator: Add comparator driver")
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >
> > History:
> >
> > Dan was the originator of this patch and the author of the commit log,
> > but produced 2 code solutions which I wasn't happy with. The first
> > submission [0] introduced a COMP device, which after a quick check of
> > the datasheet [1] appeared to be fictitious. A subsequent submission
> > [2] conducted arithmetic in array indexes.
> >
> > It is my belief that the correct solution is to roll which the
> > situation the hardware engineers presented us with and define COMP1
> > at position 0 and COMP2 at position 1 such that we can use the
> > simplest code possible to select them.
> >
> > Dan wasn't happy to put his name to this, which I completely
> > understand. Calling SOMETHING1 0 (zero) is a little unnatural.
> >
> > However, since I have no shame, I offered to submit it.
>
> As an idly-curious passer-by, this looks perfectly reasonable to me - I
> don't see why a mapping between names and indices should have to be
> artificially constrained just because the names happen to contain numerals.
Right. This was my thinking too.
> If it's really that abhorrent, then I guess they could be named something
> like COMPn_ID for even more clarity.
>
> That said, now that I've gone and looked, the whole business seems
> ridiculously over-engineered. AFAICS it would be infinitely simpler to just
> pass the register address directly where id is currently passed, statically
> define UV_MAX, and get rid of the otherwise-pointless struct comparator
> entirely. The current abstraction doesn't look like it could actually scale
> to support different chips without major surgery anyway.
Sounds good. Patches always welcome. ;)
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH 1/1] mfd: tps65911-comparator: Fix an off by one bug
@ 2018-05-01 13:22 ` Lee Jones
0 siblings, 0 replies; 10+ messages in thread
From: Lee Jones @ 2018-05-01 13:22 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, 01 May 2018, Robin Murphy wrote:
> On 01/05/18 10:45, Lee Jones 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.
> >
> > The "id" variable is never COMP (0) so that code can be removed.
> >
> > Fixes: 6851ad3ab346 ("TPS65911: Comparator: Add comparator driver")
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >
> > History:
> >
> > Dan was the originator of this patch and the author of the commit log,
> > but produced 2 code solutions which I wasn't happy with. The first
> > submission [0] introduced a COMP device, which after a quick check of
> > the datasheet [1] appeared to be fictitious. A subsequent submission
> > [2] conducted arithmetic in array indexes.
> >
> > It is my belief that the correct solution is to roll which the
> > situation the hardware engineers presented us with and define COMP1
> > at position 0 and COMP2 at position 1 such that we can use the
> > simplest code possible to select them.
> >
> > Dan wasn't happy to put his name to this, which I completely
> > understand. Calling SOMETHING1 0 (zero) is a little unnatural.
> >
> > However, since I have no shame, I offered to submit it.
>
> As an idly-curious passer-by, this looks perfectly reasonable to me - I
> don't see why a mapping between names and indices should have to be
> artificially constrained just because the names happen to contain numerals.
Right. This was my thinking too.
> If it's really that abhorrent, then I guess they could be named something
> like COMPn_ID for even more clarity.
>
> That said, now that I've gone and looked, the whole business seems
> ridiculously over-engineered. AFAICS it would be infinitely simpler to just
> pass the register address directly where id is currently passed, statically
> define UV_MAX, and get rid of the otherwise-pointless struct comparator
> entirely. The current abstraction doesn't look like it could actually scale
> to support different chips without major surgery anyway.
Sounds good. Patches always welcome. ;)
--
Lee Jones [???]
Linaro Services Technical Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-05-01 13:24 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-01 9:45 [PATCH 1/1] mfd: tps65911-comparator: Fix an off by one bug Lee Jones
2018-05-01 9:45 ` Lee Jones
2018-05-01 12:41 ` Robin Murphy
2018-05-01 12:41 ` Robin Murphy
2018-05-01 13:10 ` Dan Carpenter
2018-05-01 13:10 ` Dan Carpenter
2018-05-01 13:24 ` Lee Jones
2018-05-01 13:24 ` Lee Jones
2018-05-01 13:22 ` Lee Jones
2018-05-01 13:22 ` Lee Jones
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.