Messages in this thread | | | Date | Tue, 7 Mar 2017 13:34:12 +0100 | From | Mark Brown <> | Subject | Re: [PATCH 1/2] regulator: tps65132: add regulator driver for TI TPS65132 |
| |
On Mon, Mar 06, 2017 at 10:06:58PM +0530, Venkat Reddy Talla wrote:
> + dis_mask = (id == TPS65132_REGULATOR_ID_VPOS) ? > + TPS65132_REG_APPS_DISP : TPS65132_REG_APPS_DISN;
Please don't abuse the ternery operator, write normal if statements so that people can read the code easily.
> +static int tps65132_disable(struct regulator_dev *rdev) > +{ > + struct tps65132_regulator *tps = rdev_get_drvdata(rdev); > + int id = rdev_get_id(rdev); > + int act_dis_gpio = tps->reg_pdata[id].active_discharge_gpio; > + unsigned int act_dis_time_us = tps->reg_pdata[id].active_discharge_time; > + > + if (gpio_is_valid(act_dis_gpio)) { > + gpio_set_value_cansleep(act_dis_gpio, 1); > + usleep_range(act_dis_time_us, > + act_dis_time_us + TPS65132_ACT_DIS_TIME_SLACK); > + gpio_set_value_cansleep(act_dis_gpio, 0); > + }
Don't we need to undo the active discharge register write?
> +static int tps65132_get_regulator_dt_data(struct device *dev, > + struct tps65132_regulator *tps65132_regs) > +{ > + struct tps65132_reg_pdata *rpdata; > + struct device_node *rnode; > + int id; > + int ret; > + > + ret = of_regulator_match(dev, dev->of_node, tps65132_regulator_matches, > + ARRAY_SIZE(tps65132_regulator_matches)); > + if (ret < 0) { > + dev_err(dev, "node parsing for regulator failed: %d\n", ret); > + return ret; > + }
Don't open code this, use of_parse_cb() to parse your extra properties.
> +static int __init tps65132_init(void) > +{ > + return i2c_add_driver(&tps65132_i2c_driver); > +} > +subsys_initcall(tps65132_init);
You should use module_i2c_driver() for new drivers. [unhandled content-type:application/pgp-signature] | |