| Message ID | 20210202021747.717-1-r-rivera-matos@ti.com |
|---|---|
| Headers | show |
| Series | Introduce the BQ25790 charger driver | expand |
On Tue, 2 Feb 2021 at 03:20, Ricardo Rivera-Matos <r-rivera-matos@ti.com> wrote: > > From: Dan Murphy <dmurphy@ti.com> > > BQ25790 is a highly integrated switch-mode buck-boost charger > for 1-4 cell Li-ion battery and Li-polymer battery. > > Signed-off-by: Ricardo Rivera-Matos <r-rivera-matos@ti.com> > Signed-off-by: Dan Murphy <dmurphy@ti.com> Looks like wrong order of Sobs. Since Dan was the author, did you really contribute here before him? (...) > + > +static bool bq25790_state_changed(struct bq25790_device *bq, > + struct bq25790_state *new_state) > +{ > + struct bq25790_state old_state; > + > + mutex_lock(&bq->lock); > + old_state = bq->state; > + mutex_unlock(&bq->lock); > + > + return memcmp(&old_state, new_state, > + sizeof(struct bq25790_state)) != 0; > +} > + > +static irqreturn_t bq25790_irq_handler_thread(int irq, void *private) > +{ > + struct bq25790_device *bq = private; > + struct bq25790_state state; > + int ret; > + > + ret = bq25790_get_state(bq, &state); > + if (ret < 0) > + goto irq_out; > + > + if (!bq25790_state_changed(bq, &state)) You will be waking up user-space on every voltage or current change. It was expressed on the lists that this is not desired and instead you should notify only on change of important attributes (e.g. SoC, charge status, cable status). > + goto irq_out; > + > + mutex_lock(&bq->lock); > + bq->state = state; > + mutex_unlock(&bq->lock); > + > + power_supply_changed(bq->charger); > + > +irq_out: > + return IRQ_HANDLED; > +} > + (...) > + > +static int bq25790_parse_dt(struct bq25790_device *bq, > + struct power_supply_config *psy_cfg, struct device *dev) > +{ > + int ret = 0; > + > + psy_cfg->drv_data = bq; > + psy_cfg->of_node = dev->of_node; You parse here DT, so don't initialize power supply config in the same time. It's mixing two things in the same function. > + > + ret = device_property_read_u32(bq->dev, "ti,watchdog-timeout-ms", > + &bq->watchdog_timer); > + if (ret) > + bq->watchdog_timer = BQ25790_WATCHDOG_DIS; > + > + if (bq->watchdog_timer > BQ25790_WATCHDOG_MAX || > + bq->watchdog_timer < BQ25790_WATCHDOG_DIS) > + return -EINVAL; > + > + ret = device_property_read_u32(bq->dev, > + "input-voltage-limit-microvolt", > + &bq->init_data.vlim); > + if (ret) > + bq->init_data.vlim = BQ25790_VINDPM_DEF_uV; > + > + if (bq->init_data.vlim > BQ25790_VINDPM_V_MAX_uV || > + bq->init_data.vlim < BQ25790_VINDPM_V_MIN_uV) > + return -EINVAL; > + > + ret = device_property_read_u32(bq->dev, > + "input-current-limit-microamp", > + &bq->init_data.ilim); > + if (ret) > + bq->init_data.ilim = BQ25790_IINDPM_DEF_uA; > + > + if (bq->init_data.ilim > BQ25790_IINDPM_I_MAX_uA || > + bq->init_data.ilim < BQ25790_IINDPM_I_MIN_uA) > + return -EINVAL; > + > + return 0; > +} > + > +static int bq25790_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + struct device *dev = &client->dev; > + struct bq25790_device *bq; > + struct power_supply_config psy_cfg = { }; > + > + int ret; > + > + bq = devm_kzalloc(dev, sizeof(*bq), GFP_KERNEL); > + if (!bq) > + return -ENOMEM; > + > + bq->client = client; > + bq->dev = dev; > + > + mutex_init(&bq->lock); > + > + strncpy(bq->model_name, id->name, I2C_NAME_SIZE); > + > + bq->regmap = devm_regmap_init_i2c(client, &bq25790_regmap_config); > + Don't add blank line after every statement. All four blank lines above should be removed. > + if (IS_ERR(bq->regmap)) { > + dev_err(dev, "Failed to allocate register map\n"); > + return PTR_ERR(bq->regmap); > + } > + > + i2c_set_clientdata(client, bq); > + > + ret = bq25790_parse_dt(bq, &psy_cfg, dev); > + if (ret) { > + dev_err(dev, "Failed to read device tree properties%d\n", ret); > + return ret; > + } > + > + ret = devm_add_action_or_reset(dev, bq25790_charger_reset, bq); > + if (ret) > + return ret; > + > + /* OTG reporting */ > + bq->usb2_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2); > + if (!IS_ERR_OR_NULL(bq->usb2_phy)) { > + INIT_WORK(&bq->usb_work, bq25790_usb_work); > + bq->usb_nb.notifier_call = bq25790_usb_notifier; > + usb_register_notifier(bq->usb2_phy, &bq->usb_nb); Where is the error checking? Where is cleanup in remove()? > + } > + > + bq->usb3_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB3); > + if (!IS_ERR_OR_NULL(bq->usb3_phy)) { > + INIT_WORK(&bq->usb_work, bq25790_usb_work); > + bq->usb_nb.notifier_call = bq25790_usb_notifier; > + usb_register_notifier(bq->usb3_phy, &bq->usb_nb); The same. > + } > + > + if (client->irq) { > + ret = devm_request_threaded_irq(dev, client->irq, NULL, > + bq25790_irq_handler_thread, > + IRQF_TRIGGER_FALLING | > + IRQF_ONESHOT, > + dev_name(&client->dev), bq); > + if (ret < 0) { > + dev_err(dev, "get irq fail: %d\n", ret); > + return ret; > + } > + } > + > + ret = bq25790_power_supply_init(bq, &psy_cfg, dev); > + if (ret) { > + dev_err(dev, "Failed to register power supply\n"); > + return ret; > + } > + > + ret = bq25790_hw_init(bq); > + if (ret) { > + dev_err(dev, "Cannot initialize the chip.\n"); > + return ret; > + } > + > + return ret; > +} > + > +static const struct i2c_device_id bq25790_i2c_ids[] = { > + { "bq25790", BQ25790 }, > + { "bq25792", BQ25792 }, > + {}, > +}; > +MODULE_DEVICE_TABLE(i2c, bq25790_i2c_ids); > + > +static const struct of_device_id bq25790_of_match[] = { > + { .compatible = "ti,bq25790", }, > + { .compatible = "ti,bq25792", }, > + { }, > +}; > +MODULE_DEVICE_TABLE(of, bq25790_of_match); > + > +static const struct acpi_device_id bq25790_acpi_match[] = { > + { "bq25790", BQ25790 }, > + { "bq25792", BQ25792 }, > + {}, > +}; > +MODULE_DEVICE_TABLE(acpi, bq25790_acpi_match); > + > +static struct i2c_driver bq25790_driver = { > + .driver = { > + .name = "bq25790-charger", > + .of_match_table = bq25790_of_match, > + .acpi_match_table = bq25790_acpi_match, > + }, > + .probe = bq25790_probe, probe_new Best regards, Krzysztof
Hi, On Mon, Feb 01, 2021 at 08:17:45PM -0600, Ricardo Rivera-Matos wrote: > This patchset introduces the BQ25790 integrated buck-boost charging IC. Please add changelogs compared to previous version to your cover letters. > Dan Murphy (2): > dt-bindings: power: Add the bq25790 dt bindings > power: supply: bq25790: Introduce the BQ25790 charger driver > > .../bindings/power/supply/bq25790.yaml | 95 ++ > drivers/power/supply/Kconfig | 8 + > drivers/power/supply/Makefile | 1 + > drivers/power/supply/bq25790_charger.c | 1100 +++++++++++++++++ > drivers/power/supply/bq25790_charger.h | 148 +++ > 5 files changed, 1352 insertions(+) > create mode 100644 Documentation/devicetree/bindings/power/supply/bq25790.yaml > create mode 100644 drivers/power/supply/bq25790_charger.c > create mode 100644 drivers/power/supply/bq25790_charger.h FYI: I don't have further review notes and wait for a new version with the things pointed out by Krzysztof being fixed. For cleanup of usb_register_notifier you might want to check how you fixed the same issue for bq256xx via bq256xx_charger_reset() or my ignored comment in v4 patchset. Thanks, -- Sebastian