lkml.org 
[lkml]   [2018]   [Dec]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 2/2] mfd: tps65218.c: Add input voltage options
From
Date


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@wago.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));
>

\
 
 \ /
  Last update: 2018-12-24 10:39    [W:0.095 / U:5.292 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site