Hi
On Thu, Sep 16, 2021 at 1:31 PM Doug Anderson dianders@chromium.org wrote:
Hi,
On Thu, Sep 16, 2021 at 1:15 PM Stephen Boyd swboyd@chromium.org wrote:
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;Sure, this looks nice to me. :-)
Thanks. Implemented the fix in v4. PTAL.
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?
It seems highly likely not to matter in practice, but:
- The docs from parade clearly state that when a NAK is returned that
the number of bytes transferred before the NAK will be provided to us.
- The i2c interface specifies NAK not as a return code but as a bit in
the "reply". That presumably is to let us return the number of bytes transferred before the NAK to the next level up.
- If we're returning the number of bytes and it's a read then we'd
better return the data too!
It looks like we handled this OK in the TI bridge driver. From reading the TI docs we'll get both AUX_IRQ_STATUS_AUX_SHORT and AUX_IRQ_STATUS_NAT_I2C_FAIL in the case of a partial transfer and so we'll do the right thing.
Thanks for catching the bug. In v4, I made SWAUX_STATUS_I2C_NACK fall through to SWAUX_STATUS_ACKM and store the number of read/written bytes in len w/o returning immediately. PTAL.
-Doug