Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/39376 )
Change subject: util/superiotool: Drop one SCH5317 entry ......................................................................
util/superiotool: Drop one SCH5317 entry
The SCH5317 can have either 0x85 or 0x8c as device ID. However, the former results in false positives on any ITE IT85xx series embedded controller, which has led some people to think that chip was actually in their laptops. Moreover, there is no register dump for the SCH5317.
Since nobody has touched this in over a decade, avoid further confusion by dropping the misleading definition.
Change-Id: I4d1d34d1b88b878461499e52f1a916ee1e33210d Signed-off-by: Angel Pons th3fanbus@gmail.com --- M util/superiotool/smsc.c 1 file changed, 0 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/39376/1
diff --git a/util/superiotool/smsc.c b/util/superiotool/smsc.c index ab61ba4..7e50548 100644 --- a/util/superiotool/smsc.c +++ b/util/superiotool/smsc.c @@ -860,9 +860,6 @@ {EOT}}}, {0x83, "SCH5514D", { /* From sensors-detect */ {EOT}}}, - {0x85, "SCH5317", { /* From sensors-detect */ - /* The SCH5317 can have either 0x85 or 0x8c as device ID. */ - {EOT}}}, {0x86, "SCH5127", { /* From sensors-detect, dump from datasheet */ {NOLDN, NULL, {0x02,0x03,0x21,0x22,0x23,0x24,0x26,0x27,
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39376 )
Change subject: util/superiotool: Drop one SCH5317 entry ......................................................................
Patch Set 1: Code-Review+1
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39376 )
Change subject: util/superiotool: Drop one SCH5317 entry ......................................................................
Patch Set 1: Code-Review+2
No strong feelings about this. But it's just working around a bigger issue: Probing a single byte is not a good idea. We are hit by the 0x85 entry because IT85xx chips are popular atm. But all the other entries can give false positives, too. IMHO, the probing function should be adapted, maybe even hidden behind a command line switch.
It's not the first workaround btw., somebody already started to filter false-positives in probe_idregs_smsc_helper().
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39376 )
Change subject: util/superiotool: Drop one SCH5317 entry ......................................................................
Patch Set 1:
Patch Set 1: Code-Review+2
No strong feelings about this. But it's just working around a bigger issue: Probing a single byte is not a good idea. We are hit by the 0x85 entry because IT85xx chips are popular atm. But all the other entries can give false positives, too. IMHO, the probing function should be adapted, maybe even hidden behind a command line switch.
It's not the first workaround btw., somebody already started to filter false-positives in probe_idregs_smsc_helper().
I see. However, the FDC37N972 has a register dump, so it was a good idea to keep it. However, as there's no datasheet for the SCH5317, checking more bytes is not really feasible to do.
Let's not forget that the IT8502E exists as well, which would be detected as a SCH5317 with revision 0x02...
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39376 )
Change subject: util/superiotool: Drop one SCH5317 entry ......................................................................
util/superiotool: Drop one SCH5317 entry
The SCH5317 can have either 0x85 or 0x8c as device ID. However, the former results in false positives on any ITE IT85xx series embedded controller, which has led some people to think that chip was actually in their laptops. Moreover, there is no register dump for the SCH5317.
Since nobody has touched this in over a decade, avoid further confusion by dropping the misleading definition.
Change-Id: I4d1d34d1b88b878461499e52f1a916ee1e33210d Signed-off-by: Angel Pons th3fanbus@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/39376 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Paul Menzel paulepanter@users.sourceforge.net Reviewed-by: Nico Huber nico.h@gmx.de --- M util/superiotool/smsc.c 1 file changed, 0 insertions(+), 3 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Paul Menzel: Looks good to me, but someone else must approve
diff --git a/util/superiotool/smsc.c b/util/superiotool/smsc.c index ab61ba4..7e50548 100644 --- a/util/superiotool/smsc.c +++ b/util/superiotool/smsc.c @@ -860,9 +860,6 @@ {EOT}}}, {0x83, "SCH5514D", { /* From sensors-detect */ {EOT}}}, - {0x85, "SCH5317", { /* From sensors-detect */ - /* The SCH5317 can have either 0x85 or 0x8c as device ID. */ - {EOT}}}, {0x86, "SCH5127", { /* From sensors-detect, dump from datasheet */ {NOLDN, NULL, {0x02,0x03,0x21,0x22,0x23,0x24,0x26,0x27,
-
Angel Pons (Code Review) -
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)