Re: [PATCH 2/3] regulator: add a subdriver for TI TPS6105x regulator portions

[Linus Walleij]

On Tue, Mar 8, 2011 at 12:56 PM, Mark Brown
<broonie@xxxxxxxxxxxxxxxxxxxxxxxxxxx> wrote:

> On Tue, Mar 08, 2011 at 10:42:00AM +0100, Linus Walleij wrote:
>
>> +static int tps6105x_regulator_get_voltage(struct regulator_dev *rdev)
>> +{
>
> It'd probably simplify things to use the _sel variants for the voltage
> operations.

Good idea. Fixed it.

>> +static int __devinit tps6105x_regulator_probe(struct platform_device *pdev)
>> +{
>> +     struct tps6105x *tps6105x = platform_get_drvdata(pdev);
>> +     struct tps6105x_platform_data *pdata = tps6105x->pdata;
>> +     int ret;
>> +
>> +     /* This instance is not set for regulator mode so bail out */
>> +     if (pdata->mode != TPS6105X_MODE_VOLTAGE)
>> +             return 0;
>
> -ENODEV?  Logging to explain why we're bombing out might be helpful too.

Not in this case actually, the chip is configured by platform data to
operate as regulator, LED or flashgun. All three drivers may be
compiled in at some point, and a system may have three TPS chips
performing each function. They will spawn three children each,
but only one of them should probe.

So the fact that the regulator driver doesn't probe doesn't mean any
failure, some other driver will likely probe successfully for the chip.

>> +     /* Set lowest voltage 4.5V to begin */
>> +     ret = tps6105x_mask_and_set(tps6105x, TPS6105X_REG_0,
>> +             TPS6105X_REG0_VOLTAGE_MASK,
>> +             TPS6105X_REG0_VOLTAGE_450 << TPS6105X_REG0_VOLTAGE_SHIFT);
>> +     if (ret)
>> +             return ret;
>
> Don't do this, if the user wants to set a defualt voltage they can use
> constraints but if the driver does it then it may disrupt a running
> system.

OK dropped it.

>> +#if 0
>> +     /* Activate voltage mode - use to check if voltage comes out */
>> +     ret = tps6105x_mask_and_set(tps6105x, TPS6105X_REG_0,
>> +             TPS6105X_REG0_MODE_MASK,
>> +             TPS6105X_REG0_MODE_VOLTAGE << TPS6105X_REG0_MODE_SHIFT);
>> +     if (ret)
>> +             return ret;
>> +#endif
>
> Don't include if 0 code.

Dropped it.

>> +subsys_initcall(tps6105x_regulator_init);
>> +module_exit(tps6105x_regulator_exit);
>
> These usually go next to the functions.

Reordered.

Thanks Mark,
Linus Walleij
--
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/