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 I2CLooks good. As a self reminder:
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://scanmail.trustwave.com/?c=20988&d=2Imq58SgjkO2w5EzbSeL1kys6iYwJJIG5Ij2dyaU8A&u=https%3a%2f%2fgit%2eopenwrt%2eorg%2f%3fp%3dopenwrt%2fopenwrt%2egit%3ba%3dblob%3bf%3dtarget%2flinux%2frealtek%2ffiles-5%2e15%2fdrivers%2fi2c%2fbusses%2fi2c-rtl9300%2ec
Signed-off-by: Chris Packham <chris.packham@xxxxxxxxxxxxxxxxxxx>
Reviewed-by: Andi Shyti <andi.shyti@xxxxxxxxxx>
Some comments below, though.
...
+#define RTL9300_I2C_MST_CTRL1 0x0Not everything here is perfectly aligned, but I'm not going to
+#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
be too picky.
...
+static int rtl9300_i2c_execute_xfer(struct rtl9300_i2c *i2c, char read_write,You could align this a little better.
+ int size, union i2c_smbus_data *data, int len)
+{...
+ 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'?
...
Ack.+static int rtl9300_i2c_probe(struct platform_device *pdev)"chan" and "adap" can be declared inside
+{
+ struct device *dev = &pdev->dev;
+ struct rtl9300_i2c_chan *chan;
+ struct rtl9300_i2c *i2c;
+ struct i2c_adapter *adap;
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) {Thanks,
+ chan = &i2c->chans[i];
+ adap = &chan->adap;
+
Andi