Re: [PATCH v2 2/6] ARM: EXYNOS: Move SYS_I2C_CFG register save/restore to i2c driver

[Pankaj Dubey]

On 05/07/2014 04:04 AM, Tomasz Figa wrote:
> Hi Pankaj,
>
> On 06.05.2014 10:51, Pankaj Dubey wrote:
> > Let's move SYS_I2C_CFG register save/restore during s2r into i2c driver.
> > This will help in removing static iodesc based mapping from exynos.c.
> > Also will help in removing SoC specific checks in pm.c making it
> > more independent of such macros.
> >
> > CC: Wolfram Sang <wsa@xxxxxxxxxxxxx>
> > CC: Russell King <linux@xxxxxxxxxxxxxxxx>
> > CC: linux-i2c@xxxxxxxxxxxxxxx
> > Signed-off-by: Pankaj Dubey <pankaj.dubey@xxxxxxxxxxx>
> > ---
> >   arch/arm/mach-exynos/exynos.c           |   12 +-----------
> >   arch/arm/mach-exynos/include/mach/map.h |    3 ---
> >   arch/arm/mach-exynos/pm.c               |   10 ----------
> >   arch/arm/mach-exynos/regs-sys.h         |   22 ----------------------
> >   drivers/i2c/busses/i2c-s3c2410.c        |    8 ++++++++
> >   5 files changed, 9 insertions(+), 46 deletions(-)
> >   delete mode 100644 arch/arm/mach-exynos/regs-sys.h
>
> [snip]
>
> > diff --git a/drivers/i2c/busses/i2c-s3c2410.c 
> > b/drivers/i2c/busses/i2c-s3c2410.c
> > index 0420150..2095a01 100644
> > --- a/drivers/i2c/busses/i2c-s3c2410.c
> > +++ b/drivers/i2c/busses/i2c-s3c2410.c
> > @@ -133,6 +133,7 @@ struct s3c24xx_i2c {
> >       struct notifier_block    freq_transition;
> >   #endif
> >       struct regmap        *sysreg;
> > +    unsigned int        syc_cfg;
>
> I suspect this is a typo, as the name syc_cfg looks a bit strange. 
> Shouldn't it be sys_i2c_cfg?
Oops. Will correct it in next version.
> >   };
> >
> >   static struct platform_device_id s3c24xx_driver_ids[] = {
> > @@ -1293,6 +1294,9 @@ static int s3c24xx_i2c_suspend_noirq(struct device 
> > *dev)
> >       struct platform_device *pdev = to_platform_device(dev);
> >       struct s3c24xx_i2c *i2c = platform_get_drvdata(pdev);
> >
> > +    if (i2c->sysreg)
>
> IS_ERR() should be used.
>
> > +        regmap_read(i2c->sysreg, EXYNOS5_SYS_I2C_CFG, &i2c->syc_cfg);
>
> Aha, so this is where the reference to the regmap gets used outside the 
> probe. I'd say that changes to the i2c driver from this patch should be 
> squashed with previous patch and this patch should contain only arch 
> changes to remove old code.

OK, in next version I will update accordingly.

> However, I wonder if this is really the right approach to this, as now 
> you have save and restore duplicated for every instance of s3c24xx-i2c IP 
> block. If there are no bits other than interrupt mux selectors in this 
> registers then I guess this is fine (I can't look it up in the 
> documentation at the moment), but otherwise you can end-up with multiple 
> paths doing read-modify-write to this register in parallel possibly with 
> different values.

SYS_I2C_CFG for exynos5250 has only bits for mux selectors for i2c.

> Needless to say, this isn't very elegant, but I'm not opposed too much, 
> as I can't really think of anything better right now.
>
> > +
> >       i2c->suspended = 1;
> >
> >       return 0;
> > @@ -1304,6 +1308,10 @@ static int s3c24xx_i2c_resume(struct device *dev)
> >       struct s3c24xx_i2c *i2c = platform_get_drvdata(pdev);
> >
> >       i2c->suspended = 0;
> > +
> > +    if (i2c->sysreg)
> > +        regmap_write(i2c->sysreg, i2c->syc_cfg, EXYNOS5_SYS_I2C_CFG);
> > +
>
> I'd say this should be happening before setting i2c->suspended to 0 to 
> account for possible i2c transfers being requested in parallel. Also see 
> patch [1].
>
> [1] https://lkml.org/lkml/2014/4/11/632

OK, will move this before i2c->suspended = 0.

If possible please review other patches also in this series so that I can 
update
whole series with addressing all review comments.

> Best regards,
> Tomasz


--
Best Regards,
Pankaj Dubey

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/