From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752396AbdJSJNw (ORCPT ); Thu, 19 Oct 2017 05:13:52 -0400 Received: from mx2.suse.de ([195.135.220.15]:59055 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751161AbdJSJNu (ORCPT ); Thu, 19 Oct 2017 05:13:50 -0400 Date: Thu, 19 Oct 2017 11:13:48 +0200 Message-ID: From: Takashi Iwai To: Vinod Koul Cc: Greg Kroah-Hartman , ALSA , Charles Keepax , Sudheer Papothi , plai@codeaurora.org, LKML , Pierre , patches.audio@intel.com, Mark , srinivas.kandagatla@linaro.org, Shreyas NC , Sanyog Kale , Sagar Dharia , alan@linux.intel.com Subject: Re: [alsa-devel] [PATCH 06/14] soundwire: Add IO transfer In-Reply-To: <1508382211-3154-7-git-send-email-vinod.koul@intel.com> References: <1508382211-3154-1-git-send-email-vinod.koul@intel.com> <1508382211-3154-7-git-send-email-vinod.koul@intel.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.3 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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