Hi Kevin,
Thanks for your review,
On Tue, Apr 17, 2012 at 7:15 PM, Kevin Hilman <khilman@xxxxxx> wrote:
> Shubhrajyoti D <shubhrajyoti@xxxxxx> writes:
>
>> Currently i2c register restore is done always.
>> Adding conditional restore.
>> The i2c register restore is done only if the context is lost.
>
> OK, minor comment below.
>
>> Also remove the definition of SYSS_RESETDONE_MASK and use the
>> one in omap_hwmod.h.
>
> The definition is removed, but I don't see any of the users removed.
> If the users were cleaned up earlier in the series, then the removal of
> this should be done with that, or as a separate patch. I don't see why
> it belongs with this patch.
I have not removed the users actually the macro was redefined.
I have instead used the definition in omap_hwmod.h which gets added
when I include omap_device.h
Let me know if you prefer a separate patch?
>
>> Signed-off-by: Shubhrajyoti D <shubhrajyoti@xxxxxx>
>> ---
>> arch/arm/plat-omap/i2c.c | 3 ++
>> drivers/i2c/busses/i2c-omap.c | 52 ++++++++++++++++++++++++++--------------
>> include/linux/i2c-omap.h | 1 +
>> 3 files changed, 38 insertions(+), 18 deletions(-)
>>
>> diff --git a/arch/arm/plat-omap/i2c.c b/arch/arm/plat-omap/i2c.c
>> index db071bc..4ccab07 100644
>> --- a/arch/arm/plat-omap/i2c.c
>> +++ b/arch/arm/plat-omap/i2c.c
>> @@ -179,6 +179,9 @@ static inline int omap2_i2c_add_bus(int bus_id)
>> */
>> if (cpu_is_omap34xx())
>> pdata->set_mpu_wkup_lat = omap_pm_set_max_mpu_wakeup_lat_compat;
>> +
>> + pdata->get_context_loss_count = omap_pm_get_dev_context_loss_count;
>> +
>> pdev = omap_device_build(name, bus_id, oh, pdata,
>> sizeof(struct omap_i2c_bus_platform_data),
>> NULL, 0, 0);
>> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
>> index a882558..45389db 100644
>> --- a/drivers/i2c/busses/i2c-omap.c
>> +++ b/drivers/i2c/busses/i2c-omap.c
>> @@ -43,6 +43,7 @@
>> #include <linux/slab.h>
>> #include <linux/i2c-omap.h>
>> #include <linux/pm_runtime.h>
>> +#include <plat/omap_device.h>
>>
>> /* I2C controller revisions */
>> #define OMAP_I2C_OMAP1_REV_2 0x20
>> @@ -157,9 +158,6 @@ enum {
>> #define OMAP_I2C_SYSTEST_SDA_I (1 << 1) /* SDA line sense in */
>> #define OMAP_I2C_SYSTEST_SDA_O (1 << 0) /* SDA line drive out */
>>
>> -/* OCP_SYSSTATUS bit definitions */
>> -#define SYSS_RESETDONE_MASK (1 << 0)
>> -
>> /* OCP_SYSCONFIG bit definitions */
>> #define SYSC_CLOCKACTIVITY_MASK (0x3 << 8)
>> #define SYSC_SIDLEMODE_MASK (0x3 << 3)
>> @@ -184,6 +182,7 @@ struct omap_i2c_dev {
>> u32 latency; /* maximum mpu wkup latency */
>> void (*set_mpu_wkup_lat)(struct device *dev,
>> long latency);
>> + int (*get_context_loss_count)(struct device *dev);
>> u32 speed; /* Speed of bus in kHz */
>> u32 dtrev; /* extra revision from DT */
>> u32 flags;
>> @@ -206,6 +205,7 @@ struct omap_i2c_dev {
>> u16 syscstate;
>> u16 westate;
>> u16 errata;
>> + int dev_lost_count;
>> };
>>
>> static const u8 reg_map_ip_v1[] = {
>> @@ -1025,6 +1025,7 @@ omap_i2c_probe(struct platform_device *pdev)
>> dev->speed = pdata->clkrate;
>> dev->flags = pdata->flags;
>> dev->set_mpu_wkup_lat = pdata->set_mpu_wkup_lat;
>> + dev->get_context_loss_count = pdata->get_context_loss_count;
>> dev->dtrev = pdata->rev;
>> }
>>
>> @@ -1151,12 +1152,32 @@ omap_i2c_remove(struct platform_device *pdev)
>> }
>>
>> #ifdef CONFIG_PM_RUNTIME
>> +static void omap_i2c_restore(struct omap_i2c_dev *dev)
>> +{
>> + omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
>> + omap_i2c_write_reg(dev, OMAP_I2C_PSC_REG, dev->pscstate);
>> + omap_i2c_write_reg(dev, OMAP_I2C_SCLL_REG, dev->scllstate);
>> + omap_i2c_write_reg(dev, OMAP_I2C_SCLH_REG, dev->sclhstate);
>> + omap_i2c_write_reg(dev, OMAP_I2C_BUF_REG, dev->bufstate);
>> + omap_i2c_write_reg(dev, OMAP_I2C_WE_REG, dev->westate);
>> + omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
>> + /*
>> + * Don't write to this register if the IE state is 0 as it can
>> + * cause deadlock.
>> + */
>> + if (dev->iestate)
>> + omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev->iestate);
>> +
>> +}
>> static int omap_i2c_runtime_suspend(struct device *dev)
>> {
>> struct platform_device *pdev = to_platform_device(dev);
>> struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
>> u16 iv;
>>
>> + if (_dev->get_context_loss_count)
>> + _dev->dev_lost_count = _dev->get_context_loss_count(dev);
>> +
>> _dev->iestate = omap_i2c_read_reg(_dev, OMAP_I2C_IE_REG);
>> if (_dev->dtrev == OMAP_I2C_IP_VERSION_2)
>> omap_i2c_write_reg(_dev, OMAP_I2C_IP_V2_IRQENABLE_CLR, 1);
>> @@ -1179,24 +1200,19 @@ static int omap_i2c_runtime_resume(struct device *dev)
>> {
>> struct platform_device *pdev = to_platform_device(dev);
>> struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
>> + int loss_cnt;
>>
>> - if (_dev->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) {
>> - omap_i2c_write_reg(_dev, OMAP_I2C_CON_REG, 0);
>> - omap_i2c_write_reg(_dev, OMAP_I2C_PSC_REG, _dev->pscstate);
>> - omap_i2c_write_reg(_dev, OMAP_I2C_SCLL_REG, _dev->scllstate);
>> - omap_i2c_write_reg(_dev, OMAP_I2C_SCLH_REG, _dev->sclhstate);
>> - omap_i2c_write_reg(_dev, OMAP_I2C_BUF_REG, _dev->bufstate);
>> - omap_i2c_write_reg(_dev, OMAP_I2C_SYSC_REG, _dev->syscstate);
>> - omap_i2c_write_reg(_dev, OMAP_I2C_WE_REG, _dev->westate);
>> - omap_i2c_write_reg(_dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
>> + if (_dev->get_context_loss_count) {
>> + loss_cnt = _dev->get_context_loss_count(dev);
>> + if (loss_cnt < 0)
>> + return loss_cnt;
>
> To avoid messing up driver state, upon error, you should probably
> restore context, not abort.
Agreed , will fix this.
>
>> + if (_dev->dev_lost_count == loss_cnt && _dev->dev_lost_count)
>> + return 0;
>
> Why the non-zero check?
Actually the driver probe-->omap_i2c_init
here
<snip>
dev->pscstate = psc;
dev->scllstate = scll;
dev->sclhstate = sclh;
dev->bufstate = buf;
<snip>
variables are updated and the first write happens in the handler.
>
> Seems like all you need to check is if (_dev->dev_lost_count != loss_cnt).
>
> Kevin
>
>> }
>>
>> - /*
>> - * Don't write to this register if the IE state is 0 as it can
>> - * cause deadlock.
>> - */
>> - if (_dev->iestate)
>> - omap_i2c_write_reg(_dev, OMAP_I2C_IE_REG, _dev->iestate);
>> + if (_dev->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE)
>> + omap_i2c_restore(_dev);
>>
>> return 0;
>> }
>> diff --git a/include/linux/i2c-omap.h b/include/linux/i2c-omap.h
>> index 92a0dc7..c76cbc0 100644
>> --- a/include/linux/i2c-omap.h
>> +++ b/include/linux/i2c-omap.h
>> @@ -35,6 +35,7 @@ struct omap_i2c_bus_platform_data {
>> u32 rev;
>> u32 flags;
>> void (*set_mpu_wkup_lat)(struct device *dev, long set);
>> + int (*get_context_loss_count)(struct device *dev);
>> };
>>
>> #endif
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
