Message ID | cover.1670293176.git.zhoubinbin@loongson.cn |
---|---|
Headers | show |
Series | i2c: ls2x: Add support for the Loongson-2K/LS7A I2C controller | expand |
On Tue, Dec 06, 2022 at 11:16:54AM +0800, Binbin Zhou wrote: > Add support for the ACPI-based device registration, so that the driver > can be also enabled through ACPI table. ... > -#include <linux/of.h> Cool, but don't you need to add property.h? Otherwise looks good to me.
On Tue, Dec 06, 2022 at 11:16:56AM +0800, Binbin Zhou wrote: > This I2C module is integrated into the Loongson-2K SoCs and Loongson > LS7A bridge chip. Much better, thanks! ... > +/* > + * The I2C controller has a fixed I2C bus frequency by default, but to > + * be compatible with more client devices, we can obtain the set I2C > + * bus frequency from ACPI or FDT. > + */ > +static void ls2x_i2c_adjust_bus_speed(struct ls2x_i2c_priv *priv) > +{ > + u16 val = 0x12c; /* Default value of I2C divider latch register */ Besides comment better to be placed on top of the commented line, the value is better to have its own definition where you place the comment and elaborate what it means in practice (The clock frequency is changed? Bus speed is different?) > + struct i2c_timings *t = &priv->i2c_t; > + u32 acpi_speed = i2c_acpi_find_bus_speed(priv->dev); > + > + i2c_parse_fw_timings(priv->dev, t, false); > + > + if (acpi_speed || t->bus_freq_hz) > + val = 10 * HZ_PER_MHZ / max(t->bus_freq_hz, acpi_speed) - 1; > + > + /* Set LS2X I2C frequency */ > + writel(val, priv->base + I2C_LS2X_PRER_LO); > +} ... > + writeb(data | LS2X_CTR_EN | LS2X_CTR_IEN | LS2X_CTR_MST, > + priv->base + I2C_LS2X_CTR); Wrong indentation. ... > + r = devm_request_irq(dev, irq, ls2x_i2c_irq_handler, > + IRQF_SHARED, "ls2x-i2c", priv); > + if (r < 0) > + return dev_err_probe(dev, r, "Unable to request irq %d\n", irq); You requested IRQ without filling all data structures. Is it fine? Have you checked that with CONFIG_DEBUG_SHIRQ being enabled? ... > + r = devm_i2c_add_adapter(dev, adap); > + if (r) > + return dev_err_probe(dev, r, "Failure adding adapter\n"); > + > + return 0; > +} Looking at the above... > +static int ls2x_i2c_remove(struct platform_device *pdev) > +{ > + struct ls2x_i2c_priv *priv = platform_get_drvdata(pdev); > + > + i2c_del_adapter(&priv->adapter); ...are you sure this is correct? > + return 0; > +} ... > +static int ls2x_i2c_suspend(struct device *dev) > +{ > + struct ls2x_i2c_priv *priv = dev_get_drvdata(dev); > + priv->suspended = 1; No protection needed? > + return 0; > +} > + > +static int ls2x_i2c_resume(struct device *dev) > +{ > + struct ls2x_i2c_priv *priv = dev_get_drvdata(dev); > + priv->suspended = 0; Ditto. > + ls2x_i2c_reginit(priv); > + return 0; > +} ... > +MODULE_ALIAS("platform:ls2x-i2c"); Why is this required?
在 2022/12/6 23:13, Andy Shevchenko 写道: > On Tue, Dec 06, 2022 at 11:16:54AM +0800, Binbin Zhou wrote: >> Add support for the ACPI-based device registration, so that the driver >> can be also enabled through ACPI table. > ... > >> -#include <linux/of.h> > Cool, but don't you need to add property.h? Get it, I should have been more careful, maybe I'm still not very sensitive to header files. Now I'm still basically judging based on whether I compile with errors or not. Thanks. Binbin > > Otherwise looks good to me. >
Hi Andy: 在 2022/12/6 23:23, Andy Shevchenko 写道: > On Tue, Dec 06, 2022 at 11:16:56AM +0800, Binbin Zhou wrote: >> This I2C module is integrated into the Loongson-2K SoCs and Loongson >> LS7A bridge chip. > Much better, thanks! > > ... > >> +/* >> + * The I2C controller has a fixed I2C bus frequency by default, but to >> + * be compatible with more client devices, we can obtain the set I2C >> + * bus frequency from ACPI or FDT. >> + */ >> +static void ls2x_i2c_adjust_bus_speed(struct ls2x_i2c_priv *priv) >> +{ >> + u16 val = 0x12c; /* Default value of I2C divider latch register */ > Besides comment better to be placed on top of the commented line, the value > is better to have its own definition where you place the comment and elaborate > what it means in practice (The clock frequency is changed? Bus speed is > different?) Ok, I'll put this comment on a separate line. The LS2X I2C supports STANDARD_MODE and FAST_MODE, so the maximum bus frequency is 400kHz. "0x12c" is our empirical value after experimentation and represents 33KHz. Also, I think the better way is: @@ -53,6 +53,15 @@ #define LS2X_CTR_IEN BIT(6) /* Enable i2c interrupt */ #define LS2X_CTR_MST BIT(5) /* 0: Slave mode 1: Master mode */ +/* The PCLK clock frequency input from the LPB bus */ +#define LS2X_I2C_PCLK_FREQ (50 * HZ_PER_MHZ) +/* + * The LS2X I2C controller supports standard mode and fast mode, + * so the maximum bus frequency is 400kHz. + * The '33KHz' is our empirical value after experimentation. + */ +#define LS2X_I2C_FREQ_STD (33 * HZ_PER_KHZ) + struct ls2x_i2c_priv { struct i2c_adapter adapter; struct device *dev; @@ -231,17 +240,19 @@ static irqreturn_t ls2x_i2c_irq_handler(int this_irq, void *dev_id) */ static void ls2x_i2c_adjust_bus_speed(struct ls2x_i2c_priv *priv) { - u16 val = 0x12c; /* Default value of I2C divider latch register */ struct i2c_timings *t = &priv->i2c_t; u32 acpi_speed = i2c_acpi_find_bus_speed(priv->dev); i2c_parse_fw_timings(priv->dev, t, false); if (acpi_speed || t->bus_freq_hz) - val = 10 * HZ_PER_MHZ / max(t->bus_freq_hz, acpi_speed) - 1; + t->bus_freq_hz = max(t->bus_freq_hz, acpi_speed); + else + t->bus_freq_hz = LS2X_I2C_FREQ_STD; - /* Set LS2X I2C frequency */ - writel(val, priv->base + I2C_LS2X_PRER_LO); + /* Calculate and set LS2X I2C frequency */ + writel((LS2X_I2C_PCLK_FREQ / (5 * t->bus_freq_hz) - 1), + priv->base + I2C_LS2X_PRER_LO); } >> + struct i2c_timings *t = &priv->i2c_t; >> + u32 acpi_speed = i2c_acpi_find_bus_speed(priv->dev); >> + >> + i2c_parse_fw_timings(priv->dev, t, false); >> + >> + if (acpi_speed || t->bus_freq_hz) >> + val = 10 * HZ_PER_MHZ / max(t->bus_freq_hz, acpi_speed) - 1; >> + >> + /* Set LS2X I2C frequency */ >> + writel(val, priv->base + I2C_LS2X_PRER_LO); >> +} > ... > >> + writeb(data | LS2X_CTR_EN | LS2X_CTR_IEN | LS2X_CTR_MST, >> + priv->base + I2C_LS2X_CTR); > Wrong indentation. > > ... > >> + r = devm_request_irq(dev, irq, ls2x_i2c_irq_handler, >> + IRQF_SHARED, "ls2x-i2c", priv); >> + if (r < 0) >> + return dev_err_probe(dev, r, "Unable to request irq %d\n", irq); > You requested IRQ without filling all data structures. Is it fine? Have you > checked that with CONFIG_DEBUG_SHIRQ being enabled? Sorry, I don't quite understand what you mean by "without filling all data structures", I need call ls2x_i2c_reginit(priv) before it ? I see that other i2c drivers request interrupts at about the same time as I do. I tested it with CONFIG_DEBUG_SHIRQ and no exceptions were reported. > ... > >> + r = devm_i2c_add_adapter(dev, adap); >> + if (r) >> + return dev_err_probe(dev, r, "Failure adding adapter\n"); >> + >> + return 0; >> +} > Looking at the above... > >> +static int ls2x_i2c_remove(struct platform_device *pdev) >> +{ >> + struct ls2x_i2c_priv *priv = platform_get_drvdata(pdev); >> + >> + i2c_del_adapter(&priv->adapter); > ...are you sure this is correct? When we use devm_i2c_add_adapter(), the adapter will be auto deleted on driver detach. So I just drop the ls2x_i2c_remove() ? >> + return 0; >> +} > ... > >> +static int ls2x_i2c_suspend(struct device *dev) >> +{ >> + struct ls2x_i2c_priv *priv = dev_get_drvdata(dev); >> + priv->suspended = 1; > No protection needed? Actually this variable is not used elsewhere, maybe it is useless, I will try to remove it and add some necessary actions in the suspend/rusume callbacks, such as disable i2c interrupts, to ensure integrity. >> + return 0; >> +} >> + >> +static int ls2x_i2c_resume(struct device *dev) >> +{ >> + struct ls2x_i2c_priv *priv = dev_get_drvdata(dev); >> + priv->suspended = 0; > Ditto. > >> + ls2x_i2c_reginit(priv); >> + return 0; >> +} > ... > >> +MODULE_ALIAS("platform:ls2x-i2c"); > Why is this required? I just referred to other drivers before, and now the MODULE_DEVICE_TABLE already creates proper alias for platform driver. I will drop it. Thanks. Binbin
On Wed, Dec 07, 2022 at 03:10:43PM +0800, Binbin Zhou wrote: > 在 2022/12/6 23:13, Andy Shevchenko 写道: > > On Tue, Dec 06, 2022 at 11:16:54AM +0800, Binbin Zhou wrote: ... > > > -#include <linux/of.h> > > Cool, but don't you need to add property.h? > > Get it, I should have been more careful, maybe I'm still not very sensitive > to header files. > > Now I'm still basically judging based on whether I compile with errors or > not. You can generate the include tree by running `make includecheck` The rule of thumb is to include what you are the direct user of. With a few exceptions where we have the guarantees that one header is always included by another (e.g. bits.h included by bitops.h). > > Otherwise looks good to me.
On Thu, Dec 08, 2022 at 04:55:39PM +0800, Binbin Zhou wrote: > 在 2022/12/6 23:23, Andy Shevchenko 写道: > > On Tue, Dec 06, 2022 at 11:16:56AM +0800, Binbin Zhou wrote: ... > > > +/* > > > + * The I2C controller has a fixed I2C bus frequency by default, but to > > > + * be compatible with more client devices, we can obtain the set I2C > > > + * bus frequency from ACPI or FDT. > > > + */ > > > +static void ls2x_i2c_adjust_bus_speed(struct ls2x_i2c_priv *priv) > > > +{ > > > + u16 val = 0x12c; /* Default value of I2C divider latch register */ > > Besides comment better to be placed on top of the commented line, the value > > is better to have its own definition where you place the comment and elaborate > > what it means in practice (The clock frequency is changed? Bus speed is > > different?) > > Ok, I'll put this comment on a separate line. > > The LS2X I2C supports STANDARD_MODE and FAST_MODE, so the maximum bus > frequency is 400kHz. > "0x12c" is our empirical value after experimentation and represents 33KHz. > > Also, I think the better way is: > > @@ -53,6 +53,15 @@ > #define LS2X_CTR_IEN BIT(6) /* Enable i2c interrupt */ > #define LS2X_CTR_MST BIT(5) /* 0: Slave mode 1: Master mode */ > > +/* The PCLK clock frequency input from the LPB bus */ > +#define LS2X_I2C_PCLK_FREQ (50 * HZ_PER_MHZ) > +/* > + * The LS2X I2C controller supports standard mode and fast mode, > + * so the maximum bus frequency is 400kHz. > + * The '33KHz' is our empirical value after experimentation. > + */ > +#define LS2X_I2C_FREQ_STD (33 * HZ_PER_KHZ) > + > struct ls2x_i2c_priv { > struct i2c_adapter adapter; > struct device *dev; > @@ -231,17 +240,19 @@ static irqreturn_t ls2x_i2c_irq_handler(int this_irq, > void *dev_id) > */ > static void ls2x_i2c_adjust_bus_speed(struct ls2x_i2c_priv *priv) > { > - u16 val = 0x12c; /* Default value of I2C divider latch register */ > struct i2c_timings *t = &priv->i2c_t; > u32 acpi_speed = i2c_acpi_find_bus_speed(priv->dev); > > i2c_parse_fw_timings(priv->dev, t, false); > > if (acpi_speed || t->bus_freq_hz) > - val = 10 * HZ_PER_MHZ / max(t->bus_freq_hz, acpi_speed) - 1; > + t->bus_freq_hz = max(t->bus_freq_hz, acpi_speed); > + else > + t->bus_freq_hz = LS2X_I2C_FREQ_STD; > > - /* Set LS2X I2C frequency */ > - writel(val, priv->base + I2C_LS2X_PRER_LO); > + /* Calculate and set LS2X I2C frequency */ > + writel((LS2X_I2C_PCLK_FREQ / (5 * t->bus_freq_hz) - 1), Fine with me, but drop unneeded parentheses. > + priv->base + I2C_LS2X_PRER_LO); > } > > > + struct i2c_timings *t = &priv->i2c_t; > > > + u32 acpi_speed = i2c_acpi_find_bus_speed(priv->dev); > > > + > > > + i2c_parse_fw_timings(priv->dev, t, false); > > > + > > > + if (acpi_speed || t->bus_freq_hz) > > > + val = 10 * HZ_PER_MHZ / max(t->bus_freq_hz, acpi_speed) - 1; > > > + > > > + /* Set LS2X I2C frequency */ > > > + writel(val, priv->base + I2C_LS2X_PRER_LO); > > > +} ... > > > + r = devm_request_irq(dev, irq, ls2x_i2c_irq_handler, > > > + IRQF_SHARED, "ls2x-i2c", priv); > > > + if (r < 0) > > > + return dev_err_probe(dev, r, "Unable to request irq %d\n", irq); > > You requested IRQ without filling all data structures. Is it fine? Have you > > checked that with CONFIG_DEBUG_SHIRQ being enabled? > > Sorry, I don't quite understand what you mean by "without filling all data > structures", I need call ls2x_i2c_reginit(priv) before it ? When you register an IRQ handler (which is that call) it needs to be prepared to handle interrupt immediately. Which means that your data structures has to be filled properly. If you can guarantee that with the current code, fine then. > I see that other i2c drivers request interrupts at about the same time as I > do. > > I tested it with CONFIG_DEBUG_SHIRQ and no exceptions were reported. Good. And you removed and reinserted module? At least this helps to detect some of the potential issues. ... > > > + r = devm_i2c_add_adapter(dev, adap); > > > + if (r) > > > + return dev_err_probe(dev, r, "Failure adding adapter\n"); > > > + > > > + return 0; > > > +} > > Looking at the above... > > > > > +static int ls2x_i2c_remove(struct platform_device *pdev) > > > +{ > > > + struct ls2x_i2c_priv *priv = platform_get_drvdata(pdev); > > > + > > > + i2c_del_adapter(&priv->adapter); > > ...are you sure this is correct? > > When we use devm_i2c_add_adapter(), the adapter will be auto deleted on > driver detach. > > So I just drop the ls2x_i2c_remove() ? Correct. > > > + return 0; > > > +} ... > > > +static int ls2x_i2c_suspend(struct device *dev) > > > +{ > > > + struct ls2x_i2c_priv *priv = dev_get_drvdata(dev); > > > + priv->suspended = 1; > > No protection needed? > > Actually this variable is not used elsewhere, maybe it is useless, I will > try to remove it and add some necessary actions in the suspend/rusume > callbacks, such as disable i2c interrupts, to ensure integrity. Is your interrupt a wake source? It might be that you will need a special handling of it. > > > + return 0; > > > +}
Hi Andy: On Mon, Dec 12, 2022 at 5:47 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Dec 08, 2022 at 04:55:39PM +0800, Binbin Zhou wrote: > > 在 2022/12/6 23:23, Andy Shevchenko 写道: > > > On Tue, Dec 06, 2022 at 11:16:56AM +0800, Binbin Zhou wrote: > > ... > > > > > +/* > > > > + * The I2C controller has a fixed I2C bus frequency by default, but to > > > > + * be compatible with more client devices, we can obtain the set I2C > > > > + * bus frequency from ACPI or FDT. > > > > + */ > > > > +static void ls2x_i2c_adjust_bus_speed(struct ls2x_i2c_priv *priv) > > > > +{ > > > > + u16 val = 0x12c; /* Default value of I2C divider latch register */ > > > Besides comment better to be placed on top of the commented line, the value > > > is better to have its own definition where you place the comment and elaborate > > > what it means in practice (The clock frequency is changed? Bus speed is > > > different?) > > > > Ok, I'll put this comment on a separate line. > > > > The LS2X I2C supports STANDARD_MODE and FAST_MODE, so the maximum bus > > frequency is 400kHz. > > "0x12c" is our empirical value after experimentation and represents 33KHz. > > > > Also, I think the better way is: > > > > @@ -53,6 +53,15 @@ > > #define LS2X_CTR_IEN BIT(6) /* Enable i2c interrupt */ > > #define LS2X_CTR_MST BIT(5) /* 0: Slave mode 1: Master mode */ > > > > +/* The PCLK clock frequency input from the LPB bus */ > > +#define LS2X_I2C_PCLK_FREQ (50 * HZ_PER_MHZ) > > +/* > > + * The LS2X I2C controller supports standard mode and fast mode, > > + * so the maximum bus frequency is 400kHz. > > + * The '33KHz' is our empirical value after experimentation. > > + */ > > +#define LS2X_I2C_FREQ_STD (33 * HZ_PER_KHZ) > > + > > struct ls2x_i2c_priv { > > struct i2c_adapter adapter; > > struct device *dev; > > @@ -231,17 +240,19 @@ static irqreturn_t ls2x_i2c_irq_handler(int this_irq, > > void *dev_id) > > */ > > static void ls2x_i2c_adjust_bus_speed(struct ls2x_i2c_priv *priv) > > { > > - u16 val = 0x12c; /* Default value of I2C divider latch register */ > > struct i2c_timings *t = &priv->i2c_t; > > u32 acpi_speed = i2c_acpi_find_bus_speed(priv->dev); > > > > i2c_parse_fw_timings(priv->dev, t, false); > > > > if (acpi_speed || t->bus_freq_hz) > > - val = 10 * HZ_PER_MHZ / max(t->bus_freq_hz, acpi_speed) - 1; > > + t->bus_freq_hz = max(t->bus_freq_hz, acpi_speed); > > + else > > + t->bus_freq_hz = LS2X_I2C_FREQ_STD; > > > > - /* Set LS2X I2C frequency */ > > - writel(val, priv->base + I2C_LS2X_PRER_LO); > > + /* Calculate and set LS2X I2C frequency */ > > > + writel((LS2X_I2C_PCLK_FREQ / (5 * t->bus_freq_hz) - 1), > > Fine with me, but drop unneeded parentheses. > > > + priv->base + I2C_LS2X_PRER_LO); > > } > > > > > + struct i2c_timings *t = &priv->i2c_t; > > > > + u32 acpi_speed = i2c_acpi_find_bus_speed(priv->dev); > > > > + > > > > + i2c_parse_fw_timings(priv->dev, t, false); > > > > + > > > > + if (acpi_speed || t->bus_freq_hz) > > > > + val = 10 * HZ_PER_MHZ / max(t->bus_freq_hz, acpi_speed) - 1; > > > > + > > > > + /* Set LS2X I2C frequency */ > > > > + writel(val, priv->base + I2C_LS2X_PRER_LO); > > > > +} > > ... > > > > > + r = devm_request_irq(dev, irq, ls2x_i2c_irq_handler, > > > > + IRQF_SHARED, "ls2x-i2c", priv); > > > > + if (r < 0) > > > > + return dev_err_probe(dev, r, "Unable to request irq %d\n", irq); > > > You requested IRQ without filling all data structures. Is it fine? Have you > > > checked that with CONFIG_DEBUG_SHIRQ being enabled? > > > > Sorry, I don't quite understand what you mean by "without filling all data > > structures", I need call ls2x_i2c_reginit(priv) before it ? > > When you register an IRQ handler (which is that call) it needs to be prepared > to handle interrupt immediately. Which means that your data structures has to > be filled properly. If you can guarantee that with the current code, fine then. Emm. If so, I think it might be safer to put ls2x_i2c_reginit() in front of it, since the bus frequency needs to be set correctly. > > > I see that other i2c drivers request interrupts at about the same time as I > > do. > > > > I tested it with CONFIG_DEBUG_SHIRQ and no exceptions were reported. > > Good. And you removed and reinserted module? > > At least this helps to detect some of the potential issues. Yes, all of these seem normal. > > ... > > > > > + r = devm_i2c_add_adapter(dev, adap); > > > > + if (r) > > > > + return dev_err_probe(dev, r, "Failure adding adapter\n"); > > > > + > > > > + return 0; > > > > +} > > > Looking at the above... > > > > > > > +static int ls2x_i2c_remove(struct platform_device *pdev) > > > > +{ > > > > + struct ls2x_i2c_priv *priv = platform_get_drvdata(pdev); > > > > + > > > > + i2c_del_adapter(&priv->adapter); > > > ...are you sure this is correct? > > > > When we use devm_i2c_add_adapter(), the adapter will be auto deleted on > > driver detach. > > > > So I just drop the ls2x_i2c_remove() ? > > Correct. > > > > > + return 0; > > > > +} > > ... > > > > > +static int ls2x_i2c_suspend(struct device *dev) > > > > +{ > > > > + struct ls2x_i2c_priv *priv = dev_get_drvdata(dev); > > > > + priv->suspended = 1; > > > No protection needed? .> > > > Actually this variable is not used elsewhere, maybe it is useless, I will > > try to remove it and add some necessary actions in the suspend/rusume > > callbacks, such as disable i2c interrupts, to ensure integrity. > > Is your interrupt a wake source? It might be that you will need a special > handling of it. No. It isn't a wake source. I think it is enough to clear the interrupt enable bit in the control register. As follows: writeb(readb(priv->base + I2C_LS2X_CTR) & ~LS2X_CTR_IEN, priv->base + I2C_LS2X_CTR); Thanks. Binbin > > > > > + return 0; > > > > +} > > -- > With Best Regards, > Andy Shevchenko > > >