Hi Olof, Thanks for the review. Pushed v4 with suggested fixes. Let us know if series looks good and we can create pull request for same.
Thanks, Jolly Shah > -----Original Message----- > From: Olof Johansson [mailto:o...@lixom.net] > Sent: Wednesday, September 26, 2018 1:49 PM > To: Jolly Shah <jol...@xilinx.com> > Cc: Michael Turquette <mturque...@baylibre.com>; Stephen Boyd > <sb...@codeaurora.org>; Michal Simek <mich...@xilinx.com>; ARM-SoC > Maintainers <a...@kernel.org>; linux-clk <linux-...@vger.kernel.org>; Rajan > Vaja <raj...@xilinx.com>; Linux ARM Mailing List <linux-arm- > ker...@lists.infradead.org>; Linux Kernel Mailing List <linux- > ker...@vger.kernel.org>; Rajan Vaja <raj...@xilinx.com>; Jolly Shah > <jol...@xilinx.com> > Subject: Re: [PATCH v3 2/4] firmware: xilinx: Add zynqmp IOCTL API for device > control > > Hi, > > Just nits on code readability below. Approach looks OK to me. > > On Wed, Sep 26, 2018 at 11:13 AM Jolly Shah <jolly.s...@xilinx.com> wrote: > > > > From: Rajan Vaja <rajan.v...@xilinx.com> > > > > Add ZynqMP firmware IOCTL API to control and configure devices like > > PLLs, SD, Gem, etc. > > > > Signed-off-by: Rajan Vaja <rajan.v...@xilinx.com> > > Signed-off-by: Jolly Shah <jol...@xilinx.com> > > --- > > drivers/firmware/xilinx/zynqmp.c | 43 > ++++++++++++++++++++++++++++++++++++ > > include/linux/firmware/xlnx-zynqmp.h | 4 +++- > > 2 files changed, 46 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/firmware/xilinx/zynqmp.c > > b/drivers/firmware/xilinx/zynqmp.c > > index 84b3fd2..671a37a 100644 > > --- a/drivers/firmware/xilinx/zynqmp.c > > +++ b/drivers/firmware/xilinx/zynqmp.c > > @@ -428,6 +428,48 @@ static int zynqmp_pm_clock_getparent(u32 clock_id, > u32 *parent_id) > > return ret; > > } > > > > +/** > > + * zynqmp_is_valid_ioctl() - Check whether IOCTL ID is valid or not > > + * @ioctl_id: IOCTL ID > > + * > > + * Return: 0 if IOCTL is valid, else -EINVAL */ static inline int > > +zynqmp_is_valid_ioctl(u32 ioctl_id) > > I think most who come across the use of this would expect an > .*is_valid() to return true (non-0) when valid, and 0 otherwise. > > > +{ > > + if (ioctl_id == IOCTL_SET_PLL_FRAC_MODE || > > + ioctl_id == IOCTL_GET_PLL_FRAC_MODE || > > + ioctl_id == IOCTL_SET_PLL_FRAC_DATA || > > + ioctl_id == IOCTL_GET_PLL_FRAC_DATA) > > + return 0; > > This is purely a matter of taste, and no requirement to change, but I find a > switch slightly easier to read for this kind of usage: > > switch(ioctl_id) { > case IOCTL_SET_PLL_FRAC_MODE: > case IOCTL_GET_PLL_FRAC_MODE: > case IOCTL_SET_PLL_FRAC_DATA: > case IOCTL_GET_PLL_FRAC_DATA: > return 1; > default: > return 0; > } > > > + > > + return -EINVAL; > > +} > > + > > +/** > > + * zynqmp_pm_ioctl() - PM IOCTL API for device control and configs > > + * @node_id: Node ID of the device > > + * @ioctl_id: ID of the requested IOCTL > > + * @arg1: Argument 1 to requested IOCTL call > > + * @arg2: Argument 2 to requested IOCTL call > > + * @out: Returned output value > > + * > > + * This function calls IOCTL to firmware for device control and > > configuration. > > + * > > + * Return: Returns status, either success or error+reason */ static > > +int zynqmp_pm_ioctl(u32 node_id, u32 ioctl_id, u32 arg1, u32 arg2, > > + u32 *out) > > +{ > > + int ret; > > + > > + ret = zynqmp_is_valid_ioctl(ioctl_id); > > + if (ret) > > + return ret; > > So with changed return values, this would turn into: > > if (!zynqmp_is_valid_ioctl(ioctl_id)) > return -EINVAL; > > > + > > + return zynqmp_pm_invoke_fn(PM_IOCTL, node_id, ioctl_id, > > + arg1, arg2, out); } > > + > > static const struct zynqmp_eemi_ops eemi_ops = { > > .get_api_version = zynqmp_pm_get_api_version, > > .query_data = zynqmp_pm_query_data, @@ -440,6 +482,7 @@ static > > const struct zynqmp_eemi_ops eemi_ops = { > > .clock_getrate = zynqmp_pm_clock_getrate, > > .clock_setparent = zynqmp_pm_clock_setparent, > > .clock_getparent = zynqmp_pm_clock_getparent, > > + .ioctl = zynqmp_pm_ioctl, > > }; > > > > /** > > diff --git a/include/linux/firmware/xlnx-zynqmp.h > > b/include/linux/firmware/xlnx-zynqmp.h > > index 015e130..7a9db08 100644 > > --- a/include/linux/firmware/xlnx-zynqmp.h > > +++ b/include/linux/firmware/xlnx-zynqmp.h > > @@ -34,7 +34,8 @@ > > > > enum pm_api_id { > > PM_GET_API_VERSION = 1, > > - PM_QUERY_DATA = 35, > > + PM_IOCTL = 34, > > + PM_QUERY_DATA, > > PM_CLOCK_ENABLE, > > PM_CLOCK_DISABLE, > > PM_CLOCK_GETSTATE, > > @@ -99,6 +100,7 @@ struct zynqmp_eemi_ops { > > int (*clock_getrate)(u32 clock_id, u64 *rate); > > int (*clock_setparent)(u32 clock_id, u32 parent_id); > > int (*clock_getparent)(u32 clock_id, u32 *parent_id); > > + int (*ioctl)(u32 node_id, u32 ioctl_id, u32 arg1, u32 arg2, > > + u32 *out); > > }; > > > > #if IS_REACHABLE(CONFIG_ARCH_ZYNQMP) > > -- > > 2.7.4 > >
