Re: FW: [PATCH 2/2] usb: typec: tcpci_rt1718s: Add Richtek RT1718S tcpci driver
From: Guenter Roeck
Date: Thu Jan 12 2023 - 09:28:15 EST
On 1/12/23 00:05, Gene Chen wrote:
-----Original Message-----
From: Guenter Roeck [mailto:groeck7@xxxxxxxxx] On Behalf Of Guenter Roeck
Sent: Tuesday, January 10, 2023 2:52 AM
To: gene_chen(陳俊宇) <gene_chen@xxxxxxxxxxx>; robh+dt@xxxxxxxxxx; krzysztof.kozlowski+dt@xxxxxxxxxx; heikki.krogerus@xxxxxxxxxxxxxxx
Cc: gregkh@xxxxxxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
Subject: Re: [PATCH 2/2] usb: typec: tcpci_rt1718s: Add Richtek RT1718S tcpci driver
On 1/9/23 01:31, gene_chen@xxxxxxxxxxx wrote:
From: Gene Chen <gene_chen@xxxxxxxxxxx>
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@xxxxxxxxxxx>
---
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@xxxxxxxxxxx>");
+MODULE_DESCRIPTION("RT1718S USB Type-C Port Controller Interface
+Driver"); MODULE_LICENSE("GPL");