Skip to contentSkip to conversation

drivers/media: nuvoton: always true expression

PATCH In review Nicolas Kaiser 3 participants 7 messages Nov 16, 2010 – Nov 19, 2010 Media (V4L/DVB)

lore
Share & tools
lore

Nicolas Kaiser

author
#0 lore raw

PATCH 1/1 drivers/media: nuvoton: always true expression

I noticed that the second part of this conditional is always true. Would the intention be to strictly check on both chip_major and chip_minor?

Signed-off-by Nicolas Kaiser <[email protected]>
---
 drivers/media/IR/nuvoton-cir.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
drivers/media/IR/nuvoton-cir.c +2 -2
index 301be53..896463b 100644
@@ -249,8 +249,8 @@ static int nvt_hw_detect(struct nvt_dev *nvt)
 	chip_minor = nvt_cr_read(nvt, CR_CHIP_ID_LO);
 	nvt_dbg("%s: chip id: 0x%02x 0x%02x", chip_id, chip_major, chip_minor);
 
-	if (chip_major != CHIP_ID_HIGH &&
-	    (chip_minor != CHIP_ID_LOW || chip_minor != CHIP_ID_LOW2))
+	if (chip_major != CHIP_ID_HIGH ||
+	    (chip_minor != CHIP_ID_LOW && chip_minor != CHIP_ID_LOW2))
 		ret = -ENODEV;
 
 	nvt_efm_disable(nvt);
Signature · 1 line
1.7.2.2
Acked-by on v1 1/1
#1 lore raw
Quoted 5 lines · Nicolas Kaiser

I noticed that the second part of this conditional is always true. Would the intention be to strictly check on both chip_major and chip_minor?

Signed-off-by Nicolas Kaiser <[email protected]>

Hrm, yeah, looks like I screwed that one up. You're correct, the intention was to make sure we have a matching chip id high and one or the other of the chip id low values.

Acked-by Jarod Wilson <[email protected]>
Signature · 2 lines
Jarod Wilson
[email protected]
#2 lore raw

Em 16-11-2010 19:54, Jarod Wilson escreveu:

Quoted 5 lines · Nicolas Kaiser
Quoted 4 lines

I noticed that the second part of this conditional is always true. Would the intention be to strictly check on both chip_major and chip_minor?

Quoted 8 lines
Quoted 1 line
Signed-off-by Nicolas Kaiser <[email protected]>

Hrm, yeah, looks like I screwed that one up. You're correct, the intention was to make sure we have a matching chip id high and one or the other of the chip id low values.

Acked-by Jarod Wilson <[email protected]>

I wander if it wouldn't be good to print something if the probe fails due to the wrong chip ID. It may help if someone complain about a different revision.

Mauro.

author
#3 lore raw

PATCH 1/1 drivers/media: nuvoton: fix chip id probe

Make sure we have a matching chip id high and one or the other of the chip id low values.
Print the values if the probe fails.

Signed-off-by Nicolas Kaiser <[email protected]>

---
Like this?
Supersedes patch "drivers/media: nuvoton: always true expression".

 drivers/media/IR/nuvoton-cir.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)
drivers/media/IR/nuvoton-cir.c +5 -2
index 301be53..92d32c8 100644
@@ -249,9 +249,12 @@ static int nvt_hw_detect(struct nvt_dev *nvt)
 	chip_minor = nvt_cr_read(nvt, CR_CHIP_ID_LO);
 	nvt_dbg("%s: chip id: 0x%02x 0x%02x", chip_id, chip_major, chip_minor);
 
-	if (chip_major != CHIP_ID_HIGH &&
-	    (chip_minor != CHIP_ID_LOW || chip_minor != CHIP_ID_LOW2))
+	if (chip_major != CHIP_ID_HIGH ||
+	    (chip_minor != CHIP_ID_LOW && chip_minor != CHIP_ID_LOW2)) {
+		nvt_pr(KERN_ERR, "%s: chip id mismatch: 0x%02x 0x%02x",
+		       chip_id, chip_major, chip_minor);
 		ret = -ENODEV;
+	}
 
 	nvt_efm_disable(nvt);
Signature · 1 line
1.7.2.2
#4 lore raw

Re: [PATCH] drivers/media: nuvoton: fix chip id probe

Quoted 17 lines · Nicolas Kaiser

Make sure we have a matching chip id high and one or the other of the chip id low values.
Print the values if the probe fails.

Signed-off-by Nicolas Kaiser <[email protected]>

---
Like this?
Supersedes patch "drivers/media: nuvoton: always true expression".

 drivers/media/IR/nuvoton-cir.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)
drivers/media/IR/nuvoton-cir.c +0 -0
index 301be53..92d32c8 100644

drivers/media/IR/nuvoton-cir.c static int nvt_hw_detect(struct nvt_dev *nvt) · line 254

Quoted 8 lines
drivers/media/IR/nuvoton-cir.c +3 -2
@@ -249,9 +249,12 @@ static int nvt_hw_detect(struct nvt_dev *nvt)
 	chip_minor = nvt_cr_read(nvt, CR_CHIP_ID_LO);
 	nvt_dbg("%s: chip id: 0x%02x 0x%02x", chip_id, chip_major, chip_minor);
 
-	if (chip_major != CHIP_ID_HIGH &&
-	    (chip_minor != CHIP_ID_LOW || chip_minor != CHIP_ID_LOW2))
+	if (chip_major != CHIP_ID_HIGH ||
+	    (chip_minor != CHIP_ID_LOW && chip_minor != CHIP_ID_LOW2)) {
+		nvt_pr(KERN_ERR, "%s: chip id mismatch: 0x%02x 0x%02x",

I'd probably go with "unsupported chip, id: " instead, since it makes the message a bit clearer, but generally speaking, yeah, something along those lines should be fine.

Signature · 2 lines
Jarod Wilson
[email protected]
author
#5 lore raw

PATCH 1/1 drivers/media: nuvoton: fix chip id probe v2

Make sure we have a matching chip id high and one or the other of the chip id low values.
Print the values if the probe fails.

Signed-off-by Nicolas Kaiser <[email protected]>
---
 drivers/media/IR/nuvoton-cir.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)
drivers/media/IR/nuvoton-cir.c +5 -2
index 301be53..e3274ef 100644
@@ -249,9 +249,12 @@ static int nvt_hw_detect(struct nvt_dev *nvt)
 	chip_minor = nvt_cr_read(nvt, CR_CHIP_ID_LO);
 	nvt_dbg("%s: chip id: 0x%02x 0x%02x", chip_id, chip_major, chip_minor);
 
-	if (chip_major != CHIP_ID_HIGH &&
-	    (chip_minor != CHIP_ID_LOW || chip_minor != CHIP_ID_LOW2))
+	if (chip_major != CHIP_ID_HIGH ||
+	    (chip_minor != CHIP_ID_LOW && chip_minor != CHIP_ID_LOW2)) {
+		nvt_pr(KERN_ERR, "%s: unsupported chip, id: 0x%02x 0x%02x",
+		       chip_id, chip_major, chip_minor);
 		ret = -ENODEV;
+	}
 
 	nvt_efm_disable(nvt);
Signature · 1 line
1.7.2.2