Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34773 )
Change subject: soc/mediatek: dsi: Support sending MIPI init commands ......................................................................
Patch Set 12:
(7 comments)
https://review.coreboot.org/c/coreboot/+/34773/12/src/soc/mediatek/common/ds... File src/soc/mediatek/common/dsi.c:
https://review.coreboot.org/c/coreboot/+/34773/12/src/soc/mediatek/common/ds... PS12, Line 265: u32 delay_us, nit: Is it really worth allowing the caller to pass a custom delay here? Generally, I think you don't need an explicit delay at all to busy wait on a register. All udelay() does is busy-wait on a timer register instead, doesn't really make a difference.
https://review.coreboot.org/c/coreboot/+/34773/12/src/soc/mediatek/common/ds... PS12, Line 314: write32(&dsi0->dsi_cmdq[i], 0); Is this necessary? I have a feeling that this is only here because the weird read-modify-write for each byte thing below may have you keep stale data from a previous command if your length isn't divisible by 4. If you fix that, maybe this won't be necessary?
https://review.coreboot.org/c/coreboot/+/34773/12/src/soc/mediatek/common/ds... PS12, Line 318: clrsetbits_le32(cmdq0 + ((cmdq_off + i) & (0xfffffffc)), This is pretty nasty. You're reading and writing each peripheral register four times. Can't we rather do something like (replacing some of the above as well... the whole cmdq_size/cmdq_off calculation stuff seems more trouble than it's worth to me, honestly):
if (len <= 2) { uint32_t val = (type << 8) | config; for (j = 0; j < MIN(2, len); j++) val |= tx_buf[j] << (j + 2); write32(&dsi0->dsi_cmdq[0], val); write32(&dsi0->dsi_cmdq_size, 1); } else { write32(&dsi0->dsi_cmdq[0], (len << 16) | (type << 8) | config); for (i = 0; i < len; i += 4) { uint32_t val = 0; for (j = 0; j < MIN(len - i, 4); j++) val |= tx_buf[i + j] << j; write32(&dsi0->dsi_cmdq[i/4 + 1], val); } write32(&dsi0->dsi_cmdq_size, 1 + DIV_ROUND_UP(len, 4)); }
https://review.coreboot.org/c/coreboot/+/34773/12/src/soc/mediatek/common/ds... PS12, Line 324: clrsetbits_le32(&dsi0->dsi_cmdq_size, CMDQ_SIZE, cmdq_size); nit: Does this need to be a clrsetbits? The rest of this driver isn't shy about setting unused bits to 0 either.
https://review.coreboot.org/c/coreboot/+/34773/12/src/soc/mediatek/common/ds... PS12, Line 333: assert(init); nit: maybe just do the
if (!init) return;
in here so you don't need to check in the caller?
https://review.coreboot.org/c/coreboot/+/34773/12/src/soc/mediatek/common/ds... PS12, Line 355: type = MIPI_DSI_DCS_LONG_WRITE; A lot of this reminds me of Rockchip code, we should probably spend some time looking for things to factor out and generalize from our various MIPI implementations some day... but not today.
https://review.coreboot.org/c/coreboot/+/34773/12/src/soc/mediatek/common/in... File src/soc/mediatek/common/include/soc/dsi_common.h:
https://review.coreboot.org/c/coreboot/+/34773/12/src/soc/mediatek/common/in... PS12, Line 334: u16 len; See my earlier comments about structure packing