Re: [PATCH v3 11/14] power: supply: mt6370: Add Mediatek MT6370 charger driver

From: Andy Shevchenko
Date: Thu Jun 23 2022 - 15:28:43 EST

Next message: Eric Biggers: "Re: [PATCH v3 1/8] statx: add direct I/O alignment information"
Previous message: Jason A. Donenfeld: "[GIT PULL] random number generator fixes for 5.19-rc4"
In reply to: ChiaEn Wu: "[PATCH v3 11/14] power: supply: mt6370: Add Mediatek MT6370 charger driver"
Next in thread: ChiaEn Wu: "[PATCH v3 08/14] usb: typec: tcpci_mt6370: Add Mediatek MT6370 tcpci driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Thu, Jun 23, 2022 at 2:00 PM ChiaEn Wu <peterwu.pub@xxxxxxxxx> wrote:
>
> From: ChiaEn Wu <chiaen_wu@xxxxxxxxxxx>
>
> Add Mediatek MT6370 charger driver.

...

> +config CHARGER_MT6370
> + tristate "Mediatek MT6370 Charger Driver"
> + depends on MFD_MT6370
> + depends on REGULATOR
> + select LINEAR_RANGES
> + help
> + Say Y here to enable MT6370 Charger Part.
> + The device supports High-Accuracy Voltage/Current Regulation,
> + Average Input Current Regulation, Battery Temperature Sensing,
> + Over-Temperature Protection, DPDM Detection for BC1.2.

Module name?

...

> +#include <dt-bindings/iio/adc/mediatek,mt6370_adc.h>

This usually goes after linux/*

> +#include <linux/atomic.h>
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/iio/consumer.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>

> +#include <linux/of.h>

> +#include <linux/platform_device.h>
> +#include <linux/power_supply.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/driver.h>
> +#include <linux/workqueue.h>

...

> +#define MT6370_MIVR_IBUS_TH 100000 /* 100 mA */

Instead of comment, add proper units.

...

> + MT6370_USB_STAT_DCP,
> + MT6370_USB_STAT_CDP,
> + MT6370_USB_STAT_MAX,

No comma for a terminator line.

...

> +static inline u32 mt6370_chg_val_to_reg(const struct mt6370_chg_range *range,
> + u32 val)
> +static inline u32 mt6370_chg_reg_to_val(const struct mt6370_chg_range *range,
> + u8 reg)

I'm wondering if you can use the
https://elixir.bootlin.com/linux/v5.19-rc3/source/include/linux/linear_range.h
APIs.

...

> + int ret = 0;

This seems a redundant assignment, see below.

> + rcfg->ena_gpiod = fwnode_gpiod_get_index(of_fwnode_handle(of),
> + "enable", 0,

For index == 0 don't use _index API.

> + GPIOD_OUT_LOW |
> + GPIOD_FLAGS_BIT_NONEXCLUSIVE,
> + rdesc->name);
> + if (IS_ERR(rcfg->ena_gpiod)) {
> + dev_err(priv->dev, "Failed to requeset OTG EN Pin\n");

request

> + rcfg->ena_gpiod = NULL;

So, use _optional and return any errors you got.

> + } else {
> + val = MT6370_OPA_MODE_MASK | MT6370_OTG_PIN_EN_MASK;
> + ret = regmap_update_bits(priv->regmap, MT6370_REG_CHG_CTRL1,
> + val, val);
> + if (ret)
> + dev_err(priv->dev, "Failed to set otg bits\n");
> + }

...

> + irq_num = platform_get_irq_byname(pdev, irq_name);

> +

Unwanted blank line.

> + if (irq_num < 0) {

> + dev_err(priv->dev, "Failed to get platform resource\n");

Isn't it printed by the call?

> + } else {
> + if (en)
> + enable_irq(irq_num);
> + else
> + disable_irq_nosync(irq_num);
> + }

...

> +toggle_cfo_exit:

The useless label.

> + return ret;
> +}

...

> + ret = mt6370_chg_get_online(priv, val);
> + if (!val->intval) {

No error check?

> + val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
> + return 0;
> + }

...

