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]
Message-ID: <CAJKOXPfDcTMEd576cVspXKT2kb__49rQSk-BbxXy5ZinO8n5yA@mail.gmail.com>
Date:	Tue, 19 May 2015 17:40:25 +0900
From:	Krzysztof Kozlowski <k.kozlowski@...sung.com>
To:	Laurentiu Palcu <laurentiu.palcu@...el.com>
Cc:	Rob Herring <robh+dt@...nel.org>, Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	Sebastian Reichel <sre@...nel.org>,
	Krzysztof Kozlowski <k.kozlowski@...sung.com>,
	Dmitry Eremin-Solenikov <dbaryshkov@...il.com>,
	David Woodhouse <dwmw2@...radead.org>,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-pm@...r.kernel.org
Subject: Re: [PATCH 2/2] power_supply: Add support for TI BQ25890 charger chip

2015-05-15 23:06 GMT+09:00 Laurentiu Palcu <laurentiu.palcu@...el.com>:
> More details about the chip can be found here:
> http://www.ti.com/product/bq25890
>
> Signed-off-by: Laurentiu Palcu <laurentiu.palcu@...el.com>
> ---
>  drivers/power/Kconfig           |   7 +
>  drivers/power/Makefile          |   1 +
>  drivers/power/bq25890_charger.c | 998 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 1006 insertions(+)
>  create mode 100644 drivers/power/bq25890_charger.c

Hi,

Some comments (nothing serious) inline.

(...)

