Hi Chris, On Wed, Sep 18, 2024 at 11:29:29AM GMT, Chris Packham wrote: > Add support for the I2C controller on the RTL9300 SoC. This is based on > the openwrt implementation[1] but cleaned up to make use of the regmap > APIs. Can you please add a few more words to describe the device? > [1] - https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/realtek/files-5.15/drivers/i2c/busses/i2c-rtl9300.c > > Signed-off-by: Chris Packham <chris.packham@xxxxxxxxxxxxxxxxxxx> ... > +#define I2C_MST_CTRL1 0x0 > +#define MEM_ADDR_OFS 8 > +#define MEM_ADDR_MASK 0xffffff > +#define SDA_OUT_SEL_OFS 4 > +#define SDA_OUT_SEL_MASK 0x7 > +#define GPIO_SCL_SEL BIT(3) > +#define RWOP BIT(2) > +#define I2C_FAIL BIT(1) > +#define I2C_TRIG BIT(0) > +#define I2C_MST_CTRL2 0x4 > +#define RD_MODE BIT(15) > +#define DEV_ADDR_OFS 8 > +#define DEV_ADDR_MASK 0x7f > +#define DATA_WIDTH_OFS 4 > +#define DATA_WIDTH_MASK 0xf > +#define MEM_ADDR_WIDTH_OFS 2 > +#define MEM_ADDR_WIDTH_MASK 0x3 can we have these masked already shifted? You could use GENMASK(). > +#define SCL_FREQ_OFS 0 > +#define SCL_FREQ_MASK 0x3 > +#define I2C_MST_DATA_WORD0 0x8 > +#define I2C_MST_DATA_WORD1 0xc > +#define I2C_MST_DATA_WORD2 0x10 > +#define I2C_MST_DATA_WORD3 0x14 Can we use a prefix for all these defines? > + > +#define RTL9300_I2C_STD_FREQ 0 > +#define RTL9300_I2C_FAST_FREQ 1 This can also be an enum. > + > +DEFINE_MUTEX(i2c_lock); ... > +static int rtl9300_i2c_write(struct rtl9300_i2c *i2c, u8 *buf, int len) > +{ > + u32 vals[4] = {}; > + int i, ret; > + > + if (len > 16) > + return -EIO; > + > + for (i = 0; i < len; i++) { > + if (i % 4 == 0) > + vals[i/4] = 0; > + vals[i/4] <<= 8; > + vals[i/4] |= buf[i]; > + } > + > + ret = regmap_bulk_write(i2c->regmap, i2c->i2c_mst_ofs + I2C_MST_DATA_WORD0, > + vals, ARRAY_SIZE(vals)); > + if (ret) > + return ret; > + > + return len; why returning "len"? And in any case this is ignored. > +} > + > +static int rtl9300_i2c_writel(struct rtl9300_i2c *i2c, u32 data) > +{ > + int ret; > + > + ret = regmap_write(i2c->regmap, i2c->i2c_mst_ofs + I2C_MST_DATA_WORD0, data); > + if (ret) > + return ret; > + > + return 0; return regmap_write(...) ? In any case, the returned value of these functions is completely ignored, not even printed. Should we either: - condier the return value in the _xfer() functions or - make all these functions void? > +} > + > +static int rtl9300_i2c_execute_xfer(struct rtl9300_i2c *i2c, char read_write, > + int size, union i2c_smbus_data *data, int len) > +{ > + u32 val, mask; > + int ret; > + > + if (read_write == I2C_SMBUS_READ) > + val = 0; > + else > + val = RWOP; > + mask = RWOP; > + > + val |= I2C_TRIG; > + mask |= I2C_TRIG; how about "mask = RWOP | I2C_TRIG" to make it in one line? Also val can be simplified as: val = I2C_TRIG; if (read_write == I2C_SMBUS_WRITE) val |= RWOP; Not a binding commeent, as you wish. > + > + ret = regmap_update_bits(i2c->regmap, i2c->i2c_mst_ofs + I2C_MST_CTRL1, mask, val); > + if (ret) > + return ret; > + > + ret = regmap_read_poll_timeout(i2c->regmap, i2c->i2c_mst_ofs + I2C_MST_CTRL1, > + val, !(val & I2C_TRIG), 100, 2000); > + if (ret) > + return ret; > + > + if (val & I2C_FAIL) where is val taking taking this bit? > + return -EIO; > + ... > + switch (size) { > + case I2C_SMBUS_QUICK: ... > + case I2C_SMBUS_BYTE: ... > + case I2C_SMBUS_BYTE_DATA: ... > + case I2C_SMBUS_WORD_DATA: ... > + case I2C_SMBUS_BLOCK_DATA: ... > + default: > + dev_warn(&adap->dev, "Unsupported transaction %d\n", size); dev_err() ? > + ret = -EOPNOTSUPP; > + goto out_unlock; > + } ... > + switch (clock_freq) { > + case I2C_MAX_STANDARD_MODE_FREQ: ... > + case I2C_MAX_FAST_MODE_FREQ: ... > + default: > + dev_warn(i2c->dev, "clock-frequency %d not supported\n", clock_freq); > + return -EINVAL; If we are returning an error we should print an error, let's make it a "return dev_err_probe()" But, I was thinking that by default we can assign I2C_MAX_STANDARD_MODE_FREQ and if the DTS defines a different frequency we could just print an error and stick to the default value. Makes sense? > + } ... > + return i2c_add_adapter(adap); return devm_i2c_add_adapter(adap); and the remove function is not needed. > +} > + > +static void rtl9300_i2c_remove(struct platform_device *pdev) > +{ > + struct rtl9300_i2c *i2c = platform_get_drvdata(pdev); > + > + i2c_del_adapter(&i2c->adap); > +} > + > +static const struct of_device_id i2c_rtl9300_dt_ids[] = { > + { .compatible = "realtek,rtl9300-i2c" }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, i2c_rtl9300_dt_ids); > + > +static struct platform_driver rtl9300_i2c_driver = { > + .probe = rtl9300_i2c_probe, > + .remove = rtl9300_i2c_remove, > + .driver = { > + .name = "i2c-rtl9300", > + .of_match_table = i2c_rtl9300_dt_ids, > + }, > +}; > + > +module_platform_driver(rtl9300_i2c_driver); > + > +MODULE_DESCRIPTION("RTL9300 I2C controller driver"); > +MODULE_LICENSE("GPL"); > + Just a trailing blank line here. Thanks, Andi
