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

[Krzysztof Kozlowski <krzk@kernel.org>]

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

> +	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.

> +
> +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

> +	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.

> +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