> +
> +static int bq25890_field_write(struct bq25890_device *bq,
> +                              enum bq25890_fields field_id, u8 val)
> +{
> +       return regmap_field_write(bq->rmap_fields[field_id], val);
> +}
> +
> +static u8 bq25890_find_idx(u32 value, enum bq25890_table_ids id)
> +{
> +       u8 idx;
> +
> +       if (id >= TBL_TREG) {
> +               const u32 *tbl = bq25890_tables[id].lt.tbl;
> +               u32 tbl_size = bq25890_tables[id].lt.size;
> +
> +               for (idx = 1; idx < tbl_size && tbl[idx] <= value; idx++)
> +                       ;
> +       } else {
> +               const struct bq25890_range *rtbl = &bq25890_tables[id].rt;
> +               u8 rtbl_size;
> +
> +               rtbl_size = (rtbl->max - rtbl->min) / rtbl->step + 1;
> +
> +               for (idx = 1;
> +                    idx < rtbl_size && idx * rtbl->step + rtbl->min <= value;

Could you add parentheses around part of this expression? It is non
obvious to find the comparison statements.

> +                    idx++)
> +                       ;
> +       }
> +
> +       return idx - 1;
> +}
> +
> +static u32 bq25890_find_val(u8 idx, enum bq25890_table_ids id)
> +{
> +       const struct bq25890_range *rtbl;
> +
> +       /* lookup table? */
> +       if (id >= TBL_TREG)
> +               return bq25890_tables[id].lt.tbl[idx];
> +
> +       /* range table */
> +       rtbl = &bq25890_tables[id].rt;
> +
> +       return(rtbl->min + idx * rtbl->step);

A nit: space between return and parenthesis would be nice.

> +}
> +
> +enum bq25890_status {
> +       STATUS_NOT_CHARGING,
> +       STATUS_PRE_CHARGING,
> +       STATUS_FAST_CHARGING,
> +       STATUS_TERMINATION_DONE,
> +};
> +
> +enum bq25890_chrg_fault {
> +       CHRG_FAULT_NORMAL,
> +       CHRG_FAULT_INPUT,
> +       CHRG_FAULT_THERMAL_SHUTDOWN,
> +       CHRG_FAULT_TIMER_EXPIRED,
> +};
> +
> +static int bq25890_power_supply_get_property(struct power_supply *psy,
> +                                            enum power_supply_property psp,
> +                                            union power_supply_propval *val)
> +{
> +       int ret;
> +       struct bq25890_device *bq = power_supply_get_drvdata(psy);
> +       struct bq25890_state state;
> +
> +       mutex_lock(&bq->lock);
> +       state = bq->state;
> +       mutex_unlock(&bq->lock);
> +
> +       switch (psp) {
> +       case POWER_SUPPLY_PROP_STATUS:
> +               if (!state.online)
> +                       val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
> +               else if (state.chrg_status == STATUS_NOT_CHARGING)
> +                       val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
> +               else if (state.chrg_status == STATUS_PRE_CHARGING ||
> +                        state.chrg_status == STATUS_FAST_CHARGING)
> +                       val->intval = POWER_SUPPLY_STATUS_CHARGING;
> +               else if (state.chrg_status == STATUS_TERMINATION_DONE)
> +                       val->intval = POWER_SUPPLY_STATUS_FULL;
> +               else
> +                       val->intval = POWER_SUPPLY_STATUS_UNKNOWN;
> +
> +               break;
> +
> +       case POWER_SUPPLY_PROP_MANUFACTURER:
> +               val->strval = BQ25890_MANUFACTURER;
> +               break;
> +
> +       case POWER_SUPPLY_PROP_ONLINE:
> +               val->intval = state.online;
> +               break;
> +
> +       case POWER_SUPPLY_PROP_HEALTH:
> +               if (!state.chrg_fault && !state.bat_fault && !state.boost_fault)
> +                       val->intval = POWER_SUPPLY_HEALTH_GOOD;
> +               else if (state.bat_fault)
> +                       val->intval = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
> +               else if (state.chrg_fault == CHRG_FAULT_TIMER_EXPIRED)
> +                       val->intval = POWER_SUPPLY_HEALTH_SAFETY_TIMER_EXPIRE;
> +               else if (state.chrg_fault == CHRG_FAULT_THERMAL_SHUTDOWN)
> +                       val->intval = POWER_SUPPLY_HEALTH_OVERHEAT;
> +               else
> +                       val->intval = POWER_SUPPLY_HEALTH_UNSPEC_FAILURE;
> +               break;
> +
> +       case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
> +               ret = bq25890_field_read(bq, F_ICHGR); /* read measured value */
> +               if (ret < 0)
> +                       return ret;
> +
> +               /* converted_val = ADC_val * 50mA (table 10.3.19) */
> +               val->intval = ret * 50000;
> +               break;
> +
> +       case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
> +               val->intval = bq25890_tables[TBL_ICHG].rt.max;
> +               break;
> +
> +       case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
> +               if (!state.online) {
> +                       val->intval = 0;
> +                       break;
> +               }
> +
> +               ret = bq25890_field_read(bq, F_BATV); /* read measured value */
> +               if (ret < 0)
> +                       return ret;
> +
> +               /* converted_val = 2.304V + ADC_val * 20mV (table 10.3.15) */
> +               val->intval = 2304000 + ret * 20000;
> +               break;
> +
> +       case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX:
> +               val->intval = bq25890_tables[TBL_VREG].rt.max;
> +               break;
> +
> +       case POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT:
> +               val->intval = bq25890_find_val(bq->init_data.iterm, TBL_ITERM);
> +               break;
> +
> +       default:
> +               return -EINVAL;
> +       }
> +
> +       return 0;
> +}
> +
> +static int bq25890_get_chip_state(struct bq25890_device *bq,
> +                                 struct bq25890_state *state)
> +{
> +       int i, ret;
> +
> +       struct {
> +               enum bq25890_fields id;
> +               u8 *data;
> +       } state_fields[] = {
> +               {F_CHG_STAT,    &state->chrg_status},
> +               {F_PG_STAT,     &state->online},
> +               {F_VSYS_STAT,   &state->vsys_status},
> +               {F_BOOST_FAULT, &state->boost_fault},
> +               {F_BAT_FAULT,   &state->bat_fault},
> +               {F_CHG_FAULT,   &state->chrg_fault}
> +       };
> +
> +       for (i = 0; i < ARRAY_SIZE(state_fields); i++) {
> +               ret = bq25890_field_read(bq, state_fields[i].id);
> +               if (ret < 0)
> +                       return ret;
> +
> +               *state_fields[i].data = ret;
> +       }
> +
> +       dev_dbg(bq->dev, "S:CHG/PG/VSYS=%d/%d/%d, F:CHG/BOOST/BAT=%d/%d/%d\n",
> +               state->chrg_status, state->online, state->vsys_status,
> +               state->chrg_fault, state->boost_fault, state->bat_fault);
> +
> +       return 0;
> +}
> +
> +static bool bq25890_state_changed(struct bq25890_device *bq,
> +                                 struct bq25890_state *new_state)
> +{
> +       struct bq25890_state old_state;
> +
> +       mutex_lock(&bq->lock);
> +       old_state = bq->state;
> +       mutex_unlock(&bq->lock);
> +
> +       return (old_state.chrg_status != new_state->chrg_status ||
> +               old_state.chrg_fault != new_state->chrg_fault   ||
> +               old_state.online != new_state->online           ||
> +               old_state.bat_fault != new_state->bat_fault     ||
> +               old_state.boost_fault != new_state->boost_fault ||
> +               old_state.vsys_status != new_state->vsys_status);
> +}
> +
> +static void bq25890_handle_state_change(struct bq25890_device *bq,
> +                                       struct bq25890_state *new_state)
> +{
> +       int ret;
> +       struct bq25890_state old_state;
> +
> +       mutex_lock(&bq->lock);
> +       old_state = bq->state;
> +       mutex_unlock(&bq->lock);
> +
> +       if (!new_state->online) {                            /* power removed */
> +               /* disable ADC */
> +               ret = bq25890_field_write(bq, F_CONV_START, 0);
> +               if (ret < 0)
> +                       goto error;
> +       } else if (!old_state.online) {                     /* power inserted */
> +               /* enable ADC, to have control of charge current/voltage */
> +               ret = bq25890_field_write(bq, F_CONV_START, 1);
> +               if (ret < 0)
> +                       goto error;
> +       }
> +
> +       return;
> +
> +error:
> +       dev_err(bq->dev, "%s: Error communicating with the chip.\n", __func__);

The __func__ is not needed here, there is only one kind of such message.

> +}
> +
> +static irqreturn_t bq25890_irq_handler_thread(int irq, void *private)
> +{
> +       struct bq25890_device *bq = private;
> +       int ret;
> +       struct bq25890_state state;
> +
> +       ret = bq25890_get_chip_state(bq, &state);
> +       if (ret < 0)
> +               goto handled;
> +
> +       if (!bq25890_state_changed(bq, &state))
> +               goto handled;
> +
> +       bq25890_handle_state_change(bq, &state);
> +
> +       mutex_lock(&bq->lock);
> +       bq->state = state;
> +       mutex_unlock(&bq->lock);
> +
> +       power_supply_changed(bq->charger);
> +
> +handled:
> +       return IRQ_HANDLED;
> +}
> +
> +static int bq25890_chip_reset(struct bq25890_device *bq)
> +{
> +       int ret;
> +
> +       ret = bq25890_field_write(bq, F_REG_RST, 1);
> +       if (ret < 0)
> +               return ret;
> +
> +       do {
> +               ret = bq25890_field_read(bq, F_REG_RST);
> +               if (ret < 0)
> +                       return ret;
> +
> +               usleep_range(5, 10);
> +       } while (ret == 1);

Is it possible to loop here indefinetely?

> +
> +       return 0;
> +}
> +
> +static int bq25890_hw_init(struct bq25890_device *bq)
> +{
> +       int ret;
> +       int i;
> +       struct bq25890_state state;
> +
> +       const struct {
> +               enum bq25890_fields id;
> +               u32 value;
> +       } init_data[] = {
> +               {F_ICHG,         bq->init_data.ichg},
> +               {F_VREG,         bq->init_data.vreg},
> +               {F_ITERM,        bq->init_data.iterm},
> +               {F_IPRECHG,      bq->init_data.iprechg},
> +               {F_SYSVMIN,      bq->init_data.sysvmin},
> +               {F_BOOSTV,       bq->init_data.boostv},
> +               {F_BOOSTI,       bq->init_data.boosti},
> +               {F_BOOSTF,       bq->init_data.boostf},
> +               {F_EN_ILIM,      bq->init_data.ilim_en},
> +               {F_TREG,         bq->init_data.treg}
> +       };
> +
> +       ret = bq25890_chip_reset(bq);
> +       if (ret < 0)
> +               return ret;
> +
> +       /* disable watchdog */
> +       ret = bq25890_field_write(bq, F_WD, 0);
> +       if (ret < 0)
> +               return ret;
> +
> +       /* initialize currents/voltages and other parameters */
> +       for (i = 0; i < ARRAY_SIZE(init_data); i++) {
> +               ret = bq25890_field_write(bq, init_data[i].id,
> +                                         init_data[i].value);
> +               if (ret < 0)
> +                       return ret;
> +       }
> +
> +       /* Configure ADC for continuous conversions. This does not enable it. */
> +       ret = bq25890_field_write(bq, F_CONV_RATE, 1);
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = bq25890_get_chip_state(bq, &state);
> +       if (ret < 0)
> +               return ret;
> +
> +       mutex_lock(&bq->lock);
> +       bq->state = state;
> +       mutex_unlock(&bq->lock);
> +
> +       return 0;
> +}
> +
> +static enum power_supply_property bq25890_power_supply_props[] = {
> +       POWER_SUPPLY_PROP_MANUFACTURER,
> +       POWER_SUPPLY_PROP_STATUS,
> +       POWER_SUPPLY_PROP_ONLINE,
> +       POWER_SUPPLY_PROP_HEALTH,
> +       POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT,
> +       POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX,
> +       POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE,
> +       POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX,
> +       POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT,
> +};
> +
> +static char *bq25890_charger_supplied_to[] = {
> +       "main-battery",
> +};
> +
> +static const struct power_supply_desc bq25890_power_supply_desc = {
> +       .name = "bq25890-charger",
> +       .type = POWER_SUPPLY_TYPE_USB,
> +       .properties = bq25890_power_supply_props,
> +       .num_properties = ARRAY_SIZE(bq25890_power_supply_props),
> +       .get_property = bq25890_power_supply_get_property,
> +};
> +
> +static int bq25890_power_supply_init(struct bq25890_device *bq)
> +{
> +       struct power_supply_config psy_cfg = { .drv_data = bq, };
> +
> +       psy_cfg.supplied_to = bq25890_charger_supplied_to;
> +       psy_cfg.num_supplicants = ARRAY_SIZE(bq25890_charger_supplied_to);
> +
> +       bq->charger = power_supply_register(bq->dev, &bq25890_power_supply_desc,
> +                                           &psy_cfg);
> +       if (IS_ERR(bq->charger))
> +               return PTR_ERR(bq->charger);
> +
> +       return 0;

return PTR_ERR_OR_ZERO

> +}
> +
> +static void bq25890_usb_work(struct work_struct *data)
> +{
> +       int ret;
> +       struct bq25890_device *bq =
> +                       container_of(data, struct bq25890_device, usb_work);
> +
> +       switch (bq->usb_event) {
> +       case USB_EVENT_ID:
> +               /* Enable boost mode */
> +               ret = bq25890_field_write(bq, F_OTG_CFG, 1);
> +               if (ret < 0)
> +                       goto error;
> +               break;
> +
> +       case USB_EVENT_NONE:
> +               /* Disable boost mode */
> +               ret = bq25890_field_write(bq, F_OTG_CFG, 0);
> +               if (ret < 0)
> +                       goto error;
> +
> +               power_supply_changed(bq->charger);
> +               break;
> +       }
> +
> +       return;
> +
> +error:
> +       dev_err(bq->dev, "%s - Error switching to boost/charger mode.\n",
> +               __func__);

Again the __func__. It shouldn't be here.

> +}
> +
> +static int bq25890_usb_notifier(struct notifier_block *nb, unsigned long val,
> +                               void *priv)
> +{
> +       struct bq25890_device *bq =
> +                       container_of(nb, struct bq25890_device, usb_nb);
> +
> +       bq->usb_event = val;
> +       schedule_work(&bq->usb_work);

If this shouldn't be executed on exactly this CPU then usage of
system_power_efficient_wq is encouraged.

> +
> +       return NOTIFY_OK;
> +}
> +
> +static int bq25890_irq_probe(struct bq25890_device *bq)
> +{
> +       int ret;
> +       struct gpio_desc *irq;
> +
> +       irq = devm_gpiod_get_index(bq->dev, BQ25890_IRQ_PIN, 0);
> +       if (IS_ERR(irq)) {
> +               dev_err(bq->dev, "could not probe irq pin\n");
> +               return PTR_ERR(irq);
> +       }
> +
> +       ret = gpiod_direction_input(irq);
> +       if (ret < 0)
> +               return ret;
> +
> +       return gpiod_to_irq(irq);
> +}
> +
> +static int bq25890_fw_read_u32_props(struct bq25890_device *bq)
> +{
> +       int ret;
> +       u32 property;
> +       int i;
> +       struct bq25890_init_data *init = &bq->init_data;
> +       struct {
> +               char *name;
> +               bool optional;
> +               enum bq25890_table_ids tbl_id;
> +               u8 *conv_data; /* holds converted value from given property */
> +       } props[] = {
> +               /* required properties */
> +               {"ti,charge-current", false, TBL_ICHG, &init->ichg},
> +               {"ti,battery-regulation-voltage", false, TBL_VREG, &init->vreg},
> +               {"ti,termination-current", false, TBL_ITERM, &init->iterm},
> +               {"ti,precharge-current", false, TBL_ITERM, &init->iprechg},
> +               {"ti,minimum-sys-voltage", false, TBL_SYSVMIN, &init->sysvmin},
> +               {"ti,boost-voltage", false, TBL_BOOSTV, &init->boostv},
> +               {"ti,boost-max-current", false, TBL_BOOSTI, &init->boosti},
> +
> +               /* optional properties */
> +               {"ti,thermal-regulation-threshold", true, TBL_TREG, &init->treg}
> +       };
> +
> +       /* initialize data for optional properties */
> +       init->treg = 3; /* 120 degrees Celsius */
> +
> +       for (i = 0; i < ARRAY_SIZE(props); i++) {
> +               ret = device_property_read_u32(bq->dev, props[i].name,
> +                                              &property);
> +               if (ret < 0) {
> +                       if (props[i].optional)
> +                               continue;
> +
> +                       return ret;
> +               }
> +
> +               *props[i].conv_data = bq25890_find_idx(property,
> +                                                      props[i].tbl_id);
> +       }
> +
> +       return 0;
> +}
> +
> +static int bq25890_fw_probe(struct bq25890_device *bq)
> +{
> +       int ret;
> +       struct bq25890_init_data *init = &bq->init_data;
> +
> +       ret = bq25890_fw_read_u32_props(bq);
> +       if (ret < 0)
> +               return ret;
> +
> +       init->ilim_en = device_property_read_bool(bq->dev, "ti,use-ilim-pin");
> +       init->boostf = device_property_read_bool(bq->dev, "ti,boost-low-freq");
> +
> +       return 0;
> +}
> +
> +static int bq25890_probe(struct i2c_client *client,
> +                        const struct i2c_device_id *id)
> +{
> +       struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
> +       struct device *dev = &client->dev;
> +       struct bq25890_device *bq;
> +       int ret;
> +       int i;
> +
> +       if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
> +               dev_err(dev, "No support for SMBUS_BYTE_DATA\n");
> +               return -ENODEV;
> +       }
> +
> +       bq = devm_kzalloc(dev, sizeof(*bq), GFP_KERNEL);
> +       if (!bq)
> +               return -ENOMEM;
> +
> +       bq->client = client;
> +       bq->dev = dev;
> +
> +       mutex_init(&bq->lock);
> +
> +       bq->rmap = devm_regmap_init_i2c(client, &bq25890_regmap_config);
> +       if (IS_ERR(bq->rmap)) {
> +               dev_err(dev, "failed to allocate register map\n");
> +               return PTR_ERR(bq->rmap);
> +       }
> +
> +       for (i = 0; i < ARRAY_SIZE(bq25890_reg_fields); i++) {
> +               const struct reg_field *reg_fields = bq25890_reg_fields;
> +
> +               bq->rmap_fields[i] = devm_regmap_field_alloc(dev, bq->rmap,
> +                                                            reg_fields[i]);
> +               if (IS_ERR(bq->rmap_fields[i])) {
> +                       dev_err(dev, "cannot allocate regmap field\n");
> +                       return PTR_ERR(bq->rmap_fields[i]);
> +               }
> +       }
> +
> +       i2c_set_clientdata(client, bq);
> +
> +       bq->chip_id = bq25890_field_read(bq, F_PN);
> +       if (bq->chip_id < 0) {
> +               dev_err(dev, "Cannot read chip ID.\n");
> +               return ret;
> +       }
> +
> +       if (bq->chip_id != BQ25890_ID) {
> +               dev_err(dev, "Chip with ID=%d, not supported!\n", bq->chip_id);
> +               return -ENODEV;
> +       }
> +
> +       if (!dev->platform_data) {
> +               ret = bq25890_fw_probe(bq);
> +               if (ret < 0) {
> +                       dev_err(dev, "Cannot read device properties.\n");
> +                       return ret;
> +               }
> +       } else {
> +               return -ENODEV;
> +       }
> +
> +       ret = bq25890_hw_init(bq);
> +       if (ret < 0) {
> +               dev_err(dev, "Cannot initialize the chip.\n");
> +               return ret;
> +       }
> +
> +       if (client->irq <= 0)
> +               client->irq = bq25890_irq_probe(bq);
> +
> +       if (client->irq < 0) {
> +               dev_err(dev, "no irq resource found\n");

A nit: stick to one convention - capitilize first letter of error message.

> +               return client->irq;
> +       }
> +
> +       /* OTG reporting */
> +       bq->usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
> +       if (!IS_ERR_OR_NULL(bq->usb_phy)) {
> +               INIT_WORK(&bq->usb_work, bq25890_usb_work);
> +               bq->usb_nb.notifier_call = bq25890_usb_notifier;
> +               usb_register_notifier(bq->usb_phy, &bq->usb_nb);
> +       }
> +
> +       ret = devm_request_threaded_irq(dev, client->irq, NULL,
> +                                       bq25890_irq_handler_thread,
> +                                       IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +                                       BQ25890_IRQ_PIN, bq);
> +       if (ret)
> +               goto irq_fail;
> +
> +       ret = bq25890_power_supply_init(bq);
> +       if (ret < 0) {
> +               dev_err(dev, "Failed to register power supply\n");
> +               goto irq_fail;
> +       }
> +
> +       return 0;
> +
> +irq_fail:
> +       if (!IS_ERR_OR_NULL(bq->usb_phy))
> +               usb_unregister_notifier(bq->usb_phy, &bq->usb_nb);
> +
> +       return ret;
> +}
> +
> +static int bq25890_remove(struct i2c_client *client)
> +{
> +       struct bq25890_device *bq = i2c_get_clientdata(client);
> +
> +       if (!IS_ERR_OR_NULL(bq->usb_phy))
> +               usb_unregister_notifier(bq->usb_phy, &bq->usb_nb);
> +
> +       power_supply_unregister(bq->charger);

I would prefer cleaning in reversed order of probe, so first would be
power_supply_unregister(). I think this is expected in usual case.
Often when I encounter such code I wonder - is this non-standard order
of cleanup is important?

> +
> +       /* reset all registers to default values */
> +       bq25890_chip_reset(bq);
> +
> +       return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int bq25890_suspend(struct device *dev)
> +{
> +       struct bq25890_device *bq = dev_get_drvdata(dev);
> +
> +       /*
> +        * If charger is removed, while in suspend, make sure ADC is diabled
> +        * since it consumes slightly more power.
> +        */
> +       return bq25890_field_write(bq, F_CONV_START, 0);
> +}
> +
> +static int bq25890_resume(struct device *dev)
> +{
> +       int ret;
> +       struct bq25890_state state;
> +       struct bq25890_device *bq = dev_get_drvdata(dev);
> +
> +       ret = bq25890_get_chip_state(bq, &state);
> +       if (ret < 0)
> +               return ret;
> +
> +       mutex_lock(&bq->lock);
> +       bq->state = state;
> +       mutex_unlock(&bq->lock);
> +
> +       /* Re-enable ADC only if charger is plugged in. */
> +       if (bq->state.online) {

Shouldn't you be here accessing local variable state?

Best regards,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