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