| Message ID | 20220721061144.35139-1-gene.chen.richtek@gmail.com |
|---|---|
| Headers | show |
| Series | usb: typec: tcpci_rt1711h: Add compatible with rt1715 | expand |
On 7/20/22 23:11, Gene Chen wrote: > From: Gene Chen <gene_chen@richtek.com> > > Add regulator support when source vbus > > Signed-off-by: Gene Chen <gene_chen@richtek.com> > --- > drivers/usb/typec/tcpm/tcpci_rt1711h.c | 28 ++++++++++++++++++++++++++ > 1 file changed, 28 insertions(+) > > diff --git a/drivers/usb/typec/tcpm/tcpci_rt1711h.c b/drivers/usb/typec/tcpm/tcpci_rt1711h.c > index 3309ceace2b2..52c9594e531d 100644 > --- a/drivers/usb/typec/tcpm/tcpci_rt1711h.c > +++ b/drivers/usb/typec/tcpm/tcpci_rt1711h.c > @@ -10,6 +10,7 @@ > #include <linux/i2c.h> > #include <linux/interrupt.h> > #include <linux/gpio/consumer.h> > +#include <linux/regulator/consumer.h> > #include <linux/usb/tcpm.h> > #include <linux/regmap.h> > #include "tcpci.h" > @@ -40,6 +41,8 @@ struct rt1711h_chip { > struct tcpci_data data; > struct tcpci *tcpci; > struct device *dev; > + struct regulator *vbus; > + bool src_en; > }; > > static int rt1711h_read16(struct rt1711h_chip *chip, unsigned int reg, u16 *val) > @@ -103,6 +106,26 @@ static int rt1711h_init(struct tcpci *tcpci, struct tcpci_data *tdata) > > /* dcSRC.DRP : 33% */ > return rt1711h_write16(chip, RT1711H_RTCTRL16, 330); > + > +} > + > +static int rt1711h_set_vbus(struct tcpci *tcpci, struct tcpci_data *tdata, > + bool src, bool snk) > +{ > + struct rt1711h_chip *chip = tdata_to_rt1711h(tdata); > + int ret; > + > + if (chip->src_en == src) > + return 1; > + > + if (src) > + ret = regulator_enable(chip->vbus); > + else > + ret = regulator_disable(chip->vbus); > + > + if (!ret) > + chip->src_en = src; > + return ret ? ret : 1; Are you sure this is what you want ? Returning 1 bypasses the code setting the vbus registers in tcpci.c. If that is on purpose it might make sense to explain it. > } > > static int rt1711h_set_vconn(struct tcpci *tcpci, struct tcpci_data *tdata, > @@ -246,7 +269,12 @@ static int rt1711h_probe(struct i2c_client *client, > if (ret < 0) > return ret; > > + chip->vbus = devm_regulator_get(&client->dev, "vbus"); > + if (IS_ERR(chip->vbus)) > + return PTR_ERR(chip->vbus); > + This makes regulator support mandatory, which so far was not the case. That warrants an explanation why it is not a problem for existing users. Thanks, Guenter > chip->data.init = rt1711h_init; > + chip->data.set_vbus = rt1711h_set_vbus; > chip->data.set_vconn = rt1711h_set_vconn; > chip->data.start_drp_toggling = rt1711h_start_drp_toggling; > chip->tcpci = tcpci_register_port(chip->dev, &chip->data);
On 7/20/22 23:11, Gene Chen wrote: > From: Gene Chen <gene_chen@richtek.com> > > replace overwrite whole register with update bits > > Signed-off-by: Gene Chen <gene_chen@richtek.com> > --- > drivers/usb/typec/tcpm/tcpci_rt1711h.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/typec/tcpm/tcpci_rt1711h.c b/drivers/usb/typec/tcpm/tcpci_rt1711h.c > index b56a0880a044..3309ceace2b2 100644 > --- a/drivers/usb/typec/tcpm/tcpci_rt1711h.c > +++ b/drivers/usb/typec/tcpm/tcpci_rt1711h.c > @@ -23,6 +23,7 @@ > #define RT1711H_RTCTRL8_SET(ck300, ship_off, auto_idle, tout) \ > (((ck300) << 7) | ((ship_off) << 5) | \ > ((auto_idle) << 3) | ((tout) & 0x07)) > +#define RT1711H_AUTOIDLEEN_MASK BIT(3) Simple RT1711H_AUTOIDLEEN would be sufficient. Also, when using BIT(), include the necessary include file. > > #define RT1711H_RTCTRL11 0x9E > > @@ -109,8 +110,8 @@ static int rt1711h_set_vconn(struct tcpci *tcpci, struct tcpci_data *tdata, > { > struct rt1711h_chip *chip = tdata_to_rt1711h(tdata); > > - return rt1711h_write8(chip, RT1711H_RTCTRL8, > - RT1711H_RTCTRL8_SET(0, 1, !enable, 2)); > + return regmap_update_bits(chip->data.regmap, RT1711H_RTCTRL8, > + RT1711H_AUTOIDLEEN_MASK, enable ? 0 : 0xFF); I would suggest return regmap_update_bits(chip->data.regmap, RT1711H_RTCTRL8, RT1711H_AUTOIDLEEN, enable ? 0 : RT1711H_AUTOIDLEEN); to avoid confusion why 0xFF is used. Thanks, Guenter > } > > static int rt1711h_start_drp_toggling(struct tcpci *tcpci,
On 7/20/22 23:11, Gene Chen wrote: > From: Gene Chen <gene_chen@richtek.com> > > Add initial phy setting about phy dicard retry, > rx filter deglitech time and BMC-encoded wait time > > Signed-off-by: Gene Chen <gene_chen@richtek.com> Reviewed-by: Guenter Roeck <linux@roeck-us.net> > --- > drivers/usb/typec/tcpm/tcpci_rt1711h.c | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/typec/tcpm/tcpci_rt1711h.c b/drivers/usb/typec/tcpm/tcpci_rt1711h.c > index 52c9594e531d..e65b568959e9 100644 > --- a/drivers/usb/typec/tcpm/tcpci_rt1711h.c > +++ b/drivers/usb/typec/tcpm/tcpci_rt1711h.c > @@ -18,6 +18,9 @@ > #define RT1711H_VID 0x29CF > #define RT1711H_PID 0x1711 > > +#define RT1711H_PHYCTRL1 0x80 > +#define RT1711H_PHYCTRL2 0x81 > + > #define RT1711H_RTCTRL8 0x9B > > /* Autoidle timeout = (tout * 2 + 1) * 6.4ms */ > @@ -105,8 +108,18 @@ static int rt1711h_init(struct tcpci *tcpci, struct tcpci_data *tdata) > return ret; > > /* dcSRC.DRP : 33% */ > - return rt1711h_write16(chip, RT1711H_RTCTRL16, 330); > + ret = rt1711h_write16(chip, RT1711H_RTCTRL16, 330); > + if (ret < 0) > + return ret; > + > + /* Enable phy discard retry, retry count 7, rx filter deglitech 100 us */ > + ret = rt1711h_write8(chip, RT1711H_PHYCTRL1, 0xF1); > + if (ret < 0) > + return ret; > > + /* Decrease wait time of BMC-encoded 1 bit from 2.67us to 2.55us */ > + /* wait time : (val * .4167) us */ > + return rt1711h_write8(chip, RT1711H_PHYCTRL2, 62); > } > > static int rt1711h_set_vbus(struct tcpci *tcpci, struct tcpci_data *tdata,
On 7/20/22 23:11, Gene Chen wrote: > From: Gene Chen <gene_chen@richtek.com> > > Add compatible id with rt1715, and add initial setting for > specific support PD30 command. > > Signed-off-by: Gene Chen <gene_chen@richtek.com> > --- > drivers/usb/typec/tcpm/tcpci_rt1711h.c | 24 ++++++++++++++++++++++-- > 1 file changed, 22 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/typec/tcpm/tcpci_rt1711h.c b/drivers/usb/typec/tcpm/tcpci_rt1711h.c > index e65b568959e9..3316dfaeee0d 100644 > --- a/drivers/usb/typec/tcpm/tcpci_rt1711h.c > +++ b/drivers/usb/typec/tcpm/tcpci_rt1711h.c > @@ -17,6 +17,7 @@ > > #define RT1711H_VID 0x29CF > #define RT1711H_PID 0x1711 > +#define RT1715_DID 0x2173 > > #define RT1711H_PHYCTRL1 0x80 > #define RT1711H_PHYCTRL2 0x81 > @@ -28,6 +29,7 @@ > (((ck300) << 7) | ((ship_off) << 5) | \ > ((auto_idle) << 3) | ((tout) & 0x07)) > #define RT1711H_AUTOIDLEEN_MASK BIT(3) > +#define RT1711H_ENEXTMSG_MASK BIT(4) I would suggest to drop _MASK. > > #define RT1711H_RTCTRL11 0x9E > > @@ -46,6 +48,7 @@ struct rt1711h_chip { > struct device *dev; > struct regulator *vbus; > bool src_en; > + u16 did; > }; > > static int rt1711h_read16(struct rt1711h_chip *chip, unsigned int reg, u16 *val) > @@ -82,8 +85,9 @@ static struct rt1711h_chip *tdata_to_rt1711h(struct tcpci_data *tdata) > > static int rt1711h_init(struct tcpci *tcpci, struct tcpci_data *tdata) > { > - int ret; > struct rt1711h_chip *chip = tdata_to_rt1711h(tdata); > + struct regmap *regmap = chip->data.regmap; > + int ret; > > /* CK 300K from 320K, shipping off, auto_idle enable, tout = 32ms */ > ret = rt1711h_write8(chip, RT1711H_RTCTRL8, > @@ -91,6 +95,14 @@ static int rt1711h_init(struct tcpci *tcpci, struct tcpci_data *tdata) > if (ret < 0) > return ret; > > + /* Enable PD30 extended message for RT1715 */ > + if (chip->did == RT1715_DID) { > + ret = regmap_update_bits(regmap, RT1711H_RTCTRL8, > + RT1711H_ENEXTMSG_MASK, 0xFF); 0xFF -> RT1711H_ENEXTMSG > + if (ret < 0) > + return ret; > + } > + > /* I2C reset : (val + 1) * 12.5ms */ > ret = rt1711h_write8(chip, RT1711H_RTCTRL11, > RT1711H_RTCTRL11_SET(1, 0x0F)); > @@ -246,7 +258,11 @@ static int rt1711h_check_revision(struct i2c_client *i2c) > dev_err(&i2c->dev, "pid is not correct, 0x%04x\n", ret); > return -ENODEV; > } > - return 0; > + ret = i2c_smbus_read_word_data(i2c, TCPC_BCD_DEV); > + if (ret < 0) > + return ret; > + dev_info(&i2c->dev, "did is 0x%04x\n", ret); Unnecessary noise. If needed for testing, please make it dev_dbg. > + return ret; I think it would make sense to pass chip as parameter and set chip->did here. Also, validation is missing. This function is supposed to check/validate revision data, but it just accepts all DIDs. Then later DID values are used to distinguish functionality. At the same time, the new device ID and OF compatible strings are not used for that purpose and thus have no real value. Since there can be chips with different DIDs which require different functionality, DID values should be validated, and only chips with supported DIDs should be accepted. Also, since there are separate device IDs and devicetree compatible properties, the DIDs associated with supported chips should be referenced there. Thanks, Guenter > } > > static int rt1711h_probe(struct i2c_client *client, > @@ -265,6 +281,8 @@ static int rt1711h_probe(struct i2c_client *client, > if (!chip) > return -ENOMEM; > > + chip->did = ret; > + > chip->data.regmap = devm_regmap_init_i2c(client, > &rt1711h_regmap_config); > if (IS_ERR(chip->data.regmap)) > @@ -315,6 +333,7 @@ static int rt1711h_remove(struct i2c_client *client) > > static const struct i2c_device_id rt1711h_id[] = { > { "rt1711h", 0 }, > + { "rt1715", 0 }, > { } > }; > MODULE_DEVICE_TABLE(i2c, rt1711h_id); > @@ -322,6 +341,7 @@ MODULE_DEVICE_TABLE(i2c, rt1711h_id); > #ifdef CONFIG_OF > static const struct of_device_id rt1711h_of_match[] = { > { .compatible = "richtek,rt1711h", }, > + { .compatible = "richtek,rt1715", }, > {}, > }; > MODULE_DEVICE_TABLE(of, rt1711h_of_match);
On 7/20/22 23:11, Gene Chen wrote: > From: Gene Chen <gene_chen@richtek.com> > > Fix CC PHY noise filter of voltage level according to > current cc voltage level > > Signed-off-by: Gene Chen <gene_chen@richtek.com> > --- > drivers/usb/typec/tcpm/tcpci_rt1711h.c | 83 +++++++++++++++++++++++++- > 1 file changed, 81 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/typec/tcpm/tcpci_rt1711h.c b/drivers/usb/typec/tcpm/tcpci_rt1711h.c > index 3316dfaeee0d..f0c46bf7f00b 100644 > --- a/drivers/usb/typec/tcpm/tcpci_rt1711h.c > +++ b/drivers/usb/typec/tcpm/tcpci_rt1711h.c > @@ -22,8 +22,11 @@ > #define RT1711H_PHYCTRL1 0x80 > #define RT1711H_PHYCTRL2 0x81 > > -#define RT1711H_RTCTRL8 0x9B > +#define RT1711H_RTCTRL4 0x93 > +/* rx threshold of rd/rp: 1b0 for level 0.4V/0.7V, 1b1 for 0.35V/0.75V */ > +#define RT1711H_BMCIO_RXDZSEL BIT(0) > > +#define RT1711H_RTCTRL8 0x9B > /* Autoidle timeout = (tout * 2 + 1) * 6.4ms */ > #define RT1711H_RTCTRL8_SET(ck300, ship_off, auto_idle, tout) \ > (((ck300) << 7) | ((ship_off) << 5) | \ > @@ -32,7 +35,6 @@ > #define RT1711H_ENEXTMSG_MASK BIT(4) > > #define RT1711H_RTCTRL11 0x9E > - > /* I2C timeout = (tout + 1) * 12.5ms */ > #define RT1711H_RTCTRL11_SET(en, tout) \ > (((en) << 7) | ((tout) & 0x0F)) > @@ -42,6 +44,10 @@ > #define RT1711H_RTCTRL15 0xA2 > #define RT1711H_RTCTRL16 0xA3 > > +#define RT1711H_RTCTRL18 0xAF > +/* 1b0 as fixed rx threshold of rd/rp 0.55V, 1b1 depends on RTCRTL4[0] */ > +#define BMCIO_RXDZEN_MASK BIT(0) I really dislike the use of _MASK for register bit values. > + > struct rt1711h_chip { > struct tcpci_data data; > struct tcpci *tcpci; > @@ -162,6 +168,77 @@ static int rt1711h_set_vconn(struct tcpci *tcpci, struct tcpci_data *tdata, > RT1711H_AUTOIDLEEN_MASK, enable ? 0 : 0xFF); > } > > +#define tcpc_presenting_rd(reg, cc) \ > + (!(TCPC_ROLE_CTRL_DRP & (reg)) && \ > + (((reg) & (TCPC_ROLE_CTRL_## cc ##_MASK << TCPC_ROLE_CTRL_## cc ##_SHIFT)) == \ > + (TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_## cc ##_SHIFT))) > + > +static enum typec_cc_status tcpci_to_typec_cc(unsigned int cc, bool sink) > +{ > + switch (cc) { > + case 0x1: > + return sink ? TYPEC_CC_RP_DEF : TYPEC_CC_RA; > + case 0x2: > + return sink ? TYPEC_CC_RP_1_5 : TYPEC_CC_RD; > + case 0x3: > + if (sink) > + return TYPEC_CC_RP_3_0; > + fallthrough; > + case 0x0: > + default: > + return TYPEC_CC_OPEN; > + } > +} > + The above is a straight copy from tcpci.c. Can it be moved to include/linux/usb/tcpci.h ? > +/* > + * Selects the CC PHY noise filter voltage level according to the current > + * CC voltage level. > + * > + * @param cc_level The CC voltage level for the port's current role I seem to be missing that parameter. > + * @return 0 if writes succeed; failure code otherwise > + */ > +static inline int rt1711h_init_cc_params(struct rt1711h_chip *chip, u8 status) > +{ > + int ret, cc1, cc2; > + u8 role = 0; > + u32 rxdz_en = 0, rxdz_sel = 0; Those variables are always set and thus do not need to be initialized. > + > + ret = rt1711h_read8(chip, TCPC_ROLE_CTRL, &role); > + if (ret < 0) > + return ret; > + > + cc1 = tcpci_to_typec_cc((status >> TCPC_CC_STATUS_CC1_SHIFT) & > + TCPC_CC_STATUS_CC1_MASK, > + status & TCPC_CC_STATUS_TERM || > + tcpc_presenting_rd(role, CC1)); > + cc2 = tcpci_to_typec_cc((status >> TCPC_CC_STATUS_CC2_SHIFT) & > + TCPC_CC_STATUS_CC2_MASK, > + status & TCPC_CC_STATUS_TERM || > + tcpc_presenting_rd(role, CC2)); > + > + if ((cc1 >= TYPEC_CC_RP_1_5 && cc2 < TYPEC_CC_RP_DEF) || > + (cc2 >= TYPEC_CC_RP_1_5 && cc1 < TYPEC_CC_RP_DEF)) { > + if (chip->did == RT1715_DID) { > + rxdz_en = 1; This should be rxdz_en = BMCIO_RXDZEN; > + rxdz_sel = 1; This should be rxdz_sel = RT1711H_BMCIO_RXDZSEL; > + } else { > + rxdz_en = 1; > + rxdz_sel = 0; > + } The assignment of rxdz_en can be moved outside the if/else block. > + } else { > + rxdz_en = 0; > + rxdz_sel = 1; > + } > + > + ret = regmap_update_bits(chip->data.regmap, RT1711H_RTCTRL18, > + BMCIO_RXDZEN_MASK, rxdz_en); > + if (ret < 0) > + return ret; > + > + return regmap_update_bits(chip->data.regmap, RT1711H_RTCTRL4, > + RT1711H_BMCIO_RXDZSEL, rxdz_sel); > +} > + > static int rt1711h_start_drp_toggling(struct tcpci *tcpci, > struct tcpci_data *tdata, > enum typec_cc_status cc) > @@ -222,6 +299,8 @@ static irqreturn_t rt1711h_irq(int irq, void *dev_id) > /* Clear cc change event triggered by starting toggling */ > if (status & TCPC_CC_STATUS_TOGGLING) > rt1711h_write8(chip, TCPC_ALERT, TCPC_ALERT_CC_STATUS); > + else > + rt1711h_init_cc_params(chip, status); > } > > out:
Guenter Roeck <linux@roeck-us.net> 於 2022年7月21日 週四 晚上10:28寫道： > > On 7/20/22 23:11, Gene Chen wrote: > > From: Gene Chen <gene_chen@richtek.com> > > > > Add regulator support when source vbus > > > > Signed-off-by: Gene Chen <gene_chen@richtek.com> > > --- > > drivers/usb/typec/tcpm/tcpci_rt1711h.c | 28 ++++++++++++++++++++++++++ > > 1 file changed, 28 insertions(+) > > > > diff --git a/drivers/usb/typec/tcpm/tcpci_rt1711h.c b/drivers/usb/typec/tcpm/tcpci_rt1711h.c > > index 3309ceace2b2..52c9594e531d 100644 > > --- a/drivers/usb/typec/tcpm/tcpci_rt1711h.c > > +++ b/drivers/usb/typec/tcpm/tcpci_rt1711h.c > > @@ -10,6 +10,7 @@ > > #include <linux/i2c.h> > > #include <linux/interrupt.h> > > #include <linux/gpio/consumer.h> > > +#include <linux/regulator/consumer.h> > > #include <linux/usb/tcpm.h> > > #include <linux/regmap.h> > > #include "tcpci.h" > > @@ -40,6 +41,8 @@ struct rt1711h_chip { > > struct tcpci_data data; > > struct tcpci *tcpci; > > struct device *dev; > > + struct regulator *vbus; > > + bool src_en; > > }; > > > > static int rt1711h_read16(struct rt1711h_chip *chip, unsigned int reg, u16 *val) > > @@ -103,6 +106,26 @@ static int rt1711h_init(struct tcpci *tcpci, struct tcpci_data *tdata) > > > > /* dcSRC.DRP : 33% */ > > return rt1711h_write16(chip, RT1711H_RTCTRL16, 330); > > + > > +} > > + > > +static int rt1711h_set_vbus(struct tcpci *tcpci, struct tcpci_data *tdata, > > + bool src, bool snk) > > +{ > > + struct rt1711h_chip *chip = tdata_to_rt1711h(tdata); > > + int ret; > > + > > + if (chip->src_en == src) > > + return 1; > > + > > + if (src) > > + ret = regulator_enable(chip->vbus); > > + else > > + ret = regulator_disable(chip->vbus); > > + > > + if (!ret) > > + chip->src_en = src; > > + return ret ? ret : 1; > > Are you sure this is what you want ? Returning 1 bypasses the code setting > the vbus registers in tcpci.c. If that is on purpose it might make sense > to explain it. > ACK, return 0 is more compatible with next generation chip, and writing tcpci vbus command won't affect to ic if not supported. > > } > > > > static int rt1711h_set_vconn(struct tcpci *tcpci, struct tcpci_data *tdata, > > @@ -246,7 +269,12 @@ static int rt1711h_probe(struct i2c_client *client, > > if (ret < 0) > > return ret; > > > > + chip->vbus = devm_regulator_get(&client->dev, "vbus"); > > + if (IS_ERR(chip->vbus)) > > + return PTR_ERR(chip->vbus); > > + > > This makes regulator support mandatory, which so far was not the case. > That warrants an explanation why it is not a problem for existing users. > We verified ic behavior as SNK only, because we couldn't add tcpci set vbus callback and external boost otg vbus. And we use our own type-c state machine and pd policy engine for mass production to user. > Thanks, > Guenter > > > chip->data.init = rt1711h_init; > > + chip->data.set_vbus = rt1711h_set_vbus; > > chip->data.set_vconn = rt1711h_set_vconn; > > chip->data.start_drp_toggling = rt1711h_start_drp_toggling; > > chip->tcpci = tcpci_register_port(chip->dev, &chip->data); >
Guenter Roeck <linux@roeck-us.net> 於 2022年7月21日 週四 晚上10:44寫道： > > On 7/20/22 23:11, Gene Chen wrote: > > From: Gene Chen <gene_chen@richtek.com> > > > > Add compatible id with rt1715, and add initial setting for > > specific support PD30 command. > > > > Signed-off-by: Gene Chen <gene_chen@richtek.com> > > --- > > drivers/usb/typec/tcpm/tcpci_rt1711h.c | 24 ++++++++++++++++++++++-- > > 1 file changed, 22 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/usb/typec/tcpm/tcpci_rt1711h.c b/drivers/usb/typec/tcpm/tcpci_rt1711h.c > > index e65b568959e9..3316dfaeee0d 100644 > > --- a/drivers/usb/typec/tcpm/tcpci_rt1711h.c > > +++ b/drivers/usb/typec/tcpm/tcpci_rt1711h.c > > @@ -17,6 +17,7 @@ > > > > #define RT1711H_VID 0x29CF > > #define RT1711H_PID 0x1711 > > +#define RT1715_DID 0x2173 > > > > #define RT1711H_PHYCTRL1 0x80 > > #define RT1711H_PHYCTRL2 0x81 > > @@ -28,6 +29,7 @@ > > (((ck300) << 7) | ((ship_off) << 5) | \ > > ((auto_idle) << 3) | ((tout) & 0x07)) > > #define RT1711H_AUTOIDLEEN_MASK BIT(3) > > +#define RT1711H_ENEXTMSG_MASK BIT(4) > > I would suggest to drop _MASK. > > > > > #define RT1711H_RTCTRL11 0x9E > > > > @@ -46,6 +48,7 @@ struct rt1711h_chip { > > struct device *dev; > > struct regulator *vbus; > > bool src_en; > > + u16 did; > > }; > > > > static int rt1711h_read16(struct rt1711h_chip *chip, unsigned int reg, u16 *val) > > @@ -82,8 +85,9 @@ static struct rt1711h_chip *tdata_to_rt1711h(struct tcpci_data *tdata) > > > > static int rt1711h_init(struct tcpci *tcpci, struct tcpci_data *tdata) > > { > > - int ret; > > struct rt1711h_chip *chip = tdata_to_rt1711h(tdata); > > + struct regmap *regmap = chip->data.regmap; > > + int ret; > > > > /* CK 300K from 320K, shipping off, auto_idle enable, tout = 32ms */ > > ret = rt1711h_write8(chip, RT1711H_RTCTRL8, > > @@ -91,6 +95,14 @@ static int rt1711h_init(struct tcpci *tcpci, struct tcpci_data *tdata) > > if (ret < 0) > > return ret; > > > > + /* Enable PD30 extended message for RT1715 */ > > + if (chip->did == RT1715_DID) { > > + ret = regmap_update_bits(regmap, RT1711H_RTCTRL8, > > + RT1711H_ENEXTMSG_MASK, 0xFF); > > 0xFF -> RT1711H_ENEXTMSG > > > + if (ret < 0) > > + return ret; > > + } > > + > > /* I2C reset : (val + 1) * 12.5ms */ > > ret = rt1711h_write8(chip, RT1711H_RTCTRL11, > > RT1711H_RTCTRL11_SET(1, 0x0F)); > > @@ -246,7 +258,11 @@ static int rt1711h_check_revision(struct i2c_client *i2c) > > dev_err(&i2c->dev, "pid is not correct, 0x%04x\n", ret); > > return -ENODEV; > > } > > - return 0; > > + ret = i2c_smbus_read_word_data(i2c, TCPC_BCD_DEV); > > + if (ret < 0) > > + return ret; > > + dev_info(&i2c->dev, "did is 0x%04x\n", ret); > > Unnecessary noise. If needed for testing, please make it dev_dbg. > > > + return ret; > > I think it would make sense to pass chip as parameter and set chip->did here. > > Also, validation is missing. This function is supposed to check/validate > revision data, but it just accepts all DIDs. Then later DID values are used > to distinguish functionality. At the same time, the new device ID and OF > compatible strings are not used for that purpose and thus have no real value. > > Since there can be chips with different DIDs which require different functionality, > DID values should be validated, and only chips with supported DIDs should be > accepted. Also, since there are separate device IDs and devicetree compatible > properties, the DIDs associated with supported chips should be referenced there. > > Thanks, > Guenter > ACK, I will add RT1711H_DID, and check did in order to only support for rt1711h and rt1715. The difference between RT1711H and RT1715 is that whether PD3.0 is supported and rx dead zone workaround setting. PD3.0 is set in pd_version of typec_caps which is fixed while calling tcpm_register_port. Therefore, the data of struct of_device_id isn't needed. Should I just remove the compatible name "rt1715"? > > } > > > > static int rt1711h_probe(struct i2c_client *client, > > @@ -265,6 +281,8 @@ static int rt1711h_probe(struct i2c_client *client, > > if (!chip) > > return -ENOMEM; > > > > + chip->did = ret; > > + > > chip->data.regmap = devm_regmap_init_i2c(client, > > &rt1711h_regmap_config); > > if (IS_ERR(chip->data.regmap)) > > @@ -315,6 +333,7 @@ static int rt1711h_remove(struct i2c_client *client) > > > > static const struct i2c_device_id rt1711h_id[] = { > > { "rt1711h", 0 }, > > + { "rt1715", 0 }, > > { } > > }; > > MODULE_DEVICE_TABLE(i2c, rt1711h_id); > > @@ -322,6 +341,7 @@ MODULE_DEVICE_TABLE(i2c, rt1711h_id); > > #ifdef CONFIG_OF > > static const struct of_device_id rt1711h_of_match[] = { > > { .compatible = "richtek,rt1711h", }, > > + { .compatible = "richtek,rt1715", }, > > {}, > > }; > > MODULE_DEVICE_TABLE(of, rt1711h_of_match); >