| Message ID | cover.1670897253.git.zhoubinbin@loongson.cn |
|---|---|
| Headers | show |
| Series | i2c: ls2x: Add support for the Loongson-2K/LS7A I2C controller | expand |
On Tue, Dec 13, 2022 at 04:26:55PM +0800, Binbin Zhou wrote: > Add support for the ACPI-based device registration, so that the driver > can be also enabled through ACPI table. LGTM now, Reviewed-by: Andy Shevchenko <andy@kernel.org> > Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn> > --- > drivers/i2c/busses/i2c-gpio.c | 28 ++++++++++++++++++---------- > 1 file changed, 18 insertions(+), 10 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c > index 0e4385a9bcf7..680936234ef8 100644 > --- a/drivers/i2c/busses/i2c-gpio.c > +++ b/drivers/i2c/busses/i2c-gpio.c > @@ -13,9 +13,9 @@ > #include <linux/init.h> > #include <linux/interrupt.h> > #include <linux/module.h> > -#include <linux/of.h> > #include <linux/platform_data/i2c-gpio.h> > #include <linux/platform_device.h> > +#include <linux/property.h> > #include <linux/slab.h> > > struct i2c_gpio_private_data { > @@ -300,22 +300,23 @@ static inline void i2c_gpio_fault_injector_init(struct platform_device *pdev) {} > static inline void i2c_gpio_fault_injector_exit(struct platform_device *pdev) {} > #endif /* CONFIG_I2C_GPIO_FAULT_INJECTOR*/ > > -static void of_i2c_gpio_get_props(struct device_node *np, > - struct i2c_gpio_platform_data *pdata) > +/* Get i2c-gpio properties from DT or ACPI table */ > +static void i2c_gpio_get_properties(struct device *dev, > + struct i2c_gpio_platform_data *pdata) > { > u32 reg; > > - of_property_read_u32(np, "i2c-gpio,delay-us", &pdata->udelay); > + device_property_read_u32(dev, "i2c-gpio,delay-us", &pdata->udelay); > > - if (!of_property_read_u32(np, "i2c-gpio,timeout-ms", ®)) > + if (!device_property_read_u32(dev, "i2c-gpio,timeout-ms", ®)) > pdata->timeout = msecs_to_jiffies(reg); > > pdata->sda_is_open_drain = > - of_property_read_bool(np, "i2c-gpio,sda-open-drain"); > + device_property_read_bool(dev, "i2c-gpio,sda-open-drain"); > pdata->scl_is_open_drain = > - of_property_read_bool(np, "i2c-gpio,scl-open-drain"); > + device_property_read_bool(dev, "i2c-gpio,scl-open-drain"); > pdata->scl_is_output_only = > - of_property_read_bool(np, "i2c-gpio,scl-output-only"); > + device_property_read_bool(dev, "i2c-gpio,scl-output-only"); > } > > static struct gpio_desc *i2c_gpio_get_desc(struct device *dev, > @@ -373,8 +374,8 @@ static int i2c_gpio_probe(struct platform_device *pdev) > bit_data = &priv->bit_data; > pdata = &priv->pdata; > > - if (np) { > - of_i2c_gpio_get_props(np, pdata); > + if (dev_fwnode(dev)) { > + i2c_gpio_get_properties(dev, pdata); > } else { > /* > * If all platform data settings are zero it is OK > @@ -489,10 +490,17 @@ static const struct of_device_id i2c_gpio_dt_ids[] = { > > MODULE_DEVICE_TABLE(of, i2c_gpio_dt_ids); > > +static const struct acpi_device_id i2c_gpio_acpi_match[] = { > + { "LOON0005" }, /* LoongArch */ > + { } > +}; > +MODULE_DEVICE_TABLE(acpi, i2c_gpio_acpi_match); > + > static struct platform_driver i2c_gpio_driver = { > .driver = { > .name = "i2c-gpio", > .of_match_table = i2c_gpio_dt_ids, > + .acpi_match_table = i2c_gpio_acpi_match, > }, > .probe = i2c_gpio_probe, > .remove = i2c_gpio_remove, > -- > 2.31.1 >
On Tue, Dec 13, 2022 at 04:26:57PM +0800, Binbin Zhou wrote: > This I2C module is integrated into the Loongson-2K SoCs and Loongson > LS7A bridge chip. ... > +// SPDX-License-Identifier: GPL-2.0 GPL-2.0-only > +/* > + * Loongson-2K/Loongson LS7A I2C master mode driver > + * > + * Copyright (C) 2013 Loongson Technology Corporation Limited. > + * Copyright (C) 2014-2017 Lemote, Inc. > + * Copyright (C) 2018-2022 Loongson Technology Corporation Limited. > + * > + * Originally written by liushaozong > + * No need to have this blank line. > + * Rewritten for mainline by Binbin Zhou <zhoubinbin@loongson.cn> > + */ ... > +#include <linux/bits.h> > +#include <linux/completion.h> > +#include <linux/device.h> > +#include <linux/i2c.h> > +#include <linux/init.h> > +#include <linux/interrupt.h> > +#include <linux/io.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/pm_runtime.h> > +#include <linux/platform_device.h> + property.h at least for dev_fwnode(). > +#include <linux/units.h> ... > +#define I2C_LS2X_PRER_LO 0x0 /* Freq Division Low Byte Register */ > +#define I2C_LS2X_PRER_HI 0x1 /* Freq Division High Byte Register */ _HI is not used, can we just drop the suffix? ... > +struct ls2x_i2c_priv { > + struct i2c_adapter adapter; > + struct device *dev; In some cases you are using adapter.dev, in some this one. Also isn't it the dev is the same as adapter.dev.parent? Hence, why do you need this one? > + void __iomem *base; > + struct i2c_timings i2c_t; > + struct completion cmd_complete; > +}; ... > + return ls2x_i2c_send_byte(adap, (LS2X_CR_START | LS2X_CR_WRITE)); Too many parentheses. ... > +static int ls2x_i2c_xfer_one(struct i2c_adapter *adap, > + struct i2c_msg *msg, bool stop) > +{ > + int ret; > + bool is_read = msg->flags & I2C_M_RD; > + > + /* Contains steps to send start condition and address */ > + ret = ls2x_i2c_start(adap, msg); > + if (ret) > + return ret; > + > + if (is_read) > + ret = ls2x_i2c_rx(adap, msg->buf, msg->len); > + else > + ret = ls2x_i2c_tx(adap, msg->buf, msg->len); > + > + /* could not acquire bus. bail out without STOP */ > + if (ret == -EAGAIN) > + return ret; So, if ret is *not* 0 and *not* --EAGAIN, why don't we bail out here? It needs at least a comment. > + if (stop) > + ret = ls2x_i2c_stop(adap); > + > + return ret; > +} ... > +static int ls2x_i2c_master_xfer(struct i2c_adapter *adap, > + struct i2c_msg *msgs, int num) > +{ > + int ret; > + struct i2c_msg *msg, *emsg = msgs + num; > + > + for (msg = msgs; msg < emsg; msg++) { > + /* Emit STOP if it is the last message or I2C_M_STOP is set */ > + bool stop = (msg + 1 == emsg) || (msg->flags & I2C_M_STOP); I'm wondering if we can always set the _STOP bit in the flags of the last message before entering this loop. In such case you can reduce to one part and supply as a parameter directly. > + > + ret = ls2x_i2c_xfer_one(adap, msg, stop); > + if (ret) > + return ret; > + } > + > + return num; > +} ... > + /* Calculate and set LS2X I2C frequency */ > + writel(LS2X_I2C_PCLK_FREQ / (5 * t->bus_freq_hz) - 1, > + priv->base + I2C_LS2X_PRER_LO); writel()?! Shouldn't be writew()? ... > + r = devm_request_irq(dev, irq, ls2x_i2c_isr, > + IRQF_SHARED, "ls2x-i2c", priv); There is a room on the previous line for at least one more argument. > + if (r < 0) > + return dev_err_probe(dev, r, "Unable to request irq %d\n", irq); ... > +subsys_initcall(ls2x_i2c_init_driver); Non-standard init calls should be commented.
Hi Andy: On Tue, Dec 13, 2022 at 10:58 PM Andy Shevchenko <andy@kernel.org> wrote: > > On Tue, Dec 13, 2022 at 04:26:57PM +0800, Binbin Zhou wrote: > > This I2C module is integrated into the Loongson-2K SoCs and Loongson > > LS7A bridge chip. > > ... > > > +// SPDX-License-Identifier: GPL-2.0 > > GPL-2.0-only > > > +/* > > + * Loongson-2K/Loongson LS7A I2C master mode driver > > + * > > + * Copyright (C) 2013 Loongson Technology Corporation Limited. > > + * Copyright (C) 2014-2017 Lemote, Inc. > > + * Copyright (C) 2018-2022 Loongson Technology Corporation Limited. > > + * > > + * Originally written by liushaozong > > > + * > > No need to have this blank line. > > > + * Rewritten for mainline by Binbin Zhou <zhoubinbin@loongson.cn> > > + */ > > ... > > > +#include <linux/bits.h> > > +#include <linux/completion.h> > > +#include <linux/device.h> > > +#include <linux/i2c.h> > > +#include <linux/init.h> > > +#include <linux/interrupt.h> > > +#include <linux/io.h> > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > +#include <linux/pm_runtime.h> > > +#include <linux/platform_device.h> > > + property.h at least for dev_fwnode(). > > > +#include <linux/units.h> > > ... > > > +#define I2C_LS2X_PRER_LO 0x0 /* Freq Division Low Byte Register */ > > +#define I2C_LS2X_PRER_HI 0x1 /* Freq Division High Byte Register */ > > _HI is not used, can we just drop the suffix? I will drop it. > > ... > > > +struct ls2x_i2c_priv { > > > + struct i2c_adapter adapter; > > > + struct device *dev; > > In some cases you are using adapter.dev, in some this one. > Also isn't it the dev is the same as adapter.dev.parent? > > Hence, why do you need this one? I checked the use of 'dev' and it can be completely replaced by 'adapter.dev.parent'. I will drop it. > > > + void __iomem *base; > > + struct i2c_timings i2c_t; > > + struct completion cmd_complete; > > +}; > > ... > > > + return ls2x_i2c_send_byte(adap, (LS2X_CR_START | LS2X_CR_WRITE)); > > Too many parentheses. > > ... > > > +static int ls2x_i2c_xfer_one(struct i2c_adapter *adap, > > + struct i2c_msg *msg, bool stop) > > +{ > > + int ret; > > + bool is_read = msg->flags & I2C_M_RD; > > + > > + /* Contains steps to send start condition and address */ > > + ret = ls2x_i2c_start(adap, msg); > > + if (ret) > > + return ret; > > + > > + if (is_read) > > + ret = ls2x_i2c_rx(adap, msg->buf, msg->len); > > + else > > + ret = ls2x_i2c_tx(adap, msg->buf, msg->len); > > + > > + /* could not acquire bus. bail out without STOP */ > > + if (ret == -EAGAIN) > > + return ret; > > So, if ret is *not* 0 and *not* --EAGAIN, why don't we bail out here? It needs > at least a comment. After more tests, these judgment conditions may be incomplete. The following judgment may make more sense: 1. The return value of ls2x_i2c_start() should also be judged. 2. When ret = -ENXIO (no ack), we should stop the bus; for other errors, such as "arbitration lost", we should reinitialize the i2c bus. As follows: @@ -162,23 +222,24 @@ static int ls2x_i2c_xfer_one(struct i2c_adapter *adap, { int ret; bool is_read = msg->flags & I2C_M_RD; + struct ls2x_i2c_priv *priv = i2c_get_adapdata(adap); /* Contains steps to send start condition and address */ ret = ls2x_i2c_start(adap, msg); - if (ret) - return ret; - - if (is_read) - ret = ls2x_i2c_rx(adap, msg->buf, msg->len); - else - ret = ls2x_i2c_tx(adap, msg->buf, msg->len); - - /* could not acquire bus. bail out without STOP */ - if (ret == -EAGAIN) - return ret; + if (!ret) { + if (is_read) + ret = ls2x_i2c_rx(adap, msg->buf, msg->len); + else + ret = ls2x_i2c_tx(adap, msg->buf, msg->len); + + if (!ret && stop) + ret = ls2x_i2c_stop(adap); + } - if (stop) - ret = ls2x_i2c_stop(adap); + if (ret == -ENXIO) + ls2x_i2c_stop(adap); + else if (ret < 0) + ls2x_i2c_reginit(priv); return ret; } > > > + if (stop) > > + ret = ls2x_i2c_stop(adap); > > + > > + return ret; > > +} > > ... > > > +static int ls2x_i2c_master_xfer(struct i2c_adapter *adap, > > + struct i2c_msg *msgs, int num) > > +{ > > + int ret; > > + struct i2c_msg *msg, *emsg = msgs + num; > > + > > + for (msg = msgs; msg < emsg; msg++) { > > + /* Emit STOP if it is the last message or I2C_M_STOP is set */ > > > + bool stop = (msg + 1 == emsg) || (msg->flags & I2C_M_STOP); > > I'm wondering if we can always set the _STOP bit in the flags of the last > message before entering this loop. In such case you can reduce to one part > and supply as a parameter directly. Here, I think the I2C_M_STOP flag can be ignored as long as the last msg is checked. As follows: @@ -190,10 +190,7 @@ static int ls2x_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msg, *emsg = msgs + num; for (msg = msgs; msg < emsg; msg++) { - /* Emit STOP if it is the last message or I2C_M_STOP is set */ - bool stop = (msg + 1 == emsg) || (msg->flags & I2C_M_STOP); - - ret = ls2x_i2c_xfer_one(adap, msg, stop); + ret = ls2x_i2c_xfer_one(adap, msg, msg == (emsg - 1)); if (ret) return ret; } > > > + > > + ret = ls2x_i2c_xfer_one(adap, msg, stop); > > + if (ret) > > + return ret; > > + } > > + > > + return num; > > +} > > ... > > > + /* Calculate and set LS2X I2C frequency */ > > + writel(LS2X_I2C_PCLK_FREQ / (5 * t->bus_freq_hz) - 1, > > + priv->base + I2C_LS2X_PRER_LO); > > writel()?! Shouldn't be writew()? Sorry, it's my fault, it should be writew(). > > ... > > > + r = devm_request_irq(dev, irq, ls2x_i2c_isr, > > + IRQF_SHARED, "ls2x-i2c", priv); > > There is a room on the previous line for at least one more argument. > > > + if (r < 0) > > + return dev_err_probe(dev, r, "Unable to request irq %d\n", irq); > > ... > > > +subsys_initcall(ls2x_i2c_init_driver); > > Non-standard init calls should be commented. On the LS7A bridge chip, the display subsystem depends on it. The i2c channel to read the EEID is fixed and the i2c driver should be initialized earlier than it, otherwise it will cause conflicts. I will comment this. Thanks. Binbin > > -- > With Best Regards, > Andy Shevchenko > > >
Hi all: This patch series adds support for the I2C module found on various Loongson systems with the Loongson-2K SoC or the Loongson LS7A bridge chip. For now, the I2C driver is suitable for DT-based or ACPI-based systems. I have tested on Loongson-3A5000LA+LS7A1000/LS7A2000, Loongson-2K1000LA and Loongson-2K0500. Thanks. Changes since V5: - patch (1/4) - Add property.h. - patch (3/4) - Put the ls2x_i2c_reginit() in front of devm_request_irq(); - Refact ls2x_i2c_adjust_bus_speed: discard the magic value of the divider register and LS2X_I2C_FREQ_STD is used to calculate the frequency; - Drop useless parameters: priv->suspended, and also disable I2C interrupts during suspend; - Drop ls2x_i2c_remove(), for the adapter will be auto deleted on driver detach; - Drop MODULE_ALIAS; - Code formatting, such as alignment. Thanks Andy for your comments. Changes since V4: - patch (1/4) - Drop unneeded headers: of.h; - xxx_props -> xxx_properties. - patch (2/4) - Add interrupt headers to fix syntax error found by Rob. - patch (3/4) - Drop atmoic loop in ls2x_i2c_master_xfer(), I have tested it on the appropriate environment with no problems; - Define the corresponding bits in I2C_LS2X_CTR to avoid magic numbers; - dev_get_drvdata() is used to get ls2x_i2c_priv() in ls2x_i2c_suspend(); - i2c_add_adapter() -> devm_i2c_add_adapter(); - SET_SYSTEM_SLEEP_PM_OPS() -> DEFINE_RUNTIME_DEV_PM_OPS(); - Code formatting, such as alignment. Details: https://lore.kernel.org/all/Y4e%2F6KewuHjAluSZ@smile.fi.intel.com/ Changes since V3: - Addressed all review comments from v3 - Change the changelog text to make it clearer (1/5); - Fix some minor bugs, such as formatting issues (2/5); - Fix some formatting issues (3/5); - Deep refactoring of code for clarity (4/5). Details: https://lore.kernel.org/all/Y4S2cnlAm3YYvZ8E@smile.fi.intel.com/ Thanks to all for their suggestions. Changes since V2: - Addressed all review comments from v2 - Drop of_match_ptr() in i2c-gpio to avoid potential unused warnings (1/5); - Introduce i2c_gpio_get_props() function as the generic interface to get i2c-gpio props from DT or ACPI table (2/5); - Refact ls2x i2c code, similar to removing excessive goto tags (4/5). Thanks to Andy and Mika for their suggestions. Changes since V1: - Remove the function of getting the static i2c bus number from ACPI "_UID"; - Fix build warning from kernel test robot. Binbin Zhou (4): i2c: gpio: Add support on ACPI-based system dt-bindings: i2c: add Loongson LS2X I2C controller i2c: ls2x: Add driver for Loongson-2K/LS7A I2C controller LoongArch: Enable LS2X I2C in loongson3_defconfig .../bindings/i2c/loongson,ls2x-i2c.yaml | 51 +++ arch/loongarch/configs/loongson3_defconfig | 1 + drivers/i2c/busses/Kconfig | 11 + drivers/i2c/busses/Makefile | 1 + drivers/i2c/busses/i2c-gpio.c | 28 +- drivers/i2c/busses/i2c-ls2x.c | 376 ++++++++++++++++++ 6 files changed, 458 insertions(+), 10 deletions(-) create mode 100644 Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.yaml create mode 100644 drivers/i2c/busses/i2c-ls2x.c