From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757332AbcHWITv (ORCPT ); Tue, 23 Aug 2016 04:19:51 -0400 Received: from mail-wm0-f50.google.com ([74.125.82.50]:35141 "EHLO mail-wm0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756613AbcHWITs (ORCPT ); Tue, 23 Aug 2016 04:19:48 -0400 Subject: Re: [PATCH 04/13] scpi: Add legacy SCP functions calling legacy_scpi_send_message To: Sudeep Holla , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org References: <1471515066-3626-1-git-send-email-narmstrong@baylibre.com> <1471515066-3626-5-git-send-email-narmstrong@baylibre.com> Cc: linux-amlogic@lists.infradead.org, khilman@baylibre.com, heiko@sntech.de, wxt@rock-chips.com, frank.wang@rock-chips.com From: Neil Armstrong Organization: Baylibre Message-ID: <53093fb5-22e2-3c17-3345-50c7e9df7089@baylibre.com> Date: Tue, 23 Aug 2016 10:19:43 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 08/19/2016 06:22 PM, Sudeep Holla wrote: > > > On 18/08/16 11:10, Neil Armstrong wrote: >> In order to support legacy SCP functions from kernel-wide driver, add legacy >> functions using the legacy command enums and calling legacy_scpi_send_message. >> >> Signed-off-by: Neil Armstrong >> --- >> drivers/firmware/arm_scpi.c | 118 ++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 118 insertions(+) >> >> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c >> index 50b1297..bb9965f 100644 >> --- a/drivers/firmware/arm_scpi.c >> +++ b/drivers/firmware/arm_scpi.c >> @@ -578,6 +578,8 @@ scpi_clk_get_range(u16 clk_id, unsigned long *min, unsigned long *max) >> return ret; >> } >> >> +/* scpi_clk_get_range not available for legacy */ >> + >> static unsigned long scpi_clk_get_val(u16 clk_id) >> { >> int ret; >> @@ -589,6 +591,18 @@ static unsigned long scpi_clk_get_val(u16 clk_id) >> return ret ? ret : le32_to_cpu(clk.rate); >> } >> >> +static unsigned long legacy_scpi_clk_get_val(u16 clk_id) >> +{ >> + int ret; >> + struct clk_get_value clk; >> + __le16 le_clk_id = cpu_to_le16(clk_id); >> + >> + ret = legacy_scpi_send_message(LEGACY_SCPI_CMD_GET_CLOCK_VALUE, >> + &le_clk_id, sizeof(le_clk_id), >> + &clk, sizeof(clk)); >> + return ret ? ret : le32_to_cpu(clk.rate); >> +} >> + >> static int scpi_clk_set_val(u16 clk_id, unsigned long rate) >> { >> int stat; >> @@ -601,6 +615,19 @@ static int scpi_clk_set_val(u16 clk_id, unsigned long rate) >> &stat, sizeof(stat)); >> } >> >> +static int legacy_scpi_clk_set_val(u16 clk_id, unsigned long rate) >> +{ >> + int stat; >> + struct legacy_clk_set_value clk = { >> + .id = cpu_to_le16(clk_id), >> + .rate = cpu_to_le32(rate) >> + }; >> + >> + return legacy_scpi_send_message(LEGACY_SCPI_CMD_SET_CLOCK_VALUE, >> + &clk, sizeof(clk), >> + &stat, sizeof(stat)); > > Except this one which has a different structure format, why do we need > to define legacy versions of other functions ? Can't we play with > function pointer or have a boolean in drvinfo structure and use then in > the existing functions as I had shown in one of the earlier emails. > The main problem is that the command indexes deviates starting at SCPI_CMD_SET_CSS_PWR_STATE, I'll be pleased to know how to implement it. Should I add a test : if (scpi_drvinfo->is_legacy) legacy_scpi_send_message(...) else scpi_send_message(...) In each function ? My strategy was to leave the "final" function untouched ans provide alternatives to legacy. I can add this "is_legacy" if/else instead of ops structures. Please tell me how you'll implement this, so I'll adapt the merge. Neil