On Thu, 19 Oct 2017 05:03:22 +0200, Vinod Koul wrote:
+static inline int find_error_code(unsigned int sdw_ret) +{
- switch (sdw_ret) {
- case SDW_CMD_OK:
return 0;- case SDW_CMD_IGNORED:
return -ENODATA;- case SDW_CMD_TIMEOUT:
return -ETIMEDOUT;- }
- return -EIO;
+}
+static inline int do_transfer(struct sdw_bus *bus,
struct sdw_msg *msg, bool page)+{
- int retry = bus->prop.err_threshold;
- int ret, i;
- for (ret = 0, i = 0; i <= retry; i++) {
Initializing ret here is a bit messy. Better to do it outside.
ret = bus->ops->xfer_msg(bus, msg, page);ret = find_error_code(ret);/* if cmd is ok or ignored return */if (ret == 0 || ret == -ENODATA)return ret;
Hmm, it's not good to use the same variable for representing two different things. Either drop the substitution to ret for bus->ops->xfer_msg() call, or use another variable to make clear which one is for SDW_CMD_* and which one is for -EXXX. The former should be basically an enum.
+/**
- sdw_transfer: Synchronous transfer message to a SDW Slave device
- @bus: SDW bus
- @slave: SDW Slave
- @msg: SDW message to be xfered
- */
+int sdw_transfer(struct sdw_bus *bus, struct sdw_slave *slave,
struct sdw_msg *msg)+{
- bool page;
- int ret;
- mutex_lock(&bus->msg_lock);
- page = sdw_get_page(slave, msg);
- ret = do_transfer(bus, msg, page);
- if (ret != 0 && ret != -ENODATA) {
dev_err(bus->dev, "trf on Slave %d failed:%d\n",msg->dev_num, ret);goto error;- }
- if (page)
ret = sdw_reset_page(bus, msg->dev_num);+error:
- mutex_unlock(&bus->msg_lock);
- return ret;
So the logic here is that when -ENODATA is returned and page is false, this function should return -ENODATA to the caller, too, but when page is set, it returns 0?
+static inline int sdw_fill_msg(struct sdw_msg *msg, u16 addr,
size_t count, u16 dev_num, u8 flags, u8 *buf)+{
- msg->addr = (addr >> SDW_REG_SHIFT(SDW_REGADDR));
- msg->len = count;
- msg->dev_num = dev_num;
- msg->addr_page1 = (addr >> SDW_REG_SHIFT(SDW_SCP_ADDRPAGE1_MASK));
- msg->addr_page2 = (addr >> SDW_REG_SHIFT(SDW_SCP_ADDRPAGE2_MASK));
- msg->flags = flags;
- msg->buf = buf;
- msg->ssp_sync = false;
- return 0;
This function can be void.
thanks,
Takashi