On 6/11/24 10:10, Andi Shyti wrote:
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://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>
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.
...
Since I'm making other changes I'll tidy up the alignment.
+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.
Ack.
+{
+ 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'?
...
I think it'll be OK. In the write case the len should be set via
data->block[0]. I the read case we will pass len=0 which will cause
rtl9300_i2c_execute_xfer() to grab all 16 bytes from the i2c controller
registers but for the read block data the i2c device should provide the
correct number bytes in byte 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()
Ack.
+ 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
