Re: [PATCH v3 2/2] usb: typec: rt1719: Add support for Richtek RT1719

[Heikki Krogerus]

Hi,

Sorry, there is still one more problem that I realised - not your
problem, but a problem with fw_devlink_purge_absent_suppliers()...

On Mon, Feb 07, 2022 at 11:16:10PM +0800, cy_huang wrote:
> +static int rt1719_probe(struct i2c_client *i2c)
> +{
> +	struct rt1719_data *data;
> +	struct fwnode_handle *fwnode;
> +	struct typec_capability typec_cap = { };
> +	int ret;
> +
> +	data = devm_kzalloc(&i2c->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->dev = &i2c->dev;
> +	init_completion(&data->req_completion);
> +
> +	data->regmap = devm_regmap_init_i2c(i2c, &rt1719_regmap_config);
> +	if (IS_ERR(data->regmap)) {
> +		ret = PTR_ERR(data->regmap);
> +		dev_err(&i2c->dev, "Failed to init regmap (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	ret = rt1719_check_exist(data);
> +	if (ret)
> +		return ret;
> +
> +	ret = rt1719_get_caps(data);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * This fwnode has a "compatible" property, but is never populated as a
> +	 * struct device. Instead we simply parse it to read the properties.
> +	 * This it breaks fw_devlink=on. To maintain backward compatibility
> +	 * with existing DT files, we work around this by deleting any
> +	 * fwnode_links to/from this fwnode.
> +	 */
> +	fwnode = device_get_named_child_node(&i2c->dev, "connector");
> +	if (!fwnode)
> +		return -ENODEV;
> +
> +	fw_devlink_purge_absent_suppliers(fwnode);

So don't use that function unless you really see some issue that it's
fixing yourself.

That function is broken. It breaks at least if the fwnode is shared -
fwnodes can be, and are, shared - most likely it's broken in
some other ways too. That function seems to be a hack for some
individual problem that was caused by some deeper problem with the
device links.

We really need to figure out a fix for the core problem now instead of
trying to come up with these hacks for every single separate scenario
that is breaking because of the core problem, what ever that core
problem may be.

> +	data->role_sw = fwnode_usb_role_switch_get(fwnode);
> +	if (IS_ERR(data->role_sw)) {
> +		ret = PTR_ERR(data->role_sw);
> +		dev_err(&i2c->dev, "Failed to get usb role switch (%d)\n", ret);
> +		goto err_fwnode_put;
> +	}
> +
> +	ret = devm_rt1719_psy_register(data);
> +	if (ret) {
> +		dev_err(&i2c->dev, "Failed to register psy (%d)\n", ret);
> +		goto err_role_put;
> +	}
> +
> +	typec_cap.revision = USB_TYPEC_REV_1_2;
> +	typec_cap.pd_revision = 0x300;	/* USB-PD spec release 3.0 */
> +	typec_cap.type = TYPEC_PORT_SNK;
> +	typec_cap.data = TYPEC_PORT_DRD;
> +	typec_cap.ops = &rt1719_port_ops;
> +	typec_cap.fwnode = fwnode;
> +	typec_cap.driver_data = data;
> +	typec_cap.accessory[0] = TYPEC_ACCESSORY_DEBUG;
> +
> +	data->partner_desc.identity = &data->partner_ident;
> +
> +	data->port = typec_register_port(&i2c->dev, &typec_cap);
> +	if (IS_ERR(data->port)) {
> +		ret = PTR_ERR(data->port);
> +		dev_err(&i2c->dev, "Failed to register typec port (%d)\n", ret);
> +		goto err_role_put;
> +	}
> +
> +	ret = rt1719_init_attach_state(data);
> +	if (ret) {
> +		dev_err(&i2c->dev, "Failed to init attach state (%d)\n", ret);
> +		goto err_role_put;
> +	}
> +
> +	ret = rt1719_irq_init(data);
> +	if (ret) {
> +		dev_err(&i2c->dev, "Failed to init irq\n");
> +		goto err_role_put;
> +	}
> +
> +	fwnode_handle_put(fwnode);
> +
> +	i2c_set_clientdata(i2c, data);
> +
> +	return 0;
> +
> +err_role_put:
> +	usb_role_switch_put(data->role_sw);
> +err_fwnode_put:
> +	fwnode_handle_put(fwnode);
> +
> +	return ret;
> +}
> +
> +static int rt1719_remove(struct i2c_client *i2c)
> +{
> +	struct rt1719_data *data = i2c_get_clientdata(i2c);
> +
> +	typec_unregister_port(data->port);
> +	usb_role_switch_put(data->role_sw);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id __maybe_unused rt1719_device_table[] = {
> +	{ .compatible = "richtek,rt1719", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, rt1719_device_table);
> +
> +static struct i2c_driver rt1719_driver = {
> +	.driver = {
> +		.name = "rt1719",
> +		.of_match_table = rt1719_device_table,
> +	},
> +	.probe_new = rt1719_probe,
> +	.remove = rt1719_remove,
> +};
> +module_i2c_driver(rt1719_driver);
> +
> +MODULE_AUTHOR("ChiYuan Huang <cy_huang@xxxxxxxxxxx>");
> +MODULE_DESCRIPTION("Richtek RT1719 Sink Only USBPD Controller Driver");
> +MODULE_LICENSE("GPL v2");

Oh, you probable want the make that "GPL" instead. See
Documentation/process/license-rules.rst.

thanks,

-- 
heikki