[PATCH v5 4/5] power: supply: max77759: add charger driver

[André Draszik]

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:
> > On Tue, 2026-02-03 at 22:50 +0000, Amit Sunil Dhamne via B4 Relay wrote:
> > 
> > > +
> > > +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;
+	}

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

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