RE: [PATCH v27 3/4] i2c: ast2600: Add controller driver for AST2600 new register set

> Subject: Re: [PATCH v27 3/4] i2c: ast2600: Add controller driver for AST2600
> new register set
> 
> 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.
>
I will separate with following.
 
static int ast2600_i2c_xfer_mode_parse(const char *buf, enum xfer_mode *mode)
{
    if (sysfs_streq(buf, "byte"))   { *mode = BYTE_MODE; return 0; }
    if (sysfs_streq(buf, "buffer")) { *mode = BUFF_MODE; return 0; }
    if (sysfs_streq(buf, "dma"))    { *mode = DMA_MODE;  return 0; }
    return -EINVAL;
}

static int ast2600_i2c_xfer_mode_check(struct ast2600_i2c_bus *i2c_bus,
                                       enum xfer_mode mode)
{
    if (mode == BUFF_MODE && !i2c_bus->buf_base)
        return -EINVAL;
    if (mode == DMA_MODE && !i2c_bus->dma_available)
        return -EINVAL;
    return 0;
}

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

Since it is in sysfs /sys/bus/platform/drivers/i2c_ast2600
So I add Documentation/ABI/testing/sysfs-bus-platform-drivers-i2c-ast2600 
am I right? 

What:		/sys/bus/platform/drivers/i2c-ast2600/.../xfer_mode
Date:		March 2026
KernelVersion:	6.x
Contact:	Ryan Chen <[email protected]>
Description:
		Shows or sets the transfer mode for the ASPEED AST2600 I2C
		controller. Valid values are:

		- "byte":   Programmed I/O, one byte at a time.
		- "buffer": Programmed I/O using the hardware FIFO buffer.
		- "dma":    DMA transfer (only available if aspeed,enable-dma
		            is set in the device tree).

		Writing an unsupported mode returns -EINVAL.

> 
> > +
> > +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? :(
OH, Sorry it is my bad. 
> 
> > +               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.


Will modify with following.

i2c_bus->buf_base = devm_platform_get_and_ioremap_resource(pdev, 1, &res);
if (!IS_ERR(i2c_bus->buf_base))
    i2c_bus->buf_size = resource_size(res) / 2;
else
    i2c_bus->buf_base = NULL;


> 
> 
> > +               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?
Will update 

if (i2c_bus->dma_available)
    mode = DMA_MODE;
else if (i2c_bus->buf_base)
    mode = BUFF_MODE;
else
    mode = BYTE_MODE;

ast2600_i2c_set_xfer_mode(i2c_bus, mode);
> 
> > +
> > +       /*
> > +        * 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)

Will update 
> 
> > +
> > +       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).

Will update
sysfs_create_file();
if (ret)
    goto err;    

devm_i2c_add_adapter();
if (ret) {
    sysfs_remove_file(); 
    goto err;
}
> 
> > +
> > +       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 <[email protected]>");
> > +MODULE_DESCRIPTION("ASPEED AST2600 I2C Controller Driver");
> > +MODULE_LICENSE("GPL");
> 
> Cheers,
> 
> 
> Jeremy