From: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
To: Gene Chen <gene.chen.richtek@gmail.com>,
sre@kernel.org, matthias.bgg@gmail.com, robh+dt@kernel.org
Cc: linux-pm@vger.kernel.org, devicetree@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org,
gene_chen@richtek.com, Wilma.Wu@mediatek.com,
shufan_lee@richtek.com, cy_huang@richtek.com,
benjamin.chao@mediatek.com
Subject: Re: [PATCH v4 2/2] power: supply: mt6360_charger: add MT6360 charger support
Date: Tue, 30 Mar 2021 14:48:05 +0300 [thread overview]
Message-ID: <771c7da0584cf37da6ba370207a89a7401a20c33.camel@fi.rohmeurope.com> (raw)
In-Reply-To: <1610973703-676-3-git-send-email-gene.chen.richtek@gmail.com>
On Mon, 2021-01-18 at 20:41 +0800, Gene Chen wrote:
> From: Gene Chen <gene_chen@richtek.com>
>
> Add basic support for the battery charger for MT6360 PMIC
>
> Signed-off-by: Gene Chen <gene_chen@richtek.com>
> ---
> drivers/power/supply/Kconfig | 10 +
> drivers/power/supply/Makefile | 1 +
> drivers/power/supply/mt6360_charger.c | 914
> ++++++++++++++++++++++++++++++++++
> 3 files changed, 925 insertions(+)
> create mode 100644 drivers/power/supply/mt6360_charger.c
>
Thanks for the contribution :)
Few comments which I am not demanding to be 'fixed' - but which might
be good to be checked. Eg, please consider my comments as 'nit's.
...
> +static unsigned int mt6360_map_reg_sel(u32 data, u32 min, u32 max,
> u32 step)
> +{
> + u32 target = 0, max_sel;
> +
> + if (data >= min) {
> + target = (data - min) / step;
> + max_sel = (max - min) / step;
> + if (target > max_sel)
> + target = max_sel;
> + }
> + return target;
> +}
lib/linear_ranges.c might already implement this ...
> +
> +static u32 mt6360_map_real_val(u32 sel, u32 min, u32 max, u32 step)
> +{
> + u32 target = 0;
> +
> + target = min + (sel * step);
> + if (target > max)
> + target = max;
> + return target;
> +}
...and this.
> +static int mt6360_charger_get_ichg(struct mt6360_chg_info *mci,
> + union power_supply_propval *val)
> +{
> + int ret;
> + unsigned int regval;
> +
> + ret = regmap_read(mci->regmap, MT6360_PMU_CHG_CTRL7, ®val);
> + if (ret < 0)
> + return ret;
> + regval = (regval & MT6360_ICHG_MASK) >> MT6360_ICHG_SHFT;
> + val->intval = mt6360_map_real_val(regval,
> + MT6360_ICHG_MIN,
> + MT6360_ICHG_MAX,
> + MT6360_ICHG_STEP);
linear_ranges?
> + return 0;
> +}
> +
> +static int mt6360_charger_get_max_ichg(struct mt6360_chg_info *mci,
> + union power_supply_propval *val)
> +{
> + val->intval = MT6360_ICHG_MAX;
> + return 0;
> +}
> +
> +static int mt6360_charger_get_cv(struct mt6360_chg_info *mci,
> + union power_supply_propval *val)
> +{
> + int ret;
> + unsigned int regval;
> +
> + ret = regmap_read(mci->regmap, MT6360_PMU_CHG_CTRL4, ®val);
> + if (ret < 0)
> + return ret;
> + regval = (regval & MT6360_VOREG_MASK) >> MT6360_VOREG_SHFT;
> + val->intval = mt6360_map_real_val(regval,
> + MT6360_VOREG_MIN,
> + MT6360_VOREG_MAX,
> + MT6360_VOREG_STEP);
linear_ranges?
> + return 0;
> +}
> +
> +static int mt6360_charger_get_max_cv(struct mt6360_chg_info *mci,
> + union power_supply_propval *val)
> +{
> + val->intval = MT6360_VOREG_MAX;
> + return 0;
> +}
> +
> +static int mt6360_charger_get_aicr(struct mt6360_chg_info *mci,
> + union power_supply_propval *val)
> +{
> + int ret;
> + unsigned int regval;
> +
> + ret = regmap_read(mci->regmap, MT6360_PMU_CHG_CTRL3, ®val);
> + if (ret < 0)
> + return ret;
> + regval = (regval & MT6360_IAICR_MASK) >> MT6360_IAICR_SHFT;
> + val->intval = mt6360_map_real_val(regval,
> + MT6360_AICR_MIN,
> + MT6360_AICR_MAX,
> + MT6360_AICR_STEP);
linear_ranges?
> + return 0;
> +}
> +
> +static int mt6360_charger_get_mivr(struct mt6360_chg_info *mci,
> + union power_supply_propval *val)
> +{
> + int ret;
> + unsigned int regval;
> +
> + ret = regmap_read(mci->regmap, MT6360_PMU_CHG_CTRL6, ®val);
> + if (ret < 0)
> + return ret;
> + regval = (regval & MT6360_VMIVR_MASK) >> MT6360_VMIVR_SHFT;
> + val->intval = mt6360_map_real_val(regval,
> + MT6360_VMIVR_MIN,
> + MT6360_VMIVR_MAX,
> + MT6360_VMIVR_STEP);
linear_ranges?
> + return 0;
> +}
> +
> +static int mt6360_charger_get_iprechg(struct mt6360_chg_info *mci,
> + union power_supply_propval *val)
> +{
> + int ret;
> + unsigned int regval;
> +
> + ret = regmap_read(mci->regmap, MT6360_PMU_CHG_CTRL8, ®val);
> + if (ret < 0)
> + return ret;
> + regval = (regval & MT6360_IPREC_MASK) >> MT6360_IPREC_SHFT;
> + val->intval = mt6360_map_real_val(regval,
> + MT6360_IPREC_MIN,
> + MT6360_IPREC_MAX,
> + MT6360_IPREC_STEP);
linear_ranges?
> + return 0;
> +}
> +
> +static int mt6360_charger_get_ieoc(struct mt6360_chg_info *mci,
> + union power_supply_propval *val)
> +{
> + int ret;
> + unsigned int regval;
> +
> + ret = regmap_read(mci->regmap, MT6360_PMU_CHG_CTRL9, ®val);
> + if (ret < 0)
> + return ret;
> + regval = (regval & MT6360_IEOC_MASK) >> MT6360_IEOC_SHFT;
> + val->intval = mt6360_map_real_val(regval,
> + MT6360_IEOC_MIN,
> + MT6360_IEOC_MAX,
> + MT6360_IEOC_STEP);
linear_ranges?
> + return 0;
> +}
> +
> +static int mt6360_charger_set_online(struct mt6360_chg_info *mci,
> + const union power_supply_propval
> *val)
> +{
> + u8 force_sleep = val->intval ? 0 : 1;
> +
> + return regmap_update_bits(mci->regmap,
> + MT6360_PMU_CHG_CTRL1,
> + MT6360_FSLP_MASK,
> + force_sleep << MT6360_FSLP_SHFT);
> +}
> +
> +static int mt6360_charger_set_ichg(struct mt6360_chg_info *mci,
> + const union power_supply_propval
> *val)
> +{
> + u8 sel;
> +
> + sel = mt6360_map_reg_sel(val->intval,
> + MT6360_ICHG_MIN,
> + MT6360_ICHG_MAX,
> + MT6360_ICHG_STEP);
linear_ranges?
> + return regmap_update_bits(mci->regmap,
> + MT6360_PMU_CHG_CTRL7,
> + MT6360_ICHG_MASK,
> + sel << MT6360_ICHG_SHFT);
> +}
> +
> +static int mt6360_charger_set_cv(struct mt6360_chg_info *mci,
> + const union power_supply_propval *val)
> +{
> + u8 sel;
> +
> + sel = mt6360_map_reg_sel(val->intval,
> + MT6360_VOREG_MIN,
> + MT6360_VOREG_MAX,
> + MT6360_VOREG_STEP);
linear_ranges?
> + return regmap_update_bits(mci->regmap,
> + MT6360_PMU_CHG_CTRL4,
> + MT6360_VOREG_MASK,
> + sel << MT6360_VOREG_SHFT);
> +}
> +
> +static int mt6360_charger_set_aicr(struct mt6360_chg_info *mci,
> + const union power_supply_propval
> *val)
> +{
> + u8 sel;
> +
> + sel = mt6360_map_reg_sel(val->intval,
> + MT6360_AICR_MIN,
> + MT6360_AICR_MAX,
> + MT6360_AICR_STEP);
linear_ranges?
> + return regmap_update_bits(mci->regmap,
> + MT6360_PMU_CHG_CTRL3,
> + MT6360_IAICR_MASK,
> + sel << MT6360_IAICR_SHFT);
> +}
> +
> +static int mt6360_charger_set_mivr(struct mt6360_chg_info *mci,
> + const union power_supply_propval
> *val)
> +{
> + u8 sel;
> +
> + sel = mt6360_map_reg_sel(val->intval,
> + MT6360_VMIVR_MIN,
> + MT6360_VMIVR_MAX,
> + MT6360_VMIVR_STEP);
linear_ranges?
> + return regmap_update_bits(mci->regmap,
> + MT6360_PMU_CHG_CTRL3,
> + MT6360_VMIVR_MASK,
> + sel << MT6360_VMIVR_SHFT);
> +}
> +
> +static int mt6360_charger_set_iprechg(struct mt6360_chg_info *mci,
> + const union power_supply_propval
> *val)
> +{
> + u8 sel;
> +
> + sel = mt6360_map_reg_sel(val->intval,
> + MT6360_IPREC_MIN,
> + MT6360_IPREC_MAX,
> + MT6360_IPREC_STEP);
linear_ranges?
> + return regmap_update_bits(mci->regmap,
> + MT6360_PMU_CHG_CTRL8,
> + MT6360_IPREC_MASK,
> + sel << MT6360_IPREC_SHFT);
> +}
> +
> +static int mt6360_charger_set_ieoc(struct mt6360_chg_info *mci,
> + const union power_supply_propval
> *val)
> +{
> + u8 sel;
> +
> + sel = mt6360_map_reg_sel(val->intval,
> + MT6360_IEOC_MIN,
> + MT6360_IEOC_MAX,
> + MT6360_IEOC_STEP);
linear_ranges?
> + return regmap_update_bits(mci->regmap,
> + MT6360_PMU_CHG_CTRL9,
> + MT6360_IEOC_MASK,
> + sel << MT6360_IEOC_SHFT);
> +}
> +
> +
> +static const struct regulator_ops mt6360_chg_otg_ops = {
> + .list_voltage = regulator_list_voltage_linear,
> + .enable = regulator_enable_regmap,
> + .disable = regulator_disable_regmap,
> + .is_enabled = regulator_is_enabled_regmap,
> + .set_voltage_sel = regulator_set_voltage_sel_regmap,
> + .get_voltage_sel = regulator_get_voltage_sel_regmap,
> +};
> +
> +static const struct regulator_desc mt6360_otg_rdesc = {
> + .of_match = "usb-otg-vbus",
> + .name = "usb-otg-vbus",
> + .ops = &mt6360_chg_otg_ops,
> + .owner = THIS_MODULE,
> + .type = REGULATOR_VOLTAGE,
> + .min_uV = 4425000,
> + .uV_step = 25000,
> + .n_voltages = 57,
> + .vsel_reg = MT6360_PMU_CHG_CTRL5,
> + .vsel_mask = MT6360_VOBST_MASK,
> + .enable_reg = MT6360_PMU_CHG_CTRL1,
> + .enable_mask = MT6360_OPA_MODE_MASK,
> +};
Any particular reason why these are here and not in a regulator driver?
...
> +static int mt6360_charger_probe(struct platform_device *pdev)
> +{
> + struct mt6360_chg_info *mci;
> + struct power_supply_config charger_cfg = {};
> + struct regulator_config config = { };
> + int ret;
> +
> + mci = devm_kzalloc(&pdev->dev, sizeof(*mci), GFP_KERNEL);
> + if (!mci)
> + return -ENOMEM;
> +
> + ret = mt6360_parse_dt(pdev);
> + if (ret)
> + return dev_err_probe(&pdev->dev, ret, "Failed to parse
> dt\n");
> +
> + mci->dev = &pdev->dev;
> + mci->vinovp = 6500000;
> + mutex_init(&mci->chgdet_lock);
> + platform_set_drvdata(pdev, mci);
> + INIT_WORK(&mci->chrdet_work, &mt6360_chrdet_work);
> +
> + mci->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> + if (!mci->regmap)
> + return dev_err_probe(&pdev->dev, -ENODEV, "Failed to
> get parent regmap\n");
> +
> + ret = mt6360_apply_dt(pdev);
> + if (ret)
> + return dev_err_probe(&pdev->dev, ret, "Failed to apply
> dt\n");
> +
> + memcpy(&mci->psy_desc, &mt6360_charger_desc, sizeof(mci-
> >psy_desc));
> + mci->psy_desc.name = dev_name(&pdev->dev);
> + charger_cfg.drv_data = mci;
> + charger_cfg.of_node = pdev->dev.of_node;
> + mci->psy = devm_power_supply_register(&pdev->dev,
> + &mci->psy_desc,
> &charger_cfg);
> + if (IS_ERR(mci->psy))
> + return dev_err_probe(&pdev->dev, PTR_ERR(mci->psy),
> + "Failed to register power supply
> dev\n");
> +
> + ret = mt6360_chg_init_setting(mci);
> + if (ret)
> + return dev_err_probe(&pdev->dev, ret, "Failed to
> initial setting\n");
> +
> + schedule_work(&mci->chrdet_work);
Is this work scheduled anywhere else? If not - why doing this in wq
context? If yes - does this wq need cancellation upon exit?
> +
> + ret = mt6360_chg_irq_register(pdev);
> + if (ret)
> + return dev_err_probe(&pdev->dev, ret, "Failed to
> register irqs\n");
> +
> + config.dev = &pdev->dev;
> + config.regmap = mci->regmap;
> + mci->otg_rdev = devm_regulator_register(&pdev->dev,
> &mt6360_otg_rdesc,
> + &config);
> + if (IS_ERR(mci->otg_rdev))
> + return PTR_ERR(mci->otg_rdev);
> +
> + return 0;
> +}
> +
Best Regards
Matti Vaittinen
next prev parent reply other threads:[~2021-03-30 11:49 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-18 12:41 [PATCH v4 0/2] power: supply: mt6360_charger: add MT6360 charger support Gene Chen
2021-01-18 12:41 ` [PATCH v4 1/2] dt-bindings: power: Add bindings document for Charger support on MT6360 PMIC Gene Chen
2021-01-18 12:41 ` [PATCH v4 2/2] power: supply: mt6360_charger: add MT6360 charger support Gene Chen
2021-03-02 7:27 ` Gene Chen
2021-03-22 10:58 ` Gene Chen
2021-03-29 17:13 ` Matthias Brugger
2021-03-30 11:48 ` Matti Vaittinen [this message]
2021-05-26 9:40 ` Gene Chen
2021-05-27 4:25 ` Matti Vaittinen
2021-05-27 9:58 ` Gene Chen
2021-05-27 10:22 ` Matti Vaittinen
2021-04-02 11:57 ` Sebastian Reichel
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=771c7da0584cf37da6ba370207a89a7401a20c33.camel@fi.rohmeurope.com \
--to=matti.vaittinen@fi.rohmeurope.com \
--cc=Wilma.Wu@mediatek.com \
--cc=benjamin.chao@mediatek.com \
--cc=cy_huang@richtek.com \
--cc=devicetree@vger.kernel.org \
--cc=gene.chen.richtek@gmail.com \
--cc=gene_chen@richtek.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=linux-pm@vger.kernel.org \
--cc=matthias.bgg@gmail.com \
--cc=robh+dt@kernel.org \
--cc=shufan_lee@richtek.com \
--cc=sre@kernel.org \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).