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
