Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Sender: Guenter Roeck <groeck7@gmail.com>
Message-ID: <0eb24b16-7837-cc79-fbb9-da8ceec72e63@roeck-us.net>
Date:   Thu, 21 Jul 2022 07:44:16 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
Content-Language: en-US
To:     Gene Chen <gene.chen.richtek@gmail.com>,
        heikki.krogerus@linux.intel.com, gregkh@linuxfoundation.org,
        robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org
Cc:     linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
        devicetree@vger.kernel.org, gene_chen@richtek.com,
        cy_huang@richtek.com
References: <20220721061144.35139-1-gene.chen.richtek@gmail.com>
 <20220721061144.35139-6-gene.chen.richtek@gmail.com>
From:   Guenter Roeck <linux@roeck-us.net>
Subject: Re: [PATCH v2 5/6] usb: typec: tcpci_rt1711h: Add compatible id with
 rt1715
In-Reply-To: <20220721061144.35139-6-gene.chen.richtek@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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);