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
