Hi,
On Sat, Sep 18, 2021 at 10:21 AM Philip Chen philipchen@chromium.org wrote:
+static ssize_t ps8640_aux_transfer(struct drm_dp_aux *aux,
struct drm_dp_aux_msg *msg)+{
struct ps8640 *ps_bridge = aux_to_ps8640(aux);struct regmap *map = ps_bridge->regmap[PAGE0_DP_CNTL];struct device *dev = &ps_bridge->page[PAGE0_DP_CNTL]->dev;unsigned int len = msg->size;
nit: usually no blank lines in the variable definition section.
base = PAGE0_SWAUX_ADDR_7_0;addr_len[PAGE0_SWAUX_ADDR_7_0 - base] = msg->address;addr_len[PAGE0_SWAUX_ADDR_15_8 - base] = msg->address >> 8;addr_len[PAGE0_SWAUX_ADDR_23_16 - base] = (msg->address >> 16) &SWAUX_ADDR_19_16_MASK;addr_len[PAGE0_SWAUX_ADDR_23_16 - base] |= (msg->request << 4) &SWAUX_CMD_MASK;
optional nit: Probably you could get rid of the mask for the request. After all, you're storing it to a thing that's a byte (so bits above bit 7 will implicitly be masked) and you're left shifting by 4 (so bits 0-3 will implicitly be masked) so this just makes it uglier. ;-)
optional nit: In theory you could also get rid of the SWAUX_ADDR_19_16_MASK and if you really wanted to you could error check that the address wasn't bigger than 20-bits since giving an error for an invalid address would actually be better than silently masking it anyway...
if (len && (request == DP_AUX_NATIVE_READ ||request == DP_AUX_I2C_READ)) {/* Read from the internal FIFO buffer */for (i = 0; i < len; i++) {ret = regmap_read(map, PAGE0_SWAUX_RDATA,(unsigned int *)(buf + i));
The cast to "unsigned int *" looks wrong to me. You can't just cast like this for a number of reasons. Go back to reading into a local variable and copy the byte into your buffer.
Other than the regmap_read() this looks fine to me. If you send a v6 with that fixed I'll plan to wait a day or two and then apply it with Sam's tags.
-Doug