Re: [alsa-devel] [PATCH 06/14] soundwire: Add IO transfer

[Takashi Iwai <tiwai@suse.de>]

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