Re: [PATCH v3 3/3] drm/bridge: parade-ps8640: Add support for AUX channel

Quoting Philip Chen (2021-09-14 16:28:45)

diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c index 8d3e7a147170..dc349d729f5a 100644 --- a/drivers/gpu/drm/bridge/parade-ps8640.c +++ b/drivers/gpu/drm/bridge/parade-ps8640.c @@ -117,6 +144,129 @@ static inline struct ps8640 *bridge_to_ps8640(struct drm_bridge *e)

[...]

•   case DP_AUX_I2C_WRITE:
  

•   case DP_AUX_I2C_READ:
  

•           break;
  

•   default:
  

•           return -EINVAL;
  

•   }
  

•   ret = regmap_write(map, PAGE0_AUXCH_CFG3, AUXCH_CFG3_RESET);
  

•   if (ret) {
  

•           dev_err(dev, "failed to write PAGE0_AUXCH_CFG3: %d\n", ret);
  

Can we use DRM_DEV_ERROR()?

•           return ret;
  

•   }
  

•   /* Assume it's good */
  

•   msg->reply = 0;
  

•   addr_len[0] = msg->address & 0xff;
  

•   addr_len[1] = (msg->address >> 8) & 0xff;
  

•   addr_len[2] = ((msg->request << 4) & SWAUX_CMD_MASK) |
  

•           ((msg->address >> 16) & SWAUX_ADDR_19_16_MASK);
  

It really feels like this out to be possible with some sort of cpu_to_le32() API. We're shoving msg->address into 3 bytes and then adding in the request and some length. So we could do something like:

u32 addr_len;

addr_len = FIELD_PREP(SWAUX_ADDR_MASK, msg->address); addr_len |= FIELD_PREP(SWAUX_CMD_MASK, msg->request); if (len) addr_len |= FIELD_PREP(LEN_MASK, len - 1); else addr_len |= FIELD_PREP(LEN_MASK, SWAUX_NO_PAYLOAD ); cpu_to_le32s(&addr_len);

regmap_bulk_write(map, PAGE0_SWAUX_ADDR_7_0, &addr_len, sizeof(addr_len));

•   addr_len[3] = (len == 0) ? SWAUX_NO_PAYLOAD :
  

•                   ((len - 1) & SWAUX_LENGTH_MASK);
  

•   regmap_bulk_write(map, PAGE0_SWAUX_ADDR_7_0, addr_len,
  

•                     ARRAY_SIZE(addr_len));
  

•   if (len && (request == DP_AUX_NATIVE_WRITE ||
  

•               request == DP_AUX_I2C_WRITE)) {
  

•           /* Write to the internal FIFO buffer */
  

•           for (i = 0; i < len; i++) {
  

•                   ret = regmap_write(map, PAGE0_SWAUX_WDATA, buf[i]);
  

•                   if (ret) {
  

•                           dev_err(dev, "failed to write WDATA: %d\n",
  

DRM_DEV_ERROR?

•                                   ret);
  

•                           return ret;
  

•                   }
  

•           }
  

•   }
  

•   regmap_write(map, PAGE0_SWAUX_CTRL, SWAUX_SEND);
  

•   /* Zero delay loop because i2c transactions are slow already */
  

•   regmap_read_poll_timeout(map, PAGE0_SWAUX_CTRL, data,
  

•                            !(data & SWAUX_SEND), 0, 50 * 1000);
  

•   regmap_read(map, PAGE0_SWAUX_STATUS, &data);
  

•   if (ret) {
  

•           dev_err(dev, "failed to read PAGE0_SWAUX_STATUS: %d\n", ret);
  

DRM_DEV_ERROR?

•           return ret;
  

•   }
  

•   switch (data & SWAUX_STATUS_MASK) {
  

•   /* Ignore the DEFER cases as they are already handled in hardware */
  

•   case SWAUX_STATUS_NACK:
  

•   case SWAUX_STATUS_I2C_NACK:
  

•           /*
  

•            * The programming guide is not clear about whether a I2C NACK
  

•            * would trigger SWAUX_STATUS_NACK or SWAUX_STATUS_I2C_NACK. So
  

•            * we handle both cases together.
  

•            */
  

•           if (is_native_aux)
  

•                   msg->reply |= DP_AUX_NATIVE_REPLY_NACK;
  

•           else
  

•                   msg->reply |= DP_AUX_I2C_REPLY_NACK;
  

•           len = data & SWAUX_M_MASK;
  

•           return len;
  

Why no 'return data & SWAUX_M_MASK;' and skip the assignment?

•   case SWAUX_STATUS_ACKM:
  

Move this up and add fallthrough?

•           len = data & SWAUX_M_MASK;
  

•           return len;
  

•   case SWAUX_STATUS_INVALID:
  

•           return -EOPNOTSUPP;
  

•   case SWAUX_STATUS_TIMEOUT:
  

•           return -ETIMEDOUT;
  

•   }
  

•   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, &data);
  

•                   buf[i] = data;
  

Can drop a line

ret = regmap_read(map, PAGE0_SWAUX_RDATA, buf + i);

•                   if (ret) {
  

•                           dev_err(dev, "failed to read RDATA: %d\n",
  

•                                   ret);
  

•                           return ret;
  

•                   }
  

•           }
  

•   }
  

•   return len;
  

+}