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

[Vinod Koul]

On Thu, Oct 19, 2017 at 11:13:48AM +0200, Takashi Iwai wrote:
> 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.

sounds good

> > +		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.

yes will do, sometimes we should not reuse :)

> > +/**
> > + * 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,  but when page
> is set, it returns 0?

Sorry no. do_transfer can succced (0) or in some case where Slaves didn't
care for return error (ENODATA), or other errors.
No ENODATA can be error depending on message sent so we dont treat this as
failure and let caller decide.

In case of errors (others) we don't need to reset page and we bail out

> 
> > +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.

yup

-- 
~Vinod