From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.6 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_2 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7CAE6C3524B for ; Sun, 2 Feb 2020 09:43:49 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 3D8AD206E6 for ; Sun, 2 Feb 2020 09:43:49 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1580636629; bh=EYnP/SHj2gpLPkJ+FZsF45R0Ky7pbHvS/PbRHT9UwUk=; h=Date:From:To:Cc:Subject:In-Reply-To:References:List-ID:From; b=ite1L8yBBX/4CaEKjaG8TlVMJRiB83pppRCEMQmUF+EziC0vZDM9r32Ti0wd6Ja2X 4hP4N+e4uVXvuASxqvBRzmwVYnU8oNN3tBa4iSskSQ6/n5XkZWGFtL1WniMXjOrbxi e+YU0xX14DyWpPh3ulPr9O0rIAEfq2igW0XCuSZQ= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726747AbgBBJns (ORCPT ); Sun, 2 Feb 2020 04:43:48 -0500 Received: from mail.kernel.org ([198.145.29.99]:48404 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725942AbgBBJns (ORCPT ); Sun, 2 Feb 2020 04:43:48 -0500 Received: from archlinux (cpc149474-cmbg20-2-0-cust94.5-4.cable.virginm.net [82.4.196.95]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 7386020679; Sun, 2 Feb 2020 09:43:45 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1580636627; bh=EYnP/SHj2gpLPkJ+FZsF45R0Ky7pbHvS/PbRHT9UwUk=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=cakVawf/poqHSYOL9ydo2KqPubVXeEz1/P/uE2X3ymKdETd4WRIEx0VbaZvQ1Wzpf PHzDfqXG0EnVR5jCysZpEY+2c262svf1REPmo6mWshdc27EwvjcjtjCCaNpciMDC1r StC3TTPnZYTRbl4MGpGuoVjfcFEd9szwKqejv688= Date: Sun, 2 Feb 2020 09:43:42 +0000 From: Jonathan Cameron To: Prashant Malani Cc: linux-kernel@vger.kernel.org, Hartmut Knaack , Lars-Peter Clausen , Peter Meerwald-Stadler , Benson Leung , Enric Balletbo i Serra , Guenter Roeck , Gwendal Grignou , Fabien Lahoudere , Nick Vaccaro , linux-iio@vger.kernel.org (open list:IIO SUBSYSTEM AND DRIVERS) Subject: Re: [PATCH 10/17] iio: cros_ec: Use cros_ec_send_cmd_msg() Message-ID: <20200202094342.108849ab@archlinux> In-Reply-To: <20200130203106.201894-11-pmalani@chromium.org> References: <20200130203106.201894-1-pmalani@chromium.org> <20200130203106.201894-11-pmalani@chromium.org> X-Mailer: Claws Mail 3.17.4 (GTK+ 2.24.32; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 30 Jan 2020 12:30:54 -0800 Prashant Malani wrote: > Replace cros_ec_cmd_xfer_status() with cros_ec_send_cmd_msg() > which does the message buffer setup and cleanup. > > Signed-off-by: Prashant Malani In a series like this, make sure that patch 1 with the actual code being pulled out is sent to everyone. Looking at what we have here, this doesn't seem to fit well at all for one case, and I'm can't say the other case shows much advantage either. Jonathan > --- > .../cros_ec_sensors/cros_ec_sensors_core.c | 43 +++++++++---------- > 1 file changed, 21 insertions(+), 22 deletions(-) > > diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c > index 81a7f692de2f37..f92032e97a84d7 100644 > --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c > +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c > @@ -31,24 +31,16 @@ static int cros_ec_get_host_cmd_version_mask(struct cros_ec_device *ec_dev, > u16 cmd_offset, u16 cmd, u32 *mask) > { > int ret; > - struct { > - struct cros_ec_command msg; > - union { > - struct ec_params_get_cmd_versions params; > - struct ec_response_get_cmd_versions resp; > - }; > - } __packed buf = { > - .msg = { > - .command = EC_CMD_GET_CMD_VERSIONS + cmd_offset, > - .insize = sizeof(struct ec_response_get_cmd_versions), > - .outsize = sizeof(struct ec_params_get_cmd_versions) > - }, > - .params = {.cmd = cmd} > - }; > - > - ret = cros_ec_cmd_xfer_status(ec_dev, &buf.msg); > + struct ec_params_get_cmd_versions params = {0}; > + struct ec_response_get_cmd_versions resp = {0}; > + > + params.cmd = cmd; Use c99 element setting to set this directly rather than zeroing explicitly then setting the element. Something like. struct ec_params_get_cmde_versions params = { .cmd = cmd; }; > + ret = cros_ec_send_cmd_msg(ec_dev, 0, > + EC_CMD_GET_CMD_VERSIONS + cmd_offset, > + ¶ms, sizeof(params), > + &resp, sizeof(resp)); > if (ret >= 0) > - *mask = buf.resp.version_mask; > + *mask = resp.version_mask; > return ret; > } > > @@ -164,15 +156,22 @@ int cros_ec_motion_send_host_cmd(struct cros_ec_sensors_core_state *state, > u16 opt_length) > { > int ret; > + struct cros_ec_command *msg = state->msg; With this change the code becomes less readable and needs a comment to explain why it is doing something odd. Either you need to figure out how to make this fit properly such that the comment is not needed, or leave the code alone. > > if (opt_length) > - state->msg->insize = min(opt_length, state->ec->max_response); > + msg->insize = min(opt_length, state->ec->max_response); > else > - state->msg->insize = state->ec->max_response; > + msg->insize = state->ec->max_response; > > - memcpy(state->msg->data, &state->param, sizeof(state->param)); > - > - ret = cros_ec_cmd_xfer_status(state->ec, state->msg); > + /* > + * In order to not disrupt the usage of struct cros_ec_command *msg, > + * which is defined higher up in the call stack, we pass in its > + * members to cros_ec_send_cmd_msg, instead of removing it at all > + * calling locations. > + */ > + ret = cros_ec_send_cmd_msg(state->ec, msg->version, msg->command, > + &state->param, sizeof(state->param), > + msg->data, msg->insize); > if (ret < 0) > return ret; >