Evgeny Zinoviev has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32719 ) Change subject: drivers/apple: Add hybrid graphics driver ...................................................................... Patch Set 9: (5 comments) https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/ch... File src/drivers/apple/hybrid_graphics/chip.h: https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/ch... PS8, Line 27: unsigned int
`bool`? Done
https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/gm... File src/drivers/apple/hybrid_graphics/gmux.h: https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/gm... PS8, Line 45: int port
Looking at the code it seems limited to 8 bits? So why isn't it a `u8`? you're right.
https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/gm... File src/drivers/apple/hybrid_graphics/gmux.c: https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/gm... PS8, Line 34: udelay(100);
I think this belongs before the line above it. Currently the […] Well, what you're saying seems logical, but this code is from apple-gmux linux driver. Maybe it's just a mistake, maybe not. I will eventually test all this more deeply. But for now I can't answer to this and some other your comments
https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/hy... File src/drivers/apple/hybrid_graphics/hybrid_graphics.c: https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/hy... PS8, Line 32: /* Don't confuse anyone else and disable the fake device */
Why? Who would get confused? This is a copy-pasta from lenovo/hybrid_graphics driver, siro should know :)
But my guess is that devices might be enumarated somewhere else in the code and this fake device should be marked as disabled because this is not a real device https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/ro... File src/drivers/apple/hybrid_graphics/romstage.c: https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/ro... PS8, Line 30: /* TODO: Use generic device instead of dummy PNP device */
The generic device couldn't be used at time of writing the lenovo hybrid graphics driver. […] It's based on lenovo/hybrid_graphics. I can rewrite it the right way, though I don't know it, tell me please where I can see an example or something
-- To view, visit https://review.coreboot.org/c/coreboot/+/32719 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I22b66622cd2da0e9951ee726d650d204fbb8a5bc Gerrit-Change-Number: 32719 Gerrit-PatchSet: 9 Gerrit-Owner: Evgeny Zinoviev <me@ch1p.com> Gerrit-Reviewer: Evgeny Zinoviev <me@ch1p.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Sun, 26 May 2019 22:15:51 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Patrick Rudolph <siro@das-labor.org> Comment-In-Reply-To: Nico Huber <nico.h@gmx.de> Gerrit-MessageType: comment