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

[Pankaj Dubey <pankaj.dubey@samsung.com>]

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