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
