Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210202021747.717-1-r-rivera-matos@ti.com> <20210202021747.717-3-r-rivera-matos@ti.com>
In-Reply-To: <20210202021747.717-3-r-rivera-matos@ti.com>
From:   Krzysztof Kozlowski <krzk@kernel.org>
Date:   Wed, 10 Feb 2021 09:35:40 +0100
Message-ID: <CAJKOXPdmJEVNNj4+d+GV4zchw=87ZKMiEpA6naADTMMMz-3j=w@mail.gmail.com>
Subject: Re: [PATCH v5 2/2] power: supply: bq25790: Introduce the BQ25790
 charger driver
To:     Ricardo Rivera-Matos <r-rivera-matos@ti.com>
Cc:     sre@kernel.org, linux-pm@vger.kernel.org,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        dmurphy@ti.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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