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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 8 Mar 2011 14:26:14 +0100
From:	Linus Walleij <linus.walleij@...aro.org>
To:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
Cc:	Linus Walleij <linus.walleij@...ricsson.com>,
	Liam Girdwood <lrg@...mlogic.co.uk>,
	linux-kernel@...r.kernel.org, Lee Jones <lee.jones@...aro.org>,
	Samuel Ortiz <samuel.ortiz@...el.com>,
	Ola Lilja <ola.o.lilja@...ricsson.com>
Subject: Re: [PATCH 2/3] regulator: add a subdriver for TI TPS6105x regulator portions

On Tue, Mar 8, 2011 at 12:56 PM, Mark Brown
<broonie@...nsource.wolfsonmicro.com> 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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/