linux-kernel - Re: [PATCH 2/2] mfd: tps65218.c: Add input voltage options

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 for Android: free password hash cracker in your pocket

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

Date:   Mon, 24 Dec 2018 15:08:57 +0530
From:   "J, KEERTHY" <j-keerthy@...com>
To:     Lee Jones <lee.jones@...aro.org>,
        Christian Hohnstaedt <Christian.Hohnstaedt@...o.com>
CC:     Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Rob Herring <robh+dt@...nel.org>,
        Tony Lindgren <tony@...mide.com>,
        <linux-kernel@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-omap@...r.kernel.org>
Subject: Re: [PATCH 2/2] mfd: tps65218.c: Add input voltage options



On 12/21/2018 4:31 PM, Lee Jones wrote:
> On Tue, 18 Dec 2018, Christian Hohnstaedt wrote:
> 
>> These options apply to all regulators in this chip.
>>
>> strict-supply-voltage:
>>    Set STRICT flag in CONFIG1
>> under-voltage-limit:
>>    Select 2.75, 2.95, 3.25 or 3.35 V UVLO in CONFIG1
>> under-voltage-hysteresis:
>>    Select 200mV or 400mV UVLOHYS in CONFIG2
>>
>> Signed-off-by: Christian Hohnstaedt <Christian.Hohnstaedt@...o.com>
>> ---
>>   drivers/mfd/tps65218.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 46 insertions(+)
> 
> This needs a close review by Tony and any of the other OMAP guys.
> 
> At the very least, please put '\n's between the if() statements.  You
> also need to return after an error print, else I suggest it's not an
> error.
> 
> It would also look tidier if you changed the if()s to one liners to
> assign to different variables, then dealt with them separately later
> on.  The way it's done here looks messy to say the least.
> 
>> diff --git a/drivers/mfd/tps65218.c b/drivers/mfd/tps65218.c
>> index 8bcdecf..f5e559b 100644
>> --- a/drivers/mfd/tps65218.c
>> +++ b/drivers/mfd/tps65218.c
>> @@ -211,6 +211,50 @@ static const struct of_device_id of_tps65218_match_table[] = {
>>   };
>>   MODULE_DEVICE_TABLE(of, of_tps65218_match_table);
>>   
>> +static void tps65218_options(struct tps65218 *tps)
>> +{
>> +	struct device *dev = tps->dev;
>> +	struct device_node *np = dev->of_node;
>> +	u32 pval;
> 
> What does pval mean?  I suggest just val is more common.
> 
>> +	if (!of_property_read_u32(np, "strict-supply-voltage", &pval)) {
>> +		tps65218_update_bits(tps, TPS65218_REG_CONFIG1,
>> +			TPS65218_CONFIG1_STRICT,
>> +			pval ? TPS65218_CONFIG1_STRICT : 0,
>> +			TPS65218_PROTECT_L1);
>> +		dev_dbg(dev, "tps65218 strict-supply-voltage: %d\n", pval);
>> +	}
>> +	if (!of_property_read_u32(np, "under-voltage-hysteresis", &pval)) {
>> +		if (pval != 400000 && pval != 200000) {
>> +			dev_err(dev,
>> +				 "under-voltage-hysteresis must be %d or %d\n",
>> +				 200000, 400000);
>> +		} else {
>> +			tps65218_update_bits(tps, TPS65218_REG_CONFIG2,
>> +				TPS65218_CONFIG2_UVLOHYS,
>> +				pval == 400000 ? TPS65218_CONFIG2_UVLOHYS : 0,
>> +				TPS65218_PROTECT_L1);
>> +		}
>> +		dev_dbg(dev, "tps65218 under-voltage-hysteresis: %d\n", pval);
>> +	}
>> +	if (!of_property_read_u32(np, "under-voltage-limit", &pval)) {
>> +		int i, vals[] = { 275, 295, 325, 335 };
>> +
>> +		for (i = 0; i < ARRAY_SIZE(vals); i++) {
>> +			if (pval == vals[i] * 10000)
> 
> Just use the full value in 'vals'.
> 
>> +				break;
>> +		}
> 
> It took me a few seconds to realise what you're doing here.
> 
> I think a switch() statement would be cleaner.
> 
> You should also #define the values.
> 
> TPS65218_UNDER_VOLT_LIM_2750000 0
> 
> Etc.
> 
>> +		if (i < ARRAY_SIZE(vals)) {
>> +			tps65218_update_bits(tps, TPS65218_REG_CONFIG1,
>> +				TPS65218_CONFIG1_UVLO_MASK, i,
>> +				TPS65218_PROTECT_L1);
>> +		} else {
>> +			dev_err(dev, "Invalid under-voltage-limit: %d\n", pval);
> 
> This could go in the default: section.
> 
>> +		}
>> +		dev_dbg(dev, "tps65218 under-voltage-limit: %d=%d\n", pval, i);
> 
> I suggest considering removing these.
> 
>> +	}
>> +}
>> +
>>   static int tps65218_probe(struct i2c_client *client,
>>   				const struct i2c_device_id *ids)
>>   {
>> @@ -249,6 +293,8 @@ static int tps65218_probe(struct i2c_client *client,
>>   
>>   	tps->rev = chipid & TPS65218_CHIPID_REV_MASK;
>>   
>> +	tps65218_options(tps);
> 
> Options is not good nomenclature as it doesn't really tell us
> anything.  Looks like all the values are voltage related to me?

Can we simply call them tps65218_voltage_set_strict, 
tps65218_voltage_set_uvlo, tps65218_voltage_set_uv_hyst?

or if you want them under one function then i would suggest 
tps65218_set_voltage_quirks or something like that.

- Keerthy
> 
>>   	ret = mfd_add_devices(tps->dev, PLATFORM_DEVID_AUTO, tps65218_cells,
>>   			      ARRAY_SIZE(tps65218_cells), NULL, 0,
>>   			      regmap_irq_get_domain(tps->irq_data));
>