Re: [PATCH 2/4] usb: typec: add CIX Type-C PD controller driver

[Gary Yang <gary.yang@cixtech.com>]

Hi Krzysztof:

Thanks for your comments!

On 2026-11-07 11:56, Krzysztof Kozlowski wrote:

> EXTERNAL EMAIL
> 
> On 07/11/2026 02:10, Gary Yang wrote:
> >  CLANG CONTEXT ANALYSIS
> >  M:   Marco Elver <elver@google.com>
> >  R:   Bart Van Assche <bvanassche@acm.org>
> > diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
> > index 2f80c2792dbd..b81a0d946d00 100644
> > --- a/drivers/usb/typec/Kconfig
> > +++ b/drivers/usb/typec/Kconfig
> > @@ -110,6 +110,17 @@ config TYPEC_WUSB3801
> >         If you choose to build this driver as a dynamically linked module, the
> >         module will be called wusb3801.ko.
> >
> > +config TYPEC_CIX_PD
> > +     tristate "CIX Type-C PD controller driver"
> > +     depends on I2C
> 
> depends on your ARCH || COMPILE_TEST

agree. add it in v2.

> 
> > +     select REGMAP_I2C
> > +     help
> > +       Say Y or M here if your system has a CIX Type-C PD controller.
> > +       This driver currently supports the Realtek RTS5453H controller.
> > +
> > +       If you choose to build this driver as a dynamically linked module, the
> > +       module will be called cix_pd.ko.
> 
> 
> ...
> 
> 
> > +static int cix_pd_init_ports(struct cix_pd *typec)
> > +{
> > +     struct device *dev = typec->dev;
> > +     struct typec_altmode_desc desc = cix_pd_dp_altmode;
> > +     int ret, nports;
> > +
> > +     nports = device_get_child_node_count(dev);
> > +     if (nports == 0) {
> > +             dev_err(dev, "No port entries found.\n");
> > +             return -ENODEV;
> > +     }
> > +
> > +     if (nports > 1) {
> > +             dev_err(dev, "More ports listed than can be supported.\n");
> > +             return -EINVAL;
> > +     }
> > +
> > +     device_for_each_child_node_scoped(dev, fwnode) {
> > +             ret = fwnode_property_read_u32(fwnode, "reg", &typec->id);
> > +             if (ret)
> > +                     goto unregister_ports;
> > +
> > +             ret = cix_pd_typec_parse_port_props(&typec->caps, fwnode,
> > +                                                 dev);
> > +             if (ret)
> > +                     goto unregister_ports;
> > +
> > +             typec->port = typec_register_port(dev, &typec->caps);
> > +             if (IS_ERR(typec->port)) {
> > +                     ret = PTR_ERR(typec->port);
> > +                     typec->port = NULL;
> > +                     dev_err(dev, "Failed to register port: %d\n", ret);
> > +                     goto unregister_ports;
> > +             }
> > +
> > +             desc.roles = typec->caps.data;
> > +             typec->port_altmode = typec_port_register_altmode(typec->port,
> > +                                                               &desc);
> > +             if (IS_ERR(typec->port_altmode)) {
> > +                     ret = PTR_ERR(typec->port_altmode);
> > +                     typec->port_altmode = NULL;
> > +                     dev_err(dev, "Failed to register DP altmode: %d\n", ret);
> > +                     goto unregister_ports;
> > +             }
> > +
> > +             ret = cix_pd_typec_get_handles(typec, fwnode, dev);
> > +             if (ret)
> > +                     goto unregister_ports;
> > +     }
> > +
> > +     return 0;
> > +
> > +unregister_ports:
> > +     cix_pd_unregister_ports(typec);
> > +     return ret;
> > +}
> > +
> > +/*
> > + * The IRQ is disabled when either the user asked for it or the device is
> > + * suspended. Keep the disable/enable calls balanced and idempotent.
> > + */
> > +static void cix_pd_sync_irq(struct cix_pd *rts)
> > +{
> > +     bool disable = rts->pd_int_disabled || rts->suspended;
> > +
> > +     if (!rts->irq_requested || rts->irq_disabled == disable)
> > +             return;
> > +
> > +     rts->irq_disabled = disable;
> > +     if (disable)
> > +             disable_irq(rts->irq);
> > +     else
> > +             enable_irq(rts->irq);
> > +}
> > +
> > +static ssize_t disable_pd_interrupt_show(struct device *dev,
> > +                                      struct device_attribute *attr,
> > +                                      char *buf)
> > +{
> > +     struct i2c_client *client = to_i2c_client(dev);
> > +     struct cix_pd *rts = i2c_get_clientdata(client);
> > +     const char *p;
> > +
> > +     if (rts->pd_int_disabled)
> > +             p = "disabled";
> > +     else
> > +             p = "enabled";
> > +
> > +     return sysfs_emit(buf, "%s\n", p);
> > +}
> > +
> > +static ssize_t disable_pd_interrupt_store(struct device *dev,
> > +                                       struct device_attribute *attr,
> > +                                       const char *buf, size_t count)
> > +{
> > +     struct i2c_client *client = to_i2c_client(dev);
> > +     struct cix_pd *rts = i2c_get_clientdata(client);
> > +     bool value;
> > +     int ret;
> > +
> > +     ret = kstrtobool(buf, &value);
> > +     if (ret)
> > +             return ret;
> > +
> > +     mutex_lock(&rts->irq_lock);
> > +     rts->pd_int_disabled = value;
> > +     cix_pd_sync_irq(rts);
> > +     mutex_unlock(&rts->irq_lock);
> > +
> > +     dev_alert(dev, "Only for USB Compliance test, PD int is %s\n",
> > +               value ? "disabled" : "enabled");
> 
> What?
> 
> > +
> > +     return count;
> > +}
> > +
> > +DEVICE_ATTR_RW(disable_pd_interrupt);
> 
> You need to document the ABI. But really, we do not enable/disable
> interrupts through custom sysfs hooks. Drop.
> 

