Hi,

On Mon, Jan 11, 2021 at 08:15:33PM +0800, Gene Chen wrote:

Sebastian Reichel [off-list ref] 於 2021年1月7日 週四 上午4:16寫道：

quoted

quoted

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

[...]
 

quoted

quoted

+     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;

[...]

quoted

quoted

+     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