Re: [PATCH v3 11/14] power: supply: mt6370: Add Mediatek MT6370 charger driver
flat view
From: Andy Shevchenko <hidden>
Date: 2022-06-23 19:28:43
Also in:
dri-devel, linux-arm-kernel, linux-devicetree, linux-iio, linux-leds, linux-mediatek, linux-pm, linux-usb, lkml
On Thu, Jun 23, 2022 at 2:00 PM ChiaEn Wu [off-list ref] wrote:
From: ChiaEn Wu <redacted> Add Mediatek MT6370 charger driver.
...
quoted hunk ↗ jump to hunk
+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/*
quoted hunk ↗ jump to hunk
+#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>
quoted hunk ↗ jump to hunk
+#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. ...
quoted hunk ↗ jump to hunk
+ MT6370_USB_STAT_DCP, + MT6370_USB_STAT_CDP, + MT6370_USB_STAT_MAX,
No comma for a terminator line. ...
quoted hunk ↗ jump to hunk
+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.
quoted hunk ↗ jump to hunk
+ 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.
quoted hunk ↗ jump to hunk
+ } 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?
quoted hunk ↗ jump to hunk
+ } 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?
quoted hunk ↗ jump to hunk
+ val->intval = POWER_SUPPLY_STATUS_DISCHARGING; + return 0; + }
...
quoted hunk ↗ jump to hunk
+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.
+}
quoted hunk ↗ jump to hunk
+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.
+}
...
quoted hunk ↗ jump to hunk
+ 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. ...
quoted hunk ↗ jump to hunk
+ 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() ?
quoted hunk ↗ jump to hunk
+ }; + + 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()
+}
...
quoted hunk ↗ jump to hunk
+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?
quoted hunk ↗ jump to hunk
+ } + + if (otg_en) + return IRQ_HANDLED;
quoted hunk ↗ jump to hunk
+ 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...
quoted hunk ↗ jump to hunk
+ if (!queue_work(priv->wq, &priv->bc12_work)) + dev_err(priv->dev, "bc12 work has already queued\n"); + + return IRQ_HANDLED; +}
...
quoted hunk ↗ jump to hunk
+ 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?
quoted hunk ↗ jump to hunk
+ 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.
quoted hunk ↗ jump to hunk
+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; +}
...
quoted hunk ↗ jump to hunk
+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?
quoted hunk ↗ jump to hunk
+ 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; +}
...
quoted hunk ↗ jump to hunk
+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