Hi Chris, On Fri, Nov 01, 2024 at 09:03:50AM +1300, Chris Packham wrote: > Add support for the I2C controller on the RTL9300 SoC. There are two I2C > controllers in the RTL9300 that are part of the Ethernet switch register > block. Each of these controllers owns a SCL pin (GPIO8 for the fiorst > I2C controller, GPIO17 for the second). There are 8 possible SDA pins > (GPIO9-16) that can be assigned to either I2C controller. This > relationship is represented in the device tree with a child node for > each SDA line in use. > > This is based on the openwrt implementation[1] but has been > significantly modified > > [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> Looks good. As a self reminder: Reviewed-by: Andi Shyti <andi.shyti@xxxxxxxxxx> Some comments below, though. ... > +#define RTL9300_I2C_MST_CTRL1 0x0 > +#define RTL9300_I2C_MST_CTRL1_MEM_ADDR_OFS 8 > +#define RTL9300_I2C_MST_CTRL1_MEM_ADDR_MASK GENMASK(31, 8) > +#define RTL9300_I2C_MST_CTRL1_SDA_OUT_SEL_OFS 4 > +#define RTL9300_I2C_MST_CTRL1_SDA_OUT_SEL_MASK GENMASK(6, 4) > +#define RTL9300_I2C_MST_CTRL1_GPIO_SCL_SEL BIT(3) > +#define RTL9300_I2C_MST_CTRL1_RWOP BIT(2) > +#define RTL9300_I2C_MST_CTRL1_I2C_FAIL BIT(1) > +#define RTL9300_I2C_MST_CTRL1_I2C_TRIG BIT(0) > +#define RTL9300_I2C_MST_CTRL2 0x4 > +#define RTL9300_I2C_MST_CTRL2_RD_MODE BIT(15) > +#define RTL9300_I2C_MST_CTRL2_DEV_ADDR_OFS 8 > +#define RTL9300_I2C_MST_CTRL2_DEV_ADDR_MASK GENMASK(14, 8) > +#define RTL9300_I2C_MST_CTRL2_DATA_WIDTH_OFS 4 > +#define RTL9300_I2C_MST_CTRL2_DATA_WIDTH_MASK GENMASK(7, 4) > +#define RTL9300_I2C_MST_CTRL2_MEM_ADDR_WIDTH_OFS 2 > +#define RTL9300_I2C_MST_CTRL2_MEM_ADDR_WIDTH_MASK GENMASK(3, 2) > +#define RTL9300_I2C_MST_CTRL2_SCL_FREQ_OFS 0 > +#define RTL9300_I2C_MST_CTRL2_SCL_FREQ_MASK GENMASK(1, 0) > +#define RTL9300_I2C_MST_DATA_WORD0 0x8 > +#define RTL9300_I2C_MST_DATA_WORD1 0xc > +#define RTL9300_I2C_MST_DATA_WORD2 0x10 > +#define RTL9300_I2C_MST_DATA_WORD3 0x14 Not everything here is perfectly aligned, but I'm not going to be too picky. ... > +static int rtl9300_i2c_execute_xfer(struct rtl9300_i2c *i2c, char read_write, > + int size, union i2c_smbus_data *data, int len) You could align this a little better. > +{ > + u32 val, mask; > + int ret; ... > +static int rtl9300_i2c_smbus_xfer(struct i2c_adapter *adap, u16 addr, unsigned short flags, > + char read_write, u8 command, int size, > + union i2c_smbus_data *data) > +{ > + struct rtl9300_i2c_chan *chan = i2c_get_adapdata(adap); > + struct rtl9300_i2c *i2c = chan->i2c; > + int len = 0, ret; ... > + ret = rtl9300_i2c_execute_xfer(i2c, read_write, size, data, len); do we want to bail out if len is '0'? ... > +static int rtl9300_i2c_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct rtl9300_i2c_chan *chan; > + struct rtl9300_i2c *i2c; > + struct i2c_adapter *adap; "chan" and "adap" can be declared inside the device_for_each_child_node() > + u32 clock_freq, sda_pin; > + int ret, i = 0; > + struct fwnode_handle *child; ... > + device_for_each_child_node(dev, child) { > + chan = &i2c->chans[i]; > + adap = &chan->adap; > + Thanks, Andi
