[PATCH v2] i2c: omap: re-factor omap_i2c_init function

[Shubhrajyoti Datta]

On Thu, Oct 25, 2012 at 12:06 PM, Felipe Balbi <balbi at ti.com> wrote:
> Hi,
>
> On Thu, Oct 25, 2012 at 12:06:51PM +0530, Shubhrajyoti D wrote:
>> re-factor omap_i2c_init() so that we can re-use it for resume.
>> While at it also remove the bufstate variable as we write it
>> in omap_i2c_resize_fifo for every transfer.
>>
>> Signed-off-by: Shubhrajyoti D <shubhrajyoti at ti.com>
>> ---
>> v2 - add the iestate 0 check back.
>>    - Remove a stray change.
>> - Applies on top of Felipe's patches.
>> http://www.spinics.net/lists/linux-omap/msg79995.html
>>
>>
>> Tested with Terro sys fix + Felipe's stop sched_clock() during suspend
>> on omap3636 beagle both idle and suspend.
>>
>> Functional testing on omap4sdp.
>>
>>  drivers/i2c/busses/i2c-omap.c |   71 ++++++++++++++++++----------------------
>>  1 files changed, 32 insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
>> index 5e5cefb..3d400b1 100644
>> --- a/drivers/i2c/busses/i2c-omap.c
>> +++ b/drivers/i2c/busses/i2c-omap.c
>> @@ -209,7 +209,6 @@ struct omap_i2c_dev {
>>       u16                     pscstate;
>>       u16                     scllstate;
>>       u16                     sclhstate;
>> -     u16                     bufstate;
>>       u16                     syscstate;
>>       u16                     westate;
>>       u16                     errata;
>> @@ -285,9 +284,31 @@ static inline u16 omap_i2c_read_reg(struct omap_i2c_dev *i2c_dev, int reg)
>>       }
>>  }
>>
>> +static void __omap_i2c_init(struct omap_i2c_dev *dev)
>> +{
>> +
>> +     omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
>> +     /* Setup clock prescaler to obtain approx 12MHz I2C module clock: */
>> +     omap_i2c_write_reg(dev, OMAP_I2C_PSC_REG, dev->pscstate);
>> +
>> +     /* SCL low and high time values */
>> +     omap_i2c_write_reg(dev, OMAP_I2C_SCLL_REG, dev->scllstate);
>> +     omap_i2c_write_reg(dev, OMAP_I2C_SCLH_REG, dev->sclhstate);
>> +     if (dev->rev >= OMAP_I2C_REV_ON_3430_3530)
>> +             omap_i2c_write_reg(dev, OMAP_I2C_WE_REG, dev->westate);
>> +     /* Take the I2C module out of reset: */
>> +     omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
>
> a few blank lines in this function wouldn't hurt and they would help
> with readability.

Will add .

>
>> +     /*
>> +      * 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_init(struct omap_i2c_dev *dev)
>>  {
>> -     u16 psc = 0, scll = 0, sclh = 0, buf = 0;
>> +     u16 psc = 0, scll = 0, sclh = 0;
>>       u16 fsscll = 0, fssclh = 0, hsscll = 0, hssclh = 0;
>>       unsigned long fclk_rate = 12000000;
>>       unsigned long timeout;
>> @@ -337,11 +358,8 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
>>                        * REVISIT: Some wkup sources might not be needed.
>>                        */
>>                       dev->westate = OMAP_I2C_WE_ALL;
>> -                     omap_i2c_write_reg(dev, OMAP_I2C_WE_REG,
>> -                                                     dev->westate);
>
> remove the comment too since now that's done by some other function ?
>
>>               }
>>       }
>> -     omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
>>
>>       if (dev->flags & OMAP_I2C_FLAG_ALWAYS_ARMXOR_CLK) {
>>               /*
>> @@ -426,28 +444,18 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
>>               sclh = fclk_rate / (dev->speed * 2) - 7 + psc;
>>       }
>>
>> -     /* Setup clock prescaler to obtain approx 12MHz I2C module clock: */
>> -     omap_i2c_write_reg(dev, OMAP_I2C_PSC_REG, psc);
>> -
>> -     /* SCL low and high time values */
>> -     omap_i2c_write_reg(dev, OMAP_I2C_SCLL_REG, scll);
>> -     omap_i2c_write_reg(dev, OMAP_I2C_SCLH_REG, sclh);
>> -
>> -     /* Take the I2C module out of reset: */
>> -     omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
>> -
>>       /* Enable interrupts */
>>       dev->iestate = (OMAP_I2C_IE_XRDY | OMAP_I2C_IE_RRDY |
>>                       OMAP_I2C_IE_NACK | OMAP_I2C_IE_AL)  |
>>                       ((dev->fifo_size) ? (OMAP_I2C_IE_RDR |
>>                               OMAP_I2C_IE_XDR) : 0);
>> -     omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev->iestate);
>> -     if (dev->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) {
>> -             dev->pscstate = psc;
>> -             dev->scllstate = scll;
>> -             dev->sclhstate = sclh;
>> -             dev->bufstate = buf;
>> -     }
>> +
>> +     dev->pscstate = psc;
>> +     dev->scllstate = scll;
>> +     dev->sclhstate = sclh;
>> +
>> +     __omap_i2c_init(dev);
>> +
>>       return 0;
>>  }
>>
>> @@ -1268,23 +1276,8 @@ static int omap_i2c_runtime_resume(struct device *dev)
>>  {
>>       struct omap_i2c_dev *_dev = dev_get_drvdata(dev);
>>
>> -     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);
>> -     }
>> -
>> -     /*
>> -      * 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_init(_dev);
>>
>>       return 0;
>>  }
>
> you continue to miss the changes in omap_i2c_xfer_msg() and your
> explanation of why not doing it wasn't good enough IMHO.

Will do that . I am preparing a seperate patch for moving the
calculation to a seperate function.


>
> --
> balbi