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
WARNING: multiple messages have this Message-ID (diff)
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
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
WARNING: multiple messages have this Message-ID (diff)
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
_______________________________________________
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:[~2021-03-30 11:49 UTC|newest]
Thread overview: 36+ 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 ` Gene Chen
2021-01-18 12:41 ` 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 ` Gene Chen
2021-01-18 12:41 ` Gene Chen
2021-01-18 12:41 ` [PATCH v4 2/2] power: supply: mt6360_charger: add MT6360 charger support Gene Chen
2021-01-18 12:41 ` Gene Chen
2021-01-18 12:41 ` Gene Chen
2021-03-02 7:27 ` Gene Chen
2021-03-02 7:27 ` Gene Chen
2021-03-02 7:27 ` Gene Chen
2021-03-22 10:58 ` Gene Chen
2021-03-22 10:58 ` Gene Chen
2021-03-22 10:58 ` Gene Chen
2021-03-29 17:13 ` Matthias Brugger
2021-03-29 17:13 ` Matthias Brugger
2021-03-29 17:13 ` Matthias Brugger
2021-03-30 11:48 ` Matti Vaittinen [this message]
2021-03-30 11:48 ` Matti Vaittinen
2021-03-30 11:48 ` Matti Vaittinen
2021-05-26 9:40 ` Gene Chen
2021-05-26 9:40 ` Gene Chen
2021-05-26 9:40 ` Gene Chen
2021-05-27 4:25 ` Matti Vaittinen
2021-05-27 4:25 ` Matti Vaittinen
2021-05-27 4:25 ` Matti Vaittinen
2021-05-27 9:58 ` Gene Chen
2021-05-27 9:58 ` Gene Chen
2021-05-27 9:58 ` Gene Chen
2021-05-27 10:22 ` Matti Vaittinen
2021-05-27 10:22 ` Matti Vaittinen
2021-05-27 10:22 ` Matti Vaittinen
2021-04-02 11:57 ` Sebastian Reichel
2021-04-02 11:57 ` Sebastian Reichel
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 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.