Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753295AbeAQNmc (ORCPT + 1 other); Wed, 17 Jan 2018 08:42:32 -0500 Received: from out1-smtp.messagingengine.com ([66.111.4.25]:37699 "EHLO out1-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752276AbeAQNm3 (ORCPT ); Wed, 17 Jan 2018 08:42:29 -0500 X-ME-Sender: Date: Wed, 17 Jan 2018 14:42:19 +0100 From: Greg KH To: ShuFanLee Cc: heikki.krogerus@linux.intel.com, cy_huang@richtek.com, shufan_lee@richtek.com, linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org Subject: Re: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver Message-ID: <20180117134219.GE3188@kroah.com> References: <1515567552-7692-1-git-send-email-leechu729@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1515567552-7692-1-git-send-email-leechu729@gmail.com> User-Agent: Mutt/1.9.2 (2017-12-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: On Wed, Jan 10, 2018 at 02:59:12PM +0800, ShuFanLee wrote: > From: ShuFanLee > > Richtek RT1711H Type-C chip driver that works with > Type-C Port Controller Manager to provide USB PD and > USB Type-C functionalities. > > Signed-off-by: ShuFanLee Minor review of your main structure and your debugfs code and other stuff, all of which need work: > --- > .../devicetree/bindings/usb/richtek,rt1711h.txt | 38 + > arch/arm64/boot/dts/hisilicon/rt1711h.dtsi | 11 + > drivers/usb/typec/Kconfig | 2 + > drivers/usb/typec/Makefile | 1 + > drivers/usb/typec/rt1711h/Kconfig | 7 + > drivers/usb/typec/rt1711h/Makefile | 2 + > drivers/usb/typec/rt1711h/rt1711h.c | 2241 ++++++++++++++++++++ > drivers/usb/typec/rt1711h/rt1711h.h | 300 +++ > 8 files changed, 2602 insertions(+) > create mode 100644 Documentation/devicetree/bindings/usb/richtek,rt1711h.txt > create mode 100644 arch/arm64/boot/dts/hisilicon/rt1711h.dtsi > create mode 100644 drivers/usb/typec/rt1711h/Kconfig > create mode 100644 drivers/usb/typec/rt1711h/Makefile > create mode 100644 drivers/usb/typec/rt1711h/rt1711h.c > create mode 100644 drivers/usb/typec/rt1711h/rt1711h.h > > diff --git a/Documentation/devicetree/bindings/usb/richtek,rt1711h.txt b/Documentation/devicetree/bindings/usb/richtek,rt1711h.txt > new file mode 100644 > index 0000000..c28299c > --- /dev/null > +++ b/Documentation/devicetree/bindings/usb/richtek,rt1711h.txt > @@ -0,0 +1,38 @@ > +Richtek RT1711H Type-C Port Controller. > + > +Required properties: > +- compatible : Must be "richtek,typec_rt1711h"; > +- reg : Must be 0x4e, it's default slave address of RT1711H. > +- rt,intr_gpio : IRQ GPIO pin that's connected to RT1711H interrupt. > + > +Optional node: > +- rt,name : Name used for registering IRQ and creating kthread. > + If this property is not specified, "default" will be applied. > +- rt,def_role : Default port role (TYPEC_SINK(0) or TYPEC_SOURCE(1)). > + Set to TYPEC_NO_PREFERRED_ROLE(-1) if no default role. > + If this property is not specified, TYPEC_SINK will be applied. > +- rt,port_type : Port type (TYPEC_PORT_DFP(0), TYPEC_PORT_UFP(1), > + or TYPEC_PORT_DRP(2)). If this property is not specified, > + TYPEC_PORT_DRP will be applied. > +- rt,max_snk_mv : Maximum acceptable sink voltage in mV. > + If this property is not specified, 5000mV will be applied. > +- rt,max_snk_ma : Maximum sink current in mA. > + If this property is not specified, 3000mA will be applied. > +- rt,max_snk_mw : Maximum required sink power in mW. > + If this property is not specified, 15000mW will be applied. > +- rt,operating_snk_mw : Required operating sink power in mW. > + If this property is not specified, > + 2500mW will be applied. > +- rt,try_role_hw : True if try.{Src,Snk} is implemented in hardware. > + If this property is not specified, False will be applied. > + > +Example: > +rt1711h@4e { > + status = "ok"; > + compatible = "richtek,typec_rt1711h"; > + reg = <0x4e>; > + rt,intr_gpio = <&gpio26 0 0x0>; > + rt,name = "rt1711h"; > + rt,port_type = <2>; /* 0: DFP, 1: UFP, 2: DRP */ > + rt,def_role = <0>; /* 0: SNK, 1: SRC, -1: TYPEC_NO_PREFERRED_ROLE */ > +}; dts stuff needs to always be in a separate file so the DT maintainers can review/ack it. Split this patch up into smaller pieces please. > diff --git a/arch/arm64/boot/dts/hisilicon/rt1711h.dtsi b/arch/arm64/boot/dts/hisilicon/rt1711h.dtsi > new file mode 100644 > index 0000000..4196cc0 > --- /dev/null > +++ b/arch/arm64/boot/dts/hisilicon/rt1711h.dtsi > @@ -0,0 +1,11 @@ > +&i2c7 { > + rt1711h@4e { > + status = "ok"; > + compatible = "richtek,typec_rt1711h"; > + reg = <0x4e>; > + rt,intr_gpio = <&gpio26 0 0x0>; > + rt,name = "rt1711h"; > + rt,port_type = <2>; /* 0: DFP, 1: UFP, 2: DRP */ > + rt,def_role = <0>; /* 0: SNK, 1: SRC */ > + }; > +}; > diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig > index bcb2744..7bede0b 100644 > --- a/drivers/usb/typec/Kconfig > +++ b/drivers/usb/typec/Kconfig > @@ -56,6 +56,8 @@ if TYPEC_TCPM > > source "drivers/usb/typec/fusb302/Kconfig" > > +source "drivers/usb/typec/rt1711h/Kconfig" > + > config TYPEC_WCOVE > tristate "Intel WhiskeyCove PMIC USB Type-C PHY driver" > depends on ACPI > diff --git a/drivers/usb/typec/Makefile b/drivers/usb/typec/Makefile > index bb3138a..e3aaf3c 100644 > --- a/drivers/usb/typec/Makefile > +++ b/drivers/usb/typec/Makefile > @@ -2,6 +2,7 @@ > obj-$(CONFIG_TYPEC) += typec.o > obj-$(CONFIG_TYPEC_TCPM) += tcpm.o > obj-y += fusb302/ > +obj-$(CONFIG_TYPEC_RT1711H) += rt1711h/ Why do you need a whole directory for one file? > obj-$(CONFIG_TYPEC_WCOVE) += typec_wcove.o > obj-$(CONFIG_TYPEC_UCSI) += ucsi/ > obj-$(CONFIG_TYPEC_TPS6598X) += tps6598x.o > diff --git a/drivers/usb/typec/rt1711h/Kconfig b/drivers/usb/typec/rt1711h/Kconfig > new file mode 100644 > index 0000000..2fbfff5 > --- /dev/null > +++ b/drivers/usb/typec/rt1711h/Kconfig > @@ -0,0 +1,7 @@ > +config TYPEC_RT1711H > + tristate "Richtek RT1711H Type-C chip driver" > + depends on I2C && POWER_SUPPLY > + help > + The Richtek RT1711H Type-C chip driver that works with > + Type-C Port Controller Manager to provide USB PD and USB > + Type-C functionalities. > diff --git a/drivers/usb/typec/rt1711h/Makefile b/drivers/usb/typec/rt1711h/Makefile > new file mode 100644 > index 0000000..5fab8ae > --- /dev/null > +++ b/drivers/usb/typec/rt1711h/Makefile > @@ -0,0 +1,2 @@ > +# SPDX-License-Identifier: GPL-2.0 > +obj-$(CONFIG_TYPEC_RT1711H) += rt1711h.o > diff --git a/drivers/usb/typec/rt1711h/rt1711h.c b/drivers/usb/typec/rt1711h/rt1711h.c > new file mode 100644 > index 0000000..1aef3e8 > --- /dev/null > +++ b/drivers/usb/typec/rt1711h/rt1711h.c > @@ -0,0 +1,2241 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright 2017 Richtek Technologh Corp. > + * > + * Richtek RT1711H Type-C Chip Driver > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include This last #include should not be needed. If it does, you are doing something really wrong... > + > +#include "rt1711h.h" Why a .h file for a single .c file? > + > +#define RT1711H_DRV_VERSION "1.0.3" When code is in the kernel tree, versions mean nothing, you will note that no other USB driver has them, right? Please remove. > + > +#define LOG_BUFFER_ENTRIES 1024 > +#define LOG_BUFFER_ENTRY_SIZE 128 /* 128 char per line */ > + > +enum { > + RT1711H_DBG_LOG = 0, > + RT1711H_DBG_REGS, > + RT1711H_DBG_REG_ADDR, > + RT1711H_DBG_DATA, > + RT1711H_DBG_MAX, > +}; > + > +struct rt1711h_dbg_info { > + struct rt1711h_chip *chip; > + int id; > +}; > + > + > +struct rt1711h_chip { > + struct i2c_client *i2c; > + struct device *dev; > + uint16_t did; kernel types are u16, u32, u8, and the like, not uint16_t, those are for userspace code only. Yeah, other drivers do it, but you shouldn't :) > + int irq_gpio; > + int irq; > + char *name; > + struct tcpc_dev tcpc_dev; > + struct tcpc_config tcpc_cfg; > + struct tcpm_port *tcpm_port; > + struct regulator *vbus; > + struct extcon_dev *extcon; > + > + /* IRQ */ > + struct kthread_worker irq_worker; > + struct kthread_work irq_work; > + struct task_struct *irq_worker_task; 3 things for an irq handler? That feels wrong. > + atomic_t poll_count; Like I said before, why is this an atomic? > + struct delayed_work poll_work; > + > + /* LPM */ > + struct delayed_work wakeup_work; > + struct alarm wakeup_timer; > + struct mutex wakeup_lock; > + enum typec_cc_pull lpm_pull; > + bool wakeup_once; > + bool low_rp_duty_cntdown; > + bool cable_only; > + bool lpm; > + > + /* I2C */ > + atomic_t i2c_busy; > + atomic_t pm_suspend; Why are these atomic? You know that doesn't mean they do not need locking, right? > + > + /* psy + psy status */ > + struct power_supply *psy; > + u32 current_limit; > + u32 supply_voltage; > + > + /* lock for sharing chip states */ > + struct mutex lock; How many locks do you have in this structure? You should only need 1. > + > + /* port status */ > + bool vconn_on; > + bool vbus_on; > + bool charge_on; > + bool vbus_present; > + enum typec_cc_polarity polarity; > + enum typec_cc_status cc1; > + enum typec_cc_status cc2; > + enum typec_role pwr_role; > + bool drp_toggling; > + > +#ifdef CONFIG_DEBUG_FS > + struct dentry *dbgdir; > + struct rt1711h_dbg_info dbg_info[RT1711H_DBG_MAX]; > + struct dentry *dbg_files[RT1711H_DBG_MAX]; > + int dbg_regidx; > + struct mutex dbgops_lock; > + /* lock for log buffer access */ > + struct mutex logbuffer_lock; > + int logbuffer_head; > + int logbuffer_tail; > + u8 *logbuffer[LOG_BUFFER_ENTRIES]; > +#endif /* CONFIG_DEBUG_FS */ That's a lot of stuff jsut for debugfs. Why do you care about #define at all? The code should not. And another 2 locks? Ick, no. > +}; > + > +/* > + * Logging & debugging > + */ > + > +#ifdef CONFIG_DEBUG_FS > + > +static int rt1711h_reg_block_read(struct rt1711h_chip *chip, uint8_t reg, > + int len, uint8_t *data); > +static int rt1711h_reg_block_write(struct rt1711h_chip *chip, uint8_t reg, > + int len, const uint8_t *data); > + > +struct reg_desc { > + uint8_t addr; > + uint8_t size; > +}; > +#define DECL_REG(_addr, _size) {.addr = _addr, .size = _size} > + > +static struct reg_desc rt1711h_reg_desc[] = { > + DECL_REG(RT1711H_REG_VID, 2), > + DECL_REG(RT1711H_REG_PID, 2), > + DECL_REG(RT1711H_REG_DID, 2), > + DECL_REG(RT1711H_REG_TYPEC_REV, 2), > + DECL_REG(RT1711H_REG_PD_REV, 2), > + DECL_REG(RT1711H_REG_PDIF_REV, 2), > + DECL_REG(RT1711H_REG_ALERT, 2), > + DECL_REG(RT1711H_REG_ALERT_MASK, 2), > + DECL_REG(RT1711H_REG_POWER_STATUS_MASK, 1), > + DECL_REG(RT1711H_REG_FAULT_STATUS_MASK, 1), > + DECL_REG(RT1711H_REG_TCPC_CTRL, 1), > + DECL_REG(RT1711H_REG_ROLE_CTRL, 1), > + DECL_REG(RT1711H_REG_FAULT_CTRL, 1), > + DECL_REG(RT1711H_REG_POWER_CTRL, 1), > + DECL_REG(RT1711H_REG_CC_STATUS, 1), > + DECL_REG(RT1711H_REG_POWER_STATUS, 1), > + DECL_REG(RT1711H_REG_FAULT_STATUS, 1), > + DECL_REG(RT1711H_REG_COMMAND, 1), > + DECL_REG(RT1711H_REG_MSG_HDR_INFO, 1), > + DECL_REG(RT1711H_REG_RX_DETECT, 1), > + DECL_REG(RT1711H_REG_RX_BYTE_CNT, 1), > + DECL_REG(RT1711H_REG_RX_BUF_FRAME_TYPE, 1), > + DECL_REG(RT1711H_REG_RX_HDR, 2), > + DECL_REG(RT1711H_REG_RX_DATA, 1), > + DECL_REG(RT1711H_REG_TRANSMIT, 1), > + DECL_REG(RT1711H_REG_TX_BYTE_CNT, 1), > + DECL_REG(RT1711H_REG_TX_HDR, 2), > + DECL_REG(RT1711H_REG_TX_DATA, 1), > + DECL_REG(RT1711H_REG_CLK_CTRL2, 1), > + DECL_REG(RT1711H_REG_CLK_CTRL3, 1), > + DECL_REG(RT1711H_REG_BMC_CTRL, 1), > + DECL_REG(RT1711H_REG_BMCIO_RXDZSEL, 1), > + DECL_REG(RT1711H_REG_VCONN_CLIMITEN, 1), > + DECL_REG(RT1711H_REG_RT_STATUS, 1), > + DECL_REG(RT1711H_REG_RT_INT, 1), > + DECL_REG(RT1711H_REG_RT_MASK, 1), > + DECL_REG(RT1711H_REG_IDLE_CTRL, 1), > + DECL_REG(RT1711H_REG_INTRST_CTRL, 1), > + DECL_REG(RT1711H_REG_WATCHDOG_CTRL, 1), > + DECL_REG(RT1711H_REG_I2CRST_CTRL, 1), > + DECL_REG(RT1711H_REG_SWRESET, 1), > + DECL_REG(RT1711H_REG_TTCPC_FILTER, 1), > + DECL_REG(RT1711H_REG_DRP_TOGGLE_CYCLE, 1), > + DECL_REG(RT1711H_REG_DRP_DUTY_CTRL, 1), > + DECL_REG(RT1711H_REG_BMCIO_RXDZEN, 1), > +}; > + > +static const char *rt1711h_dbg_filename[RT1711H_DBG_MAX] = { > + "log", "regs", "reg_addr", "data", > +}; > + > +static bool rt1711h_log_full(struct rt1711h_chip *chip) > +{ > + return chip->logbuffer_tail == > + (chip->logbuffer_head + 1) % LOG_BUFFER_ENTRIES; > +} > + > +static void _rt1711h_log(struct rt1711h_chip *chip, const char *fmt, > + va_list args) > +{ > + char tmpbuffer[LOG_BUFFER_ENTRY_SIZE]; > + u64 ts_nsec = local_clock(); > + unsigned long rem_nsec; > + > + if (!chip->logbuffer[chip->logbuffer_head]) { > + chip->logbuffer[chip->logbuffer_head] = > + devm_kzalloc(chip->dev, LOG_BUFFER_ENTRY_SIZE, GFP_KERNEL); > + if (!chip->logbuffer[chip->logbuffer_head]) > + return; > + } > + > + vsnprintf(tmpbuffer, sizeof(tmpbuffer), fmt, args); > + > + mutex_lock(&chip->logbuffer_lock); > + > + if (rt1711h_log_full(chip)) { > + chip->logbuffer_head = max(chip->logbuffer_head - 1, 0); > + strlcpy(tmpbuffer, "overflow", sizeof(tmpbuffer)); > + } > + > + if (chip->logbuffer_head < 0 || > + chip->logbuffer_head >= LOG_BUFFER_ENTRIES) { > + dev_warn(chip->dev, "%s bad log buffer index %d\n", __func__, > + chip->logbuffer_head); > + goto abort; > + } > + > + if (!chip->logbuffer[chip->logbuffer_head]) { > + dev_warn(chip->dev, "%s log buffer index %d is NULL\n", > + __func__, chip->logbuffer_head); > + goto abort; > + } > + > + rem_nsec = do_div(ts_nsec, 1000000000); > + scnprintf(chip->logbuffer[chip->logbuffer_head], LOG_BUFFER_ENTRY_SIZE, > + "[%5lu.%06lu] %s", (unsigned long)ts_nsec, rem_nsec / 1000, > + tmpbuffer); > + chip->logbuffer_head = (chip->logbuffer_head + 1) % LOG_BUFFER_ENTRIES; > + > +abort: > + mutex_unlock(&chip->logbuffer_lock); > +} > + > +static void rt1711h_log(struct rt1711h_chip *chip, > + const char *fmt, ...) > +{ > + va_list args; > + > + va_start(args, fmt); > + _rt1711h_log(chip, fmt, args); > + va_end(args); > +} > + > +static int rt1711h_log_show(struct rt1711h_chip *chip, struct seq_file *s) > +{ > + int tail; > + > + mutex_lock(&chip->logbuffer_lock); > + tail = chip->logbuffer_tail; > + while (tail != chip->logbuffer_head) { > + seq_printf(s, "%s", chip->logbuffer[tail]); > + tail = (tail + 1) % LOG_BUFFER_ENTRIES; > + } > + if (!seq_has_overflowed(s)) > + chip->logbuffer_tail = tail; > + mutex_unlock(&chip->logbuffer_lock); > + > + return 0; > +} > + > +static int rt1711h_regs_show(struct rt1711h_chip *chip, struct seq_file *s) > +{ > + int ret = 0; > + int i = 0, j = 0; > + struct reg_desc *desc = NULL; > + uint8_t regval[2] = {0}; > + > + for (i = 0; i < ARRAY_SIZE(rt1711h_reg_desc); i++) { > + desc = &rt1711h_reg_desc[i]; > + ret = rt1711h_reg_block_read(chip, desc->addr, desc->size, > + regval); > + if (ret < 0) { > + dev_err(chip->dev, "%s read reg0x%02X fail\n", > + __func__, desc->addr); > + continue; > + } > + > + seq_printf(s, "reg0x%02x:0x", desc->addr); > + for (j = 0; j < desc->size; j++) > + seq_printf(s, "%02x,", regval[j]); > + seq_puts(s, "\n"); > + } > + > + return 0; > +} > + > +static inline int rt1711h_reg_addr_show(struct rt1711h_chip *chip, > + struct seq_file *s) > +{ > + struct reg_desc *desc = &rt1711h_reg_desc[chip->dbg_regidx]; > + > + seq_printf(s, "0x%02x\n", desc->addr); > + return 0; > +} > + > +static inline int rt1711h_data_show(struct rt1711h_chip *chip, > + struct seq_file *s) > +{ > + int ret = 0, i = 0; > + struct reg_desc *desc = &rt1711h_reg_desc[chip->dbg_regidx]; > + uint8_t regval[2] = {0}; > + > + ret = rt1711h_reg_block_read(chip, desc->addr, desc->size, regval); > + if (ret < 0) > + return ret; > + > + seq_printf(s, "reg0x%02x=0x", desc->addr); > + for (i = 0; i < desc->size; i++) > + seq_printf(s, "%02x,", regval[i]); > + seq_puts(s, "\n"); > + return 0; > +} > + > +static int rt1711h_dbg_show(struct seq_file *s, void *v) > +{ > + int ret = 0; > + struct rt1711h_dbg_info *info = (struct rt1711h_dbg_info *)s->private; > + struct rt1711h_chip *chip = info->chip; > + > + mutex_lock(&chip->dbgops_lock); > + switch (info->id) { > + case RT1711H_DBG_LOG: > + ret = rt1711h_log_show(chip, s); > + break; > + case RT1711H_DBG_REGS: > + ret = rt1711h_regs_show(chip, s); > + break; > + case RT1711H_DBG_REG_ADDR: > + ret = rt1711h_reg_addr_show(chip, s); > + break; > + case RT1711H_DBG_DATA: > + ret = rt1711h_data_show(chip, s); > + break; > + default: > + ret = -EINVAL; > + break; > + } > + > + mutex_unlock(&chip->dbgops_lock); > + return ret; > +} > + > +static int rt1711h_dbg_open(struct inode *inode, struct file *file) > +{ > + if (file->f_mode & FMODE_READ) > + return single_open(file, rt1711h_dbg_show, inode->i_private); > + file->private_data = inode->i_private; > + return 0; > +} > + > +static int get_parameters(char *buf, long int *param1, int num_of_par) > +{ > + char *token; > + int base, cnt; > + > + token = strsep(&buf, " "); > + > + for (cnt = 0; cnt < num_of_par; cnt++) { > + if (token != NULL) { > + if ((token[1] == 'x') || (token[1] == 'X')) > + base = 16; > + else > + base = 10; > + > + if (kstrtoul(token, base, ¶m1[cnt]) != 0) > + return -EINVAL; > + > + token = strsep(&buf, " "); > + } else > + return -EINVAL; > + } > + return 0; > +} What is this function doing? What is your debugfs files for? > + > +static int get_datas(const char *buf, const int length, > + unsigned char *data_buffer, unsigned char data_length) > +{ > + int i, ptr; > + long int value; > + char token[5]; > + > + token[0] = '0'; > + token[1] = 'x'; > + token[4] = 0; > + if (buf[0] != '0' || buf[1] != 'x') > + return -EINVAL; > + > + ptr = 2; > + for (i = 0; (i < data_length) && (ptr + 2 <= length); i++) { > + token[2] = buf[ptr++]; > + token[3] = buf[ptr++]; > + ptr++; > + if (kstrtoul(token, 16, &value) != 0) > + return -EINVAL; > + data_buffer[i] = value; > + } > + return 0; > +} > + > +static int rt1711h_regaddr2idx(uint8_t reg_addr) > +{ > + int i = 0; > + struct reg_desc *desc = NULL; > + > + for (i = 0; i < ARRAY_SIZE(rt1711h_reg_desc); i++) { > + desc = &rt1711h_reg_desc[i]; > + if (desc->addr == reg_addr) > + return i; > + } > + return -EINVAL; > +} > + > +static ssize_t rt1711h_dbg_write(struct file *file, const char __user *ubuf, > + size_t count, loff_t *ppos) > +{ > + int ret = 0; > + struct rt1711h_dbg_info *info = > + (struct rt1711h_dbg_info *)file->private_data; > + struct rt1711h_chip *chip = info->chip; > + struct reg_desc *desc = NULL; > + char lbuf[128]; > + long int param[5]; > + unsigned char reg_data[2] = {0}; > + > + if (count > sizeof(lbuf) - 1) > + return -EFAULT; > + > + ret = copy_from_user(lbuf, ubuf, count); > + if (ret) > + return -EFAULT; > + lbuf[count] = '\0'; > + > + mutex_lock(&chip->dbgops_lock); > + switch (info->id) { > + case RT1711H_DBG_REG_ADDR: > + ret = get_parameters(lbuf, param, 1); > + if (ret < 0) { > + dev_err(chip->dev, "%s get param fail\n", __func__); > + ret = -EINVAL; > + goto out; > + } > + ret = rt1711h_regaddr2idx(param[0]); > + if (ret < 0) { > + dev_err(chip->dev, "%s addr2idx fail\n", __func__); > + ret = -EINVAL; > + goto out; > + } > + chip->dbg_regidx = ret; > + break; > + case RT1711H_DBG_DATA: > + desc = &rt1711h_reg_desc[chip->dbg_regidx]; > + if ((desc->size - 1) * 3 + 5 != count) { > + dev_err(chip->dev, "%s incorrect input length\n", > + __func__); > + ret = -EINVAL; > + goto out; > + } > + ret = get_datas((char *)ubuf, count, reg_data, desc->size); > + if (ret < 0) { > + dev_err(chip->dev, "%s get data fail\n", __func__); > + ret = -EINVAL; > + goto out; > + } > + ret = rt1711h_reg_block_write(chip, desc->addr, desc->size, > + reg_data); > + break; > + default: > + ret = -EINVAL; > + break; > + } > + > +out: > + mutex_unlock(&chip->dbgops_lock); > + return ret < 0 ? ret : count; > +} > + > +static int rt1711h_dbg_release(struct inode *inode, struct file *file) > +{ > + if (file->f_mode & FMODE_READ) > + return single_release(inode, file); > + return 0; > +} > + > +static const struct file_operations rt1711h_dbg_ops = { > + .open = rt1711h_dbg_open, > + .llseek = seq_lseek, > + .read = seq_read, > + .write = rt1711h_dbg_write, > + .release = rt1711h_dbg_release, > +}; > + > + > +static int rt1711h_debugfs_init(struct rt1711h_chip *chip) > +{ > + int ret = 0, i = 0; > + struct rt1711h_dbg_info *info = NULL; > + int len = 0; > + char *dirname = NULL; > + > + mutex_init(&chip->logbuffer_lock); > + mutex_init(&chip->dbgops_lock); > + len = strlen(dev_name(chip->dev)); > + dirname = devm_kzalloc(chip->dev, len + 9, GFP_KERNEL); > + if (!dirname) > + return -ENOMEM; > + snprintf(dirname, len + 9, "rt1711h-%s", dev_name(chip->dev)); > + if (!chip->dbgdir) { > + chip->dbgdir = debugfs_create_dir(dirname, NULL); > + if (!chip->dbgdir) > + return -ENOMEM; No need to ever check the return value of debugfs_ calls, you should not care and can always use the value to any future debugfs calls, if you really need it. > + } > + > + for (i = 0; i < RT1711H_DBG_MAX; i++) { > + info = &chip->dbg_info[i]; static array of debug info? That feels odd. > + info->chip = chip; > + info->id = i; > + chip->dbg_files[i] = debugfs_create_file( > + rt1711h_dbg_filename[i], S_IFREG | 0444, > + chip->dbgdir, info, &rt1711h_dbg_ops); > + if (!chip->dbg_files[i]) { > + ret = -EINVAL; Like here, you don't need this, and you don't need to care about the return value. > + goto err; > + } > + } > + > + return 0; > +err: > + debugfs_remove_recursive(chip->dbgdir); > + return ret; Why do you care about an error here? Your code should not do anything different if debugfs stuff does not work or if it does. It's debugging only. > +} > + > +static void rt1711h_debugfs_exit(struct rt1711h_chip *chip) > +{ > + debugfs_remove_recursive(chip->dbgdir); See, you didn't need those file handles :) thanks, greg k-h