Re: [PATCH 12/12] i2c: pxa: enable/disable i2c module across msg xfer

[Yi Zhang <yizhang-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>]

On Thu, Jun 04, 2015 at 12:49:48PM +0530, Vaibhav Hiremath wrote:
> 
> 
> On Thursday 04 June 2015 11:59 AM, Yi Zhang wrote:
> >On Wed, Jun 03, 2015 at 12:59:23AM +0800, Vaibhav Hiremath wrote:
> >>
> >>
> >>On Tuesday 02 June 2015 10:22 PM, Vaibhav Hiremath wrote:
> >>>
> >>>
> >>>On Thursday 28 May 2015 06:53 PM, Russell King - ARM Linux wrote:
> >>>>On Thu, May 28, 2015 at 06:33:44PM +0530, Vaibhav Hiremath wrote:
> >>>>>From: Yi Zhang <yizhang-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> >>>>>
> >>>>>Enable i2c module/unit before transmission and disable when it finishes.
> >>>>>
> >>>>>why?
> >>>>>It's because the i2c bus may be distrubed if the slave device,
> >>>>>typically a touch, powers on.
> >>>>
> >>>>"disturbed"
> >>>>
> >>>>I'd recommend that this is a DT property - not every platform is going to
> >>>>want this, and as there is rudimentary I2C slave support in this driver,
> >>>>this change breaks that.
> >>>>
> >>>
> >>>I would take it as two different comments here, and I believe you also
> >>>meant same,
> >>>
> >>>1. Not breaking I2C slave support
> >>>
> >>>Not sure whether enabling/disabling module in ISR would suffice here.
> >>>To be specific, in the functions i2c_pxa_slave_start() &
> >>>i2c_pxa_slave_stop()
> >>>
> >>>
> >>
> >>Please ignore this option, as enable is important to generate interrupt
> >>on slave start.
> >
> >   Hi, Vaibhav:
> >
> >   sorry there is something wrong with my mailbox, so I list the LCR/WCR
> >   information here, hoping not to confuse you;genr
> >
> 
> Thanks for the info.
> 
> 
> >   LCR: it's used to minor adjustments to the SCL, increasing it
> >   decreases the SCL frequency, vice versa;
> >   yes, it has relation with the I2C input clock:
> >   if input_clk = 32MHz, LCR = 0x0804_1c96
> >      LCR[SLV] = 0x096, bit 8~0
> >      LCR[FLV] = 0xe,   bit 17~9
> >      LCR[HLVH] = 0x1,  bit 26~18
> >      LCR[HLVL] = 0x1,  bit 31~27
> >   if input_clk = 52MHz,   LCR = 0x081c_60f9
> >   if input_clk = 62.4MHz, LCR = 0x082c_8330
> >
> 
> Whether they are simply counters which count downs to generate required
> timing profile on the bus.
> 
> So would be possible for you to provide timing values generated by this?
> Or please confirm below understanding,
> 
> 
> @32MHz, SLV = 0x96,
> thigh/tlow = 31nsec * 150 = 4.650 usec
> thigh/tlow = 32nsec * 150 = 4.8 usec	[using ceil()]
> 
> 
> and as per I2C spec,
> 
> thigh,min = 4 usec
> tlow,min = 4.7usec
> 
> I believe my understanding is correct, please confirm.

  Yes, you are right, as the name shows it, SLV covers standard mode,
  FLV is for fastmode, HLVH is for high phase of high speed mode, while
  HLVL is for low phase of low speed mode;
> 
> 
> >   WCR: it's used to control setup and hold timing during slave mode
> 
> This is is useful info. So in master mode we can ignore this.
> 
> Just to clarify, this bit is don't care in master-transmit and
> master-receive, right?

  Yes, you are right;
> 
> >   WCR[31: 15], reserved, always write 0;
> >   WCR[14: 10], default 0x5, and should not be changed
> >   WCR[9: 5], default 0x1, and should not be changed
> >   WCR[4:0], default 0x1a, if input_clk = 33MHz, it's 0x0a;
> >                           if input_clk = 66MHz, it's 0x14;
> >
> >   hope it's helpful;
> 
> 
> Yes certainly. Thanks.
> 
> Thanks,
> Vaibhav
> >
> >   ---
> >   Yi Zhang <yizhang-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> >
> >>
> >>I will add DT property with clear note on SLAVE support issue, so that
> >>it can be disabled and SLAVE support should work as is.
> >>
> >>Thanks,
> >>Vaibhav

---
Yi Zhang <yizhang-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>