| Message ID | 1673256674-25165-1-git-send-email-gene_chen@richtek.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [1/2] dt-bindings usb: typec: rt1718s: Add binding for Richtek RT1718S | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On 09/01/2023 10:31, gene_chen@richtek.com wrote: > From: Gene Chen <gene_chen@richtek.com> > Subject: drop second, redundant "binding for". > Add binding for Richtek RT1718s > > Signed-off-by: Gene Chen <gene_chen@richtek.com> > --- > .../devicetree/bindings/usb/richtek,rt1718s.yaml | 98 ++++++++++++++++++++++ > 1 file changed, 98 insertions(+) > create mode 100644 Documentation/devicetree/bindings/usb/richtek,rt1718s.yaml > > diff --git a/Documentation/devicetree/bindings/usb/richtek,rt1718s.yaml b/Documentation/devicetree/bindings/usb/richtek,rt1718s.yaml > new file mode 100644 > index 00000000..7797fc6 > --- /dev/null > +++ b/Documentation/devicetree/bindings/usb/richtek,rt1718s.yaml > @@ -0,0 +1,98 @@ > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > +%YAML 1.2 > +--- > +$id: "http://devicetree.org/schemas/usb/richtek,rt1718s.yaml#" > +$schema: "http://devicetree.org/meta-schemas/core.yaml#" Drop quotes from both. > + > +title: Richtek RT1718S Type-C Port Switch and Power Delivery controller > + > +maintainers: > + - Gene Chen <gene_chen@richtek.com> > + > +description: | > + The RT1718S is a USB Type-C controller that complies with the latest > + USB Type-C and PD standards. It does the USB Type-C detection including attach > + and orientation. It integrates the physical layer of the USB BMC power > + delivery protocol to allow up to 100W of power. The BMC PD block enables full > + support for alternative interfaces of the Type-C specification. > + > +properties: > + compatible: > + enum: > + - richtek,rt1718s > + > + reg: > + maxItems: 1 > + > + interrupts: > + maxItems: 1 > + > + wakeup-source: > + description: enable IRQ remote wakeup, see power/wakeup-source.txt Drop description, you are copying generic description. > + type: boolean Drop. Just wakeup-soource: true > + > + connector: > + type: object > + $ref: ../connector/usb-connector.yaml# Full path, so /schemas/usb/connector .... > + description: > + Properties for usb c connector. That's not accurate description. Everything in properties is a property, so no need to say that properties are properties. Actually this looks the same as existing rt1711, so please do not duplicate stuff. Especially, do not duplicate mistakes... > + > +additionalProperties: false > + > +required: > + - compatible > + - reg > + - connector > + - interrupts > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/irq.h> > + #include <dt-bindings/usb/pd.h> > + i2c0 { i2c > + #address-cells = <1>; > + #size-cells = <0>; > + > + rt1718s@43 { Node names should be generic. https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation Best regards, Krzysztof
On 09/01/2023 10:31, gene_chen@richtek.com wrote: > From: Gene Chen <gene_chen@richtek.com> > > Richtek RT1718S is highly integrated TCPC and Power Delivery (PD) > controller with IEC-ESD Protection on SBU/CC/DP/DM, USB2.0 Switch, > Charging Port Controller and Power-Path Control. > > Signed-off-by: Gene Chen <gene_chen@richtek.com> > --- > drivers/usb/typec/tcpm/Kconfig | 9 + > drivers/usb/typec/tcpm/Makefile | 1 + > drivers/usb/typec/tcpm/tcpci_rt1718s.c | 349 +++++++++++++++++++++++++++++++++ > 3 files changed, 359 insertions(+) > create mode 100644 drivers/usb/typec/tcpm/tcpci_rt1718s.c > > diff --git a/drivers/usb/typec/tcpm/Kconfig b/drivers/usb/typec/tcpm/Kconfig > index e6b88ca..f0efb34 100644 > --- a/drivers/usb/typec/tcpm/Kconfig > +++ b/drivers/usb/typec/tcpm/Kconfig > @@ -27,6 +27,15 @@ config TYPEC_RT1711H > Type-C Port Controller Manager to provide USB PD and USB > Type-C functionalities. > > +config TYPEC_RT1718S > + tristate "Richtek RT1718S Type-C chip driver" > + depends on I2C > + help > + Richtek RT1718S Type-C chip driver that works with > + Type-C Port Controller Manager to provide USB PD and USB > + Type-C functionalities. > + Additionally, it supports BC1.2 and power-path control. > + > config TYPEC_MT6360 > tristate "Mediatek MT6360 Type-C driver" > depends on MFD_MT6360 > diff --git a/drivers/usb/typec/tcpm/Makefile b/drivers/usb/typec/tcpm/Makefile > index 906d9dc..db33ffc 100644 > --- a/drivers/usb/typec/tcpm/Makefile > +++ b/drivers/usb/typec/tcpm/Makefile > @@ -5,6 +5,7 @@ obj-$(CONFIG_TYPEC_WCOVE) += typec_wcove.o > typec_wcove-y := wcove.o > obj-$(CONFIG_TYPEC_TCPCI) += tcpci.o > obj-$(CONFIG_TYPEC_RT1711H) += tcpci_rt1711h.o > +obj-$(CONFIG_TYPEC_RT1718S) += tcpci_rt1718s.o > obj-$(CONFIG_TYPEC_MT6360) += tcpci_mt6360.o > obj-$(CONFIG_TYPEC_TCPCI_MT6370) += tcpci_mt6370.o > obj-$(CONFIG_TYPEC_TCPCI_MAXIM) += tcpci_maxim.o > diff --git a/drivers/usb/typec/tcpm/tcpci_rt1718s.c b/drivers/usb/typec/tcpm/tcpci_rt1718s.c > new file mode 100644 > index 00000000..305b39c > --- /dev/null > +++ b/drivers/usb/typec/tcpm/tcpci_rt1718s.c > @@ -0,0 +1,349 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2020 richtek Inc. > + * > + * Author: ChiYuan Huang <cy_huang@richtek.com> > + */ > + > +#include <linux/gpio/consumer.h> > +#include <linux/i2c.h> > +#include <linux/interrupt.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > +#include <linux/regulator/consumer.h> > +#include <linux/usb/tcpci.h> > +#include <linux/usb/tcpm.h> > +#include <linux/usb/pd_vdo.h> > + > +#define RT1718S_VID 0x29CF > +#define RT1718S_PID 0x1718 > + > +#define RT1718S_P1PREFIX 0x00 > +#define RT1718S_P1START (RT1718S_P1PREFIX << 8) > +#define RT1718S_P1END ((RT1718S_P1PREFIX << 8) + 0xFF) > +#define RT1718S_P2PREFIX 0xF2 > +#define RT1718S_P2START (RT1718S_P2PREFIX << 8) > +#define RT1718S_P2END ((RT1718S_P2PREFIX << 8) + 0xFF) > + > +#define RT1718S_SYS_CTRL3 0xB0 > +#define RT1718S_SWRESET_MASK BIT(0) > + > +struct rt1718s_chip { > + struct tcpci_data tdata; > + struct tcpci *tcpci; > + struct device *dev; > + struct regulator *vbus; > + bool src_en; > +}; > + > +static bool rt1718s_is_readwrite_reg(struct device *dev, unsigned int reg) > +{ > + switch (reg) { > + case RT1718S_P1START ... RT1718S_P1END: > + fallthrough; > + case RT1718S_P2START ... RT1718S_P2END: > + return true; > + } > + > + return false; > +} > + > +static const struct regmap_config rt1718s_regmap_config = { > + .reg_bits = 16, > + .val_bits = 8, > + > + .reg_format_endian = REGMAP_ENDIAN_BIG, > + > + /* page 1(TCPC) : 0x00 ~ 0xff, page 2 : 0xf200 ~0xf2ff */ > + .max_register = RT1718S_P2END, > + .writeable_reg = rt1718s_is_readwrite_reg, > + .readable_reg = rt1718s_is_readwrite_reg, > +}; > + > +static int rt1718s_regmap_read(void *context, const void *reg, size_t reg_size, > + void *val, size_t val_size) This is not a regmap read... Use true regmap or explain why you cannot use it. > +{ > + struct device *dev = context; > + struct i2c_client *i2c = to_i2c_client(dev); > + struct i2c_msg xfer[2]; > + int ret; > + > + xfer[0].addr = i2c->addr; > + xfer[0].flags = 0; > + xfer[0].len = reg_size; > + xfer[0].buf = (u8 *)reg; > + > + if (*(u8 *)reg == RT1718S_P1PREFIX) { > + xfer[0].len = 1, > + xfer[0].buf = (u8 *)(reg + 1); > + } > + > + xfer[1].addr = i2c->addr; > + xfer[1].flags = I2C_M_RD; > + xfer[1].len = val_size; > + xfer[1].buf = (u8 *)val; > + > + ret = i2c_transfer(i2c->adapter, xfer, 2); > + //pr_info("wtf i2c_read [0x%04x]:0x%02x\n", *(u16 *)(reg), *(u8 *)val); That's not a code suitable for mainline submission. > + if (ret < 0) > + return ret; > + else if (ret != 2) > + return -EIO; > + > + return 0; > +} > + > +static int rt1718s_regmap_write(void *context, const void *val, size_t val_size) > +{ > + struct device *dev = context; > + struct i2c_client *i2c = to_i2c_client(dev); > + struct i2c_msg xfer; > + int ret; > + > + xfer.addr = i2c->addr; > + xfer.flags = 0; > + xfer.len = val_size; > + xfer.buf = (u8 *)val; > + > + if (*(u8 *)val == RT1718S_P1PREFIX) { > + xfer.len = val_size - 1; > + xfer.buf = (u8 *)(val + 1); > + } > + > + ret = i2c_transfer(i2c->adapter, &xfer, 1); > + //pr_info("wtf i2c_write [0x%04x]:0x%02x\n", *(u16 *)(val), *(u8 *)(val+2)); > + if (ret < 0) > + return ret; > + if (ret != 1) > + return -EIO; > + > + return 0; > +} > + > +static const struct regmap_bus rt1718s_regmap_bus = { > + .read = rt1718s_regmap_read, > + .write = rt1718s_regmap_write, > +}; > + > +static const struct reg_sequence rt1718s_init_settings[] = { > + /* config I2C timeout reset enable , and timeout to 200ms */ > + { 0xBF, 0x8F, 0 }, > + /* config CC Detect Debounce : 250us (25*val us) */ > + { 0xB1, 0x0A, 0 }, > + /* DRP Toggle Cycle : 76.8ms (51.2 + 6.4*val ms) */ > + { 0xB2, 0x04, 0 }, > + /* DRP Duyt Ctrl : dcSRC: 331/1024 ((val+1)/1024) */ > + { 0xB3, 0x4A, 0 }, > + { 0xB4, 0x01, 0 }, > + /* Enable VCONN Current Limit function */ > + { 0x8C, 0x41, 0 }, > + /* Enable cc open 40ms when pmic send vsysuv signal */ > + { 0xCA, 0xB3, 0 }, > + /* Set GPIO2 push-pull, output-low */ > + { 0xEE, 0x0C, 0}, > + /* bg en, low power en, vbus valid detect off, vbus present on, osc off */ > + { 0xB8, 0x1A, 0}, > + /* Link GPIO2 source default vbus to TCPC command */ > + { 0xEB, 0x08, 0}, > + /* Set GPIO2 vbus path */ > + { 0xEC, 0x8E, 0 }, > + /* auto low power timer 2.5s, auto low power en, auto low power mode */ > + { 0xF210, 0x35, 0 }, > + /* Set shipping mode off, AUTOIDLE on with timeout 96ms */ > + { 0x8F, 0x7F, 0 }, > +}; > + > +static int rt1718s_init(struct tcpci *tcpci, struct tcpci_data *tdata) > +{ > + int ret; > + > + ret = regmap_register_patch(tdata->regmap, rt1718s_init_settings, > + ARRAY_SIZE(rt1718s_init_settings)); > + pr_info("%s: [TCPC-] ret=%d\n", __func__, ret); > + return ret; > +} > + > +static int rt1718s_set_vbus(struct tcpci *tcpci, struct tcpci_data *tdata, > + bool src, bool snk) > +{ > + struct rt1718s_chip *chip = container_of(tdata, struct rt1718s_chip, tdata); > + int ret; > + > + if (chip->src_en == src) > + return 0; > + > + if (src) > + ret = regulator_enable(chip->vbus); > + else > + ret = regulator_disable(chip->vbus); > + > + if (!ret) > + chip->src_en = src; > + return ret; > +} > + > +static irqreturn_t rt1718s_irq(int irq, void *dev_id) > +{ > + struct rt1718s_chip *chip = dev_id; > + > + return tcpci_irq(chip->tcpci); > +} > + > +static int rt1718s_sw_reset(struct rt1718s_chip *chip) > +{ > + int ret; > + > + ret = regmap_update_bits(chip->tdata.regmap, RT1718S_SYS_CTRL3, > + RT1718S_SWRESET_MASK, RT1718S_SWRESET_MASK); > + if (ret < 0) > + return ret; > + > + /* Wait for IC to reset done*/ > + usleep_range(1000, 2000); > + This entire code is so close to rt1711, that it does not make much sense to keep two drivers. Please work on existing driver to improve it - clean it up, add regmap patch maybe also regmap fields. Then add rt1718 support to it. Best regards, Krzysztof
On 1/9/23 01:31, gene_chen@richtek.com wrote: > From: Gene Chen <gene_chen@richtek.com> > > Richtek RT1718S is highly integrated TCPC and Power Delivery (PD) > controller with IEC-ESD Protection on SBU/CC/DP/DM, USB2.0 Switch, > Charging Port Controller and Power-Path Control. > > Signed-off-by: Gene Chen <gene_chen@richtek.com> > --- > drivers/usb/typec/tcpm/Kconfig | 9 + > drivers/usb/typec/tcpm/Makefile | 1 + > drivers/usb/typec/tcpm/tcpci_rt1718s.c | 349 +++++++++++++++++++++++++++++++++ > 3 files changed, 359 insertions(+) > create mode 100644 drivers/usb/typec/tcpm/tcpci_rt1718s.c > > diff --git a/drivers/usb/typec/tcpm/Kconfig b/drivers/usb/typec/tcpm/Kconfig > index e6b88ca..f0efb34 100644 > --- a/drivers/usb/typec/tcpm/Kconfig > +++ b/drivers/usb/typec/tcpm/Kconfig > @@ -27,6 +27,15 @@ config TYPEC_RT1711H > Type-C Port Controller Manager to provide USB PD and USB > Type-C functionalities. > > +config TYPEC_RT1718S > + tristate "Richtek RT1718S Type-C chip driver" > + depends on I2C > + help > + Richtek RT1718S Type-C chip driver that works with > + Type-C Port Controller Manager to provide USB PD and USB > + Type-C functionalities. > + Additionally, it supports BC1.2 and power-path control. > + > config TYPEC_MT6360 > tristate "Mediatek MT6360 Type-C driver" > depends on MFD_MT6360 > diff --git a/drivers/usb/typec/tcpm/Makefile b/drivers/usb/typec/tcpm/Makefile > index 906d9dc..db33ffc 100644 > --- a/drivers/usb/typec/tcpm/Makefile > +++ b/drivers/usb/typec/tcpm/Makefile > @@ -5,6 +5,7 @@ obj-$(CONFIG_TYPEC_WCOVE) += typec_wcove.o > typec_wcove-y := wcove.o > obj-$(CONFIG_TYPEC_TCPCI) += tcpci.o > obj-$(CONFIG_TYPEC_RT1711H) += tcpci_rt1711h.o > +obj-$(CONFIG_TYPEC_RT1718S) += tcpci_rt1718s.o > obj-$(CONFIG_TYPEC_MT6360) += tcpci_mt6360.o > obj-$(CONFIG_TYPEC_TCPCI_MT6370) += tcpci_mt6370.o > obj-$(CONFIG_TYPEC_TCPCI_MAXIM) += tcpci_maxim.o > diff --git a/drivers/usb/typec/tcpm/tcpci_rt1718s.c b/drivers/usb/typec/tcpm/tcpci_rt1718s.c > new file mode 100644 > index 00000000..305b39c > --- /dev/null > +++ b/drivers/usb/typec/tcpm/tcpci_rt1718s.c > @@ -0,0 +1,349 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2020 richtek Inc. > + * > + * Author: ChiYuan Huang <cy_huang@richtek.com> > + */ > + > +#include <linux/gpio/consumer.h> > +#include <linux/i2c.h> > +#include <linux/interrupt.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > +#include <linux/regulator/consumer.h> > +#include <linux/usb/tcpci.h> > +#include <linux/usb/tcpm.h> > +#include <linux/usb/pd_vdo.h> > + > +#define RT1718S_VID 0x29CF > +#define RT1718S_PID 0x1718 > + > +#define RT1718S_P1PREFIX 0x00 > +#define RT1718S_P1START (RT1718S_P1PREFIX << 8) > +#define RT1718S_P1END ((RT1718S_P1PREFIX << 8) + 0xFF) > +#define RT1718S_P2PREFIX 0xF2 > +#define RT1718S_P2START (RT1718S_P2PREFIX << 8) > +#define RT1718S_P2END ((RT1718S_P2PREFIX << 8) + 0xFF) > + > +#define RT1718S_SYS_CTRL3 0xB0 > +#define RT1718S_SWRESET_MASK BIT(0) > + > +struct rt1718s_chip { > + struct tcpci_data tdata; > + struct tcpci *tcpci; > + struct device *dev; > + struct regulator *vbus; > + bool src_en; > +}; > + > +static bool rt1718s_is_readwrite_reg(struct device *dev, unsigned int reg) > +{ > + switch (reg) { > + case RT1718S_P1START ... RT1718S_P1END: > + fallthrough; Unnecessary (and bad indentation). > + case RT1718S_P2START ... RT1718S_P2END: > + return true; > + } > + > + return false; > +} > + > +static const struct regmap_config rt1718s_regmap_config = { > + .reg_bits = 16, > + .val_bits = 8, > + > + .reg_format_endian = REGMAP_ENDIAN_BIG, > + > + /* page 1(TCPC) : 0x00 ~ 0xff, page 2 : 0xf200 ~0xf2ff */ > + .max_register = RT1718S_P2END, > + .writeable_reg = rt1718s_is_readwrite_reg, > + .readable_reg = rt1718s_is_readwrite_reg, > +}; > + > +static int rt1718s_regmap_read(void *context, const void *reg, size_t reg_size, > + void *val, size_t val_size) > +{ > + struct device *dev = context; > + struct i2c_client *i2c = to_i2c_client(dev); > + struct i2c_msg xfer[2]; > + int ret; > + > + xfer[0].addr = i2c->addr; > + xfer[0].flags = 0; > + xfer[0].len = reg_size; > + xfer[0].buf = (u8 *)reg; Unnecessary typecast > + > + if (*(u8 *)reg == RT1718S_P1PREFIX) { > + xfer[0].len = 1, > + xfer[0].buf = (u8 *)(reg + 1); Pointer arithmetic on void * (see below). > + } There is a lot of context here which needs explanation. The code extracts the upper 8 bit of the register address and drops it if the value is 0. So the address is either 8 bit or 16 bit depending on the register address. Also, this only works because reg_format_endian is set to big endian. This really needs to be documented. Assigning the values to len and buf twice is really bad style. Please use if/else. > + > + xfer[1].addr = i2c->addr; > + xfer[1].flags = I2C_M_RD; > + xfer[1].len = val_size; > + xfer[1].buf = (u8 *)val; This typecast should be unnecessary. > + > + ret = i2c_transfer(i2c->adapter, xfer, 2); > + //pr_info("wtf i2c_read [0x%04x]:0x%02x\n", *(u16 *)(reg), *(u8 *)val); No code comments in release code. > + if (ret < 0) > + return ret; > + else if (ret != 2) else after return is unencessary. > + return -EIO; > + > + return 0; > +} > + > +static int rt1718s_regmap_write(void *context, const void *val, size_t val_size) > +{ > + struct device *dev = context; > + struct i2c_client *i2c = to_i2c_client(dev); > + struct i2c_msg xfer; > + int ret; > + > + xfer.addr = i2c->addr; > + xfer.flags = 0; > + xfer.len = val_size; > + xfer.buf = (u8 *)val; That typecast should be unnecessary. > + > + if (*(u8 *)val == RT1718S_P1PREFIX) { > + xfer.len = val_size - 1; > + xfer.buf = (u8 *)(val + 1); > + } > + Same comments as above. Also, typecast seems wrong. Shouldn't it be ((u8 *)val) + 1) ? My memory may defeat me, but I think that pointer arithmetic on void * is undefined or even illegal. > + ret = i2c_transfer(i2c->adapter, &xfer, 1); > + //pr_info("wtf i2c_write [0x%04x]:0x%02x\n", *(u16 *)(val), *(u8 *)(val+2)); No comented out code. > + if (ret < 0) > + return ret; > + if (ret != 1) > + return -EIO; > + > + return 0; > +} > + > +static const struct regmap_bus rt1718s_regmap_bus = { > + .read = rt1718s_regmap_read, > + .write = rt1718s_regmap_write, > +}; > + This needs documentation: Why not use standard regmap functions ? Yes, I know, it is because of the ubusual addressing format used by the chip, but it still needs to be explained. Not everyone should have to read the datasheet to understand the code. > +static const struct reg_sequence rt1718s_init_settings[] = { > + /* config I2C timeout reset enable , and timeout to 200ms */ > + { 0xBF, 0x8F, 0 }, > + /* config CC Detect Debounce : 250us (25*val us) */ > + { 0xB1, 0x0A, 0 }, > + /* DRP Toggle Cycle : 76.8ms (51.2 + 6.4*val ms) */ > + { 0xB2, 0x04, 0 }, > + /* DRP Duyt Ctrl : dcSRC: 331/1024 ((val+1)/1024) */ > + { 0xB3, 0x4A, 0 }, > + { 0xB4, 0x01, 0 }, > + /* Enable VCONN Current Limit function */ > + { 0x8C, 0x41, 0 }, > + /* Enable cc open 40ms when pmic send vsysuv signal */ > + { 0xCA, 0xB3, 0 }, > + /* Set GPIO2 push-pull, output-low */ > + { 0xEE, 0x0C, 0}, > + /* bg en, low power en, vbus valid detect off, vbus present on, osc off */ > + { 0xB8, 0x1A, 0}, > + /* Link GPIO2 source default vbus to TCPC command */ > + { 0xEB, 0x08, 0}, > + /* Set GPIO2 vbus path */ > + { 0xEC, 0x8E, 0 }, > + /* auto low power timer 2.5s, auto low power en, auto low power mode */ > + { 0xF210, 0x35, 0 }, > + /* Set shipping mode off, AUTOIDLE on with timeout 96ms */ > + { 0x8F, 0x7F, 0 }, > +}; > + > +static int rt1718s_init(struct tcpci *tcpci, struct tcpci_data *tdata) > +{ > + int ret; > + > + ret = regmap_register_patch(tdata->regmap, rt1718s_init_settings, > + ARRAY_SIZE(rt1718s_init_settings)); > + pr_info("%s: [TCPC-] ret=%d\n", __func__, ret); Drop the noise. This is unacceptable in production code. > + return ret; > +} > + > +static int rt1718s_set_vbus(struct tcpci *tcpci, struct tcpci_data *tdata, > + bool src, bool snk) > +{ > + struct rt1718s_chip *chip = container_of(tdata, struct rt1718s_chip, tdata); > + int ret; > + > + if (chip->src_en == src) > + return 0; > + > + if (src) > + ret = regulator_enable(chip->vbus); > + else > + ret = regulator_disable(chip->vbus); > + > + if (!ret) > + chip->src_en = src; > + return ret; > +} > + > +static irqreturn_t rt1718s_irq(int irq, void *dev_id) > +{ > + struct rt1718s_chip *chip = dev_id; > + > + return tcpci_irq(chip->tcpci); > +} > + > +static int rt1718s_sw_reset(struct rt1718s_chip *chip) > +{ > + int ret; > + > + ret = regmap_update_bits(chip->tdata.regmap, RT1718S_SYS_CTRL3, > + RT1718S_SWRESET_MASK, RT1718S_SWRESET_MASK); > + if (ret < 0) > + return ret; > + > + /* Wait for IC to reset done*/ > + usleep_range(1000, 2000); > + "RT1718S will not response to the I2C commands for 2ms after writing SOFT_RESET" Timeout needs to be at least 2 ms. > + return 0; > +} > + > +static int rt1718s_check_chip_exist(struct i2c_client *i2c) > +{ > + int ret; > + > + ret = i2c_smbus_read_word_data(i2c, TCPC_VENDOR_ID); > + if (ret < 0) > + return ret; > + if (ret != RT1718S_VID) { > + dev_err(&i2c->dev, "vid is not correct, 0x%04x\n", ret); > + return -ENODEV; > + } > + ret = i2c_smbus_read_word_data(i2c, TCPC_PRODUCT_ID); > + if (ret < 0) > + return ret; > + if (ret != RT1718S_PID) { > + dev_err(&i2c->dev, "pid is not correct, 0x%04x\n", ret); > + return -ENODEV; > + } > + return 0; > +} > + > +static int rt1718s_probe(struct i2c_client *i2c) > +{ > + struct rt1718s_chip *chip; > + struct gpio_desc *gpiod; > + int ret; > + u16 clr_events = 0; > + > + ret = rt1718s_check_chip_exist(i2c); > + if (ret < 0) { > + dev_err(&i2c->dev, "check vid/pid fail(%d)\n", ret); Double error message. > + return ret; > + } > + > + chip = devm_kzalloc(&i2c->dev, sizeof(*chip), GFP_KERNEL); > + if (!chip) > + return -ENOMEM; > + > + chip->dev = &i2c->dev; > + > + chip->tdata.regmap = devm_regmap_init(&i2c->dev, > + &rt1718s_regmap_bus, &i2c->dev, > + &rt1718s_regmap_config); > + if (IS_ERR(chip->tdata.regmap)) > + return dev_err_probe(&i2c->dev, PTR_ERR(chip->tdata.regmap), > + "Failed to init regmap\n"); > + > + chip->vbus = devm_regulator_get(&i2c->dev, "vbus"); > + if (IS_ERR(chip->vbus)) > + return dev_err_probe(&i2c->dev, PTR_ERR(chip->vbus), > + "Failed to get vbus regulator\n"); > + > + ret = rt1718s_sw_reset(chip); > + if (ret < 0) > + return ret; > + > + ret = regmap_raw_write(chip->tdata.regmap, TCPC_ALERT_MASK, &clr_events, > + sizeof(clr_events)); > + > + chip->tdata.init = rt1718s_init; > + chip->tdata.set_vbus = rt1718s_set_vbus; > + chip->tcpci = tcpci_register_port(&i2c->dev, &chip->tdata); > + if (IS_ERR(chip->tcpci)) > + return dev_err_probe(&i2c->dev, PTR_ERR(chip->tcpci), > + "Failed to register tcpci port\n"); > + > + /* for platform set gpio default inpull_high */ > + gpiod = devm_gpiod_get(&i2c->dev, NULL, GPIOD_IN); > + if (IS_ERR(gpiod)) > + return dev_err_probe(&i2c->dev, PTR_ERR(gpiod), > + "Failed to get gpio\n"); > + > + ret = devm_request_threaded_irq(&i2c->dev, i2c->irq, NULL, > + rt1718s_irq, IRQF_ONESHOT, > + dev_name(&i2c->dev), chip); > + if (ret) { > + dev_err(chip->dev, "Failed to register irq\n"); > + tcpci_unregister_port(chip->tcpci); > + return ret; > + } > + > + device_init_wakeup(&i2c->dev, true); > + i2c_set_clientdata(i2c, chip); > + > + dev_info(&i2c->dev, "%s:successfully\n", __func__); Nore logging noise. > + return 0; > +} > + > +static void rt1718s_remove(struct i2c_client *i2c) > +{ > + struct rt1718s_chip *chip = i2c_get_clientdata(i2c); > + > + tcpci_unregister_port(chip->tcpci); > +} > + > +static int __maybe_unused rt1718s_suspend(struct device *dev) > +{ > + struct i2c_client *i2c = to_i2c_client(dev); > + > + if (device_may_wakeup(dev)) > + enable_irq_wake(i2c->irq); > + disable_irq(i2c->irq); > + > + return 0; > +} > + > +static int __maybe_unused rt1718s_resume(struct device *dev) > +{ > + struct i2c_client *i2c = to_i2c_client(dev); > + > + if (device_may_wakeup(dev)) > + disable_irq_wake(i2c->irq); > + enable_irq(i2c->irq); > + > + return 0; > +} > + > +static SIMPLE_DEV_PM_OPS(rt1718s_pm_ops, rt1718s_suspend, rt1718s_resume); > + > +static const struct of_device_id __maybe_unused rt1718s_of_id[] = { > + { .compatible = "richtek,rt1718s", }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, rt1718s_of_id); > + > +static struct i2c_driver rt1718s_i2c_driver = { > + .driver = { > + .name = "rt1718s", > + .pm = &rt1718s_pm_ops, > + .of_match_table = rt1718s_of_id, > + }, > + .probe_new = rt1718s_probe, > + .remove = rt1718s_remove, > +}; > +module_i2c_driver(rt1718s_i2c_driver); > + > +MODULE_AUTHOR("Gene Chen <gene_chen@richtek.com>"); > +MODULE_DESCRIPTION("RT1718S USB Type-C Port Controller Interface Driver"); > +MODULE_LICENSE("GPL");
> -----Original Message----- > From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of Guenter Roeck > Sent: Tuesday, January 10, 2023 2:52 AM > To: gene_chen(陳俊宇) <gene_chen@richtek.com>; robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; heikki.krogerus@linux.intel.com > Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH 2/2] usb: typec: tcpci_rt1718s: Add Richtek RT1718S tcpci driver > > On 1/9/23 01:31, gene_chen@richtek.com wrote: > > From: Gene Chen <gene_chen@richtek.com> > > > > Richtek RT1718S is highly integrated TCPC and Power Delivery (PD) > > controller with IEC-ESD Protection on SBU/CC/DP/DM, USB2.0 Switch, > > Charging Port Controller and Power-Path Control. > > > > Signed-off-by: Gene Chen <gene_chen@richtek.com> > > --- > > drivers/usb/typec/tcpm/Kconfig | 9 + > > drivers/usb/typec/tcpm/Makefile | 1 + > > drivers/usb/typec/tcpm/tcpci_rt1718s.c | 349 +++++++++++++++++++++++++++++++++ > > 3 files changed, 359 insertions(+) > > create mode 100644 drivers/usb/typec/tcpm/tcpci_rt1718s.c > > > > diff --git a/drivers/usb/typec/tcpm/Kconfig > > b/drivers/usb/typec/tcpm/Kconfig index e6b88ca..f0efb34 100644 > > --- a/drivers/usb/typec/tcpm/Kconfig > > +++ b/drivers/usb/typec/tcpm/Kconfig > > @@ -27,6 +27,15 @@ config TYPEC_RT1711H > > Type-C Port Controller Manager to provide USB PD and USB > > Type-C functionalities. > > > > +config TYPEC_RT1718S > > +tristate "Richtek RT1718S Type-C chip driver" > > +depends on I2C > > +help > > + Richtek RT1718S Type-C chip driver that works with > > + Type-C Port Controller Manager to provide USB PD and USB > > + Type-C functionalities. > > + Additionally, it supports BC1.2 and power-path control. > > + > > config TYPEC_MT6360 > > tristate "Mediatek MT6360 Type-C driver" > > depends on MFD_MT6360 > > diff --git a/drivers/usb/typec/tcpm/Makefile > > b/drivers/usb/typec/tcpm/Makefile index 906d9dc..db33ffc 100644 > > --- a/drivers/usb/typec/tcpm/Makefile > > +++ b/drivers/usb/typec/tcpm/Makefile > > @@ -5,6 +5,7 @@ obj-$(CONFIG_TYPEC_WCOVE)+= typec_wcove.o > > typec_wcove-y:= wcove.o > > obj-$(CONFIG_TYPEC_TCPCI)+= tcpci.o > > obj-$(CONFIG_TYPEC_RT1711H)+= tcpci_rt1711h.o > > +obj-$(CONFIG_TYPEC_RT1718S)+= tcpci_rt1718s.o > > obj-$(CONFIG_TYPEC_MT6360)+= tcpci_mt6360.o > > obj-$(CONFIG_TYPEC_TCPCI_MT6370)+= tcpci_mt6370.o > > obj-$(CONFIG_TYPEC_TCPCI_MAXIM)+= tcpci_maxim.o > > diff --git a/drivers/usb/typec/tcpm/tcpci_rt1718s.c > > b/drivers/usb/typec/tcpm/tcpci_rt1718s.c > > new file mode 100644 > > index 00000000..305b39c > > --- /dev/null > > +++ b/drivers/usb/typec/tcpm/tcpci_rt1718s.c > > + > > +if (*(u8 *)reg == RT1718S_P1PREFIX) { > > +xfer[0].len = 1, > > +xfer[0].buf = (u8 *)(reg + 1); > > Pointer arithmetic on void * (see below). > yes, should be xfer[0].buf = (u8 *)reg + 1; > > +} > > There is a lot of context here which needs explanation. The code extracts the upper 8 bit of the register address and drops it if the value is 0. > So the address is either 8 bit or 16 bit depending on the register address. > Also, this only works because reg_format_endian is set to big endian. > > This really needs to be documented. > > Assigning the values to len and buf twice is really bad style. > Please use if/else. > Because of rt1718s address which is more than 1 page(0x00-0xFF), hw design i2c pattern with register has 2 byte to access another page(0xF200-0xF2FF). The upper byte is set to 0xF2 indicated page 2. > > + > > +if (*(u8 *)val == RT1718S_P1PREFIX) { > > +xfer.len = val_size - 1; > > +xfer.buf = (u8 *)(val + 1); > > +} > > + > > Same comments as above. Also, typecast seems wrong. Shouldn't it be > ((u8 *)val) + 1) ? My memory may defeat me, but I think that pointer arithmetic on void * is undefined or even illegal. > > You are right, I will fix it, thanks. > > +if (ret < 0) > > +return ret; > > +if (ret != 1) > > +return -EIO; > > + > > +return 0; > > +} > > + > > +static const struct regmap_bus rt1718s_regmap_bus = { > > +.read= rt1718s_regmap_read, > > +.write= rt1718s_regmap_write, > > +}; > > + > > This needs documentation: Why not use standard regmap functions ? > Yes, I know, it is because of the ubusual addressing format used by the chip, but it still needs to be explained. Not everyone should have to read the datasheet to understand the code. > Should I add comment before declare rt1718s_regmap_bus? /* * Because of rt1718s address which is more than 1 page(0x00-0xFF), * hw design i2c pattern with register has 2 byte to access another page(0xF200-0xF2FF). * The upper byte is set to 0xF2 indicated page 2. */ > > +static int rt1718s_sw_reset(struct rt1718s_chip *chip) { > > +int ret; > > + > > +ret = regmap_update_bits(chip->tdata.regmap, RT1718S_SYS_CTRL3, > > + RT1718S_SWRESET_MASK, RT1718S_SWRESET_MASK); > > +if (ret < 0) > > +return ret; > > + > > +/* Wait for IC to reset done*/ > > +usleep_range(1000, 2000); > > + > > "RT1718S will not response to the I2C commands for 2ms after writing SOFT_RESET" > > Timeout needs to be at least 2 ms. > ACK > > +return 0; > > +} > > + > > +static int rt1718s_check_chip_exist(struct i2c_client *i2c) { > > +int ret; > > + > > +ret = i2c_smbus_read_word_data(i2c, TCPC_VENDOR_ID); > > +if (ret < 0) > > +return ret; > > +if (ret != RT1718S_VID) { > > +dev_err(&i2c->dev, "vid is not correct, 0x%04x\n", ret); > > +return -ENODEV; > > +} > > +ret = i2c_smbus_read_word_data(i2c, TCPC_PRODUCT_ID); > > +if (ret < 0) > > +return ret; > > +if (ret != RT1718S_PID) { > > +dev_err(&i2c->dev, "pid is not correct, 0x%04x\n", ret); > > +return -ENODEV; > > +} > > +return 0; > > +} > > + > > +static int rt1718s_probe(struct i2c_client *i2c) { > > +struct rt1718s_chip *chip; > > +struct gpio_desc *gpiod; > > +int ret; > > +u16 clr_events = 0; > > + > > +ret = rt1718s_check_chip_exist(i2c); > > +if (ret < 0) { > > +dev_err(&i2c->dev, "check vid/pid fail(%d)\n", ret); > > Double error message. > ACK, I will remove it. > > +return ret; > > +} > > + > > +chip = devm_kzalloc(&i2c->dev, sizeof(*chip), GFP_KERNEL); > > +if (!chip) > > +return -ENOMEM; > > + > > +chip->dev = &i2c->dev; > > + > > +chip->tdata.regmap = devm_regmap_init(&i2c->dev, > > + &rt1718s_regmap_bus, &i2c->dev, > > + &rt1718s_regmap_config); > > +if (IS_ERR(chip->tdata.regmap)) > > +return dev_err_probe(&i2c->dev, PTR_ERR(chip->tdata.regmap), > > + "Failed to init regmap\n"); > > + > > +chip->vbus = devm_regulator_get(&i2c->dev, "vbus"); > > +if (IS_ERR(chip->vbus)) > > +return dev_err_probe(&i2c->dev, PTR_ERR(chip->vbus), > > + "Failed to get vbus regulator\n"); > > + > > +ret = rt1718s_sw_reset(chip); > > +if (ret < 0) > > +return ret; > > + > > +ret = regmap_raw_write(chip->tdata.regmap, TCPC_ALERT_MASK, &clr_events, > > + sizeof(clr_events)); > > + > > +chip->tdata.init = rt1718s_init; > > +chip->tdata.set_vbus = rt1718s_set_vbus; > > +chip->tcpci = tcpci_register_port(&i2c->dev, &chip->tdata); > > +if (IS_ERR(chip->tcpci)) > > +return dev_err_probe(&i2c->dev, PTR_ERR(chip->tcpci), > > + "Failed to register tcpci port\n"); > > + > > +/* for platform set gpio default inpull_high */ > > +gpiod = devm_gpiod_get(&i2c->dev, NULL, GPIOD_IN); > > +if (IS_ERR(gpiod)) > > +return dev_err_probe(&i2c->dev, PTR_ERR(gpiod), > > + "Failed to get gpio\n"); > > + > > +ret = devm_request_threaded_irq(&i2c->dev, i2c->irq, NULL, > > +rt1718s_irq, IRQF_ONESHOT, > > +dev_name(&i2c->dev), chip); > > +if (ret) { > > +dev_err(chip->dev, "Failed to register irq\n"); > > +tcpci_unregister_port(chip->tcpci); > > +return ret; > > +} > > + > > +device_init_wakeup(&i2c->dev, true); > > +i2c_set_clientdata(i2c, chip); > > + > > +dev_info(&i2c->dev, "%s:successfully\n", __func__); > > Nore logging noise. > ACK > > +return 0; > > +} > > + > > +static void rt1718s_remove(struct i2c_client *i2c) { > > +struct rt1718s_chip *chip = i2c_get_clientdata(i2c); > > + > > +tcpci_unregister_port(chip->tcpci); > > +} > > + > > +static int __maybe_unused rt1718s_suspend(struct device *dev) { > > +struct i2c_client *i2c = to_i2c_client(dev); > > + > > +if (device_may_wakeup(dev)) > > +enable_irq_wake(i2c->irq); > > +disable_irq(i2c->irq); > > + > > +return 0; > > +} > > + > > +static int __maybe_unused rt1718s_resume(struct device *dev) { > > +struct i2c_client *i2c = to_i2c_client(dev); > > + > > +if (device_may_wakeup(dev)) > > +disable_irq_wake(i2c->irq); > > +enable_irq(i2c->irq); > > + > > +return 0; > > +} > > + > > +static SIMPLE_DEV_PM_OPS(rt1718s_pm_ops, rt1718s_suspend, > > +rt1718s_resume); > > + > > +static const struct of_device_id __maybe_unused rt1718s_of_id[] = { > > +{ .compatible = "richtek,rt1718s", }, > > +{}, > > +}; > > +MODULE_DEVICE_TABLE(of, rt1718s_of_id); > > + > > +static struct i2c_driver rt1718s_i2c_driver = { > > +.driver = { > > +.name = "rt1718s", > > +.pm = &rt1718s_pm_ops, > > +.of_match_table = rt1718s_of_id, > > +}, > > +.probe_new = rt1718s_probe, > > +.remove = rt1718s_remove, > > +}; > > +module_i2c_driver(rt1718s_i2c_driver); > > + > > +MODULE_AUTHOR("Gene Chen <gene_chen@richtek.com>"); > > +MODULE_DESCRIPTION("RT1718S USB Type-C Port Controller Interface > > +Driver"); MODULE_LICENSE("GPL");
On 1/12/23 00:05, Gene Chen wrote: >> -----Original Message----- >> From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of Guenter Roeck >> Sent: Tuesday, January 10, 2023 2:52 AM >> To: gene_chen(陳俊宇) <gene_chen@richtek.com>; robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; heikki.krogerus@linux.intel.com >> Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org >> Subject: Re: [PATCH 2/2] usb: typec: tcpci_rt1718s: Add Richtek RT1718S tcpci driver >> >> On 1/9/23 01:31, gene_chen@richtek.com wrote: >>> From: Gene Chen <gene_chen@richtek.com> >>> >>> Richtek RT1718S is highly integrated TCPC and Power Delivery (PD) >>> controller with IEC-ESD Protection on SBU/CC/DP/DM, USB2.0 Switch, >>> Charging Port Controller and Power-Path Control. >>> >>> Signed-off-by: Gene Chen <gene_chen@richtek.com> >>> --- >>> drivers/usb/typec/tcpm/Kconfig | 9 + >>> drivers/usb/typec/tcpm/Makefile | 1 + >>> drivers/usb/typec/tcpm/tcpci_rt1718s.c | 349 +++++++++++++++++++++++++++++++++ >>> 3 files changed, 359 insertions(+) >>> create mode 100644 drivers/usb/typec/tcpm/tcpci_rt1718s.c >>> >>> diff --git a/drivers/usb/typec/tcpm/Kconfig >>> b/drivers/usb/typec/tcpm/Kconfig index e6b88ca..f0efb34 100644 >>> --- a/drivers/usb/typec/tcpm/Kconfig >>> +++ b/drivers/usb/typec/tcpm/Kconfig >>> @@ -27,6 +27,15 @@ config TYPEC_RT1711H >>> Type-C Port Controller Manager to provide USB PD and USB >>> Type-C functionalities. >>> >>> +config TYPEC_RT1718S >>> +tristate "Richtek RT1718S Type-C chip driver" >>> +depends on I2C >>> +help >>> + Richtek RT1718S Type-C chip driver that works with >>> + Type-C Port Controller Manager to provide USB PD and USB >>> + Type-C functionalities. >>> + Additionally, it supports BC1.2 and power-path control. >>> + >>> config TYPEC_MT6360 >>> tristate "Mediatek MT6360 Type-C driver" >>> depends on MFD_MT6360 >>> diff --git a/drivers/usb/typec/tcpm/Makefile >>> b/drivers/usb/typec/tcpm/Makefile index 906d9dc..db33ffc 100644 >>> --- a/drivers/usb/typec/tcpm/Makefile >>> +++ b/drivers/usb/typec/tcpm/Makefile >>> @@ -5,6 +5,7 @@ obj-$(CONFIG_TYPEC_WCOVE)+= typec_wcove.o >>> typec_wcove-y:= wcove.o >>> obj-$(CONFIG_TYPEC_TCPCI)+= tcpci.o >>> obj-$(CONFIG_TYPEC_RT1711H)+= tcpci_rt1711h.o >>> +obj-$(CONFIG_TYPEC_RT1718S)+= tcpci_rt1718s.o >>> obj-$(CONFIG_TYPEC_MT6360)+= tcpci_mt6360.o >>> obj-$(CONFIG_TYPEC_TCPCI_MT6370)+= tcpci_mt6370.o >>> obj-$(CONFIG_TYPEC_TCPCI_MAXIM)+= tcpci_maxim.o >>> diff --git a/drivers/usb/typec/tcpm/tcpci_rt1718s.c >>> b/drivers/usb/typec/tcpm/tcpci_rt1718s.c >>> new file mode 100644 >>> index 00000000..305b39c >>> --- /dev/null >>> +++ b/drivers/usb/typec/tcpm/tcpci_rt1718s.c >>> + >>> +if (*(u8 *)reg == RT1718S_P1PREFIX) { >>> +xfer[0].len = 1, >>> +xfer[0].buf = (u8 *)(reg + 1); >> >> Pointer arithmetic on void * (see below). >> > > yes, should be xfer[0].buf = (u8 *)reg + 1; > >>> +} >> >> There is a lot of context here which needs explanation. The code extracts the upper 8 bit of the register address and drops it if the value is 0. >> So the address is either 8 bit or 16 bit depending on the register address. >> Also, this only works because reg_format_endian is set to big endian. >> >> This really needs to be documented. >> >> Assigning the values to len and buf twice is really bad style. >> Please use if/else. >> > > Because of rt1718s address which is more than 1 page(0x00-0xFF), hw > design i2c pattern with register has 2 byte to access another > page(0xF200-0xF2FF). > The upper byte is set to 0xF2 indicated page 2. > Sure. I meant something like if (*(u8 *)reg == RT1718S_P1PREFIX) { xfer[0].len = 1; <<== that should be a semicolon, not ',' xfer[0].buf = (u8 *)reg + 1; } else { ... } to avoid the double assignment. >>> + >>> +if (*(u8 *)val == RT1718S_P1PREFIX) { >>> +xfer.len = val_size - 1; >>> +xfer.buf = (u8 *)(val + 1); >>> +} >>> + >> >> Same comments as above. Also, typecast seems wrong. Shouldn't it be >> ((u8 *)val) + 1) ? My memory may defeat me, but I think that pointer arithmetic on void * is undefined or even illegal. >> >> > > You are right, I will fix it, thanks. > >>> +if (ret < 0) >>> +return ret; >>> +if (ret != 1) >>> +return -EIO; >>> + >>> +return 0; >>> +} >>> + >>> +static const struct regmap_bus rt1718s_regmap_bus = { >>> +.read= rt1718s_regmap_read, >>> +.write= rt1718s_regmap_write, >>> +}; >>> + >> >> This needs documentation: Why not use standard regmap functions ? >> Yes, I know, it is because of the ubusual addressing format used by the chip, but it still needs to be explained. Not everyone should have to read the datasheet to understand the code. >> > > Should I add comment before declare rt1718s_regmap_bus? > > /* > * Because of rt1718s address which is more than 1 page(0x00-0xFF), > * hw design i2c pattern with register has 2 byte to access another > page(0xF200-0xF2FF). > * The upper byte is set to 0xF2 indicated page 2. > */ > Yes, something like that. >>> +static int rt1718s_sw_reset(struct rt1718s_chip *chip) { >>> +int ret; >>> + >>> +ret = regmap_update_bits(chip->tdata.regmap, RT1718S_SYS_CTRL3, >>> + RT1718S_SWRESET_MASK, RT1718S_SWRESET_MASK); >>> +if (ret < 0) >>> +return ret; >>> + >>> +/* Wait for IC to reset done*/ >>> +usleep_range(1000, 2000); >>> + >> >> "RT1718S will not response to the I2C commands for 2ms after writing SOFT_RESET" >> >> Timeout needs to be at least 2 ms. >> > > ACK > >>> +return 0; >>> +} >>> + >>> +static int rt1718s_check_chip_exist(struct i2c_client *i2c) { >>> +int ret; >>> + >>> +ret = i2c_smbus_read_word_data(i2c, TCPC_VENDOR_ID); >>> +if (ret < 0) >>> +return ret; >>> +if (ret != RT1718S_VID) { >>> +dev_err(&i2c->dev, "vid is not correct, 0x%04x\n", ret); >>> +return -ENODEV; >>> +} >>> +ret = i2c_smbus_read_word_data(i2c, TCPC_PRODUCT_ID); >>> +if (ret < 0) >>> +return ret; >>> +if (ret != RT1718S_PID) { >>> +dev_err(&i2c->dev, "pid is not correct, 0x%04x\n", ret); >>> +return -ENODEV; >>> +} >>> +return 0; >>> +} >>> + >>> +static int rt1718s_probe(struct i2c_client *i2c) { >>> +struct rt1718s_chip *chip; >>> +struct gpio_desc *gpiod; >>> +int ret; >>> +u16 clr_events = 0; >>> + >>> +ret = rt1718s_check_chip_exist(i2c); >>> +if (ret < 0) { >>> +dev_err(&i2c->dev, "check vid/pid fail(%d)\n", ret); >> >> Double error message. >> > > ACK, I will remove it. > >>> +return ret; >>> +} >>> + >>> +chip = devm_kzalloc(&i2c->dev, sizeof(*chip), GFP_KERNEL); >>> +if (!chip) >>> +return -ENOMEM; >>> + >>> +chip->dev = &i2c->dev; >>> + >>> +chip->tdata.regmap = devm_regmap_init(&i2c->dev, >>> + &rt1718s_regmap_bus, &i2c->dev, >>> + &rt1718s_regmap_config); >>> +if (IS_ERR(chip->tdata.regmap)) >>> +return dev_err_probe(&i2c->dev, PTR_ERR(chip->tdata.regmap), >>> + "Failed to init regmap\n"); >>> + >>> +chip->vbus = devm_regulator_get(&i2c->dev, "vbus"); >>> +if (IS_ERR(chip->vbus)) >>> +return dev_err_probe(&i2c->dev, PTR_ERR(chip->vbus), >>> + "Failed to get vbus regulator\n"); >>> + >>> +ret = rt1718s_sw_reset(chip); >>> +if (ret < 0) >>> +return ret; >>> + >>> +ret = regmap_raw_write(chip->tdata.regmap, TCPC_ALERT_MASK, &clr_events, >>> + sizeof(clr_events)); >>> + >>> +chip->tdata.init = rt1718s_init; >>> +chip->tdata.set_vbus = rt1718s_set_vbus; >>> +chip->tcpci = tcpci_register_port(&i2c->dev, &chip->tdata); >>> +if (IS_ERR(chip->tcpci)) >>> +return dev_err_probe(&i2c->dev, PTR_ERR(chip->tcpci), >>> + "Failed to register tcpci port\n"); >>> + >>> +/* for platform set gpio default inpull_high */ >>> +gpiod = devm_gpiod_get(&i2c->dev, NULL, GPIOD_IN); >>> +if (IS_ERR(gpiod)) >>> +return dev_err_probe(&i2c->dev, PTR_ERR(gpiod), >>> + "Failed to get gpio\n"); >>> + >>> +ret = devm_request_threaded_irq(&i2c->dev, i2c->irq, NULL, >>> +rt1718s_irq, IRQF_ONESHOT, >>> +dev_name(&i2c->dev), chip); >>> +if (ret) { >>> +dev_err(chip->dev, "Failed to register irq\n"); >>> +tcpci_unregister_port(chip->tcpci); >>> +return ret; >>> +} >>> + >>> +device_init_wakeup(&i2c->dev, true); >>> +i2c_set_clientdata(i2c, chip); >>> + >>> +dev_info(&i2c->dev, "%s:successfully\n", __func__); >> >> Nore logging noise. >> > > ACK > >>> +return 0; >>> +} >>> + >>> +static void rt1718s_remove(struct i2c_client *i2c) { >>> +struct rt1718s_chip *chip = i2c_get_clientdata(i2c); >>> + >>> +tcpci_unregister_port(chip->tcpci); >>> +} >>> + >>> +static int __maybe_unused rt1718s_suspend(struct device *dev) { >>> +struct i2c_client *i2c = to_i2c_client(dev); >>> + >>> +if (device_may_wakeup(dev)) >>> +enable_irq_wake(i2c->irq); >>> +disable_irq(i2c->irq); >>> + >>> +return 0; >>> +} >>> + >>> +static int __maybe_unused rt1718s_resume(struct device *dev) { >>> +struct i2c_client *i2c = to_i2c_client(dev); >>> + >>> +if (device_may_wakeup(dev)) >>> +disable_irq_wake(i2c->irq); >>> +enable_irq(i2c->irq); >>> + >>> +return 0; >>> +} >>> + >>> +static SIMPLE_DEV_PM_OPS(rt1718s_pm_ops, rt1718s_suspend, >>> +rt1718s_resume); >>> + >>> +static const struct of_device_id __maybe_unused rt1718s_of_id[] = { >>> +{ .compatible = "richtek,rt1718s", }, >>> +{}, >>> +}; >>> +MODULE_DEVICE_TABLE(of, rt1718s_of_id); >>> + >>> +static struct i2c_driver rt1718s_i2c_driver = { >>> +.driver = { >>> +.name = "rt1718s", >>> +.pm = &rt1718s_pm_ops, >>> +.of_match_table = rt1718s_of_id, >>> +}, >>> +.probe_new = rt1718s_probe, >>> +.remove = rt1718s_remove, >>> +}; >>> +module_i2c_driver(rt1718s_i2c_driver); >>> + >>> +MODULE_AUTHOR("Gene Chen <gene_chen@richtek.com>"); >>> +MODULE_DESCRIPTION("RT1718S USB Type-C Port Controller Interface >>> +Driver"); MODULE_LICENSE("GPL");
diff --git a/Documentation/devicetree/bindings/usb/richtek,rt1718s.yaml b/Documentation/devicetree/bindings/usb/richtek,rt1718s.yaml new file mode 100644 index 00000000..7797fc6 --- /dev/null +++ b/Documentation/devicetree/bindings/usb/richtek,rt1718s.yaml @@ -0,0 +1,98 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: "http://devicetree.org/schemas/usb/richtek,rt1718s.yaml#" +$schema: "http://devicetree.org/meta-schemas/core.yaml#" + +title: Richtek RT1718S Type-C Port Switch and Power Delivery controller + +maintainers: + - Gene Chen <gene_chen@richtek.com> + +description: | + The RT1718S is a USB Type-C controller that complies with the latest + USB Type-C and PD standards. It does the USB Type-C detection including attach + and orientation. It integrates the physical layer of the USB BMC power + delivery protocol to allow up to 100W of power. The BMC PD block enables full + support for alternative interfaces of the Type-C specification. + +properties: + compatible: + enum: + - richtek,rt1718s + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + wakeup-source: + description: enable IRQ remote wakeup, see power/wakeup-source.txt + type: boolean + + connector: + type: object + $ref: ../connector/usb-connector.yaml# + description: + Properties for usb c connector. + +additionalProperties: false + +required: + - compatible + - reg + - connector + - interrupts + +examples: + - | + #include <dt-bindings/interrupt-controller/irq.h> + #include <dt-bindings/usb/pd.h> + i2c0 { + #address-cells = <1>; + #size-cells = <0>; + + rt1718s@43 { + compatible = "richtek,rt1718s"; + reg = <0x43>; + interrupts-extended = <&gpio26 3 IRQ_TYPE_LEVEL_LOW>; + wakeup-source; + + connector { + compatible = "usb-c-connector"; + label = "USB-C"; + data-role = "dual"; + power-role = "dual"; + try-power-role = "sink"; + source-pdos = <PDO_FIXED(5000, 2000, PDO_FIXED_DUAL_ROLE | PDO_FIXED_DATA_SWAP)>; + sink-pdos = <PDO_FIXED(5000, 2000, PDO_FIXED_DUAL_ROLE | PDO_FIXED_DATA_SWAP)>; + op-sink-microwatt = <10000000>; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + port@0 { + reg = <0>; + endpoint { + remote-endpoint = <&usb_hs>; + }; + }; + port@1 { + reg = <1>; + endpoint { + remote-endpoint = <&usb_ss>; + }; + }; + port@2 { + reg = <2>; + endpoint { + remote-endpoint = <&dp_aux>; + }; + }; + }; + }; + }; + }; +...