Re: [PATCH v6] I2C: add CSR SiRFprimaII on-chip I2C controllers driver

[Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 4195 bytes --]

Barry,

On Thu, Feb 16, 2012 at 10:24:00PM +0530, Shubhrajyoti Datta wrote:

please consider these comments.

Thanks,

   Wolfram

> Hello,
> 
> On Wed, Feb 8, 2012 at 8:58 PM, Barry Song <21cnbao-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > From: Zhiwu Song <Zhiwu.Song-kQvG35nSl+M@public.gmane.org>
> >
> > SiRFprimaII is the latest generation application processor from CSR’s
> > multi-function SoC product family.
> > The SoC support codes are in arch/arm/mach-prima2 from Linux mainline
> > 3.0.
> > There are two I2C controllers on primaII, features include:
> > * Two I2C controller modules are on chip
> > * RISC I/O bus read write register
> > * Up to 16 bytes data buffer for issuing commands and writing data
> >  at the same time
> > * Up to 16 commands, and receiving read data 16 bytes at a time
> > * Error INT report (ACK check)
> > * No-ACK bus protocols (SCCB bus protocols)
> >
> > Signed-off-by: Zhiwu Song <Zhiwu.Song-kQvG35nSl+M@public.gmane.org>
> > Signed-off-by: Xiangzhen Ye <Xiangzhen.Ye-kQvG35nSl+M@public.gmane.org>
> > Signed-off-by: Yuping Luo <Yuping.Luo-kQvG35nSl+M@public.gmane.org>
> > Signed-off-by: Barry Song <Baohua.Song-kQvG35nSl+M@public.gmane.org>
> > ---
> >  -v6:
> >  fix/cleanup lots of minor issues pointed out by wolfram;
> >  add OF property clock-frequency to support freq setting by DT;
> >  add lost devicetree binding document;
> >
> >  Documentation/devicetree/bindings/i2c/sirf-i2c.txt |   19 +
> >  drivers/i2c/busses/Kconfig                         |   10 +
> >  drivers/i2c/busses/Makefile                        |    1 +
> >  drivers/i2c/busses/i2c-sirf.c                      |  459 ++++++++++++++++++++
> >  4 files changed, 489 insertions(+), 0 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/i2c/sirf-i2c.txt
> >  create mode 100644 drivers/i2c/busses/i2c-sirf.c
> >
> > diff --git a/Documentation/devicetree/bindings/i2c/sirf-i2c.txt b/Documentation/devicetree/bindings/i2c/sirf-i2c.txt
> > new file mode 100644
> > index 0000000..7baf9e1
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/i2c/sirf-i2c.txt
> <snip>
> 
> > +
> > +               if (i2c_stat & SIRFSOC_I2C_STAT_NACK)
> > +                       dev_err(&siic->adapter.dev, "ACK not received\n");
> Could we send stop here?
> > +               else
> > +                       dev_err(&siic->adapter.dev, "I2C error\n");
> > +
> > +               complete(&siic->done);
> <Snip>
> 
> > +static int i2c_sirfsoc_resume(struct device *dev)
> > +{
> > +       struct platform_device *pdev = to_platform_device(dev);
> > +       struct i2c_adapter *adapter = platform_get_drvdata(pdev);
> > +       struct sirfsoc_i2c *siic = adapter->algo_data;
> > +
> > +       clk_enable(siic->clk);
> > +       writel(SIRFSOC_I2C_RESET, siic->base + SIRFSOC_I2C_CTRL);
> Could you explain why is a reset needed.
> Or am I missing something.
> 
> > +       writel(SIRFSOC_I2C_CORE_EN | SIRFSOC_I2C_MASTER_MODE,
> > +               siic->base + SIRFSOC_I2C_CTRL);
> > +       writel(siic->clk_div, siic->base + SIRFSOC_I2C_CLK_CTRL);
> > +       writel(siic->sda_delay, siic->base + SIRFSOC_I2C_SDA_DELAY);
> > +       clk_disable(siic->clk);
> > +       return 0;
> > +}
> > +
> > +static const struct dev_pm_ops i2c_sirfsoc_pm_ops = {
> > +       .suspend = i2c_sirfsoc_suspend,
> > +       .resume = i2c_sirfsoc_resume,
> Could we use simple dev_pm ops here?
> 
> > +};
> > +#endif
> > +
> > +static const struct of_device_id sirfsoc_i2c_of_match[] __devinitconst = {
> > +       { .compatible = "sirf,prima2-i2c", },
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]