Hi Amit,

On Mon, 2026-02-09 at 16:42 -0800, Amit Sunil Dhamne wrote:

Hi Andre',

On 2/4/26 4:49 AM, André Draszik wrote:

quoted

On Tue, 2026-02-03 at 22:50 +0000, Amit Sunil Dhamne via B4 Relay wrote:

quoted

+
+static void psy_work_item(struct work_struct *work)
+{
+	struct max77759_charger *chg =
+		container_of(work, struct max77759_charger, psy_work.work);
+	union power_supply_propval current_limit, online;
+	int ret;
+
+	ret = power_supply_get_property(chg->tcpm_psy,
+					POWER_SUPPLY_PROP_CURRENT_MAX,
+					&current_limit);
+	if (ret) {
+		dev_err(chg->dev,
+			"Failed to get CURRENT_MAX psy property, ret=%d",
+			ret);
+		goto err;
+	}
+
+	ret = power_supply_get_property(chg->tcpm_psy, POWER_SUPPLY_PROP_ONLINE,
+					&online);
+	if (ret) {
+		dev_err(chg->dev,
+			"Failed to get ONLINE psy property, ret=%d",
+			ret);
+		goto err;
+	}
+
+	if (online.intval && current_limit.intval) {
+		ret = set_input_current_limit(chg, current_limit.intval);
+		if (ret) {
+			dev_err(chg->dev,
+				"Unable to set current limit, ret=%d", ret);
+			goto err;
+		}
+
+		charger_set_mode(chg, MAX77759_CHGR_MODE_CHG_BUCK_ON);
+	} else {
+		charger_set_mode(chg, MAX77759_CHGR_MODE_OFF);
+	}
+
+	chg->psy_work_retry_cnt = 0;
+	return;
+
+err:
+	charger_set_mode(chg, MAX77759_CHGR_MODE_OFF);
+	if (chg->psy_work_retry_cnt >= MAX_NUM_RETRIES)
+		return;

I'd say this final giving up could benefit from a dev_err(), while ...

I want to clarify if you want me to add this final giving up print just 
once or every time I am returning early?

I meant something along the lines of this:

+	if (chg->psy_work_retry_cnt)
+		dev_dbg(chg->dev, "chg psy_work succeeded after %d\n",
+			chg->psy_work_retry_cnt)
+	chg->psy_work_retry_cnt = 0;
+	return;
+
+err:
+	charger_set_mode(chg, MAX77759_CHGR_MODE_OFF);
+	if (chg->psy_work_retry_cnt >= MAX_NUM_RETRIES) {
+		dev_warn(chg->dev, "chg psy_work failed, giving up",
+			 chg->psy_work_retry_cnt, MAX_NUM_RETRIES);
+		return;
+	}

quoted

quoted

+
+	++chg->psy_work_retry_cnt;
+	dev_err(chg->dev, "Retrying %u/%u chg psy_work",
+		chg->psy_work_retry_cnt, MAX_NUM_RETRIES);

... this one could be demoted (but doesn't have to).

That'd make it easier to determine if it's still in the process of
trying, or if it has given up fully.

I was assuming the printing of "3/3" would indicate the final giving up 
and sufficient.

If you see 3/3 in the log, you'll know that it has scheduled the work (for
the last attempt), but you won't easily know if 3/3 has ran yet or if it
has completed successfully this time or if it was still unsuccessful.

quoted

quoted

+	schedule_delayed_work(&chg->psy_work,
+			      msecs_to_jiffies(PSY_WORK_RETRY_DELAY_MS));
+}
+
+static int psy_changed(struct notifier_block *nb, unsigned long evt, void *data)
+{
+	struct max77759_charger *chg = container_of(nb, struct max77759_charger,
+						    nb);
+	static const char *psy_name = "tcpm-source";
+	struct power_supply *psy = data;
+
+	if (!strnstr(psy->desc->name, psy_name, strlen(psy_name)) ||
+	    evt != PSY_EVENT_PROP_CHANGED)
+		return NOTIFY_OK;
+
+	chg->tcpm_psy = psy;

Do you need locking here? What if this is changed while a previous
psy_work_item() is still executing?

I  don't think that's ever possible in this case though. The power 
supply that this driver registers is downstream of the tcpm's.

A previous work could still be executing (e.g. due to retrying), no? It
should also probably set chg->psy_work_retry_cnt = 0; here, to allow it
to retry up to MAX_NUM_RETRIES when a new work is queued and a previous
one was unsuccessful?




Cheers,
Andre