Re: [PATCH v3 2/2] power: supply: mt6360_charger: add MT6360 charger support

[Gene Chen <gene.chen.richtek@gmail.com>]

Sebastian Reichel <sebastian.reichel@collabora.com> 於 2021年1月16日 週六 下午6:12寫道：
>
> Hi,
>
> On Mon, Jan 11, 2021 at 08:15:33PM +0800, Gene Chen wrote:
> > Sebastian Reichel <sebastian.reichel@collabora.com> 於 2021年1月7日 週四 上午4:16寫道：
> > > > +     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;
>

Sorry I have a little misunderstand.
under going is "detect in progress".
Attachi irq will trigger when power ready(vbus=5V) and bc12
chargedetection done.
Another irq, Detachi, is indicated power not ready(vbus=0V) and which
is be masked.
So, if the usb status is not SDP/NONSTD/CDP/DCP, the result can be
ignored. (e.q. NO VBUS/Under going/BC12 disabled/Reserved address)

> [...]
>
> > > > +     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.
>

I briefly classify the whole attributes. There are either unused, or
can be replaced by POWER_SUPPLY PROPERTY,
so I will remove unuse part.

HIZ = VBUS_IN high impedance mode.
VMIVR = Maximum input voltage regulation. Let input power can provide
at the predetermined voltage level.
(like POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT)
SYSREG = Config system minimum regulation voltage.
OTG_OC = maximum current of battery boost OTG 5V.
ICHG = maximum Charging current. (like
POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT)
IEOC = Like POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT
VOREG = Input voltage regulation. (like
POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE)
LBP = Low battery protection for battery boost OTG 5V.
VPREC = Pre-charge volatge level. (maybe can add new prop
POWER_SUPPLY_PROP_PRECHARGE_VOLTAGE)
TE = Charge termination enable/disable.
CHG_WDT_EN = Charger Watch dog timer enable/disable.
CHG_WDT = Charger Watch dog timer, 8/40/80/160s.
WT_FC = Fast charge Timer, 4~20hr.
BAT_COMP = Battery IR compensation resistor setting.
VCLAMP = Battery IR compensation maximum voltage clamp.
USBCHGEN = USB charger detection flow enable/disable.
CHG_EN = Battery charging enable/disable.
CHRDET_EXT = VBUS_IN is between VBUS_UV_TH(3.7V) and VBUS_OV_TH(10.5V)

> -- Sebastian