Can’t the coreboot EDID helpers be used?

Not sure what you mean, this is already calling decode_edid(). Is there any other common code it should be using? (The whole "read EDID over I2C" thing has a bunch of common code that would be nice to factor out at some point, e.g. a couple of lines here are straight out copied from the ps8640 driver. But I'm not sure it's fair to tie that work onto this CL.)

View Change

21 comments:

• File src/drivers/ti/sn65dsi86bridge/Kconfig:

  • Patch Set #4, Line 5: y

    Drivers should always be 'default n'. You should put a 'select DRIVERS_TI_SN65DSI86BRIDGE' under BOARD_SPECIFIC_OPTIONS in src/mainboard/google/trogdor/Kconfig.

  • Patch Set #4, Line 7: TI SN65DSI86BRIDGE

    Please write at least one full sentence.

• File src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.h:

  • Patch Set #4, Line 8: enum bridge_regs {

    Please but all these enums into sn65dsi86bridge.c, not sn65dsi86bridge.h, so you don't pollute the namespace of files which are just including this to call the functions.

  • Patch Set #4, Line 90: enum dp_pll_clk_src {

    (This is the one enum that needs to stay in the sn65dsi86bridge.h file because it needs to be passed in as a parameter. That's okay because it's prefixed SN65_, unlike the others.)

  • Patch Set #4, Line 132: I2C_CLAIM_ADDR_EN1 = 0x60,

    This is a bridge register, shouldn't it be further up with the others and called SN_I2C_CLAIM_ADDR_EN1?

  • Patch Set #4, Line 138: uint8_t

    You should write 'enum dp_pll_clk_src' instead of uint8_t here to make it clearer what the argument expects.

  • Patch Set #4, Line 139: enum cb_err sn65dsi86_bridge_read_edid(uint8_t bus, uint8_t chip, struct edid *out);

    Blank line before #endif (please don't say "Done" on comments you didn't actually address without any further explanation).

• File src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.c:

  • Patch Set #3, Line 13: #define bridge_debug(x...) do {if (0) printk(BIOS_DEBUG, x); } while (0)

    Please add a separate debug macro. Please look how other drivers do it.

    This isn't used anyway, let's just drop it?

  • Patch Set #3, Line 15: #define BRIDGE_GETHIGHERBYTE(x) (uint8_t)((x&0xFF00)>>8)

    Please add spaces around the operator and use lowercase hex letters.

    I'm not aware of any rule forcing a certain capitalization of hex letters?

    This needs parentheses around the whole expression, though.

  • Patch Set #3, Line 97: int

    unsigned int

    Why? I'm not aware of any rules on signedness either (unless we're doing MISRA now... oh god please no!). For simple loop variables and other stuff that clearly fits in the signed data type I see no reason not do use 'int' (in some cases, like for loops running from high to low, using signed types can actually prevent common mistakes).

  • Patch Set #3, Line 248: printk(BIOS_ERR, "ERROR: valid dp rate not found");

    Maybe: […]

    Display doesn't work, probably?

    I guess we could argue whether we want to bubble up error values throughout everything here, though I'm not sure if it's worth making the code more complicated for it. Either way it's gonna result in the display not working (doing further display init stuff when this fails would probably be harmless, other than maybe printing some further errors).

  • Patch Set #3, Line 385: /* Disable vstream */

    Why should it be disabled?

    I think the idea is to turn it of, initialize everything correctly and then turn it on again to avoid visual glitches appearing on the screen during initialization?

  • Patch Set #3, Line 393: /* color bar disabled */

    > Disable color bar […]

    A test pattern the bridge generates by default when you turn it on. You can read the whole datasheet here if you want: https://www.ti.com/lit/ds/sllseh2b/sllseh2b.pdf

• File src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.c:

  • Patch Set #4, Line 60: i2c_writeb(bus, chip, SN_HPD_DISABLE_REG, 0x1);

    This doesn't seem thematically related to reading the EDID. Does this really have to be here (as opposed to in sn65dsi86_bridge_init())?

  • Patch Set #4, Line 150: ARRAY_SIZE

    This read is in bytes, not items. So while it makes no difference for uint8_t, it would be better to write sizeof() here.

  • Patch Set #4, Line 152: ARRAY_SIZE

    (Here ARRAY_SIZE() is the correct one, btw.)

  • Patch Set #4, Line 158: rate_mhz = rate_per_200khz * 200 / 1000;

    Are we okay with possible inaccuracy from rounding here? Since it looks like you're trying to check for exact equality, I think

     for (j = 0; j < ARRAY_SIZE(...); j++)
       if (...lut[j] * (MHz/KHz) == rate_per_200khz * 200)
         rate_valid[j] = true;

    would be more correct.

  • Patch Set #4, Line 205: 1000

    Please use the constants KHz (1000) and MHz (1000000) instead of raw numbers where appropriate.

  • Patch Set #4, Line 206: stream_bit_rate_mhz = (pixel_clk_hz / 1000000) * bpp;

    Not sure how accurate you want this but maybe write

     pixel_clk_hz * bpp / MHz

    ?

  • Patch Set #4, Line 212:

    (MIN_DSI_CLK_FREQ_MHZ / 5) +
    				(((min_req_dsi_clk - MIN_DSI_CLK_FREQ_MHZ) / 5) & 0xFF)
    

    Maybe I'm missing something but isn't this just a more complicated way to say min_req_dsi_clk / 5?

    Also, the datasheet says the valid range is 40 to 750 so maybe write

     min_req_dsi_clk = MIN(MIN_DSI_CLK_FREQ_MHZ, MAX(MAX_DSI_CLK_FREQ_MHZ, min_req_dsi_clk)) / 5;

    (with MAX_DSI_CLK_FREQ_MHZ being 750... maybe the name could be a bit shorter too).

  • Patch Set #4, Line 391: (uint8_t)

    Why is there a cast here?

To view, visit change 42899. To unsubscribe, or for help writing mail filters, visit settings.