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

[Jeremy Kerr <jk@codeconstruct.com.au>]

Hi Ryan,

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

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 <ryan_chen@aspeedtech.com>
> 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