| Message ID | 1545120356-7749-3-git-send-email-Christian.Hohnstaedt@wago.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add input voltage configuration for TPS65218 | expand |
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? > ret = mfd_add_devices(tps->dev, PLATFORM_DEVID_AUTO, tps65218_cells, > ARRAY_SIZE(tps65218_cells), NULL, 0, > regmap_irq_get_domain(tps->irq_data));
* Lee Jones <lee.jones@linaro.org> [181221 11:01]: > 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. Adding Keerthy to Cc, he's probably the best person to review these changes. Regards, Tony
On Sun, 23 Dec 2018, Tony Lindgren wrote: > * Lee Jones <lee.jones@linaro.org> [181221 11:01]: > > 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. > > Adding Keerthy to Cc, he's probably the best person to review these > changes. But you've dropped the patch, so all he has it a commit log to review?
On 12/24/2018 12:02 PM, Lee Jones wrote: > On Sun, 23 Dec 2018, Tony Lindgren wrote: > >> * Lee Jones <lee.jones@linaro.org> [181221 11:01]: >>> 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. >> >> Adding Keerthy to Cc, he's probably the best person to review these >> changes. Thanks Tony. > > But you've dropped the patch, so all he has it a commit log to review? > Lee Jones, I am subscribed to linux-omap so i have the patch. I will review them. Thanks, Keerthy
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)); >
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; + + 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) + break; + } + 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); + } + dev_dbg(dev, "tps65218 under-voltage-limit: %d=%d\n", pval, i); + } +} + 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); + ret = mfd_add_devices(tps->dev, PLATFORM_DEVID_AUTO, tps65218_cells, ARRAY_SIZE(tps65218_cells), NULL, 0, regmap_irq_get_domain(tps->irq_data));
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(+)