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
>
>
>
