Message-ID: <CAE+NS36b=8XXEHs94CZbvX8_LVcN9z1Fx-YyuCTCJ6TgJ_DAxQ@mail.gmail.com>
Date:   Mon, 25 Jul 2022 11:04:05 +0800
From:   Gene Chen <gene.chen.richtek@...il.com>
To:     Guenter Roeck <linux@...ck-us.net>
Cc:     Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Rob Herring <robh+dt@...nel.org>,
        krzysztof.kozlowski+dt@...aro.org, linux-usb@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        Gene Chen <gene_chen@...htek.com>,
        ChiYuan Huang <cy_huang@...htek.com>
Subject: Re: [PATCH v2 5/6] usb: typec: tcpci_rt1711h: Add compatible id with rt1715

Guenter Roeck <linux@...ck-us.net> 於 2022年7月21日 週四 晚上10:44寫道：
>
> On 7/20/22 23:11, Gene Chen wrote:
> > From: Gene Chen <gene_chen@...htek.com>
> >
> > Add compatible id with rt1715, and add initial setting for
> > specific support PD30 command.
> >
> > Signed-off-by: Gene Chen <gene_chen@...htek.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);
>

Powered by blists - more mailing lists