Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S941546AbcJSOUH (ORCPT ); Wed, 19 Oct 2016 10:20:07 -0400 Received: from foss.arm.com ([217.140.101.70]:53406 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S936195AbcJSOUD (ORCPT ); Wed, 19 Oct 2016 10:20:03 -0400 Subject: Re: [PATCH v4 2/8] scpi: Add alternative legacy structures, functions and macros To: Neil Armstrong , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org References: <1475652814-30619-1-git-send-email-narmstrong@baylibre.com> <1475652814-30619-3-git-send-email-narmstrong@baylibre.com> <7bf3b68c-19aa-85c3-8b20-66b7380eaf3f@arm.com> <24e6ef19-b88d-a5b8-f514-d39a7b5725e8@baylibre.com> Cc: Sudeep Holla , linux-amlogic@lists.infradead.org, khilman@baylibre.com, heiko@sntech.de, wxt@rock-chips.com, frank.wang@rock-chips.com From: Sudeep Holla Organization: ARM Message-ID: <8f47801c-558d-a270-b66e-a7da1b4d7a60@arm.com> Date: Wed, 19 Oct 2016 11:42:00 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1988 Lines: 70 On 19/10/16 11:37, Sudeep Holla wrote: > > > On 19/10/16 11:28, Neil Armstrong wrote: >> On 10/17/2016 01:16 PM, Sudeep Holla wrote: > > [...] > >>> >>> and the above 2 can be moved out of the conditions, no ? >>> >>> if (scpi_info->is_legacy) { >>> struct legacy_scpi_shared_mem *mem = ch->rx_payload; >>> len = match->rx_len; >>> } else { >>> struct scpi_shared_mem *mem = ch->rx_payload; >>> len = min(match->rx_len, CMD_SIZE(cmd)); >>> } >>> match->status = le32_to_cpu(mem->status); >>> memcpy_fromio(match->rx_buf, mem->payload, len); >>> >>> should work. >> >> Well, we will have "error: ?mem? undeclared (first use in this >> function)" since mem is not declared outside the if/else. >> >> I don't see good solutions even with an union. > > Right, I missed to see that. You can leave it as you had before then. > > > [...] >>>>>> if (t->rx_buf) { >>>>>> if (!(++ch->token)) >>>>>> ++ch->token; >>>>>> ADD_SCPI_TOKEN(t->cmd, ch->token); >>>>>> + if (scpi_info->is_legacy) >>>>>> + t->slot = t->cmd; >>>>> >>>>> I thought passing token was not an issue from your previous response, >>>>> but you are overriding it here, why ? >>>> >>>> Indeed, I can leave it, but it's useless since it won't serve to >>>> distinguish multiple similar commands. >>>> >>> >>> OK, I don't see any point in such micro optimization, so please >>> retain it. >>> >> >> I misread my code, I leaved the token passing, but I copy back the >> cmd to the slot which is used by the MHU. >> If I remove the "t->slot = t->cmd;", the token won't be passed to the >> FW. >> > > Right, sorry I think I misled you :) > On the other hand, if we are unable to use that in the driver as you just have FIFO in the list for this legacy mode, I think we can drop it completely. I know I had argued otherwise before, but that was before I realized that we need to keep FIFO in the list ;). Let me know if you are OK to drop it. -- Regards, Sudeep