| Message ID | 20240917232932.3641992-1-chris.packham@alliedtelesis.co.nz |
|---|---|
| Headers | show |
| Series | i2c: RTL9300 support | expand |
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@alliedtelesis.co.nz> ... > +#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
Hi Chris, ... > -module_platform_driver(rtl9300_i2c_driver); > +static int rtl9300_i2c_select_chan(struct i2c_mux_core *muxc, u32 chan) > +{ > + struct i2c_adapter *adap = muxc->parent; > + struct rtl9300_i2c *i2c = i2c_get_adapdata(adap); > + int ret; > + > + ret = rtl9300_i2c_config_io(i2c, chan); > + if (ret) > + return ret; > + > + return 0; return "rtl9300_i2c_config_io()"? > +} ... > +static int rtl9300_i2c_mux_probe_fw(struct rtl9300_i2c_chan *mux, struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct fwnode_handle *fwnode = dev_fwnode(dev); > + struct device_node *np = dev->of_node; > + struct device_node *adap_np; > + struct i2c_adapter *adap = NULL; > + struct fwnode_handle *child; > + unsigned int *chans; > + int i = 0; > + > + if (!is_of_node(fwnode)) > + return -EOPNOTSUPP; > + > + if (!np) > + return -ENODEV; > + > + adap_np = of_parse_phandle(np, "i2c-parent", 0); > + if (!adap_np) { > + dev_err(&pdev->dev, "Cannot parse i2c-parent\n"); > + return -ENODEV; return dev_err_probe(...)? > + } > + adap = of_find_i2c_adapter_by_node(adap_np); > + of_node_put(adap_np); ... > +static int __init rtl9300_i2c_init(void) > +{ > + return platform_register_drivers(drivers, ARRAY_SIZE(drivers)); > +} > +module_init(rtl9300_i2c_init); > + > +static void __exit rtl9300_i2c_exit(void) > +{ > + platform_unregister_drivers(drivers, ARRAY_SIZE(drivers)); > +} > +module_exit(rtl9300_i2c_exit); You could use module_platform_driver() Thanks, Andi > > MODULE_DESCRIPTION("RTL9300 I2C controller driver"); > MODULE_LICENSE("GPL"); > -- > 2.46.1 >
Hi Andy, On 19/09/24 08:27, Andi Shyti wrote: > 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? Sure will do. >> [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@alliedtelesis.co.nz> > ... > >> +#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(). I'll take a look. >> +#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? Yes will add "RTL9300_". I assume for the bit values too? So something like "MEM_ADDR_OFS" becomes "RTL9300_I2C_MST_CTRL1_MEM_ADDR_OFS" is that OK or too verbose? >> + >> +#define RTL9300_I2C_STD_FREQ 0 >> +#define RTL9300_I2C_FAST_FREQ 1 > This can also be an enum. Ack > >> + >> +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. I copied that behaviour from the openwrt driver. I think making it the same as the other functions would make more sense. > >> +} >> + >> +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? I suppose it's a bit academic. Under the hood it's mmio so it's not as if it can really fail (famous last words). That said, this switch chip can be run in a core disabled mode and you could then in theory be doing I2C over SPI from an external SoC. If someone were just naively updating a hardware design to add the external SoC they might neglect to move the I2C connections. It's also just good practice so I'll propagate the returns up to the _xfer(). >> +} >> + >> +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. I'll take a look. I kind of did like the pairing of val and mask for each thing being set. >> + >> + 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? In the regmap_read_poll_timeout(). > >> + 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() ? Ack. >> + 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? I don't have a strong opinion. Failing the probe just because something in the dts is wrong where we can have a sane default does seem overly harsh. On the other hand I've had hardware QA folks complain when the I2C bus is running at 98khz instead of 100khz. > >> + } > ... > >> + return i2c_add_adapter(adap); > return devm_i2c_add_adapter(adap); > > and the remove function is not needed. OK thanks. I did look for a devm variant but obviously not hard enough. >> +} >> + >> +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. Ack. > > Thanks, > Andi >
Hi Andi, Rob, On 19/09/24 08:36, Andi Shyti wrote: > Hi Chris, > > ... > >> -module_platform_driver(rtl9300_i2c_driver); >> +static int rtl9300_i2c_select_chan(struct i2c_mux_core *muxc, u32 chan) >> +{ >> + struct i2c_adapter *adap = muxc->parent; >> + struct rtl9300_i2c *i2c = i2c_get_adapdata(adap); >> + int ret; >> + >> + ret = rtl9300_i2c_config_io(i2c, chan); >> + if (ret) >> + return ret; >> + >> + return 0; > return "rtl9300_i2c_config_io()"? Ack. >> +} > ... > >> +static int rtl9300_i2c_mux_probe_fw(struct rtl9300_i2c_chan *mux, struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct fwnode_handle *fwnode = dev_fwnode(dev); >> + struct device_node *np = dev->of_node; >> + struct device_node *adap_np; >> + struct i2c_adapter *adap = NULL; >> + struct fwnode_handle *child; >> + unsigned int *chans; >> + int i = 0; >> + >> + if (!is_of_node(fwnode)) >> + return -EOPNOTSUPP; >> + >> + if (!np) >> + return -ENODEV; >> + >> + adap_np = of_parse_phandle(np, "i2c-parent", 0); >> + if (!adap_np) { >> + dev_err(&pdev->dev, "Cannot parse i2c-parent\n"); >> + return -ENODEV; > return dev_err_probe(...)? Ack. >> + } >> + adap = of_find_i2c_adapter_by_node(adap_np); >> + of_node_put(adap_np); > ... > >> +static int __init rtl9300_i2c_init(void) >> +{ >> + return platform_register_drivers(drivers, ARRAY_SIZE(drivers)); >> +} >> +module_init(rtl9300_i2c_init); >> + >> +static void __exit rtl9300_i2c_exit(void) >> +{ >> + platform_unregister_drivers(drivers, ARRAY_SIZE(drivers)); >> +} >> +module_exit(rtl9300_i2c_exit); > You could use module_platform_driver() Can I though? I want to support both the simple I2C controller and the MUX mode with the same driver. Which is why I've ended up with two drivers to register. On the binding patch, Rob made the suggestion that I just make the i2c-mux part of the parent. I did consider that but quickly got tied in knots because I couldn't figure out how to have a device that is both an adapter and a mux. The main problem was that any child nodes of an i2c adapter in the device tree are presumed to be I2C devices and get probed automatically by of_i2c_register_devices(). Equally I can't register a mux without having an adapter that the mux operates over. > > Thanks, > Andi > >> >> MODULE_DESCRIPTION("RTL9300 I2C controller driver"); >> MODULE_LICENSE("GPL"); >> -- >> 2.46.1 >>
On 19/09/24 09:44, Chris Packham wrote: > Hi Andi, Rob, > > On 19/09/24 08:36, Andi Shyti wrote: >> Hi Chris, >> >> ... >> >>> -module_platform_driver(rtl9300_i2c_driver); >>> +static int rtl9300_i2c_select_chan(struct i2c_mux_core *muxc, u32 >>> chan) >>> +{ >>> + struct i2c_adapter *adap = muxc->parent; >>> + struct rtl9300_i2c *i2c = i2c_get_adapdata(adap); >>> + int ret; >>> + >>> + ret = rtl9300_i2c_config_io(i2c, chan); >>> + if (ret) >>> + return ret; >>> + >>> + return 0; >> return "rtl9300_i2c_config_io()"? > > Ack. > >>> +} >> ... >> >>> +static int rtl9300_i2c_mux_probe_fw(struct rtl9300_i2c_chan *mux, >>> struct platform_device *pdev) >>> +{ >>> + struct device *dev = &pdev->dev; >>> + struct fwnode_handle *fwnode = dev_fwnode(dev); >>> + struct device_node *np = dev->of_node; >>> + struct device_node *adap_np; >>> + struct i2c_adapter *adap = NULL; >>> + struct fwnode_handle *child; >>> + unsigned int *chans; >>> + int i = 0; >>> + >>> + if (!is_of_node(fwnode)) >>> + return -EOPNOTSUPP; >>> + >>> + if (!np) >>> + return -ENODEV; >>> + >>> + adap_np = of_parse_phandle(np, "i2c-parent", 0); >>> + if (!adap_np) { >>> + dev_err(&pdev->dev, "Cannot parse i2c-parent\n"); >>> + return -ENODEV; >> return dev_err_probe(...)? > > Ack. > >>> + } >>> + adap = of_find_i2c_adapter_by_node(adap_np); >>> + of_node_put(adap_np); >> ... >> >>> +static int __init rtl9300_i2c_init(void) >>> +{ >>> + return platform_register_drivers(drivers, ARRAY_SIZE(drivers)); >>> +} >>> +module_init(rtl9300_i2c_init); >>> + >>> +static void __exit rtl9300_i2c_exit(void) >>> +{ >>> + platform_unregister_drivers(drivers, ARRAY_SIZE(drivers)); >>> +} >>> +module_exit(rtl9300_i2c_exit); >> You could use module_platform_driver() > > Can I though? I want to support both the simple I2C controller and the > MUX mode with the same driver. Which is why I've ended up with two > drivers to register. > > On the binding patch, Rob made the suggestion that I just make the > i2c-mux part of the parent. I did consider that but quickly got tied > in knots because I couldn't figure out how to have a device that is > both an adapter and a mux. The main problem was that any child nodes > of an i2c adapter in the device tree are presumed to be I2C devices > and get probed automatically by of_i2c_register_devices(). Equally I > can't register a mux without having an adapter that the mux operates > over. OK I think I've got something working that has a dt binding like i2c@36c { compatible = "realtek,rtl9300-i2c"; reg = <0x36c 0x14>; status = "okay"; #address-cells = <0x01>; #size-cells = <0x00>; i2c@0 { reg = <0x00>; #address-cells = <1>; #size-cells = <1>; gpio@20 { ... }; }; i2c@2 { reg = <0x02>; }; }; In the probe() I can iterate over the child nodes and create an adapter for each. The code is a bit fiddly but I think it's a net win if I can do away with the rtl9300-i2c-mux part. It also happily means that I don't have an extra I2C bus that is the same as the first mux channel. I'll try an tidy things up and get another iteration out before my weekend. > >> >> Thanks, >> Andi >> >>> MODULE_DESCRIPTION("RTL9300 I2C controller driver"); >>> MODULE_LICENSE("GPL"); >>> -- >>> 2.46.1 >>>