Message-ID: <CABtFH5J8GVXQMKmsUs2HfjV-p_rx4gLdCTKsDP_aCJWzE4AVeQ@mail.gmail.com>
Date:   Wed, 29 Jun 2022 23:52:33 +0800
From:   ChiaEn Wu <peterwu.pub@...il.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Lee Jones <lee.jones@...aro.org>,
        Daniel Thompson <daniel.thompson@...aro.org>,
        Jingoo Han <jingoohan1@...il.com>, Pavel Machek <pavel@....cz>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Sebastian Reichel <sre@...nel.org>,
        Chunfeng Yun <chunfeng.yun@...iatek.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jonathan Cameron <jic23@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Guenter Roeck <linux@...ck-us.net>,
        "Krogerus, Heikki" <heikki.krogerus@...ux.intel.com>,
        Helge Deller <deller@....de>,
        ChiaEn Wu <chiaen_wu@...htek.com>,
        Alice Chen <alice_chen@...htek.com>,
        cy_huang <cy_huang@...htek.com>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        Linux LED Subsystem <linux-leds@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        linux-arm Mailing List <linux-arm-kernel@...ts.infradead.org>,
        "moderated list:ARM/Mediatek SoC support" 
        <linux-mediatek@...ts.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux PM <linux-pm@...r.kernel.org>,
        USB <linux-usb@...r.kernel.org>,
        linux-iio <linux-iio@...r.kernel.org>,
        "open list:FRAMEBUFFER LAYER" <linux-fbdev@...r.kernel.org>,
        szuni chen <szunichen@...il.com>
Subject: 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 <andy.shevchenko@...il.com> 於 2022年6月24日 週五 凌晨2:56寫道：
>
> On Thu, Jun 23, 2022 at 2:00 PM ChiaEn Wu <peterwu.pub@...il.com> wrote:
> >
> > From: ChiaEn Wu <chiaen_wu@...htek.com>
> >
> > 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

Powered by blists - more mailing lists