agree. drop it in v2

> > +
> > +static int cix_pd_probe(struct i2c_client *client)
> > +{
> > +     struct cix_pd *rts;
> > +     int ret;
> > +
> > +     dev_dbg(&client->dev, "IRQ %d supplied\n", client->irq);
> 
> Drop

agree

> 
> > +     rts = devm_kzalloc(&client->dev, sizeof(*rts), GFP_KERNEL);
> > +     if (!rts)
> > +             return -ENOMEM;
> > +
> > +     rts->dev = &client->dev;
> > +     rts->irq = client->irq;
> > +     mutex_init(&rts->lock);
> > +     mutex_init(&rts->irq_lock);
> > +     cix_pd_typec_init_state(rts);
> > +
> > +     if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> > +             return dev_err_probe(&client->dev, -ENODEV,
> > +                                  "i2c check functionality failed\n");
> > +
> > +     rts->regmap = devm_regmap_init_i2c(client, &cix_pd_regmap_config);
> > +     if (IS_ERR(rts->regmap))
> > +             return dev_err_probe(&client->dev, PTR_ERR(rts->regmap),
> > +                                  "failed to init regmap\n");
> > +
> > +     i2c_set_clientdata(client, rts);
> > +
> > +     ret = cix_pd_init_ports(rts);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = cix_pd_event_handler(rts, 0);
> > +     if (ret) {
> > +             dev_err(&client->dev, "failed to read initial status: %d\n", ret);
> > +             if (ret == -ESHUTDOWN)
> > +                     ret = -EIO;
> > +             goto err_unregister_ports;
> > +     }
> > +
> > +     if (client->irq > 0) {
> > +             irq_set_status_flags(client->irq, IRQ_DISABLE_UNLAZY);
> > +             ret = request_threaded_irq(client->irq, NULL, cix_pd_irq_handle,
> > +                                        IRQF_SHARED | IRQF_ONESHOT,
> > +                                        dev_name(&client->dev), rts);
> > +             if (ret) {
> > +                     dev_err(&client->dev, "failed to request irq: %d\n", ret);
> > +                     goto err_unregister_ports;
> > +             }
> > +             rts->irq_requested = true;
> > +     }
> > +
> > +     ret = device_create_file(&client->dev, &dev_attr_disable_pd_interrupt);
> > +     if (ret)
> > +             dev_warn(&client->dev,
> > +                      "Failed to create sysfs attribute, ret = %d\n", ret);
> > +
> > +     device_set_wakeup_capable(&client->dev, true);
> > +
> > +     return 0;
> > +
> > +err_unregister_ports:
> > +     cix_pd_unregister_ports(rts);
> > +     return ret;
> > +}
> > +
> > +static void cix_pd_remove(struct i2c_client *client)
> > +{
> > +     struct cix_pd *rts = i2c_get_clientdata(client);
> > +
> > +     device_remove_file(&client->dev, &dev_attr_disable_pd_interrupt);
> > +
> > +     mutex_lock(&rts->irq_lock);
> > +     if (rts->irq_requested) {
> > +             rts->suspended = true;
> > +             cix_pd_sync_irq(rts);
> > +             if (rts->wake_enabled) {
> > +                     disable_irq_wake(rts->irq);
> > +                     rts->wake_enabled = false;
> > +             }
> > +             free_irq(rts->irq, rts);
> > +             rts->irq_requested = false;
> > +     }
> > +     mutex_unlock(&rts->irq_lock);
> > +
> > +     cix_pd_unregister_ports(rts);
> > +}
> > +
> > +static const struct of_device_id cix_pd_of_match[] = {
> > +     { .compatible = "realtek,rts5453h", },
> > +     {}
> > +};
> > +MODULE_DEVICE_TABLE(of, cix_pd_of_match);
> > +
> > +static const struct i2c_device_id cix_pd_id[] = {
> > +     { "rts5453h" },
> > +     {}
> > +};
> > +
> > +static int __maybe_unused cix_pd_suspend(struct device *dev)
> > +{
> > +     struct i2c_client *client = to_i2c_client(dev);
> > +     struct cix_pd *rts = i2c_get_clientdata(client);
> > +
> > +     mutex_lock(&rts->irq_lock);
> > +     if (rts->irq_requested) {
> > +             rts->suspended = true;
> > +             cix_pd_sync_irq(rts);
> > +             if (device_may_wakeup(dev)) {
> > +                     enable_irq_wake(rts->irq);
> > +                     rts->wake_enabled = true;
> > +             }
> > +     }
> > +     mutex_unlock(&rts->irq_lock);
> > +
> > +     return 0;
> > +}
> > +
> > +static int __maybe_unused cix_pd_resume(struct device *dev)
> > +{
> > +     struct i2c_client *client = to_i2c_client(dev);
> > +     struct cix_pd *rts = i2c_get_clientdata(client);
> > +
> > +     mutex_lock(&rts->irq_lock);
> > +     if (rts->irq_requested) {
> > +             if (rts->wake_enabled) {
> > +                     disable_irq_wake(rts->irq);
> > +                     rts->wake_enabled = false;
> > +             }
> > +             rts->suspended = false;
> > +             cix_pd_sync_irq(rts);
> > +     }
> > +     mutex_unlock(&rts->irq_lock);
> > +
> > +     return 0;
> > +}
> > +
> > +static int __maybe_unused cix_pd_restore(struct device *dev)
> > +{
> > +     struct i2c_client *client = to_i2c_client(dev);
> > +     struct cix_pd *rts = i2c_get_clientdata(client);
> > +     int ret;
> > +
> > +     ret = cix_pd_resume(dev);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* Re-trigger PD event if PD event occurs during std */
> > +     ret = cix_pd_event_handler(rts, 0);
> > +     if (ret)
> > +             dev_err(&client->dev, "typec event handling error %d\n", ret);
> > +
> > +     return ret;
> > +}
> > +
> > +static void cix_pd_shutdown(struct i2c_client *client)
> > +{
> > +     struct cix_pd *rts = i2c_get_clientdata(client);
> > +
> > +     mutex_lock(&rts->irq_lock);
> > +     rts->suspended = true;
> > +     cix_pd_sync_irq(rts);
> > +     mutex_unlock(&rts->irq_lock);
> > +}
> > +
> > +static const struct dev_pm_ops cix_pd_pm_ops = {
> > +     .suspend = pm_sleep_ptr(cix_pd_suspend),
> > +     .resume = pm_sleep_ptr(cix_pd_resume),
> > +     .freeze = pm_sleep_ptr(cix_pd_suspend),
> > +     .thaw = pm_sleep_ptr(cix_pd_resume),
> > +     .restore = pm_sleep_ptr(cix_pd_restore),
> > +};
> > +
> > +MODULE_DEVICE_TABLE(i2c, cix_pd_id);
> 
> This is placed next to the table.

Yes, you're right. It's my mistakes and fix it in v2.

If I miss your any information, please kindly remind me.

Best Regards,
Gary
> 
> > +static struct i2c_driver cix_pd_i2c_driver = {
> > +     .driver = {
> > +             .name = "rts5453h",
> > +             .of_match_table = cix_pd_of_match,
> > +             .pm = pm_ptr(&cix_pd_pm_ops),
> > +     },
> > +     .id_table = cix_pd_id,
> > +     .probe = cix_pd_probe,
> > +     .remove = cix_pd_remove,
> > +     .shutdown = cix_pd_shutdown,
> > +};
> > +
> > +module_i2c_driver(cix_pd_i2c_driver);
> > +MODULE_DESCRIPTION("CIX Type-C PD controller driver");
> > +MODULE_AUTHOR("Chao Zeng <Chao.Zeng@cixtech.com>");
> > +MODULE_AUTHOR("Gary Yang <gary.yang@cixtech.com>");
> > +MODULE_LICENSE("GPL");
> 
> 
> Best regards,
> Krzysztof