| Message ID | 1537985581-32164-3-git-send-email-jollys@xilinx.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | drivers: clk: Add ZynqMP clock driver support | expand |
Hi, Just nits on code readability below. Approach looks OK to me. On Wed, Sep 26, 2018 at 11:13 AM Jolly Shah <jolly.shah@xilinx.com> wrote: > > From: Rajan Vaja <rajan.vaja@xilinx.com> > > Add ZynqMP firmware IOCTL API to control and configure > devices like PLLs, SD, Gem, etc. > > Signed-off-by: Rajan Vaja <rajan.vaja@xilinx.com> > Signed-off-by: Jolly Shah <jollys@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 >
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:olof@lixom.net] > Sent: Wednesday, September 26, 2018 1:49 PM > To: Jolly Shah <JOLLYS@xilinx.com> > Cc: Michael Turquette <mturquette@baylibre.com>; Stephen Boyd > <sboyd@codeaurora.org>; Michal Simek <michals@xilinx.com>; ARM-SoC > Maintainers <arm@kernel.org>; linux-clk <linux-clk@vger.kernel.org>; Rajan > Vaja <RAJANV@xilinx.com>; Linux ARM Mailing List <linux-arm- > kernel@lists.infradead.org>; Linux Kernel Mailing List <linux- > kernel@vger.kernel.org>; Rajan Vaja <RAJANV@xilinx.com>; Jolly Shah > <JOLLYS@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.shah@xilinx.com> wrote: > > > > From: Rajan Vaja <rajan.vaja@xilinx.com> > > > > Add ZynqMP firmware IOCTL API to control and configure devices like > > PLLs, SD, Gem, etc. > > > > Signed-off-by: Rajan Vaja <rajan.vaja@xilinx.com> > > Signed-off-by: Jolly Shah <jollys@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 > >
On Fri, Sep 28, 2018 at 10:15 AM Jolly Shah <JOLLYS@xilinx.com> wrote: > > 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 I'm happy with this one, thanks for revising the patchset with the fixes. Feel free to add: Acked-by: Olof Johansson <olof@lixom.net> to the patches. -Olof
Hi, On 28.9.2018 23:12, Olof Johansson wrote: > On Fri, Sep 28, 2018 at 10:15 AM Jolly Shah <JOLLYS@xilinx.com> wrote: >> >> 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 > > I'm happy with this one, thanks for revising the patchset with the fixes. > > Feel free to add: > > Acked-by: Olof Johansson <olof@lixom.net> > > to the patches. Just for clarification. I have checked driver/clk and patches should still go via Michael or Stephen. Firmware code is already in arm-soc tree that's why I would expect Michael/Stephen should review it and then it shouldn't be a problem to take this via arm-soc tree. Olof: Please correct me if I am wrong and you are happy to take it via arm-soc directly. Thanks, Michal
On Mon, Oct 1, 2018 at 6:17 AM Michal Simek <michal.simek@xilinx.com> wrote: > > Hi, > > On 28.9.2018 23:12, Olof Johansson wrote: > > On Fri, Sep 28, 2018 at 10:15 AM Jolly Shah <JOLLYS@xilinx.com> wrote: > >> > >> 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 > > > > I'm happy with this one, thanks for revising the patchset with the fixes. > > > > Feel free to add: > > > > Acked-by: Olof Johansson <olof@lixom.net> > > > > to the patches. > > Just for clarification. I have checked driver/clk and patches should > still go via Michael or Stephen. > Firmware code is already in arm-soc tree that's why I would expect > Michael/Stephen should review it and then it shouldn't be a problem to > take this via arm-soc tree. > > Olof: Please correct me if I am wrong and you are happy to take it via > arm-soc directly. Honestly it might be easiest for you to prepare one branch and send to all parties in case you have contents you expect to go on top, but besides that I'd be fine with merging through either tree. -Olof
On 1.10.2018 19:26, Olof Johansson wrote: > On Mon, Oct 1, 2018 at 6:17 AM Michal Simek <michal.simek@xilinx.com> wrote: >> >> Hi, >> >> On 28.9.2018 23:12, Olof Johansson wrote: >>> On Fri, Sep 28, 2018 at 10:15 AM Jolly Shah <JOLLYS@xilinx.com> wrote: >>>> >>>> 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 >>> >>> I'm happy with this one, thanks for revising the patchset with the fixes. >>> >>> Feel free to add: >>> >>> Acked-by: Olof Johansson <olof@lixom.net> >>> >>> to the patches. >> >> Just for clarification. I have checked driver/clk and patches should >> still go via Michael or Stephen. >> Firmware code is already in arm-soc tree that's why I would expect >> Michael/Stephen should review it and then it shouldn't be a problem to >> take this via arm-soc tree. >> >> Olof: Please correct me if I am wrong and you are happy to take it via >> arm-soc directly. > > Honestly it might be easiest for you to prepare one branch and send to > all parties in case you have contents you expect to go on top, but > besides that I'd be fine with merging through either tree. ok. Stephen: Can you please take a look at 4/4? dt binding was already reviewed that's when we have ACK from 4/4 I will prepare that branch. Thanks, Michal
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) +{ + 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; + + 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; + + 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)