[PATCH v3 2/4] firmware: xilinx: Add zynqmp IOCTL API for device control
From: Olof Johansson <hidden>
Date: 2018-09-26 20:48:58
Also in:
linux-clk, lkml
Hi, Just nits on code readability below. Approach looks OK to me. On Wed, Sep 26, 2018 at 11:13 AM Jolly Shah [off-list ref] wrote:
quoted hunk ↗ jump to hunk
From: Rajan Vaja <redacted> Add ZynqMP firmware IOCTL API to control and configure devices like PLLs, SD, Gem, etc. Signed-off-by: Rajan Vaja <redacted> Signed-off-by: Jolly Shah <redacted> --- 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;
quoted hunk ↗ jump to hunk
+ + 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