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

Hi Ryan,

quoted

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;
}

OK, but with kernel-style formatting.

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;
}

quoted

quoted

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

{

quoted

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

I would suggest Documentation/ABI/testing/sysfs-driver-ast2600-i2c

What:           /sys/bus/platform/drivers/i2c-ast2600/.../xfer_mode
Date:           March 2026
KernelVersion:  6.x

KernelVersion is optional, but if you include it, it would be 7.x.

Contact:        Ryan Chen [off-list ref]
Description:

Keep the first line of the description on the same line.

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

Decouple this from the device tree configuration mechanism:

                 - "dma":    DMA transfer (if DMA is available for this
                             controller)

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;

I would suggest a temporary, so there's no chance that future changes
could see an ERR_PTR value in i2c_bus->buf_base:

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

and you have kzalloc()ed, so no need for the NULL init in the error path.

Cheers,


Jeremy