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

Hi Andy,

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

Andy Shevchenko [off-list ref] 於 2022年6月24日 週五 凌晨2:56寫道：

On Thu, Jun 23, 2022 at 2:00 PM ChiaEn Wu [off-list ref] wrote:

quoted

From: ChiaEn Wu <redacted>

Add Mediatek MT6370 charger driver.

...

quoted

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

...

quoted

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

This usually goes after linux/*

quoted

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

quoted

+#include <linux/of.h>

quoted

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

...

quoted

+#define MT6370_MIVR_IBUS_TH            100000          /* 100 mA */

Instead of comment, add proper units.

...

quoted

+       MT6370_USB_STAT_DCP,
+       MT6370_USB_STAT_CDP,
+       MT6370_USB_STAT_MAX,

No comma for a terminator line.

...

quoted

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

...

quoted

+       int ret = 0;

This seems a redundant assignment, see below.

quoted

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

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

quoted

+                                                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

quoted

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

quoted

+       } 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");
+       }

...

quoted

+       irq_num = platform_get_irq_byname(pdev, irq_name);

quoted

+

Unwanted blank line.

quoted

+       if (irq_num < 0) {

quoted

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

Isn't it printed by the call?

quoted

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

...

quoted

+toggle_cfo_exit:

The useless label.

quoted

+       return ret;
+}

...

quoted

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

quoted

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

...

quoted

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

quoted

+

Unwanted blank line.

quoted

+}

quoted

+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

+}

...

quoted

+       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

+       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();

quoted

+               }
+       }

...

quoted

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

...

quoted

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

Workaround

...

quoted

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

PTR_ERR_OR_ZERO()

...

quoted

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

dev_of_node() ?

quoted

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

+}

...

quoted

+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

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

quoted

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

quoted

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

...

quoted

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

quoted

+                       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();

quoted

+               }
+       }

...

quoted

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

Use return dev_err_probe(...); pattern.

quoted

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

quoted

+       return ret;
+}

...

quoted

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

quoted

+               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

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

quoted

+       },
+};

--
With Best Regards,
Andy Shevchenko

Thanks for your review!

Best regards,
ChiaEn Wu