Re: [PATCH v11 08/11] firmware: xilinx: Add debugfs for query data API

From: Olof Johansson <olof@lixom.net>
To: Jolly Shah <jolly.shah@xilinx.com>
Cc: "ard.biesheuvel@linaro.org" <ard.biesheuvel@linaro.org>,
	Ingo Molnar <mingo@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	matt@codeblueprint.co.uk, Sudeep Holla <sudeep.holla@arm.com>,
	hkallweit1@gmail.com, Kees Cook <keescook@chromium.org>,
	Dmitry Torokhov <dmitry.torokhov@gmail.com>,
	Michael Turquette <mturquette@baylibre.com>,
	Stephen Boyd <sboyd@codeaurora.org>,
	Michal Simek <michal.simek@xilinx.com>,
	Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	linux-clk <linux-clk@vger.kernel.org>,
	rajanv@xilinx.com,
	Linux ARM Mailing List <linux-arm-kernel@lists.infradead.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	DTML <devicetree@vger.kernel.org>, Jolly Shah <jollys@xilinx.com>
Subject: Re: [PATCH v11 08/11] firmware: xilinx: Add debugfs for query data API
Date: Sat, 8 Sep 2018 18:15:11 -0700	[thread overview]
Message-ID: <CAOesGMjMcznFAQj0L-=J5+gJ5ENsdejqP2SdYomu=C9bJqG+YQ@mail.gmail.com> (raw)
In-Reply-To: <1533318808-10781-9-git-send-email-jollys@xilinx.com>

Hi,

I noticed the below when I was reviewing the code for merge into
arm-soc. Would you mind following up with an incremental patch? I
don't think we need to ask Michal to respin for this:

On Fri, Aug 3, 2018 at 10:53 AM, Jolly Shah <jolly.shah@xilinx.com> wrote:
> From: Rajan Vaja <rajanv@xilinx.com>
>
> Add debugfs file to query platform specific data from firmware
> using debugfs interface.
>
> Signed-off-by: Rajan Vaja <rajanv@xilinx.com>
> Signed-off-by: Jolly Shah <jollys@xilinx.com>
> ---
>  drivers/firmware/xilinx/zynqmp-debug.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>
> diff --git a/drivers/firmware/xilinx/zynqmp-debug.c b/drivers/firmware/xilinx/zynqmp-debug.c
> index fc11db9..4532bd0 100644
> --- a/drivers/firmware/xilinx/zynqmp-debug.c
> +++ b/drivers/firmware/xilinx/zynqmp-debug.c
> @@ -33,6 +33,7 @@ static char debugfs_buf[PAGE_SIZE];
>  static struct pm_api_info pm_api_list[] = {
>         PM_API(PM_GET_API_VERSION),
>         PM_API(PM_IOCTL),
> +       PM_API(PM_QUERY_DATA),
>  };
>
>  /**
> @@ -105,6 +106,32 @@ static int process_api_request(u32 pm_id, u64 *pm_api_arg, u32 *pm_api_ret)
>                         sprintf(debugfs_buf, "IOCTL return value: %u\n",
>                                 pm_api_ret[1]);
>                 break;
> +       case PM_QUERY_DATA:
> +       {
> +               struct zynqmp_pm_query_data qdata = {0};
> +
> +               qdata.qid = pm_api_arg[0];
> +               qdata.arg1 = pm_api_arg[1];
> +               qdata.arg2 = pm_api_arg[2];
> +               qdata.arg3 = pm_api_arg[3];

This is usually a pattern we try to avoid (having full code blocks in
a switch statement, and local variables).

Please move the declaration of qdata to the top of the function so you
can drop the braces.

> +
> +               ret = eemi_ops->query_data(qdata, pm_api_ret);
> +               if (ret)
> +                       break;
> +
> +               if (qdata.qid == PM_QID_CLOCK_GET_NAME)
> +                       sprintf(debugfs_buf, "Clock name = %s\n",
> +                               (char *)pm_api_ret);
> +               else if (qdata.qid == PM_QID_CLOCK_GET_FIXEDFACTOR_PARAMS)
> +                       sprintf(debugfs_buf, "Multiplier = %d, Divider = %d\n",
> +                               pm_api_ret[1], pm_api_ret[2]);
> +               else
> +                       sprintf(debugfs_buf,
> +                               "data[0] = 0x%08x\ndata[1] = 0x%08x\n data[2] = 0x%08x\ndata[3] = 0x%08x\n",
> +                               pm_api_ret[0], pm_api_ret[1],
> +                               pm_api_ret[2], pm_api_ret[3]);

If you anticipate more qids here later, a switch could be nicer than a
sequence of if/else.

-Olof