From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: ChiaEn Wu <peterwu.pub@gmail.com>
Cc: Lee Jones <lee.jones@linaro.org>,
Daniel Thompson <daniel.thompson@linaro.org>,
Jingoo Han <jingoohan1@gmail.com>, Pavel Machek <pavel@ucw.cz>,
Rob Herring <robh+dt@kernel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
Matthias Brugger <matthias.bgg@gmail.com>,
Sebastian Reichel <sre@kernel.org>,
Chunfeng Yun <chunfeng.yun@mediatek.com>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Jonathan Cameron <jic23@kernel.org>,
Lars-Peter Clausen <lars@metafoo.de>,
Liam Girdwood <lgirdwood@gmail.com>,
Mark Brown <broonie@kernel.org>,
Guenter Roeck <linux@roeck-us.net>,
"Krogerus, Heikki" <heikki.krogerus@linux.intel.com>,
Helge Deller <deller@gmx.de>,
chiaen_wu@richtek.com, alice_chen@richtek.com,
cy_huang <cy_huang@richtek.com>,
dri-devel <dri-devel@lists.freedesktop.org>,
Linux LED Subsystem <linux-leds@vger.kernel.org>,
devicetree <devicetree@vger.kernel.org>,
linux-arm Mailing List <linux-arm-kernel@lists.infradead.org>,
"moderated list:ARM/Mediatek SoC support"
<linux-mediatek@lists.infradead.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Linux PM <linux-pm@vger.kernel.org>,
USB <linux-usb@vger.kernel.org>,
linux-iio <linux-iio@vger.kernel.org>,
"open list:FRAMEBUFFER LAYER" <linux-fbdev@vger.kernel.org>,
szuni chen <szunichen@gmail.com>
Subject: Re: [PATCH v3 11/14] power: supply: mt6370: Add Mediatek MT6370 charger driver
Date: Thu, 23 Jun 2022 20:56:09 +0200 [thread overview]
Message-ID: <CAHp75Vf2UAVgWS1nu8iwNjESWHQGOMWcNMUFShZ8Q_Qp3fssdQ@mail.gmail.com> (raw)
In-Reply-To: <20220623115631.22209-12-peterwu.pub@gmail.com>
On Thu, Jun 23, 2022 at 2:00 PM ChiaEn Wu <peterwu.pub@gmail.com> wrote:
>
> From: ChiaEn Wu <chiaen_wu@richtek.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.
...
> + 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.
> + } 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?
> + 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.
...
> + /* 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...
> + 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?
> + 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).
> + 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?
> + 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
WARNING: multiple messages have this Message-ID (diff)
From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: ChiaEn Wu <peterwu.pub@gmail.com>
Cc: "open list:FRAMEBUFFER LAYER" <linux-fbdev@vger.kernel.org>,
"Krogerus, Heikki" <heikki.krogerus@linux.intel.com>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
alice_chen@richtek.com, linux-iio <linux-iio@vger.kernel.org>,
dri-devel <dri-devel@lists.freedesktop.org>,
Liam Girdwood <lgirdwood@gmail.com>,
cy_huang <cy_huang@richtek.com>, Pavel Machek <pavel@ucw.cz>,
Lee Jones <lee.jones@linaro.org>,
Linux LED Subsystem <linux-leds@vger.kernel.org>,
Daniel Thompson <daniel.thompson@linaro.org>,
Helge Deller <deller@gmx.de>, Rob Herring <robh+dt@kernel.org>,
Chunfeng Yun <chunfeng.yun@mediatek.com>,
Guenter Roeck <linux@roeck-us.net>,
devicetree <devicetree@vger.kernel.org>,
Linux PM <linux-pm@vger.kernel.org>,
szuni chen <szunichen@gmail.com>, Mark Brown <broonie@kernel.org>,
"moderated list:ARM/Mediatek SoC support"
<linux-mediatek@lists.infradead.org>,
Matthias Brugger <matthias.bgg@gmail.com>,
linux-arm Mailing List <linux-arm-kernel@lists.infradead.org>,
Jingoo Han <jingoohan1@gmail.com>,
USB <linux-usb@vger.kernel.org>,
Sebastian Reichel <sre@kernel.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
chiaen_wu@richtek.com,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Jonathan Cameron <jic23@kernel.org>
Subject: Re: [PATCH v3 11/14] power: supply: mt6370: Add Mediatek MT6370 charger driver
Date: Thu, 23 Jun 2022 20:56:09 +0200 [thread overview]
Message-ID: <CAHp75Vf2UAVgWS1nu8iwNjESWHQGOMWcNMUFShZ8Q_Qp3fssdQ@mail.gmail.com> (raw)
In-Reply-To: <20220623115631.22209-12-peterwu.pub@gmail.com>
On Thu, Jun 23, 2022 at 2:00 PM ChiaEn Wu <peterwu.pub@gmail.com> wrote:
>
> From: ChiaEn Wu <chiaen_wu@richtek.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.
...
> + 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.
> + } 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?
> + 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.
...
> + /* 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...
> + 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?
> + 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).
> + 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?
> + 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
WARNING: multiple messages have this Message-ID (diff)
From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: ChiaEn Wu <peterwu.pub@gmail.com>
Cc: Lee Jones <lee.jones@linaro.org>,
Daniel Thompson <daniel.thompson@linaro.org>,
Jingoo Han <jingoohan1@gmail.com>, Pavel Machek <pavel@ucw.cz>,
Rob Herring <robh+dt@kernel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
Matthias Brugger <matthias.bgg@gmail.com>,
Sebastian Reichel <sre@kernel.org>,
Chunfeng Yun <chunfeng.yun@mediatek.com>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Jonathan Cameron <jic23@kernel.org>,
Lars-Peter Clausen <lars@metafoo.de>,
Liam Girdwood <lgirdwood@gmail.com>,
Mark Brown <broonie@kernel.org>,
Guenter Roeck <linux@roeck-us.net>,
"Krogerus, Heikki" <heikki.krogerus@linux.intel.com>,
Helge Deller <deller@gmx.de>,
chiaen_wu@richtek.com, alice_chen@richtek.com,
cy_huang <cy_huang@richtek.com>,
dri-devel <dri-devel@lists.freedesktop.org>,
Linux LED Subsystem <linux-leds@vger.kernel.org>,
devicetree <devicetree@vger.kernel.org>,
linux-arm Mailing List <linux-arm-kernel@lists.infradead.org>,
"moderated list:ARM/Mediatek SoC support"
<linux-mediatek@lists.infradead.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Linux PM <linux-pm@vger.kernel.org>,
USB <linux-usb@vger.kernel.org>,
linux-iio <linux-iio@vger.kernel.org>,
"open list:FRAMEBUFFER LAYER" <linux-fbdev@vger.kernel.org>,
szuni chen <szunichen@gmail.com>
Subject: Re: [PATCH v3 11/14] power: supply: mt6370: Add Mediatek MT6370 charger driver
Date: Thu, 23 Jun 2022 20:56:09 +0200 [thread overview]
Message-ID: <CAHp75Vf2UAVgWS1nu8iwNjESWHQGOMWcNMUFShZ8Q_Qp3fssdQ@mail.gmail.com> (raw)
In-Reply-To: <20220623115631.22209-12-peterwu.pub@gmail.com>
On Thu, Jun 23, 2022 at 2:00 PM ChiaEn Wu <peterwu.pub@gmail.com> wrote:
>
> From: ChiaEn Wu <chiaen_wu@richtek.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.
...
> + 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.
> + } 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?
> + 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.
...
> + /* 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...
> + 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?
> + 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).
> + 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?
> + 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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2022-06-23 19:28 UTC|newest]
Thread overview: 141+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-23 11:56 [PATCH v3 00/14] Add Mediatek MT6370 PMIC support ChiaEn Wu
2022-06-23 11:56 ` ChiaEn Wu
2022-06-23 11:56 ` ChiaEn Wu
2022-06-23 11:56 ` [PATCH v3 01/14] dt-bindings: usb: Add Mediatek MT6370 TCPC ChiaEn Wu
2022-06-23 11:56 ` ChiaEn Wu
2022-06-23 11:56 ` ChiaEn Wu
2022-06-23 11:56 ` [PATCH v3 02/14] dt-bindings: power: supply: Add Mediatek MT6370 Charger ChiaEn Wu
2022-06-23 11:56 ` ChiaEn Wu
2022-06-23 11:56 ` ChiaEn Wu
2022-06-24 10:27 ` Krzysztof Kozlowski
2022-06-24 10:27 ` Krzysztof Kozlowski
2022-06-24 10:27 ` Krzysztof Kozlowski
2022-06-25 0:54 ` Miles Chen
2022-06-25 0:54 ` Miles Chen
2022-06-25 0:54 ` Miles Chen
2022-06-23 11:56 ` [PATCH v3 03/14] dt-bindings: leds: mt6370: Add Mediatek mt6370 current sink type LED indicator ChiaEn Wu
2022-06-23 11:56 ` ChiaEn Wu
2022-06-23 11:56 ` ChiaEn Wu
2022-06-24 10:35 ` Krzysztof Kozlowski
2022-06-24 10:35 ` Krzysztof Kozlowski
2022-06-24 10:35 ` Krzysztof Kozlowski
2022-06-24 10:45 ` Krzysztof Kozlowski
2022-06-24 10:45 ` Krzysztof Kozlowski
2022-06-24 10:45 ` Krzysztof Kozlowski
2022-06-23 11:56 ` [PATCH v3 04/14] dt-bindings: leds: Add Mediatek MT6370 flashlight ChiaEn Wu
2022-06-23 11:56 ` ChiaEn Wu
2022-06-23 11:56 ` ChiaEn Wu
2022-06-24 11:22 ` Krzysztof Kozlowski
2022-06-24 11:22 ` Krzysztof Kozlowski
2022-06-24 11:22 ` Krzysztof Kozlowski
2022-06-23 11:56 ` [PATCH v3 05/14] dt-bindings: backlight: Add Mediatek MT6370 backlight ChiaEn Wu
2022-06-23 11:56 ` ChiaEn Wu
2022-06-23 11:56 ` ChiaEn Wu
2022-06-23 13:16 ` Joe Simmons-Talbott
2022-06-23 13:16 ` Joe Simmons-Talbott
2022-06-23 13:16 ` Joe Simmons-Talbott
2022-06-24 9:34 ` ChiaEn Wu
2022-06-24 9:34 ` ChiaEn Wu
2022-06-24 9:34 ` ChiaEn Wu
2022-06-30 21:55 ` Rob Herring
2022-06-30 21:55 ` Rob Herring
2022-06-30 21:55 ` Rob Herring
2022-06-23 11:56 ` [PATCH v3 06/14] dt-bindings: mfd: Add Mediatek MT6370 ChiaEn Wu
2022-06-23 11:56 ` ChiaEn Wu
2022-06-23 11:56 ` ChiaEn Wu
2022-06-24 11:39 ` Krzysztof Kozlowski
2022-06-24 11:39 ` Krzysztof Kozlowski
2022-06-24 11:39 ` Krzysztof Kozlowski
2022-06-23 11:56 ` [PATCH v3 07/14] mfd: mt6370: Add Mediatek MT6370 support ChiaEn Wu
2022-06-23 11:56 ` ChiaEn Wu
2022-06-23 11:56 ` ChiaEn Wu
2022-06-23 18:00 ` Andy Shevchenko
2022-06-23 18:00 ` Andy Shevchenko
2022-06-23 18:00 ` Andy Shevchenko
2022-06-24 10:19 ` ChiaEn Wu
2022-06-24 10:19 ` ChiaEn Wu
2022-06-24 10:19 ` ChiaEn Wu
2022-06-28 11:52 ` Andy Shevchenko
2022-06-28 11:52 ` Andy Shevchenko
2022-06-28 11:52 ` Andy Shevchenko
2022-07-12 15:29 ` Lee Jones
2022-07-12 15:29 ` Lee Jones
2022-07-12 15:29 ` Lee Jones
2022-07-13 2:29 ` ChiaEn Wu
2022-07-13 2:29 ` ChiaEn Wu
2022-07-13 2:29 ` ChiaEn Wu
2022-07-13 8:04 ` Lee Jones
2022-07-13 8:04 ` Lee Jones
2022-07-13 8:04 ` Lee Jones
2022-07-13 9:31 ` ChiaEn Wu
2022-07-13 9:31 ` ChiaEn Wu
2022-07-13 9:31 ` ChiaEn Wu
2022-06-23 11:56 ` [PATCH v3 08/14] usb: typec: tcpci_mt6370: Add Mediatek MT6370 tcpci driver ChiaEn Wu
2022-06-23 11:56 ` ChiaEn Wu
2022-06-23 11:56 ` ChiaEn Wu
2022-06-23 18:07 ` Andy Shevchenko
2022-06-23 18:07 ` Andy Shevchenko
2022-06-23 18:07 ` Andy Shevchenko
2022-06-24 11:33 ` Greg KH
2022-06-24 11:33 ` Greg KH
2022-06-24 11:33 ` Greg KH
2022-06-23 11:56 ` [PATCH v3 09/14] regulator: mt6370: Add mt6370 DisplayBias and VibLDO support ChiaEn Wu
2022-06-23 11:56 ` ChiaEn Wu
2022-06-23 11:56 ` ChiaEn Wu
2022-06-23 18:18 ` Andy Shevchenko
2022-06-23 18:18 ` Andy Shevchenko
2022-06-23 18:18 ` Andy Shevchenko
2022-06-24 10:32 ` ChiaEn Wu
2022-06-24 10:32 ` ChiaEn Wu
2022-06-24 10:32 ` ChiaEn Wu
2022-06-28 11:54 ` Andy Shevchenko
2022-06-28 11:54 ` Andy Shevchenko
2022-06-28 11:54 ` Andy Shevchenko
2022-06-23 11:56 ` [PATCH v3 10/14] iio: adc: mt6370: Add Mediatek MT6370 support ChiaEn Wu
2022-06-23 11:56 ` ChiaEn Wu
2022-06-23 11:56 ` ChiaEn Wu
2022-06-23 18:25 ` Andy Shevchenko
2022-06-23 18:25 ` Andy Shevchenko
2022-06-23 18:25 ` Andy Shevchenko
2022-06-23 11:56 ` [PATCH v3 11/14] power: supply: mt6370: Add Mediatek MT6370 charger driver ChiaEn Wu
2022-06-23 11:56 ` ChiaEn Wu
2022-06-23 11:56 ` ChiaEn Wu
2022-06-23 18:56 ` Andy Shevchenko [this message]
2022-06-23 18:56 ` Andy Shevchenko
2022-06-23 18:56 ` Andy Shevchenko
2022-06-29 15:52 ` ChiaEn Wu
2022-06-29 15:52 ` ChiaEn Wu
2022-06-29 15:52 ` ChiaEn Wu
2022-06-23 11:56 ` [PATCH v3 12/14] leds: mt6370: Add Mediatek MT6370 current sink type LED Indicator support ChiaEn Wu
2022-06-23 11:56 ` ChiaEn Wu
2022-06-23 11:56 ` ChiaEn Wu
2022-06-23 19:29 ` Randy Dunlap
2022-06-23 19:29 ` Randy Dunlap
2022-06-23 19:29 ` Randy Dunlap
2022-06-24 6:23 ` Linus Walleij
2022-06-24 6:23 ` Linus Walleij
2022-06-24 6:23 ` Linus Walleij
2022-06-24 6:25 ` Linus Walleij
2022-06-24 6:25 ` Linus Walleij
2022-06-24 6:25 ` Linus Walleij
2022-06-24 7:20 ` szuni chen
2022-06-24 7:20 ` szuni chen
2022-06-24 7:20 ` szuni chen
2022-06-24 21:36 ` Linus Walleij
2022-06-24 21:36 ` Linus Walleij
2022-06-24 21:36 ` Linus Walleij
2022-06-23 11:56 ` [PATCH v3 13/14] leds: flashlight: mt6370: Add Mediatek MT6370 flashlight support ChiaEn Wu
2022-06-23 11:56 ` ChiaEn Wu
2022-06-23 11:56 ` ChiaEn Wu
2022-06-23 11:56 ` [PATCH v3 14/14] video: backlight: mt6370: Add Mediatek MT6370 support ChiaEn Wu
2022-06-23 11:56 ` ChiaEn Wu
2022-06-23 11:56 ` ChiaEn Wu
2022-06-23 13:43 ` Daniel Thompson
2022-06-23 13:43 ` Daniel Thompson
2022-06-23 13:43 ` Daniel Thompson
2022-06-24 9:56 ` ChiaEn Wu
2022-06-24 9:56 ` ChiaEn Wu
2022-06-24 9:56 ` ChiaEn Wu
2022-06-23 15:48 ` (subset) [PATCH v3 00/14] Add Mediatek MT6370 PMIC support Mark Brown
2022-06-23 15:48 ` Mark Brown
2022-06-23 15:48 ` Mark Brown
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAHp75Vf2UAVgWS1nu8iwNjESWHQGOMWcNMUFShZ8Q_Qp3fssdQ@mail.gmail.com \
--to=andy.shevchenko@gmail.com \
--cc=alice_chen@richtek.com \
--cc=broonie@kernel.org \
--cc=chiaen_wu@richtek.com \
--cc=chunfeng.yun@mediatek.com \
--cc=cy_huang@richtek.com \
--cc=daniel.thompson@linaro.org \
--cc=deller@gmx.de \
--cc=devicetree@vger.kernel.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=gregkh@linuxfoundation.org \
--cc=heikki.krogerus@linux.intel.com \
--cc=jic23@kernel.org \
--cc=jingoohan1@gmail.com \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=lars@metafoo.de \
--cc=lee.jones@linaro.org \
--cc=lgirdwood@gmail.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-fbdev@vger.kernel.org \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-leds@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=linux-pm@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=linux@roeck-us.net \
--cc=matthias.bgg@gmail.com \
--cc=pavel@ucw.cz \
--cc=peterwu.pub@gmail.com \
--cc=robh+dt@kernel.org \
--cc=sre@kernel.org \
--cc=szunichen@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.