| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <a4646a277d692f4b4a04382b7367b641b2ff0fc6.camel@fi.rohmeurope.com>
Date: Thu, 27 May 2021 07:25:20 +0300
From: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>
To: Gene Chen <gene.chen.richtek@...il.com>
Cc: sre@...nel.org, Matthias Brugger <matthias.bgg@...il.com>,
Rob Herring <robh+dt@...nel.org>, linux-pm@...r.kernel.org,
devicetree <devicetree@...r.kernel.org>,
linux-arm Mailing List <linux-arm-kernel@...ts.infradead.org>,
"moderated list:ARM/Mediatek SoC support"
<linux-mediatek@...ts.infradead.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Gene Chen <gene_chen@...htek.com>, Wilma.Wu@...iatek.com,
shufan_lee@...htek.com, ChiYuan Huang <cy_huang@...htek.com>,
benjamin.chao@...iatek.com
Subject: Re: [PATCH v4 2/2] power: supply: mt6360_charger: add MT6360
charger support
On Wed, 2021-05-26 at 17:40 +0800, Gene Chen wrote:
> Matti Vaittinen <matti.vaittinen@...rohmeurope.com> 於 2021年3月30日 週二
> 下午7:48寫道：
> >
> > On Mon, 2021-01-18 at 20:41 +0800, Gene Chen wrote:
> > > From: Gene Chen <gene_chen@...htek.com>
> > >
> > > Add basic support for the battery charger for MT6360 PMIC
> > >
> > > Signed-off-by: Gene Chen <gene_chen@...htek.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 ...
> >
>
> I found we are neither linear_range_get_selector_high or
> linear_range_get_selector_low.
> When value lower than min_value, choose min_sel. If higher than
> max_value, choose max_sel.
Ah, correct.
> Should I create linear_range_get_selector() for this?
My suggestion would be yes, but I am not insisting on it.
>
> > > +
> > > +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.
> >
>
> ACK, We can use "linear_range_get_value", but maybe wait for reply
> about "mt6360_map_reg_sel"
>
> > > +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?
> >
>
> MT6360 charger is a switching charger which can charging or boost OTG
> VBUS.
>
I see. It was just strange for me to see the regulators being set-up in
the charger driver. I would have expected to see separate regulator
driver for this, I guess this is MFD in any case, right? So I would
have expected seeing a sub-device for regulators.
I however see we have couple of other charger drivers here doing the
same thing (setting up regulators in charger driver) - so if this is
normal then I for sure have no objections :)
> > ...
> >
> > > +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?
> >
>
> MT6360 MFD driver probe will clear all interrupts then add charger
> device.
> We need to schedule work for handling boot-up vbus is always exist,
> because irq is already cleared.
>
Thank you for the explanation. I was just wondering why not checking
charger status right here? I didn't understand why the checking was
pushed to a WQ. You probably have a valid reason. I assume it's just me
who is not understanding this :) Additionally I was wondering what
happens if for example the IRQ registration below fails and we detach
the driver - I don't see work-queue being flushed. Thanks for all the
explanations!
> > > +
> > > + 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;
> > > +}
> > > +
Powered by blists - more mailing lists