Hello,
On Thu, Jan 24, 2013 at 04:47:53PM +0530, Viresh Kumar wrote:
> On 24 January 2013 16:36, Uwe Kleine-König
> <u.kleine-koenig@xxxxxxxxxxxxxx> wrote:
> > On Thu, Jan 24, 2013 at 04:06:37PM +0530, Viresh Kumar wrote:
> >> Most number of versions for any patchset i submitted :)
> > So let's see if you do a v10 ... :-)
>
> Haha..
>
> >> +static int i2c_generic_recovery(struct i2c_adapter *adap)
> >> +{
> >> + struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
> >> + int i = 0, val = 1, ret = 0;
> >> +
> >> + if (bri->prepare_recovery)
> >> + bri->prepare_recovery(bri);
> >> +
> > Do we want to break out here if sda is high?
>
> Good point actually. It may happen we are asked to recover already working
> system :)
>
> >> + /*
> >> + * By this time SCL is high, as we need to give 9 falling-rising edges
> >> + */
> >> + while (i++ < RECOVERY_CLK_CNT * 2) {
> >> + /* SCL shouldn't be low here */
> >> + if (val && !bri->get_scl(adap)) {
> >> + dev_err(&adap->dev, "SCL is stuck Low exit recovery\n");
> >> + ret = -EBUSY;
> >> + goto unprepare;
> >> + }
> >> +
> >> + val = !val;
> >> + bri->set_scl(adap, val);
> >> + ndelay(clk_delay);
> >> +
> >> + /* break if sda got high, check only when scl line is high */
> > Above you wrote "SCL", here "scl". I suggest to use one of them
> > consistently and use the same capitalisation for sda.
>
> Sure.
>
> >> + if (bri->get_sda && val)
> >> + if (bri->get_sda(adap))
> >> + break;
> > Maybe better:
> > /* break if sda got high */
> > if (bri->get_sda && bri->get_sda(adap)) {
> > /* don't leave with scl low */
> > if (!val)
> > bri->set_scl(adap, 1);
> > break;
> > }
>
> So, the whole loop is for 9 pulses at max and it runs 18 times. Twice per
> clock. With my code, i only check for sda after supplying full clock pulse,
> and you are asking me to check that in middle of clock. Isn't it wrong?
Actually in the data phase of a transfer sda only changes when scl is
low. (Otherwise it's a stop condition.) So it should make sense to check
after pulling scl low, doesn't it? Hmm, it trades one ndelay for
(possibly several) get_sda. hmm, *shrug*.
> >> +int i2c_generic_scl_recovery(struct i2c_adapter *adap)
> >> +{
> >> + adap->bus_recovery_info->set_scl(adap, 1);
> > Why this?
>
> This came out of some earlier discussion we had. We should start
> with high and then do low-high 9 times.
Ah, I missed the first val = !val and so thought you start with setting
to 1 anyhow. So yes, I agree. Maybe document this assumption in a
comment?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
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
