Re: [PATCH v4 05/10] power: supply: max77693: Add USB extcon detection for enabling charging
From: Krzysztof Kozlowski <krzk@kernel.org>
Date: 2024-08-16 10:00:07
Also in:
linux-devicetree, linux-pm, linux-samsung-soc, lkml
On 16/08/2024 10:19, Artur Weber wrote:
1. Add a function that allows for enabling/disabling charging. 2. Add a device tree property, "maxim,usb-connector", that can be used to specify a USB connector to use to detect whether a charging cable has been plugged in/out, and enable/disable charging accordingly. The extcon listener/worker implementation is inspired by the rt5033_charger driver. Tested-by: Henrik Grimler <redacted> Signed-off-by: Artur Weber <aweber.kernel@gmail.com> --- Changes in v4: - Fix missing connector property causing probe deferrals
Thank you for your patch. There is something to discuss/improve.
+static void max77693_charger_extcon_work(struct work_struct *work)
+{
+ struct max77693_charger *chg = container_of(work, struct max77693_charger,
+ cable.work);
+ struct extcon_dev *edev = chg->cable.edev;
+ int connector, state;
+ int ret;
+
+ for (connector = EXTCON_USB_HOST; connector <= EXTCON_CHG_USB_PD;
+ connector++) {
+ state = extcon_get_state(edev, connector);
+ if (state == 1)
+ break;
+ }
+
+ switch (connector) {
+ case EXTCON_CHG_USB_SDP:
+ case EXTCON_CHG_USB_DCP:
+ case EXTCON_CHG_USB_CDP:
+ case EXTCON_CHG_USB_ACA:
+ case EXTCON_CHG_USB_FAST:
+ case EXTCON_CHG_USB_SLOW:
+ case EXTCON_CHG_USB_PD:
+ ret = max77693_set_charging(chg, true);
+ if (ret) {
+ dev_err(chg->dev, "failed to enable charging\n");
+ break;
+ }
+ dev_info(chg->dev, "charging. connector type: %d\n",
+ connector);This and next one should be also dev_dbg. It is completely normal condition, kind of expected thing to happen, so users do not need to be bugged every time they plugs cable.
quoted hunk ↗ jump to hunk
+ break; + default: + ret = max77693_set_charging(chg, false); + if (ret) { + dev_err(chg->dev, "failed to disable charging\n"); + break; + } + dev_info(chg->dev, "not charging. connector type: %d\n", + connector); + break; + } + + power_supply_changed(chg->charger); +} + +static int max77693_charger_extcon_notifier(struct notifier_block *nb, + unsigned long event, void *param) +{ + struct max77693_charger *chg = container_of(nb, struct max77693_charger, + cable.nb); + + schedule_work(&chg->cable.work); + + return NOTIFY_OK; +} + /* * Sets charger registers to proper and safe default values. */@@ -734,12 +814,34 @@ static int max77693_dt_init(struct device *dev, struct max77693_charger *chg) { struct device_node *np = dev->of_node; struct power_supply_battery_info *battery_info; + struct device_node *np_conn, *np_edev; if (!np) { dev_err(dev, "no charger OF node\n"); return -EINVAL; } + np_conn = of_parse_phandle(np, "maxim,usb-connector", 0);
Where is the reference dropped?
+ if (np_conn) {
+ np_edev = of_get_parent(np_conn);Same question
+ + chg->cable.edev = extcon_find_edev_by_node(np_edev);
You probably need device_link_add() as well. I don't think above extcon code cares about it.
+ if (IS_ERR(chg->cable.edev)) {
+ /*
+ * In case of deferred extcon probe, defer our probe as well
+ * until it appears.
+ */
+ if (PTR_ERR(chg->cable.edev) == -EPROBE_DEFER)
+ return PTR_ERR(chg->cable.edev);
+ /*
+ * Otherwise, ignore errors (the charger can run without a
+ * connector provided).
+ */
+ dev_warn(dev, "no extcon device found in device-tree (%ld)\n",
+ PTR_ERR(chg->cable.edev));
+ }
+ }Best regards, Krzysztof