linux-kernel - Re: [PATCH v3 2/2] power: supply: mt6360

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 16 Jan 2021 11:12:37 +0100
From:   Sebastian Reichel <sebastian.reichel@...labora.com>
To:     Gene Chen <gene.chen.richtek@...il.com>
Cc:     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, cy_huang@...htek.com,
        benjamin.chao@...iatek.com
Subject: Re: [PATCH v3 2/2] power: supply: mt6360_charger: add MT6360 charger
 support

Hi,

On Mon, Jan 11, 2021 at 08:15:33PM +0800, Gene Chen wrote:
> Sebastian Reichel <sebastian.reichel@...labora.com> 於 2021年1月7日 週四 上午4:16寫道：
> > > +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, &regval);
> > > +     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);
> > > +     return 0;
> > > +}
> >
> > It's unusual, that you do not need any scaling. Does the hardware
> > really report voltages in µV and currents in µA?
> >
> 
> Should I replace MT6360_ICHG_MIN by MT6360_ICHG_MINUA?

I just noticed, that you already have uA/uV comments above the
#defines. Should be good enough. Just keep it the way it is.

[...]
 
> > > +     last_usb_type = mci->psy_usb_type;
> > > +     /* Plug in */
> > > +     ret = regmap_read(mci->regmap, MT6360_PMU_USB_STATUS1, &usb_status);
> > > +     if (ret < 0)
> > > +             goto out;
> > > +     usb_status &= MT6360_USB_STATUS_MASK;
> > > +     usb_status >>= MT6360_USB_STATUS_SHFT;
> > > +     switch (usb_status) {
> > > +     case MT6360_CHG_TYPE_UNDER_GOING:
> > > +             dev_info(mci->dev, "%s: under going...\n", __func__);
> > > +             goto out;
> >
> > IDK what this is supposed to tell me. Do you mean "detection in
> > progress"? Also why is this info level? I would expect either
> > debug (assuming it happens regularly and is normal) or warning
> > (assuming it should not happen).
> >
> 
> When handling attach interrupt and cable plug out at the same
> time, HW change register status. So we don' need to handle this
> attach interrupt at this case.

So this is basically for debouncing? I suggest adding a comment:

/* Received attach IRQ followed by detach event, so nothing to do */
dev_dbg(mci->dev, "under going...\n");
goto out;

[...]

> > > +     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);
> > > +
> > > +     ret = mt6360_sysfs_create_group(mci);
> > > +     if (ret) {
> > > +             dev_err(&pdev->dev,
> > > +                     "%s: Failed to create sysfs attrs\n", __func__);
> > > +             return ret;
> > > +     }
> >
> > Use charger_cfg.attr_grp to register custom sysfs group for
> > power-supply devices. Otherwise your code is racy (udev may not pick
> > up the sysfs attributes). Also custom sysfs attributes need to be
> > documented in Documentation/ABI/testing/sysfs-class-power-<driver>.
> >
> > Looking at the attributes you are planning to expose, I don't think they
> > are suitable for sysfs anyways. Looks more like a debug interface, which
> > should go into debugfs instead. But it's hard to tell without any documentation
> > being provided :)
> 
> ACK, I will change to charger_cfg.attr_grp.
> I assumed the charger algorithm thread is in user space, and take
> control by sysfs node from charger device, like bq24190.c.
> Should I change to debugfs?

It's hard to tell without knowing more about the attributes
your are trying to expose. In debugfs we have relaxed ABI rules,
so it's easier to adopt naming e.t.c. later.

-- Sebastian

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)