Re: [PATCH V5 3/4] i2c: ls2x: Add driver for Loongson-2K/LS7A I2C controller

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux