Re: [PATCH 3/3] regulator: tps65090: Make FETs more reliable

[Doug Anderson]

Mark,

On Tue, Apr 15, 2014 at 3:52 PM, Mark Brown <broonie@xxxxxxxxxx> wrote:
> On Tue, Apr 15, 2014 at 01:14:36PM -0700, Doug Anderson wrote:
>
>> Mitigate the problem by:
>> * Allow setting the overcurrent wait time so devices with this problem
>>   can set it to the max.
>> * Add retry logic on enables of FETs.
>
> This is two changes, should really be two patches.

OK, sure.


>> +- ti,overcurrent-wait: This is applicable to FET registers, which have a
>> +  poorly defined "overcurrent wait" field.  If this property is present it
>> +  should be between 0 - 3.  If this property isn't present we won't touch the
>> +  "overcurrent wait" field and we'll leave it to the BIOS/EC to deal with.
>
> I take it this is the raw value to write to the register?

Yes.


>> +static int tps65090_fet_is_enabled(struct regulator_dev *rdev)
>> +{
>> +     unsigned int control;
>> +     unsigned int expected = rdev->desc->enable_mask | BIT(CTRL_PG_BIT);
>> +     int ret;
>> +
>> +     ret = regmap_read(rdev->regmap, rdev->desc->enable_reg, &control);
>> +     if (ret != 0)
>> +             return ret;
>> +
>> +     return (control & expected) == expected;
>> +}
>
> No need to open code this, regulator_is_enabled_regmap() can check for
> any value in a bitfield.

The overall problem was that we were checking a different bit than we
were setting.  We set "enabled" to turn things on and then we check
"power good".

I can avoid the open coding by doing something that's slightly a hack.
 I'll put that in V2 and you can tell me if you like it better.  I'll
set "enable_mask" and "enable_val" to include both bits.  The "power
good" is read only so it won't hurt to set it.  ...and it doesn't hurt
to check the enabled bit in addition to the power good bit.


>> +static int tps6090_try_enable_fet(struct regulator_dev *rdev)
>
> Why is this called tps6090_try_enable_fet(), looks like a missing 5?

typo.  fixed.

>
>> +     /*
>> +      * Try enabling multiple times until we succeed since sometimes the
>> +      * first try times out.
>> +      */
>> +     for (tries = 0; ; tries++) {
>> +             ret = tps6090_try_enable_fet(rdev);
>> +             if (!ret)
>> +                     break;
>> +             if (ret != -ENOTRECOVERABLE || tries == MAX_FET_ENABLE_TRIES)
>> +                     goto err;
>
> Make this a do { } while so we don't have the exit condition missing in
> the for loop please, it's doing the right thing but it's not as obvious
> as it could be.

It's not quite a "do { } while" since the break is in the middle, but
happy to change to a "while (true)".  Hope that's OK.

>
>> +     if (tries) {
>> +             dev_warn(&rdev->dev, "reg %#x enable ok after %d tries\n",
>> +                      rdev->desc->enable_reg, tries);
>> +     }
>
> No need for braces here, and I guess that ought to be retries rather
> than tries (though that is pedantry).

LOL.  I've been yelled at for the opposite.  ;)  There's at least
someone out there who thinks that we should have braces if you've got
a single statement like this that wraps to two lines, but I can't
remember who.

In any case, fixed.

-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/