> +static int mt6370_chg_set_online(struct mt6370_priv *priv,
> + const union power_supply_propval *val)
> +{
> + int attach;
> + u32 pwr_rdy = !!val->intval;
> +
> + mutex_lock(&priv->attach_lock);
> + attach = atomic_read(&priv->attach);
> + if (pwr_rdy == !!attach) {
> + dev_err(priv->dev, "pwr_rdy is same(%d)\n", pwr_rdy);
> + mutex_unlock(&priv->attach_lock);
> + return 0;
> + }
> +
> + atomic_set(&priv->attach, pwr_rdy);
> + mutex_unlock(&priv->attach_lock);
> +
> + if (!queue_work(priv->wq, &priv->bc12_work))
> + dev_err(priv->dev, "bc12 work has already queued\n");
> +
> + return 0;

> +

Unwanted blank line.

> +}

> +static int mt6370_chg_get_property(struct power_supply *psy,
> + enum power_supply_property psp,
> + union power_supply_propval *val)
> +{
> + struct mt6370_priv *priv = power_supply_get_drvdata(psy);
> + int ret = 0;
> +
> + switch (psp) {
> + case POWER_SUPPLY_PROP_ONLINE:
> + ret = mt6370_chg_get_online(priv, val);
> + break;
> + case POWER_SUPPLY_PROP_STATUS:
> + ret = mt6370_chg_get_status(priv, val);
> + break;
> + case POWER_SUPPLY_PROP_CHARGE_TYPE:
> + ret = mt6370_chg_get_charge_type(priv, val);
> + break;
> + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
> + ret = mt6370_chg_get_ichg(priv, val);
> + break;
> + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
> + ret = mt6370_chg_get_max_ichg(priv, val);
> + break;
> + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
> + ret = mt6370_chg_get_cv(priv, val);
> + break;
> + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX:
> + ret = mt6370_chg_get_max_cv(priv, val);
> + break;
> + case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
> + ret = mt6370_chg_get_aicr(priv, val);
> + break;
> + case POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT:
> + ret = mt6370_chg_get_mivr(priv, val);
> + break;
> + case POWER_SUPPLY_PROP_PRECHARGE_CURRENT:
> + ret = mt6370_chg_get_iprechg(priv, val);
> + break;
> + case POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT:
> + ret = mt6370_chg_get_ieoc(priv, val);
> + break;
> + case POWER_SUPPLY_PROP_TYPE:
> + val->intval = priv->psy_desc->type;
> + break;
> + case POWER_SUPPLY_PROP_USB_TYPE:
> + val->intval = priv->psy_usb_type;
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> +
> + return ret;

In all cases, return directly.

> +}

...

> + switch (psp) {
> + case POWER_SUPPLY_PROP_ONLINE:
> + ret = mt6370_chg_set_online(priv, val);
> + break;
> + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
> + ret = mt6370_chg_set_ichg(priv, val);
> + break;
> + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
> + ret = mt6370_chg_set_cv(priv, val);
> + break;
> + case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
> + ret = mt6370_chg_set_aicr(priv, val);
> + break;
> + case POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT:
> + ret = mt6370_chg_set_mivr(priv, val);
> + break;
> + case POWER_SUPPLY_PROP_PRECHARGE_CURRENT:
> + ret = mt6370_chg_set_iprechg(priv, val);
> + break;
> + case POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT:
> + ret = mt6370_chg_set_ieoc(priv, val);
> + break;
> + default:
> + ret = -EINVAL;
> + }
> + return ret;

As per above.

...

> + for (i = 0; i < F_MAX; i++) {
> + priv->rmap_fields[i] = devm_regmap_field_alloc(priv->dev,
> + priv->regmap,
> + fds[i].field);
> + if (IS_ERR(priv->rmap_fields[i])) {
> + dev_err(priv->dev,
> + "Failed to allocate regmap field [%s]\n",
> + fds[i].name);
> + return PTR_ERR(priv->rmap_fields[i]);

return dev_err_probe();

> + }
> + }

...

> + mutex_init(&priv->attach_lock);
> + atomic_set(&priv->attach, 0);

Why not atomic_init() ?
But yeah, usage of it and other locking mechanisms in this driver are
questionable.

...

> + /* ICHG/IEOC Workaroud, ICHG can not be set less than 900mA */

Workaround

...

> + return IS_ERR(priv->rdev) ? PTR_ERR(priv->rdev) : 0;

PTR_ERR_OR_ZERO()

...

> + .of_node = priv->dev->of_node,

dev_of_node() ?

> + };
> +
> + priv->psy_desc = &mt6370_chg_psy_desc;
> + priv->psy_desc->name = dev_name(priv->dev);
> + priv->psy = devm_power_supply_register(priv->dev, priv->psy_desc, &cfg);
> +
> + return IS_ERR(priv->psy) ? PTR_ERR(priv->psy) : 0;

