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

[J, 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@xxxxxxxx>
> > ---
> >   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));