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;
> +}
