From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757109AbcHWIWc (ORCPT ); Tue, 23 Aug 2016 04:22:32 -0400 Received: from mail-wm0-f42.google.com ([74.125.82.42]:36810 "EHLO mail-wm0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752999AbcHWIWa (ORCPT ); Tue, 23 Aug 2016 04:22:30 -0400 Subject: Re: [PATCH 06/13] scpi: add priv_scpi_ops and fill legacy structure 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-7-git-send-email-narmstrong@baylibre.com> <5c89a4e9-0100-0c67-1d6c-07651649172a@arm.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: Date: Tue, 23 Aug 2016 10:22:25 +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: <5c89a4e9-0100-0c67-1d6c-07651649172a@arm.com> 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:39 PM, Sudeep Holla wrote: > > > On 18/08/16 11:10, Neil Armstrong wrote: >> In order to use the legacy functions variants, add a new priv_scpi_ops >> structure that will contain the internal alterne functions and then use these >> alternate call in the probe function. >> >> Signed-off-by: Neil Armstrong >> --- >> drivers/firmware/arm_scpi.c | 68 ++++++++++++++++++++++++++++++++++++++++++--- >> 1 file changed, 64 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c >> index b0d911b..3fe39fe 100644 >> --- a/drivers/firmware/arm_scpi.c >> +++ b/drivers/firmware/arm_scpi.c >> @@ -213,6 +213,7 @@ struct scpi_drvinfo { >> struct scpi_ops *scpi_ops; >> struct scpi_chan *channels; >> struct scpi_dvfs_info *dvfs[MAX_DVFS_DOMAINS]; >> + const struct priv_scpi_ops *ops; >> }; >> >> /* >> @@ -299,6 +300,17 @@ struct dev_pstate_set { >> u8 pstate; >> } __packed; >> >> +struct priv_scpi_ops { >> + /* Internal Specific Ops */ >> + void (*handle_remote_msg)(struct mbox_client *c, void *msg); >> + void (*tx_prepare)(struct mbox_client *c, void *msg); >> + /* Message Specific Ops */ >> + int (*init_versions)(struct scpi_drvinfo *info); >> + int (*dvfs_get_info)(u8 domain, struct dvfs_info *buf); >> + /* System wide Ops */ >> + struct scpi_ops *scpi_ops; >> +}; >> + > > > I fail to understand the need for this. Can you please explain the issue > you would face without this ? > >> static struct scpi_drvinfo *scpi_info; >> >> static int scpi_linux_errmap[SCPI_ERR_MAX] = { >> @@ -695,9 +707,12 @@ static struct scpi_dvfs_info *scpi_dvfs_get_info(u8 domain) >> if (scpi_info->dvfs[domain]) /* data already populated */ >> return scpi_info->dvfs[domain]; >> >> - ret = scpi_send_message(SCPI_CMD_GET_DVFS_INFO, &domain, sizeof(domain), >> + if (scpi_info->ops && scpi_info->ops->dvfs_get_info) >> + ret = scpi_info->ops->dvfs_get_info(domain, &buf); >> + else >> + ret = scpi_send_message(SCPI_CMD_GET_DVFS_INFO, >> + &domain, sizeof(domain), >> &buf, sizeof(buf)); >> - >> if (ret) >> return ERR_PTR(ret); >> >> @@ -855,6 +870,22 @@ static struct scpi_ops scpi_ops = { >> .vendor_send_message = scpi_ext_send_message, >> }; >> >> +static struct scpi_ops legacy_scpi_ops = { >> + .get_version = scpi_get_version, >> + .clk_get_range = NULL, >> + .clk_get_val = legacy_scpi_clk_get_val, >> + .clk_set_val = legacy_scpi_clk_set_val, >> + .dvfs_get_idx = legacy_scpi_dvfs_get_idx, >> + .dvfs_set_idx = legacy_scpi_dvfs_set_idx, >> + .dvfs_get_info = scpi_dvfs_get_info, >> + .sensor_get_capability = legacy_scpi_sensor_get_capability, >> + .sensor_get_info = legacy_scpi_sensor_get_info, >> + .sensor_get_value = legacy_scpi_sensor_get_value, >> + .device_get_power_state = NULL, >> + .device_set_power_state = NULL, >> + .vendor_send_message = legacy_scpi_send_message, > > I think we need not have this at all if you follow the suggestion I had > in the previous patch. Try and let's see how it would look. If you confirm you want the if/else as said in patch 4. But clk_get_range, device_get/set_power_state are not available in legacy, I think we should still have this alternate structure. >> +}; >> + >> struct scpi_ops *get_scpi_ops(void) >> { >> return scpi_info ? scpi_info->scpi_ops : NULL; >> @@ -972,8 +1003,17 @@ static int scpi_alloc_xfer_list(struct device *dev, struct scpi_chan *ch) >> return 0; >> } >> >> +static const struct priv_scpi_ops scpi_legacy_ops = { >> + .handle_remote_msg = legacy_scpi_handle_remote_msg, >> + .tx_prepare = legacy_scpi_tx_prepare, >> + .init_versions = legacy_scpi_init_versions, >> + .dvfs_get_info = legacy_scpi_dvfs_get_info, >> + .scpi_ops = &legacy_scpi_ops, >> +}; >> + > > Ditto, can go away. > Yes, in the if/else is_legacy variant this should go away. Thanks, Neil