14 comments:
Patch Set #8, Line 9: Hybrid graphics driver for Apple MacBook Pro.
Would be a good place to give credit to the original author(s).
File src/drivers/apple/hybrid_graphics/chip.h:
Patch Set #8, Line 24: #define HYBRID_GRAPHICS_DEFAULT_GPU HYBRID_GRAPHICS_INTEGRATED
Make it a Kconfig?
Patch Set #8, Line 27: unsigned int
`bool`?
File src/drivers/apple/hybrid_graphics/gmux.h:
Patch Set #8, Line 43: #define GMUX_IOSTART 0x700
It would be wise to reserve this for the allocator. For this,
you'd set up a `struct device_operations` in enable_dev(). The
necessary operation would be `read_resources()` which can add
a fixed resource.
Patch Set #8, Line 45: int port
Looking at the code it seems limited to 8 bits? So why isn't it a `u8`?
File src/drivers/apple/hybrid_graphics/gmux.c:
It would be nice to give this a name. Looking at the
code, it looks much like the `output buffer full` (OBF)
of a classic keyboard controller. Whenever this bit is
set in GMUX_PORT_WRITE, there is something to read in
GMUX_PORT_READ.
Patch Set #8, Line 32: inb(GMUX_IOSTART + GMUX_PORT_READ);
Any chance you know what is read (and discarded) here? Some kind
of status code, maybe? or is it just for synchronization?
Patch Set #8, Line 34: udelay(100);
I think this belongs before the line above it. Currently the
sequence is
<-- GMUX_PORT_WRITE
<-- GMUX_PORT_READ
<-- GMUX_PORT_WRITE
delay
<-- GMUX_PORT_READ
<-- GMUX_PORT_WRITE
delay
...
<-- GMUX_PORT_READ
<-- GMUX_PORT_WRITE
delay
Which seems odd at the start.
Patch Set #8, Line 38: return !!i;
Shouldn't this be `!(gwr & 0x01)`? i.e. what if the last read succeeded?
Patch Set #8, Line 48: udelay(100);
same as above
Patch Set #8, Line 55: return !!i;
same as above
Patch Set #8, Line 68: outb((port & 0xff), GMUX_IOSTART + GMUX_PORT_READ);
I am very curious if after this another bit in
GMUX_PORT_WRITE is set; similar to IBF of a KBC.
File src/drivers/apple/hybrid_graphics/hybrid_graphics.c:
Patch Set #8, Line 32: /* Don't confuse anyone else and disable the fake device */
Why? Who would get confused?
File src/drivers/apple/hybrid_graphics/romstage.c:
Patch Set #8, Line 30: /* TODO: Use generic device instead of dummy PNP device */
Why is a PNP dev used?
To view, visit change 32719. To unsubscribe, or for help writing mail filters, visit settings.