Re: [PATCH 2/2] i3c: i3c-hub: Add Renesas RG3MxxB12A1 I3C HUB driver

[Krzysztof Kozlowski]

On 17/02/2024 14:14, Steven Niu wrote:
>  endif # I3C
> diff --git a/drivers/i3c/Makefile b/drivers/i3c/Makefile
> index 11982efbc6d9..ca298faaee9a 100644
> --- a/drivers/i3c/Makefile
> +++ b/drivers/i3c/Makefile
> @@ -2,3 +2,4 @@
>  i3c-y				:= device.o master.o
>  obj-$(CONFIG_I3C)		+= i3c.o
>  obj-$(CONFIG_I3C)		+= master/
> +obj-$(CONFIG_I3C_HUB)	+= i3c-hub.o
> diff --git a/drivers/i3c/i3c-hub.c b/drivers/i3c/i3c-hub.c
> new file mode 100644
> index 000000000000..73a9b96e6635
> --- /dev/null
> +++ b/drivers/i3c/i3c-hub.c
> @@ -0,0 +1,1982 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (C) 2021 - 2023 Intel Corporation.*/


So this explains the code... You push to us some vendor stuff with
bindings not up to any upstream style. Please clean the bindings first
so they look like other bindings.


> +
> +static int read_backend_from_i3c_hub_dts(struct device_node *i3c_node_target,
> +					 struct i3c_hub *priv)
> +{
> +	struct device_node *i3c_node_tp;

Your coding style is terrible. device_node are called np or node.

> +	const char *compatible;
> +	int tp_port, ret;
> +	u32 addr_dts;
> +	struct smbus_backend *backend;
> +
> +	if (sscanf(i3c_node_target->full_name, "target-port@%d", &tp_port) == 0)
> +		return -EINVAL;
> +
> +	if (tp_port > I3C_HUB_TP_MAX_COUNT)
> +		return -ERANGE;
> +
> +	if (tp_port < 0)
> +		return -EINVAL;
> +
> +	INIT_LIST_HEAD(&priv->logical_bus[tp_port].smbus_port_adapter.backend_entry);
> +	for_each_available_child_of_node(i3c_node_target, i3c_node_tp) {
> +		if (strcmp(i3c_node_tp->name, "backend"))

No, don't compare names. What for?

> +			continue;
> +
> +		ret = of_property_read_u32(i3c_node_tp, "target-reg", &addr_dts);
> +		if (ret)
> +			return ret;
> +
> +		if (backend_node_is_exist(tp_port, priv, addr_dts))
> +			continue;
> +
> +		ret = of_property_read_string(i3c_node_tp, "compatible", &compatible);
> +		if (ret)
> +			return ret;
> +
> +		/* Currently only the slave-mqueue backend is supported */
> +		if (strcmp("slave-mqueue", compatible))

NAK, undocumented compatible.

> +			return -EINVAL;
> +
> +		backend = kzalloc(sizeof(*backend), GFP_KERNEL);
> +		if (!backend)
> +			return -ENOMEM;
> +
> +		backend->addr = addr_dts;
> +		backend->compatible = compatible;
> +		list_add(&backend->list,
> +			 &priv->logical_bus[tp_port].smbus_port_adapter.backend_entry);
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * This function saves information about the i3c_hub's ports
> + * working in slave mode. It takes its data from the DTs
> + * (aspeed-bmc-intel-avc.dts) and saves the parameters
> + * into the coresponding target port i2c_adapter_group structure
> + * in the i3c_hub
> + *
> + * @dev: device used by i3c_hub
> + * @i3c_node_hub: device node pointing to the hub
> + * @priv: pointer to the i3c_hub structure
> + */
> +static void i3c_hub_parse_dt_tp(struct device *dev,
> +				const struct device_node *i3c_node_hub,
> +				struct i3c_hub *priv)
> +{
> +	struct device_node *i3c_node_target;
> +	int ret;
> +
> +	for_each_available_child_of_node(i3c_node_hub, i3c_node_target) {
> +		if (!strcmp(i3c_node_target->name, "target-port")) {
> +			ret = read_backend_from_i3c_hub_dts(i3c_node_target, priv);
> +			if (ret)
> +				dev_err(dev, "DTS entry invalid - error %d", ret);
> +		}
> +	}
> +}
> +
> +static int i3c_hub_probe(struct i3c_device *i3cdev)
> +{
> +	struct regmap_config i3c_hub_regmap_config = {
> +		.reg_bits = 8,
> +		.val_bits = 8,
> +	};
> +	struct device *dev = &i3cdev->dev;
> +	struct device_node *node = NULL;
> +	struct regmap *regmap;
> +	struct i3c_hub *priv;
> +	char hub_id[32];
> +	int ret;
> +	int i;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->i3cdev = i3cdev;
> +	priv->driving_master = i3c_dev_get_master(i3cdev->desc);
> +	i3cdev_set_drvdata(i3cdev, priv);
> +	INIT_DELAYED_WORK(&priv->delayed_work, i3c_hub_delayed_work);
> +	sprintf(hub_id, "i3c-hub-%d-%llx", i3c_dev_get_master(i3cdev->desc)->bus.id,
> +		i3cdev->desc->info.pid);
> +	ret = i3c_hub_debugfs_init(priv, hub_id);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to initialized DebugFS.\n");

Drop, you cannot fail probe on debugfs.

> +
> +	i3c_hub_of_default_configuration(dev);
> +
> +	regmap = devm_regmap_init_i3c(i3cdev, &i3c_hub_regmap_config);
> +	if (IS_ERR(regmap)) {
> +		ret = PTR_ERR(regmap);
> +		dev_err(dev, "Failed to register I3C HUB regmap\n");
> +		goto error;
> +	}
> +	priv->regmap = regmap;
> +
> +	ret = i3c_hub_read_id(dev);
> +	if (ret)
> +		goto error;
> +
> +	priv->hub_dt_sel_id = -1;
> +	priv->hub_dt_cp1_id = -1;
> +	if (priv->hub_pin_cp1_id >= 0 && priv->hub_pin_sel_id >= 0)
> +		/* Find hub node in DT matching HW ID or just first without ID provided in DT */
> +		node = i3c_hub_get_dt_hub_node(dev->parent->of_node, priv);
> +
> +	if (!node) {
> +		dev_info(dev, "No DT entry - running with hardware defaults.\n");
> +	} else {
> +		of_node_get(node);
> +		i3c_hub_of_get_conf_static(dev, node);
> +		i3c_hub_of_get_conf_runtime(dev, node);
> +		of_node_put(node);
> +
> +		/* Parse DTS to find data on the SMBus target mode */
> +		i3c_hub_parse_dt_tp(dev, node, priv);
> +	}
> +
> +	/* Unlock access to protected registers */
> +	ret = regmap_write(priv->regmap, I3C_HUB_PROTECTION_CODE, REGISTERS_UNLOCK_CODE);
> +	if (ret) {
> +		dev_err(dev, "Failed to unlock HUB's protected registers\n");
> +		goto error;
> +	}
> +
> +	/* Register logic for native smbus ports */
> +	for (i = 0; i < I3C_HUB_TP_MAX_COUNT; i++) {
> +		priv->logical_bus[i].smbus_port_adapter.used = 0;
> +		if (priv->settings.tp[i].mode == I3C_HUB_DT_TP_MODE_SMBUS)
> +			ret = i3c_hub_smbus_tp_algo(priv, i);
> +	}
> +
> +	ret = i3c_hub_configure_hw(dev);
> +	if (ret) {
> +		dev_err(dev, "Failed to configure the HUB\n");
> +		goto error;
> +	}
> +
> +	/* Lock access to protected registers */
> +	ret = regmap_write(priv->regmap, I3C_HUB_PROTECTION_CODE, REGISTERS_LOCK_CODE);
> +	if (ret) {
> +		dev_err(dev, "Failed to lock HUB's protected registers\n");
> +		goto error;
> +	}
> +
> +	/* TBD: Apply special/security lock here using DEV_CMD register */
> +
> +	schedule_delayed_work(&priv->delayed_work, msecs_to_jiffies(100));
> +
> +	return 0;
> +
> +error:
> +	debugfs_remove_recursive(priv->debug_dir);
> +	return ret;
> +}
> +
> +static void i3c_hub_remove(struct i3c_device *i3cdev)
> +{
> +	struct i3c_hub *priv = i3cdev_get_drvdata(i3cdev);
> +	struct i2c_adapter_group *g_adap;
> +	struct smbus_backend *backend = NULL;
> +	int i;
> +
> +	for (i = 0; i < I3C_HUB_TP_MAX_COUNT; i++) {
> +		if (priv->logical_bus[i].smbus_port_adapter.used) {
> +			g_adap = &priv->logical_bus[i].smbus_port_adapter;
> +			cancel_delayed_work_sync(&g_adap->delayed_work_polling);
> +			list_for_each_entry(backend,  &g_adap->backend_entry, list) {
> +				i2c_unregister_device(backend->client);
> +				kfree(backend);
> +			}
> +		}
> +
> +		if (priv->logical_bus[i].smbus_port_adapter.used ||
> +		    priv->logical_bus[i].registered)
> +			i3c_master_unregister(&priv->logical_bus[i].controller);
> +	}
> +
> +	cancel_delayed_work_sync(&priv->delayed_work);
> +	debugfs_remove_recursive(priv->debug_dir);
> +}
> +
> +static struct i3c_driver i3c_hub = {
> +	.driver.name = "i3c-hub",
> +	.id_table = i3c_hub_ids,
> +	.probe = i3c_hub_probe,
> +	.remove = i3c_hub_remove,
> +};
> +
> +module_i3c_driver(i3c_hub);
> +
> +MODULE_AUTHOR("Zbigniew Lukwinski <zbigniew.lukwinski@xxxxxxxxxxxxxxx>");
> +MODULE_AUTHOR("Steven Niu <steven.niu.uj@xxxxxxxxxxx>");
> +MODULE_DESCRIPTION("I3C HUB driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 3afa530c5e32..b7cf15ba4e67 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -2244,15 +2244,26 @@ static int of_populate_i3c_bus(struct i3c_master_controller *master)
>  	struct device_node *node;
>  	int ret;
>  	u32 val;
> +	bool ignore_hub_node = false;
> +	char *addr_pid;
>  
>  	if (!i3cbus_np)
>  		return 0;
>  
>  	for_each_available_child_of_node(i3cbus_np, node) {
> -		ret = of_i3c_master_add_dev(master, node);
> -		if (ret) {
> -			of_node_put(node);
> -			return ret;
> +		ignore_hub_node = false;
> +		if (node->full_name && strstr(node->full_name, "hub")) {

NAK, you cannot rely on node name. Node name can be whatever, not "hub".

Best regards,
Krzysztof