Quoting Doug Anderson (2021-09-15 14:27:40)
Hi,
On Tue, Sep 14, 2021 at 5:57 PM Stephen Boyd swboyd@chromium.org wrote:
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()?
I've never gotten clear guidance here. For instance, in some other review I suggested using the DRM wrapper and got told "no" [1]. ;-) The driver landed without the DRM_ERROR versions. I don't really care lots so it's fine with me to use use DRM_DEV_ERROR, I just wish I understood the rules...
[1] https://lore.kernel.org/all/49db7ef3-fa53-a274-7c69-c2d840b13058@denx.de/
I think the rule is that the DRM specific printk stuff should be used so that they can be stuck into the drm logs. On chromeOS we also have a record of the drm logs that we can use to debug things, split away from the general kernel printk logs. So using DRM prints when there's a DRM device around is a good thing to do.
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));You're arguing that your version of the code is more efficient? Easier to understand? Something else? To me, Philip's initial version is crystal clear and easy to map to the bridge datasheet but I need to think more to confirm that your version is right. Thinking is hard and I like to avoid it when possible.
In any case, it's definitely bikeshedding and I'll yield if everyone likes the other version better. ;-)
Yeah it's bikeshedding. I don't really care about this either but I find it easier to read when the assignment isn't wrapped across multiple lines. If the buffer approach is preferable then maybe use the address macros to clarify which register is being set?
unsigned int 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; addr_len[PAGE0_SWAUX_ADDR_23_16 - base] |= msg->request << 4; ...
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;elsemsg->reply |= DP_AUX_I2C_REPLY_NACK;len = data & SWAUX_M_MASK;return len;Why no 'return data & SWAUX_M_MASK;' and skip the assignment?
Actually, I think it's the "return" that's a bug, isn't it? If we're doing a "read" and we're returning a positive number of bytes then we need to actually _read_ them. Reading happens below, doesn't it?
Oh I missed that. We're still supposed to return data to upper layers on a NACKed read?