[PATCH v27 3/4] i2c: ast2600: Add controller driver for AST2600 new register set
Jeremy Kerr
jk at codeconstruct.com.au
Tue Mar 24 14:37:18 AEDT 2026
Hi Ryan,
> +static void ast2600_i2c_set_xfer_mode(struct ast2600_i2c_bus *i2c_bus,
> + enum xfer_mode mode)
> +{
> + i2c_bus->mode = mode;
> +
> + switch (mode) {
> + case DMA_MODE:
> + i2c_bus->setup_tx = ast2600_i2c_setup_dma_tx;
> + i2c_bus->setup_rx = ast2600_i2c_setup_dma_rx;
> + break;
> + case BYTE_MODE:
> + i2c_bus->setup_tx = ast2600_i2c_setup_byte_tx;
> + i2c_bus->setup_rx = ast2600_i2c_setup_byte_rx;
> + break;
> + case BUFF_MODE:
> + default:
> + i2c_bus->setup_tx = ast2600_i2c_setup_buff_tx;
> + i2c_bus->setup_rx = ast2600_i2c_setup_buff_rx;
> + break;
> + }
> +}
> +
> +static int ast2600_i2c_xfer_mode_parse(struct ast2600_i2c_bus *i2c_bus,
> + const char *buf, enum xfer_mode *mode)
> +{
> + if (sysfs_streq(buf, "byte")) {
> + *mode = BYTE_MODE;
> + return 0;
> + }
> +
> + if (sysfs_streq(buf, "buffer")) {
> + if (!i2c_bus->buf_base)
> + return -EINVAL;
> + *mode = BUFF_MODE;
> + return 0;
> + }
> +
> + if (sysfs_streq(buf, "dma")) {
> + if (!i2c_bus->dma_available)
> + return -EINVAL;
> + *mode = DMA_MODE;
> + return 0;
> + }
> +
> + return -EINVAL;
> +}
I would suggest separating the string parsing from the "is the mode
available" logic, more on that below.
> +
> +static const char *ast2600_i2c_xfer_mode_name(enum xfer_mode mode)
> +{
> + switch (mode) {
> + case BYTE_MODE:
> + return "byte";
> + case DMA_MODE:
> + return "dma";
> + case BUFF_MODE:
> + default:
> + return "buffer";
> + }
> +}
> +
> +static ssize_t xfer_mode_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> + struct ast2600_i2c_bus *i2c_bus = dev_get_drvdata(dev);
> +
> + return sysfs_emit(buf, "%s\n", ast2600_i2c_xfer_mode_name(i2c_bus->mode));
> +}
> +
> +static ssize_t xfer_mode_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct ast2600_i2c_bus *i2c_bus = dev_get_drvdata(dev);
> + enum xfer_mode mode;
> + int ret;
> +
> + ret = ast2600_i2c_xfer_mode_parse(i2c_bus, buf, &mode);
> + if (ret)
> + return ret;
> +
> + i2c_lock_bus(&i2c_bus->adap, I2C_LOCK_ROOT_ADAPTER);
> + ast2600_i2c_set_xfer_mode(i2c_bus, mode);
> + i2c_unlock_bus(&i2c_bus->adap, I2C_LOCK_ROOT_ADAPTER);
> +
> + return count;
> +}
> +
> +static DEVICE_ATTR_RW(xfer_mode);
This will need sysfs ABI documentation.
> +
> +static int ast2600_i2c_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct ast2600_i2c_bus *i2c_bus;
> + struct reset_control *rst;
> + struct resource *res;
> + u32 global_ctrl;
> + int ret;
> +
> + if (!device_property_present(dev, "aspeed,global-regs"))
> + return -ENODEV;
> +
> + i2c_bus = devm_kzalloc(dev, sizeof(*i2c_bus), GFP_KERNEL);
> + if (!i2c_bus)
> + return -ENOMEM;
> +
> + i2c_bus->reg_base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(i2c_bus->reg_base))
> + return PTR_ERR(i2c_bus->reg_base);
> +
> + rst = devm_reset_control_get_shared_deasserted(dev, NULL);
> + if (IS_ERR(rst))
> + return dev_err_probe(dev, PTR_ERR(rst), "Missing reset ctrl\n");
> +
> + i2c_bus->global_regs =
> + syscon_regmap_lookup_by_phandle(dev_of_node(dev), "aspeed,global-regs");
> + if (IS_ERR(i2c_bus->global_regs))
> + return PTR_ERR(i2c_bus->global_regs);
> +
> + regmap_read(i2c_bus->global_regs, AST2600_I2CG_CTRL, &global_ctrl);
> + if ((global_ctrl & AST2600_GLOBAL_INIT) != AST2600_GLOBAL_INIT) {
> + regmap_write(i2c_bus->global_regs, AST2600_I2CG_CTRL, AST2600_GLOBAL_INIT);
> + regmap_write(i2c_bus->global_regs, AST2600_I2CG_CLK_DIV_CTRL, I2CCG_DIV_CTRL);
> + }
> +
> + i2c_bus->dev = dev;
> + i2c_bus->multi_master = device_property_read_bool(dev, "multi-master");
> + i2c_bus->dma_available = device_property_read_bool(dev, "aspeed,enable-dma");
> + if (i2c_bus->dma_abailable)
dma_abailable? you didn't even build this? :(
> + i2c_bus->mode = DMA_MODE;
> + else
> + i2c_bus->mode = BUFF_MODE;
> +
> + if (i2c_bus->mode == BUFF_MODE) {
> + i2c_bus->buf_base = devm_platform_get_and_ioremap_resource(pdev, 1, &res);
So you only set ->buf_base if we are in buffer mode during probe.
However, the ->buf_base check in xfer_mode_parse() will fail when trying
to change from any other mode to buffer mode. This means you can never
select buffer mode after probe.
> + if (IS_ERR(i2c_bus->buf_base))
> + i2c_bus->mode = BYTE_MODE;
> + else
> + i2c_bus->buf_size = resource_size(res) / 2;
> + }
> +
> + ast2600_i2c_set_xfer_mode(i2c_bus, i2c_bus->mode);
Minor: set_xfer_mode() sets i2c_bus->mode itself. Maybe you want a
temporary instead, or change the semantics of that?
> +
> + /*
> + * i2c timeout counter: use base clk4 1Mhz,
> + * per unit: 1/(1000/1024) = 1024us
> + */
> + ret = device_property_read_u32(dev, "i2c-scl-clk-low-timeout-us", &i2c_bus->timeout);
> + if (!ret)
> + i2c_bus->timeout = DIV_ROUND_UP(i2c_bus->timeout, 1024);
> +
> + init_completion(&i2c_bus->cmd_complete);
> +
> + i2c_bus->irq = platform_get_irq(pdev, 0);
> + if (i2c_bus->irq < 0)
> + return i2c_bus->irq;
> +
> + platform_set_drvdata(pdev, i2c_bus);
> +
> + i2c_bus->clk = devm_clk_get(i2c_bus->dev, NULL);
> + if (IS_ERR(i2c_bus->clk))
> + return dev_err_probe(i2c_bus->dev, PTR_ERR(i2c_bus->clk), "Can't get clock\n");
> +
> + i2c_bus->apb_clk = clk_get_rate(i2c_bus->clk);
> +
> + i2c_parse_fw_timings(i2c_bus->dev, &i2c_bus->timing_info, true);
> +
> + /* Initialize the I2C adapter */
> + i2c_bus->adap.owner = THIS_MODULE;
> + i2c_bus->adap.algo = &i2c_ast2600_algorithm;
> + i2c_bus->adap.retries = 0;
> + i2c_bus->adap.dev.parent = i2c_bus->dev;
> + device_set_node(&i2c_bus->adap.dev, dev_fwnode(dev));
> + i2c_bus->adap.algo_data = i2c_bus;
> + strscpy(i2c_bus->adap.name, pdev->name);
> + i2c_set_adapdata(&i2c_bus->adap, i2c_bus);
> +
> + ret = ast2600_i2c_init(i2c_bus);
> + if (ret < 0)
> + return dev_err_probe(dev, ret, "Unable to initial i2c %d\n", ret);
Super minor: `initial` is not a verb in this context, you want
`initialise` (Australian) or `initialize` (US) or `init` (keeping
everyone happy)
> +
> + ret = devm_request_irq(dev, i2c_bus->irq, ast2600_i2c_bus_irq, 0,
> + dev_name(dev), i2c_bus);
> + if (ret < 0) {
> + ret = dev_err_probe(dev, ret, "Unable to request irq %d\n",
> + i2c_bus->irq);
> + goto err;
> + }
> +
> + writel(AST2600_I2CM_PKT_DONE | AST2600_I2CM_BUS_RECOVER,
> + i2c_bus->reg_base + AST2600_I2CM_IER);
> +
> + ret = devm_i2c_add_adapter(dev, &i2c_bus->adap);
> + if (ret)
> + goto err;
> +
> + ret = sysfs_create_file(&dev->kobj, &dev_attr_xfer_mode.attr);
> + if (ret)
> + goto err;
This error path will fail the probe but not unregister the i2c adapter.
You probably want to only register the adapter last (and remove the
sysfs file if that fails).
> +
> + return 0;
> +
> +err:
> + writel(0, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
> + writel(0, i2c_bus->reg_base + AST2600_I2CM_IER);
> + return ret;
> +}
> +
> +static void ast2600_i2c_remove(struct platform_device *pdev)
> +{
> + struct ast2600_i2c_bus *i2c_bus = platform_get_drvdata(pdev);
> +
> + sysfs_remove_file(&pdev->dev.kobj, &dev_attr_xfer_mode.attr);
> +
> + /* Disable everything. */
> + writel(0, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
> + writel(0, i2c_bus->reg_base + AST2600_I2CM_IER);
> +}
> +
> +static const struct of_device_id ast2600_i2c_of_match[] = {
> + { .compatible = "aspeed,ast2600-i2c-bus" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, ast2600_i2c_of_match);
> +
> +static struct platform_driver ast2600_i2c_driver = {
> + .probe = ast2600_i2c_probe,
> + .remove = ast2600_i2c_remove,
> + .driver = {
> + .name = "ast2600-i2c",
> + .of_match_table = ast2600_i2c_of_match,
> + },
> +};
> +module_platform_driver(ast2600_i2c_driver);
> +
> +MODULE_AUTHOR("Ryan Chen <ryan_chen at aspeedtech.com>");
> +MODULE_DESCRIPTION("ASPEED AST2600 I2C Controller Driver");
> +MODULE_LICENSE("GPL");
Cheers,
Jeremy
More information about the openbmc
mailing list