Hi Andy: On Mon, Dec 12, 2022 at 5:47 PM Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> 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 > > >