lkml.org 
[lkml]   [2014]   [May]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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



\
 
 \ /
  Last update: 2014-05-07 02:41    [W:0.057 / U:3.068 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site