View Change

7 comments:

• File src/soc/mediatek/common/dsi.c:

  • Patch Set #12, 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.

  • Patch Set #12, 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?

  • Patch Set #12, 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));
     }

  • Patch Set #12, 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.

  • Patch Set #12, Line 333: assert(init);

    nit: maybe just do the

     if (!init)
       return;

    in here so you don't need to check in the caller?

  • Patch Set #12, 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.

• File src/soc/mediatek/common/include/soc/dsi_common.h:

  • Patch Set #12, Line 334: u16 len;

    See my earlier comments about structure packing

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