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

From: ChiaEn Wu
Date: Wed Jun 29 2022 - 11:52:56 EST

Next message: Deren Wu: "[PATCH] linux-firmware: update firmware for MT7922 WiFi device"
Previous message: Peter Xu: "Re: [PATCH 1/4] mm/gup: Add FOLL_INTERRUPTIBLE"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Andy,

Sorry for the late reply, I have some questions to ask you below. Thanks!

Andy Shevchenko <andy.shevchenko@xxxxxxxxx> 於 2022年6月24日週五凌晨2:56寫道：
>
> 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.

Thanks for your helpful comments!
I will refine it in the next patch!

>
> ...
>
> > + 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.

These days, I tried to use various APIs in <gpio/consumer.h>, and also
try to use _optional APIs.
But my OTG regulator node is a child node of the charger node, like below.
----------------------------------------------------------------------------
// copy-paste from our mfd dt-binding example
charger {
compatible = "mediatek,mt6370-charger";
interrupts = <48>, <68>, <6>;
interrupt-names = "attach_i", "uvp_d_evt", "mivr";
io-channels = <&mt6370_adc MT6370_CHAN_IBUS>;

mt6370_otg_vbus: usb-otg-vbus-regulator {
regulator-name = "mt6370-usb-otg-vbus";
regulator-min-microvolt = <4350000>;
regulator-max-microvolt = <5800000>;
regulator-min-microamp = <500000>;
regulator-max-microamp = <3000000>;
};
};
----------------------------------------------------------------------------
Hence, if I use _optional APIs, it will always get NULL.
And, If I use 'gpiod_get_from_of_node' here, this API will only parse
the 'enable' property, not 'enable-gpio' or 'enable-gpios', we need to
add the '-gpio' suffix before we use this API.

Only 'fwnode_gpiod_get_index' can match this case. Although fwnode
parsing is not preferred, 'of_parse_cb' already can guarantee the
callback will only be used by the regulator of_node parsing.

>
> > + } 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?

I replace "mt6370_chg_get_online()" with "power_supply_get_property()"
and add some error check.
Could it meet your expectations??

>
> > + 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.

I will refine it in the next patch!

>
> ...
>
> > + /* 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...

I will revise it in the next patch.

>
> > + 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?

well... yes they are similar, I will remove one of them in the next patch.

>
> > + 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).

OK, I will try to revise these in the next patch!

>
> > + 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?

well... I will remove it in the next patch, sorry for making this
stupid mistake...

>
> > + 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

Thanks for your review!

Best regards,
ChiaEn Wu

Next message: Deren Wu: "[PATCH] linux-firmware: update firmware for MT7922 WiFi device"
Previous message: Peter Xu: "Re: [PATCH 1/4] mm/gup: Add FOLL_INTERRUPTIBLE"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]