Change in ...coreboot[master]: util/superiotool: detect Fintek F81866, F8196*

[Kyle Stevenson (Code Review)]

Kyle Stevenson has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31052

Change subject: util/superiotool: detect Fintek F81866, F8196*
......................................................................

util/superiotool: detect Fintek F81866, F8196*

Change-Id: I7210e1523a188a8593cd03547bb0c95cd3e7aa39
Signed-off-by: Kyle Stevenson 
---
M util/superiotool/fintek.c
1 file changed, 4 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/31052/1

diff --git a/util/superiotool/fintek.c b/util/superiotool/fintek.c
index 74e7f76..347462e 100644
--- a/util/superiotool/fintek.c
+++ b/util/superiotool/fintek.c
@@ -467,6 +467,10 @@
    		{0x30,0x60,0x61,0x70,0xf0,0xf2,0xf4,0xf5,EOT},
    		{NANA,0x00,0x00,NANA,NANA,NANA,0x00,0x00,EOT}},
    	{EOT}}},
+	{0x1010, "F81866", {
+		{EOT}}},
+	{0x0215, "F81962/F81964/F81966/F81967", {
+		{EOT}}},
    {EOT}
 };

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31052 )

Change subject: util/superiotool: detect Fintek F81866, F8196*
......................................................................

Patch Set 1: Code-Review+1

Tested how? Please add that to the commit message.

[Kyle Stevenson (Code Review)]

Hello Felix Held, Paul Menzel, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/31052

to look at the new patch set (#2).

Change subject: util/superiotool: detect Fintek F81866, F8196*
......................................................................

util/superiotool: detect Fintek F81866, F8196*

F81866 detection tested with the iBASE SI-613:

superiotool r4.9-420-g034e5e6
    Found Fintek F81866 (vid=0x3419, id=0x1010) at 0x4e

F8196* detection is based on chip IDs provided by iBASE, but untested.

Change-Id: I7210e1523a188a8593cd03547bb0c95cd3e7aa39
Signed-off-by: Kyle Stevenson 
---
M util/superiotool/fintek.c
1 file changed, 4 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/31052/2

[Kyle Stevenson (Code Review)]

Kyle Stevenson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31052 )

Change subject: util/superiotool: detect Fintek F81866, F8196*
......................................................................

Patch Set 2:
> Patch Set 1: Code-Review+1
>
> Tested how? Please add that to the commit message.

Thanks, updated.

F8196* detection is not actually tested. If that's a problem, I'll remove it and leave just the F81866 ID.

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31052 )

Change subject: util/superiotool: detect Fintek F81866, F8196*
......................................................................

Patch Set 2:
> > Patch Set 1: Code-Review+1
> >
> > Tested how? Please add that to the commit message.
>
> Thanks, updated.
>
> F8196* detection is not actually tested. If that's a problem, I'll
> remove it and leave just the F81866 ID.

actually, this will just dump the ID, isn't it?
will you add some registers to dump ?

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31052 )

Change subject: util/superiotool: detect Fintek F81866, F8196*
......................................................................

Patch Set 2: Code-Review+1
> F8196* detection is not actually tested. If that's a problem, I'll remove it and leave just the F81866 ID.

So all F8196* chips share the same ID, right? If they all share one ID (I wonder if those are the same silicon, but different packages), then I'm ok with the patch in its current state.
It would be nice if you'd add the registers and their defaults in a follow-up patch

[Kyle Stevenson (Code Review)]

Kyle Stevenson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31052 )

Change subject: util/superiotool: detect Fintek F81866, F8196*
......................................................................

Patch Set 2:
> Patch Set 2:
> > > Patch Set 1: Code-Review+1
> > >
> > > Tested how? Please add that to the commit message.
> >
> > Thanks, updated.
> >
> > F8196* detection is not actually tested. If that's a problem, I'll
> > remove it and leave just the F81866 ID.
>
> actually, this will just dump the ID, isn't it?
> will you add some registers to dump ?

Yes, this patch only enables detection. Currently, I don't have the information or time to add the registers, sorry.

[Kyle Stevenson (Code Review)]

Kyle Stevenson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31052 )

Change subject: util/superiotool: detect Fintek F81866, F8196*
......................................................................

Patch Set 2:
> Patch Set 2: Code-Review+1
> > F8196* detection is not actually tested. If that's a problem, I'll remove it and leave just the F81866 ID.
>
> So all F8196* chips share the same ID, right? If they all share one ID (I wonder if those are the same silicon, but different packages), then I'm ok with the patch in its current state.
> It would be nice if you'd add the registers and their defaults in a follow-up patch

Yes, according to the information I have, all F8196* chips share the same ID.

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31052 )

Change subject: util/superiotool: detect Fintek F81866, F8196*
......................................................................

Patch Set 2: Code-Review+2
> Yes, according to the information I have, all F8196* chips share the same ID.

Ok.

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31052 )

Change subject: util/superiotool: detect Fintek F81866, F8196*
......................................................................

util/superiotool: detect Fintek F81866, F8196*

F81866 detection tested with the iBASE SI-613:

superiotool r4.9-420-g034e5e6
    Found Fintek F81866 (vid=0x3419, id=0x1010) at 0x4e

F8196* detection is based on chip IDs provided by iBASE, but untested.

Change-Id: I7210e1523a188a8593cd03547bb0c95cd3e7aa39
Signed-off-by: Kyle Stevenson 
Reviewed-on: https://review.coreboot.org/c/31052
Reviewed-by: Felix Held 
Reviewed-by: Paul Menzel 
Tested-by: build bot (Jenkins) 
---
M util/superiotool/fintek.c
1 file changed, 4 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Paul Menzel: Looks good to me, but someone else must approve
  Felix Held: Looks good to me, approved

diff --git a/util/superiotool/fintek.c b/util/superiotool/fintek.c
index 74e7f76..347462e 100644
--- a/util/superiotool/fintek.c
+++ b/util/superiotool/fintek.c
@@ -467,6 +467,10 @@
    		{0x30,0x60,0x61,0x70,0xf0,0xf2,0xf4,0xf5,EOT},
    		{NANA,0x00,0x00,NANA,NANA,NANA,0x00,0x00,EOT}},
    	{EOT}}},
+	{0x1010, "F81866", {
+		{EOT}}},
+	{0x0215, "F81962/F81964/F81966/F81967", {
+		{EOT}}},
    {EOT}
 };