| Message ID | 1516220434-22204-1-git-send-email-jollys@xilinx.com |
|---|---|
| Headers | show |
| Series | drivers: firmware: xilinx: Add firmware driver support | expand |
On Wed, Jan 17, 2018 at 12:20:33PM -0800, Jolly Shah wrote: > Add Firmware-ggs sysfs interface which provides read/write > interface to global storage registers. > > Signed-off-by: Jolly Shah <jollys@xilinx.com> > Signed-off-by: Rajan Vaja <rajanv@xilinx.com> > --- > .../ABI/stable/sysfs-driver-zynqmp-firmware | 33 +++ > drivers/firmware/xilinx/zynqmp/Makefile | 2 +- > drivers/firmware/xilinx/zynqmp/firmware-ggs.c | 298 +++++++++++++++++++++ > drivers/firmware/xilinx/zynqmp/firmware.c | 26 ++ > include/linux/firmware/xilinx/zynqmp/firmware.h | 2 + > 5 files changed, 360 insertions(+), 1 deletion(-) > create mode 100644 Documentation/ABI/stable/sysfs-driver-zynqmp-firmware > create mode 100644 drivers/firmware/xilinx/zynqmp/firmware-ggs.c > > diff --git a/Documentation/ABI/stable/sysfs-driver-zynqmp-firmware b/Documentation/ABI/stable/sysfs-driver-zynqmp-firmware > new file mode 100644 > index 0000000..2483215 > --- /dev/null > +++ b/Documentation/ABI/stable/sysfs-driver-zynqmp-firmware > @@ -0,0 +1,33 @@ > +What: /sys/devices/platform/zynqmp-firmware/ggs* > +Date: January 2018 > +KernelVersion: 4.15.0 > +Contact: "Jolly Shah" <jollys@xilinx.com> > +Description: > + Shows PMU global general storage register value, > + GLOBAL_GEN_STORAGE{0:3}. > + Global general storage register that can be used > + by system to pass information between masters. > + > + The register is reset during system or power-on > + resets. Three registers are used by the FSBL and > + other Xilinx software products: GLOBAL_GEN_STORAGE{4:6}. > + > +Users: Xilinx > + > +What: /sys/devices/platform/zynqmp-firmware/pggs* > +Date: January 2018 > +KernelVersion: 4.15.0 > +Contact: "Jolly Shah" <jollys@xilinx.com> > +Description: > + Shows PMU persistent global general storage register > + value, PERS_GLOB_GEN_STORAGE{0:3}. > + Persistent global general storage register that > + can be used by system to pass information between > + masters. > + > + This register is only reset by the power-on reset > + and maintains its value through a system reset. > + Four registers are used by the FSBL and other Xilinx > + software products: PERS_GLOB_GEN_STORAGE{4:7}. > + Register is reset only by a POR reset. > +Users: Xilinx > diff --git a/drivers/firmware/xilinx/zynqmp/Makefile b/drivers/firmware/xilinx/zynqmp/Makefile > index c3ec669..6629781 100644 > --- a/drivers/firmware/xilinx/zynqmp/Makefile > +++ b/drivers/firmware/xilinx/zynqmp/Makefile > @@ -1,4 +1,4 @@ > # SPDX-License-Identifier: GPL-2.0+ > # Makefile for Xilinx firmwares > > -obj-$(CONFIG_ZYNQMP_FIRMWARE) += firmware.o > +obj-$(CONFIG_ZYNQMP_FIRMWARE) += firmware.o firmware-ggs.o > diff --git a/drivers/firmware/xilinx/zynqmp/firmware-ggs.c b/drivers/firmware/xilinx/zynqmp/firmware-ggs.c > new file mode 100644 > index 0000000..be47ca2 > --- /dev/null > +++ b/drivers/firmware/xilinx/zynqmp/firmware-ggs.c > @@ -0,0 +1,298 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Xilinx Zynq MPSoC Firmware layer > + * > + * Copyright (C) 2014-2018 Xilinx, Inc. > + * > + * Jolly Shah <jollys@xilinx.com> > + * Rajan Vaja <rajanv@xilinx.com> > + */ > + > +#include <linux/compiler.h> > +#include <linux/of.h> > +#include <linux/init.h> > +#include <linux/module.h> > +#include <linux/uaccess.h> > +#include <linux/slab.h> > +#include <linux/platform_device.h> > + > +#include <linux/firmware/xilinx/zynqmp/firmware.h> That's crazy deep nesting, why? > + > +static ssize_t read_register(char *buf, u32 ioctl_id, u32 reg) > +{ > + int ret; > + u32 ret_payload[PAYLOAD_ARG_CNT]; > + const struct zynqmp_eemi_ops *eemi_ops = get_eemi_ops(); > + > + if (!eemi_ops || !eemi_ops->ioctl) > + return 0; Not an error? > + > + ret = eemi_ops->ioctl(0, ioctl_id, reg, 0, ret_payload); > + if (ret) > + return ret; > + > + return snprintf(buf, PAGE_SIZE, "0x%x\n", ret_payload[1]); Minor nit, you never need to use snprintf() for a sysfs file, as you "know" the size and you can't overflow it with just a single value. Yeah, some tool-checkers hate to see a "raw" sprintf() call, but really, ignore them here :) > +} > + > +static ssize_t write_register(const char *buf, size_t count, > + u32 ioctl_id, u32 reg) > +{ > + char *kern_buff; > + char *inbuf; > + char *tok; > + long mask; > + long value; > + int ret; > + u32 ret_payload[PAYLOAD_ARG_CNT]; > + const struct zynqmp_eemi_ops *eemi_ops = get_eemi_ops(); > + > + if (!eemi_ops || !eemi_ops->ioctl) > + return -EFAULT; > + > + kern_buff = kzalloc(count, GFP_KERNEL); > + if (!kern_buff) > + return -ENOMEM; > + > + ret = strlcpy(kern_buff, buf, count); > + if (ret < 0) { > + ret = -EFAULT; > + goto err; > + } > + > + inbuf = kern_buff; > + > + /* Read the write mask */ > + tok = strsep(&inbuf, " "); > + if (!tok) { > + ret = -EFAULT; > + goto err; > + } > + > + ret = kstrtol(tok, 16, &mask); > + if (ret) { > + ret = -EFAULT; > + goto err; > + } > + > + /* Read the write value */ > + tok = strsep(&inbuf, " "); > + if (!tok) { > + ret = -EFAULT; > + goto err; > + } > + > + ret = kstrtol(tok, 16, &value); > + if (ret) { > + ret = -EFAULT; > + goto err; > + } What exactly is the format for the data to be written here? You do not document it in the ABI/ file above, and it looks to be non-trivial to understand from the code :( > + > + ret = eemi_ops->ioctl(0, ioctl_id, reg, 0, ret_payload); > + if (ret) { > + ret = -EFAULT; > + goto err; > + } > + ret_payload[1] &= ~mask; > + value &= mask; > + value |= ret_payload[1]; > + > + ret = eemi_ops->ioctl(0, ioctl_id, reg, value, NULL); > + if (ret) > + ret = -EFAULT; > + > +err: > + kfree(kern_buff); > + if (ret) > + return ret; > + > + return count; > +} > + > +/** > + * ggs_show - Show global general storage (ggs) sysfs attribute > + * @dev: Device structure > + * @attr: Device attribute structure > + * @buf: Requested available shutdown_scope attributes string > + * @reg: Register number > + * > + * Return:Number of bytes printed into the buffer. > + * > + * Helper function for viewing a ggs register value. > + * > + * User-space interface for viewing the content of the ggs0 register. > + * cat /sys/devices/platform/firmware/ggs0 > + */ > +static ssize_t ggs_show(struct device *dev, > + struct device_attribute *attr, > + char *buf, > + u32 reg) > +{ > + return read_register(buf, IOCTL_READ_GGS, reg); > +} > + > +/** > + * ggs_store - Store global general storage (ggs) sysfs attribute > + * @dev: Device structure > + * @attr: Device attribute structure > + * @buf: User entered shutdown_scope attribute string > + * @count: Size of buf > + * @reg: Register number > + * > + * Return: count argument if request succeeds, the corresponding > + * error code otherwise > + * > + * Helper function for storing a ggs register value. > + * > + * For example, the user-space interface for storing a value to the > + * ggs0 register: > + * echo 0xFFFFFFFF 0x1234ABCD > /sys/devices/platform/firmware/ggs0 > + */ > +static ssize_t ggs_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t count, > + u32 reg) > +{ > + if (!dev || !attr || !buf || !count || reg >= GSS_NUM_REGS) > + return -EINVAL; > + > + return write_register(buf, count, IOCTL_WRITE_GGS, reg); > +} > + > +/* GGS register show functions */ > +#define GGS0_SHOW(N) \ > + ssize_t ggs##N##_show(struct device *dev, \ > + struct device_attribute *attr, \ > + char *buf) \ > + { \ > + return ggs_show(dev, attr, buf, N); \ > + } > + > +static GGS0_SHOW(0); > +static GGS0_SHOW(1); > +static GGS0_SHOW(2); > +static GGS0_SHOW(3); > + > +/* GGS register store function */ > +#define GGS0_STORE(N) \ > + ssize_t ggs##N##_store(struct device *dev, \ > + struct device_attribute *attr, \ > + const char *buf, \ > + size_t count) \ > + { \ > + return ggs_store(dev, attr, buf, count, N); \ > + } > + > +static GGS0_STORE(0); > +static GGS0_STORE(1); > +static GGS0_STORE(2); > +static GGS0_STORE(3); > + > +/* GGS register device attributes */ > +static DEVICE_ATTR_RW(ggs0); > +static DEVICE_ATTR_RW(ggs1); > +static DEVICE_ATTR_RW(ggs2); > +static DEVICE_ATTR_RW(ggs3); > + > +#define CREATE_GGS_DEVICE(dev, N) \ > +do { \ > + if (device_create_file(dev, &dev_attr_ggs##N)) \ > + dev_err(dev, "unable to create ggs%d attribute\n", N); \ > +} while (0) > + > +/** > + * pggs_show - Show persistent global general storage (pggs) sysfs attribute > + * @dev: Device structure > + * @attr: Device attribute structure > + * @buf: Requested available shutdown_scope attributes string > + * @reg: Register number > + * > + * Return:Number of bytes printed into the buffer. > + * > + * Helper function for viewing a pggs register value. > + */ > +static ssize_t pggs_show(struct device *dev, > + struct device_attribute *attr, > + char *buf, > + u32 reg) > +{ > + return read_register(buf, IOCTL_READ_GGS, reg); > +} > + > +/** > + * pggs_store - Store persistent global general storage (pggs) sysfs attribute > + * @dev: Device structure > + * @attr: Device attribute structure > + * @buf: User entered shutdown_scope attribute string > + * @count: Size of buf > + * @reg: Register number > + * > + * Return: count argument if request succeeds, the corresponding > + * error code otherwise > + * > + * Helper function for storing a pggs register value. > + */ > +static ssize_t pggs_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t count, > + u32 reg) > +{ > + return write_register(buf, count, IOCTL_WRITE_PGGS, reg); > +} > + > +#define PGGS0_SHOW(N) \ > + ssize_t pggs##N##_show(struct device *dev, \ > + struct device_attribute *attr, \ > + char *buf) \ > + { \ > + return pggs_show(dev, attr, buf, N); \ > + } > + > +/* PGGS register show functions */ > +static PGGS0_SHOW(0); > +static PGGS0_SHOW(1); > +static PGGS0_SHOW(2); > +static PGGS0_SHOW(3); > + > +#define PGGS0_STORE(N) \ > + ssize_t pggs##N##_store(struct device *dev, \ > + struct device_attribute *attr, \ > + const char *buf, \ > + size_t count) \ > + { \ > + return pggs_store(dev, attr, buf, count, N); \ > + } > + > +/* PGGS register store functions */ > +static PGGS0_STORE(0); > +static PGGS0_STORE(1); > +static PGGS0_STORE(2); > +static PGGS0_STORE(3); > + > +/* PGGS register device attributes */ > +static DEVICE_ATTR_RW(pggs0); > +static DEVICE_ATTR_RW(pggs1); > +static DEVICE_ATTR_RW(pggs2); > +static DEVICE_ATTR_RW(pggs3); > + > +#define CREATE_PGGS_DEVICE(dev, N) \ > +do { \ > + if (device_create_file(dev, &dev_attr_pggs##N)) \ > + dev_err(dev, "unable to create pggs%d attribute\n", N); \ Ick, no, just use an attribute group please. Handles all of this mess for you automatically, and will unwind properly if you have an error (which this macro does not do.) thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 17, 2018 at 12:20:32PM -0800, Jolly Shah wrote: > This patch is adding communication layer with firmware. > Firmware driver provides an interface to firmware APIs. > Interface APIs can be used by any driver to communicate to > PMUFW(Platform Management Unit). All requests go through ATF. > > Signed-off-by: Jolly Shah <jollys@xilinx.com> > Signed-off-by: Rajan Vaja <rajanv@xilinx.com> > --- > arch/arm64/Kconfig.platforms | 1 + > drivers/firmware/Kconfig | 1 + > drivers/firmware/Makefile | 1 + > drivers/firmware/xilinx/Kconfig | 4 + > drivers/firmware/xilinx/Makefile | 4 + > drivers/firmware/xilinx/zynqmp/Kconfig | 16 + > drivers/firmware/xilinx/zynqmp/Makefile | 4 + > drivers/firmware/xilinx/zynqmp/firmware.c | 987 ++++++++++++++++++++++++ > include/linux/firmware/xilinx/zynqmp/firmware.h | 570 ++++++++++++++ Why does this file need to be in include/linux/ at all? Shouldn't it just live in the driver-specific subdir? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 17, 2018 at 12:20:34PM -0800, Jolly Shah wrote: > +/* Setup debugfs fops */ > +static const struct file_operations fops_zynqmp_pm_dbgfs = { > + .owner = THIS_MODULE, > + .write = zynqmp_pm_debugfs_api_write, > + .read = zynqmp_pm_debugfs_api_version_read, > +}; > + > +/** > + * zynqmp_pm_api_debugfs_init - Initialize debugfs interface > + * > + * Return: Returns 0 on success > + * Corresponding error code otherwise > + */ > +int zynqmp_pm_api_debugfs_init(void) > +{ > + int err; > + > + /* Initialize debugfs interface */ > + zynqmp_pm_debugfs_dir = debugfs_create_dir(DRIVER_NAME, NULL); > + if (!zynqmp_pm_debugfs_dir) { > + pr_err("debugfs_create_dir failed\n"); > + return -ENODEV; > + } No, you should NEVER care if a debugfs call returned an error or not, no need to check it at all. Your code path should not change based on the return value as no code should depened on the functionality of debugfs. Any error returned by a debugfs call can be passed right back into it with no problems, so again, no need to check this. > + > + zynqmp_pm_debugfs_power = > + debugfs_create_file("pm", 0220, > + zynqmp_pm_debugfs_dir, NULL, > + &fops_zynqmp_pm_dbgfs); > + if (!zynqmp_pm_debugfs_power) { > + pr_err("debugfs_create_file power failed\n"); > + err = -ENODEV; > + goto err_dbgfs; > + } > + > + zynqmp_pm_debugfs_api_version = > + debugfs_create_file("api_version", 0444, > + zynqmp_pm_debugfs_dir, NULL, > + &fops_zynqmp_pm_dbgfs); > + if (!zynqmp_pm_debugfs_api_version) { > + pr_err("debugfs_create_file api_version failed\n"); > + err = -ENODEV; > + goto err_dbgfs; > + } Why do you save these dentries at all anyway? You never do anything with them, just create the files and away you go, no need to worry about anything. Remember, debugfs was created to be very simple to use, don't make it more complex than it has to be please. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks for review Greg, > -----Original Message----- > From: Greg KH [mailto:gregkh@linuxfoundation.org] > Sent: Tuesday, January 23, 2018 12:38 AM > To: Jolly Shah <JOLLYS@xilinx.com> > Cc: ard.biesheuvel@linaro.org; mingo@kernel.org; matt@codeblueprint.co.uk; > sudeep.holla@arm.com; hkallweit1@gmail.com; keescook@chromium.org; > dmitry.torokhov@gmail.com; michal.simek@xilinx.com; robh+dt@kernel.org; > mark.rutland@arm.com; linux-arm-kernel@lists.infradead.org; linux- > kernel@vger.kernel.org; devicetree@vger.kernel.org; Rajan Vaja > <RAJANV@xilinx.com>; Jolly Shah <JOLLYS@xilinx.com> > Subject: Re: [PATCH v2 2/4] drivers: firmware: xilinx: Add ZynqMP firmware > driver > > On Wed, Jan 17, 2018 at 12:20:32PM -0800, Jolly Shah wrote: > > This patch is adding communication layer with firmware. > > Firmware driver provides an interface to firmware APIs. > > Interface APIs can be used by any driver to communicate to > > PMUFW(Platform Management Unit). All requests go through ATF. > > > > Signed-off-by: Jolly Shah <jollys@xilinx.com> > > Signed-off-by: Rajan Vaja <rajanv@xilinx.com> > > --- > > arch/arm64/Kconfig.platforms | 1 + > > drivers/firmware/Kconfig | 1 + > > drivers/firmware/Makefile | 1 + > > drivers/firmware/xilinx/Kconfig | 4 + > > drivers/firmware/xilinx/Makefile | 4 + > > drivers/firmware/xilinx/zynqmp/Kconfig | 16 + > > drivers/firmware/xilinx/zynqmp/Makefile | 4 + > > drivers/firmware/xilinx/zynqmp/firmware.c | 987 > ++++++++++++++++++++++++ > > include/linux/firmware/xilinx/zynqmp/firmware.h | 570 ++++++++++++++ > > Why does this file need to be in include/linux/ at all? Shouldn't it just live in the > driver-specific subdir? > > thanks, > > greg k-h It is used by multiple drivers so can not live in driver specific subdir. Thanks, Jolly Shah -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks for review Greg, > -----Original Message----- > From: Greg KH [mailto:gregkh@linuxfoundation.org] > Sent: Tuesday, January 23, 2018 12:38 AM > To: Jolly Shah <JOLLYS@xilinx.com> > Cc: ard.biesheuvel@linaro.org; mingo@kernel.org; matt@codeblueprint.co.uk; > sudeep.holla@arm.com; hkallweit1@gmail.com; keescook@chromium.org; > dmitry.torokhov@gmail.com; michal.simek@xilinx.com; robh+dt@kernel.org; > mark.rutland@arm.com; linux-arm-kernel@lists.infradead.org; linux- > kernel@vger.kernel.org; devicetree@vger.kernel.org; Rajan Vaja > <RAJANV@xilinx.com>; Jolly Shah <JOLLYS@xilinx.com> > Subject: Re: [PATCH v2 3/4] drivers: firmware: xilinx: Add sysfs interface > > On Wed, Jan 17, 2018 at 12:20:33PM -0800, Jolly Shah wrote: > > Add Firmware-ggs sysfs interface which provides read/write interface > > to global storage registers. > > > > Signed-off-by: Jolly Shah <jollys@xilinx.com> > > Signed-off-by: Rajan Vaja <rajanv@xilinx.com> > > --- > > .../ABI/stable/sysfs-driver-zynqmp-firmware | 33 +++ > > drivers/firmware/xilinx/zynqmp/Makefile | 2 +- > > drivers/firmware/xilinx/zynqmp/firmware-ggs.c | 298 > +++++++++++++++++++++ > > drivers/firmware/xilinx/zynqmp/firmware.c | 26 ++ > > include/linux/firmware/xilinx/zynqmp/firmware.h | 2 + > > 5 files changed, 360 insertions(+), 1 deletion(-) create mode 100644 > > Documentation/ABI/stable/sysfs-driver-zynqmp-firmware > > create mode 100644 drivers/firmware/xilinx/zynqmp/firmware-ggs.c > > > > + > > +#include <linux/firmware/xilinx/zynqmp/firmware.h> > > That's crazy deep nesting, why? It needs to be in include/linux to be used by other drivers and as it is Xilinx specific, we have it in subdirectory. > > > + > > +static ssize_t read_register(char *buf, u32 ioctl_id, u32 reg) { > > + int ret; > > + u32 ret_payload[PAYLOAD_ARG_CNT]; > > + const struct zynqmp_eemi_ops *eemi_ops = get_eemi_ops(); > > + > > + if (!eemi_ops || !eemi_ops->ioctl) > > + return 0; > > Not an error? Fixed in v3 patch series. > > > + > > + ret = eemi_ops->ioctl(0, ioctl_id, reg, 0, ret_payload); > > + if (ret) > > + return ret; > > + > > + return snprintf(buf, PAGE_SIZE, "0x%x\n", ret_payload[1]); > > Minor nit, you never need to use snprintf() for a sysfs file, as you "know" the size > and you can't overflow it with just a single value. > > Yeah, some tool-checkers hate to see a "raw" sprintf() call, but really, ignore > them here :) Changed to sprint in v3 patch series. > > + > > + /* Read the write value */ > > + tok = strsep(&inbuf, " "); > > + if (!tok) { > > + ret = -EFAULT; > > + goto err; > > + } > > + > > + ret = kstrtol(tok, 16, &value); > > + if (ret) { > > + ret = -EFAULT; > > + goto err; > > + } > > What exactly is the format for the data to be written here? You do not > document it in the ABI/ file above, and it looks to be non-trivial to understand > from the code :( > Updated documentation in v3 patch series. > > + > > +#define CREATE_PGGS_DEVICE(dev, N) \ > > +do { \ > > + if (device_create_file(dev, &dev_attr_pggs##N)) \ > > + dev_err(dev, "unable to create pggs%d attribute\n", > > +N); \ > > Ick, no, just use an attribute group please. Handles all of this mess for you > automatically, and will unwind properly if you have an error (which this macro > does not do.) > > thanks, > > greg k-h Fixed in v3 patch series. Thanks, Jolly Shah -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks for review Greg, > -----Original Message----- > From: Greg KH [mailto:gregkh@linuxfoundation.org] > Sent: Tuesday, January 23, 2018 12:41 AM > To: Jolly Shah <JOLLYS@xilinx.com> > Cc: ard.biesheuvel@linaro.org; mingo@kernel.org; matt@codeblueprint.co.uk; > sudeep.holla@arm.com; hkallweit1@gmail.com; keescook@chromium.org; > dmitry.torokhov@gmail.com; michal.simek@xilinx.com; robh+dt@kernel.org; > mark.rutland@arm.com; linux-arm-kernel@lists.infradead.org; linux- > kernel@vger.kernel.org; devicetree@vger.kernel.org; Rajan Vaja > <RAJANV@xilinx.com>; Jolly Shah <JOLLYS@xilinx.com> > Subject: Re: [PATCH v2 4/4] drivers: firmware: xilinx: Add debugfs interface > > On Wed, Jan 17, 2018 at 12:20:34PM -0800, Jolly Shah wrote: > > +/* Setup debugfs fops */ > > +static const struct file_operations fops_zynqmp_pm_dbgfs = { > > + .owner = THIS_MODULE, > > + .write = zynqmp_pm_debugfs_api_write, > > + .read = zynqmp_pm_debugfs_api_version_read, > > +}; > > + > > +/** > > + * zynqmp_pm_api_debugfs_init - Initialize debugfs interface > > + * > > + * Return: Returns 0 on success > > + * Corresponding error code otherwise > > + */ > > +int zynqmp_pm_api_debugfs_init(void) > > +{ > > + int err; > > + > > + /* Initialize debugfs interface */ > > + zynqmp_pm_debugfs_dir = debugfs_create_dir(DRIVER_NAME, NULL); > > + if (!zynqmp_pm_debugfs_dir) { > > + pr_err("debugfs_create_dir failed\n"); > > + return -ENODEV; > > + } > > No, you should NEVER care if a debugfs call returned an error or not, no need to > check it at all. Your code path should not change based on the return value as > no code should depened on the functionality of debugfs. > > Any error returned by a debugfs call can be passed right back into it with no > problems, so again, no need to check this. > Fixed in v3 patch series. Not saving dentries anymore but added check to show warning message instead of error. > > + > > + zynqmp_pm_debugfs_power = > > + debugfs_create_file("pm", 0220, > > + zynqmp_pm_debugfs_dir, NULL, > > + &fops_zynqmp_pm_dbgfs); > > + if (!zynqmp_pm_debugfs_power) { > > + pr_err("debugfs_create_file power failed\n"); > > + err = -ENODEV; > > + goto err_dbgfs; > > + } > > + > > + zynqmp_pm_debugfs_api_version = > > + debugfs_create_file("api_version", 0444, > > + zynqmp_pm_debugfs_dir, NULL, > > + &fops_zynqmp_pm_dbgfs); > > + if (!zynqmp_pm_debugfs_api_version) { > > + pr_err("debugfs_create_file api_version failed\n"); > > + err = -ENODEV; > > + goto err_dbgfs; > > + } > > Why do you save these dentries at all anyway? You never do anything with > them, just create the files and away you go, no need to worry about anything. > > Remember, debugfs was created to be very simple to use, don't make it more > complex than it has to be please. > > thanks, > > greg k-h Fixed in v3 patch series. Thanks, Jolly Shah -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Introduce firmware driver for ZynqMP core. This patchset is adding communication layer with firmware. Firmware driver provides an interface to firmware APIs. Interface APIs can be used by any driver to communicate to PMUFW(Platform Management Unit). All requests go through ATF. v2: - change SPDX-License-Identifier license text style - split patch into multiple patches - Updated copyrights - Added ABI documentation - incorporated logical review comments from previuos patch. Discussed below: https://patchwork.kernel.org/patch/10150665/ Jolly Shah (4): dt-bindings: firmware: Add bindings for ZynqMP firmware drivers: firmware: xilinx: Add ZynqMP firmware driver drivers: firmware: xilinx: Add sysfs interface drivers: firmware: xilinx: Add debugfs interface .../ABI/stable/sysfs-driver-zynqmp-firmware | 33 + .../firmware/xilinx/xlnx,zynqmp-firmware.txt | 16 + arch/arm64/Kconfig.platforms | 1 + drivers/firmware/Kconfig | 1 + drivers/firmware/Makefile | 1 + drivers/firmware/xilinx/Kconfig | 4 + drivers/firmware/xilinx/Makefile | 4 + drivers/firmware/xilinx/zynqmp/Kconfig | 23 + drivers/firmware/xilinx/zynqmp/Makefile | 5 + drivers/firmware/xilinx/zynqmp/firmware-debug.c | 524 ++++++++++ drivers/firmware/xilinx/zynqmp/firmware-ggs.c | 298 ++++++ drivers/firmware/xilinx/zynqmp/firmware.c | 1022 ++++++++++++++++++++ .../linux/firmware/xilinx/zynqmp/firmware-debug.h | 31 + include/linux/firmware/xilinx/zynqmp/firmware.h | 572 +++++++++++ 14 files changed, 2535 insertions(+) create mode 100644 Documentation/ABI/stable/sysfs-driver-zynqmp-firmware create mode 100644 Documentation/devicetree/bindings/firmware/xilinx/xlnx,zynqmp-firmware.txt create mode 100644 drivers/firmware/xilinx/Kconfig create mode 100644 drivers/firmware/xilinx/Makefile create mode 100644 drivers/firmware/xilinx/zynqmp/Kconfig create mode 100644 drivers/firmware/xilinx/zynqmp/Makefile create mode 100644 drivers/firmware/xilinx/zynqmp/firmware-debug.c create mode 100644 drivers/firmware/xilinx/zynqmp/firmware-ggs.c create mode 100644 drivers/firmware/xilinx/zynqmp/firmware.c create mode 100644 include/linux/firmware/xilinx/zynqmp/firmware-debug.h create mode 100644 include/linux/firmware/xilinx/zynqmp/firmware.h -- 2.7.4 This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html