Re: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

[Greg KH <greg@kroah.com>]

On Wed, Jan 10, 2018 at 02:59:12PM +0800, ShuFanLee wrote:
> From: ShuFanLee <shufan_lee@richtek.com>
> 
> 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 <shufan_lee@richtek.com>

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 <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/version.h>
> +#include <linux/err.h>
> +#include <linux/debugfs.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/i2c.h>
> +#include <linux/usb/typec.h>
> +#include <linux/usb/tcpm.h>
> +#include <linux/usb/pd.h>
> +#include <linux/of_gpio.h>
> +#include <linux/of.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/power_supply.h>
> +#include <linux/extcon.h>
> +#include <linux/workqueue.h>
> +#include <linux/kthread.h>
> +#include <linux/cpu.h>
> +#include <linux/alarmtimer.h>
> +#include <linux/sched/clock.h>
> +#include <uapi/linux/sched/types.h>

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, &param1[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