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

From: Greg KH
Date: Wed Jan 17 2018 - 08:42:35 EST

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

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