Messages in this thread | | | Date | Wed, 07 May 2014 09:42:25 +0900 | From | Pankaj Dubey <> | Subject | Re: [PATCH v2 2/6] ARM: EXYNOS: Move SYS_I2C_CFG register save/restore to i2c driver |
| |
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@the-dreams.de> >> CC: Russell King <linux@arm.linux.org.uk> >> CC: linux-i2c@vger.kernel.org >> Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> >> --- >> 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
| |