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,
> 
> > > 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.
Yes, will update
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?
> 
> I would suggest Documentation/ABI/testing/sysfs-driver-ast2600-i2c
Thanks  ~ will update.
> 
> >
> > 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.

Will update 
> 
> > Contact:        Ryan Chen <[email protected]>
> > Description:
> 
> Keep the first line of the description on the same line.

Will update
> 
> >                 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:
Will remove.
> 
>                  - "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.
Thanks will update.
> 
> Cheers,
> 
> 
> Jeremy