RE: [PATCH v11 08/11] firmware: xilinx: Add debugfs for query data API
Hi Olof,
> -Original Message-
> From: Olof Johansson [mailto:o...@lixom.net]
> Sent: Saturday, September 08, 2018 6:15 PM
> To: Jolly Shah
> Cc: ard.biesheu...@linaro.org; Ingo Molnar ; Greg Kroah-
> Hartman ; m...@codeblueprint.co.uk; Sudeep
> Holla ; hkallwe...@gmail.com; Kees Cook
> ; Dmitry Torokhov ;
> Michael Turquette ; Stephen Boyd
> ; Michal Simek ; Rob Herring
> ; Mark Rutland ; linux-clk
> ; Rajan Vaja ; Linux ARM
> Mailing List ; Linux Kernel Mailing List
> ; DTML ; Jolly
> Shah
> 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 wrote:
> > From: Rajan Vaja
> >
> > Add debugfs file to query platform specific data from firmware using
> > debugfs interface.
> >
> > Signed-off-by: Rajan Vaja
> > Signed-off-by: Jolly Shah
> > ---
> > 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
RE: [PATCH v11 08/11] firmware: xilinx: Add debugfs for query data API
Hi Olof,
> -Original Message-
> From: Olof Johansson [mailto:o...@lixom.net]
> Sent: Saturday, September 08, 2018 6:15 PM
> To: Jolly Shah
> Cc: ard.biesheu...@linaro.org; Ingo Molnar ; Greg Kroah-
> Hartman ; m...@codeblueprint.co.uk; Sudeep
> Holla ; hkallwe...@gmail.com; Kees Cook
> ; Dmitry Torokhov ;
> Michael Turquette ; Stephen Boyd
> ; Michal Simek ; Rob Herring
> ; Mark Rutland ; linux-clk
> ; Rajan Vaja ; Linux ARM
> Mailing List ; Linux Kernel Mailing List
> ; DTML ; Jolly
> Shah
> 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 wrote:
> > From: Rajan Vaja
> >
> > Add debugfs file to query platform specific data from firmware using
> > debugfs interface.
> >
> > Signed-off-by: Rajan Vaja
> > Signed-off-by: Jolly Shah
> > ---
> > 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
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:
On Fri, Aug 3, 2018 at 10:53 AM, Jolly Shah wrote:
> From: Rajan Vaja
>
> Add debugfs file to query platform specific data from firmware
> using debugfs interface.
>
> Signed-off-by: Rajan Vaja
> Signed-off-by: Jolly Shah
> ---
> 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
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:
On Fri, Aug 3, 2018 at 10:53 AM, Jolly Shah wrote:
> From: Rajan Vaja
>
> Add debugfs file to query platform specific data from firmware
> using debugfs interface.
>
> Signed-off-by: Rajan Vaja
> Signed-off-by: Jolly Shah
> ---
> 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
