RE: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver
From: shufan_lee(李書帆)
Date: Thu Jan 18 2018 - 08:13:39 EST
Dear Gerg,
Many thanks to your comment.
I've checked all of them and here are some questions need your help.
> +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.
Ok, I'll split it into two patches, one for source code and once for dts related files.
========================================================================
> 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?
Is the suggestion to move rt1711h.c to the same directory level as tcpm? i.e. drivers/usb/typec/rt1711h.c
========================================================================
> +<uapi/linux/sched/types.h>
This last #include should not be needed. If it does, you are doing something really wrong...
Ok, this #include will be removed
========================================================================
> +
> +#include "rt1711h.h"
Why a .h file for a single .c file?
Is the suggestion to move all content in rt1711h.h into rt1711h.c?
========================================================================
> +
> +#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.
Ok, this will be removed.
========================================================================
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 :)
Ok, I'll use u16, u32 and u8 instead of uint16_t, uint32_t and uint8_t
========================================================================
> +/* 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?
I'll use threaded_irq instead, the above things will be removed.
========================================================================
> +/* 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?
For my understanding, a single operation on atomic_t doesn't need lock, like a single atomic_set.
But two consecutive operations doesn't guarantee anything. Like atomic_set followed by an atomic_read.
This part is referenced from fusb302 used to make sure I2C is idle before system suspends.
It only needs to guarantee a single read/write on these variable is atomic operation, so atomic is used.
========================================================================
> +};
> +
> +/*
> + * 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?
There are 4 debug files.
First(log) is for logging which needs a lock for log buffer. The way to log is referenced from fusb302 and tcpm.
Second(regs) is used to dump all register of rt1711h.
Third(reg_addr)&Forth(data) are used to write/read a register specified in reg_addr.
The reason to create these debug files is to make issue support easier.
========================================================================
> +#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.
Is the suggestion to remove #ifdef CONFIG_DEBUG_FS?
And another 2 locks? Ick, no.
dbgops_lock is used to prevent user from accessing different debug files simultaneously.
Is the suggestion to use the lock of the following one?
> +/* lock for sharing chip states */
> +struct mutex lock;
========================================================================
> +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.
If it is NULL without checking and we use it in debugfs_create_file, all the debug files will be created in the root of the debugfs filesystem.
Is this correct?
========================================================================
> +for (i = 0; i < RT1711H_DBG_MAX; i++) {
> +info = &chip->dbg_info[i];
static array of debug info? That feels odd.
Is the suggestion to use pointer of array and dynamically allocated it?
========================================================================
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.
Ok, this will be removed.
========================================================================
Best Regards,
*****************************
Shu-Fan Lee
Richtek Technology Corporation
TEL: +886-3-5526789 #2359
FAX: +886-3-5526612
*****************************
-----Original Message-----
From: Greg KH [mailto:greg@xxxxxxxxx]
Sent: Wednesday, January 17, 2018 9:42 PM
To: ShuFanLee
Cc: heikki.krogerus@xxxxxxxxxxxxxxx; cy_huang(黃啟原); shufan_lee(李書帆); linux-kernel@xxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx
Subject: Re: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver
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_ENTRIES1024
> +#define LOG_BUFFER_ENTRY_SIZE128 /* 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
************* Email Confidentiality Notice ********************
The information contained in this e-mail message (including any attachments) may be confidential, proprietary, privileged, or otherwise exempt from disclosure under applicable laws. It is intended to be conveyed only to the designated recipient(s). Any use, dissemination, distribution, printing, retaining or copying of this e-mail (including its attachments) by unintended recipient(s) is strictly prohibited and may be unlawful. If you are not an intended recipient of this e-mail, or believe that you have received this e-mail in error, please notify the sender immediately (by replying to this e-mail), delete any and all copies of this e-mail (including any attachments) from your system, and do not disclose the content of this e-mail to any other person. Thank you!