Hi Olof,

-----Original Message-----
From: Olof Johansson [mailto:olof at lixom.net]
Sent: Saturday, September 08, 2018 6:15 PM
To: Jolly Shah <redacted>
Cc: ard.biesheuvel at linaro.org; Ingo Molnar <mingo@kernel.org>; Greg Kroah-
Hartman [off-list ref]; matt at codeblueprint.co.uk; Sudeep
Holla [off-list ref]; hkallweit1 at gmail.com; Kees Cook
[off-list ref]; Dmitry Torokhov [off-list ref];
Michael Turquette [off-list ref]; Stephen Boyd
[off-list ref]; Michal Simek [off-list ref]; Rob Herring
[off-list ref]; Mark Rutland [off-list ref]; linux-clk
[off-list ref]; Rajan Vaja [off-list ref]; Linux ARM
Mailing List [off-list ref]; Linux Kernel Mailing List
[off-list ref]; DTML [off-list ref]; Jolly
Shah [off-list ref]
Subject: Re: [PATCH v11 08/11] firmware: xilinx: Add debugfs for query data API

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:

Sure. I will submit incremental patches for suggested changes.

Thanks,
Jolly Shah

On Fri, Aug 3, 2018 at 10:53 AM, Jolly Shah [off-list ref] wrote:

quoted

From: Rajan Vaja <redacted>

Add debugfs file to query platform specific data from firmware using
debugfs interface.

Signed-off-by: Rajan Vaja <redacted>
Signed-off-by: Jolly Shah <redacted>
---
 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)

quoted

                        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.

quoted

+
+               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",

quoted

+                               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