PTR_ERR_OR_ZERO()

> +}

...

> +static irqreturn_t mt6370_attach_i_handler(int irq, void *data)
> +{
> + struct mt6370_priv *priv = data;
> + u32 otg_en;
> + int ret;
> +
> + /* Check in otg mode or not */
> + ret = mt6370_chg_field_get(priv, F_BOOST_STAT, &otg_en);
> + if (ret < 0) {
> + dev_err(priv->dev, "failed to get otg state\n");
> + return IRQ_HANDLED;

Handled error?

> + }
> +
> + if (otg_en)
> + return IRQ_HANDLED;

> + mutex_lock(&priv->attach_lock);
> + atomic_set(&priv->attach, MT6370_ATTACH_STAT_ATTACH_BC12_DONE);
> + mutex_unlock(&priv->attach_lock);

Mutex around atomic?! It's interesting...

> + if (!queue_work(priv->wq, &priv->bc12_work))
> + dev_err(priv->dev, "bc12 work has already queued\n");
> +
> + return IRQ_HANDLED;
> +}

...

> + for (i = 0; i < ARRAY_SIZE(mt6370_chg_irqs); i++) {
> + ret = platform_get_irq_byname(to_platform_device(priv->dev),
> + mt6370_chg_irqs[i].name);
> + if (ret < 0) {
> + dev_err(priv->dev, "Failed to get irq %s\n",
> + mt6370_chg_irqs[i].name);

Isn't the same printed by the above call?

> + return ret;
> + }
> +
> + ret = devm_request_threaded_irq(priv->dev, ret, NULL,
> + mt6370_chg_irqs[i].handler,
> + IRQF_TRIGGER_FALLING,
> + dev_name(priv->dev),
> + priv);
> +
> + if (ret < 0) {
> + dev_err(priv->dev, "Failed to request irq %s\n",
> + mt6370_chg_irqs[i].name);
> + return ret;

return dev_err_probe();

> + }
> + }

...

> +static int mt6370_chg_probe(struct platform_device *pdev)
> +{

Use return dev_err_probe(...); pattern.

> +probe_out:
> + destroy_workqueue(priv->wq);
> + mutex_destroy(&priv->attach_lock);

I don't see clearly the initialization of these in the ->probe().
Besides that, does destroy_workque() synchronize the actual queue(s)?

Mixing devm_ and non-devm_ may lead to a wrong release order that's
why it is better to see allocating and destroying resources in one
function (they may be wrapped, but should be both of them, seems like
you have done it only for the first parts).

> + return ret;
> +}

...

> +static int mt6370_chg_remove(struct platform_device *pdev)
> +{
> + struct mt6370_priv *priv = platform_get_drvdata(pdev);
> +
> + if (priv) {

Can you describe when this condition can be false?

> + mt6370_chg_enable_irq(priv, "mivr", false);
> + cancel_delayed_work_sync(&priv->mivr_dwork);
> + destroy_workqueue(priv->wq);
> + mutex_destroy(&priv->attach_lock);
> + }
> +
> + return 0;
> +}

...

> +static struct platform_driver mt6370_chg_driver = {
> + .probe = mt6370_chg_probe,
> + .remove = mt6370_chg_remove,
> + .driver = {
> + .name = "mt6370-charger",
> + .of_match_table = of_match_ptr(mt6370_chg_of_match),

No good use of of_match_ptr(), please drop it.

> + },
> +};

--
With Best Regards,
Andy Shevchenko

Next message: Eric Biggers: "Re: [PATCH v3 1/8] statx: add direct I/O alignment information"
Previous message: Jason A. Donenfeld: "[GIT PULL] random number generator fixes for 5.19-rc4"
In reply to: ChiaEn Wu: "[PATCH v3 11/14] power: supply: mt6370: Add Mediatek MT6370 charger driver"
Next in thread: ChiaEn Wu: "[PATCH v3 08/14] usb: typec: tcpci_mt6370: Add Mediatek MT6370 tcpci driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]