Re: [PATCH v2] power: supply: qcom_battmgr: Add multi-port USB-C power supply support

[Konrad Dybcio]

On 8/6/26 5:06 PM, Kamal Wadhwa wrote:
> Extend the qcom_battmgr driver to report up to MAX_USB_PORTS (3)
> USB-C power supply ports on the X1E80100 & Glymur platform, which
> exposes more than one charger port to firmware.

[...]

>  #define BATTMGR_USB_PROPERTY_GET	0x32
>  #define BATTMGR_USB_PROPERTY_SET	0x33
> +#define BATTMGR_USB2_PROPERTY_GET	0xC0
> +#define BATTMGR_USB2_PROPERTY_SET	0xC1
> +#define BATTMGR_USB3_PROPERTY_GET	0xC2
> +#define BATTMGR_USB3_PROPERTY_SET	0xC3

nit: let's keep the defines lowercase

[...]


> +static int qcom_battmgr_usb_x1e80100_update(struct qcom_battmgr *battmgr,
> +					    enum power_supply_property psp)
> +{
> +	unsigned int prop;
> +	int ret;
> +
> +	if (psp >= ARRAY_SIZE(x1e80100_usb_prop_map))
> +		return -EINVAL;
> +
> +	prop = x1e80100_usb_prop_map[psp];
> +
> +	mutex_lock(&battmgr->lock);
> +	ret = qcom_battmgr_request_property(battmgr, BATTMGR_USB_PROPERTY_GET, prop, 0);
> +	mutex_unlock(&battmgr->lock);
> +
> +	return ret;
> +}

I'd rather you pass the array and its size to this function rather than
wholly duplicating it

[...]

> +static int qcom_battmgr_usb_x1e80100_get_property(struct power_supply *psy,
> +						   enum power_supply_property psp,
> +						   union power_supply_propval *val)

Likewise there's no reason to duplicate this one

[...]

> +static int qcom_battmgr_usb2_x1e80100_update(struct qcom_battmgr *battmgr,
> +					     enum power_supply_property psp)

We can add another parameter for the property and save some duplication
as well

[...]

> +static int qcom_battmgr_usb2_get_property(struct power_supply *psy,
> +					  enum power_supply_property psp,
> +					  union power_supply_propval *val)
> +{

This function is also wholly duplicated, we can extract out the common
part

[...]

>  static const enum power_supply_property sc8280xp_usb_props[] = {
>  	POWER_SUPPLY_PROP_ONLINE,
> +	POWER_SUPPLY_PROP_VOLTAGE_NOW,
> +	POWER_SUPPLY_PROP_VOLTAGE_MAX,
> +	POWER_SUPPLY_PROP_CURRENT_NOW,
> +	POWER_SUPPLY_PROP_CURRENT_MAX,
> +	POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
> +	POWER_SUPPLY_PROP_USB_TYPE,
>  };

Hm, no one noticed this list was very short for a long time..

[...]

> +	case BATTMGR_USB_PROPERTY_GET:
> +		property = le32_to_cpu(resp->intval.property);
> +		if (payload_len != sizeof(resp->intval)) {
> +			dev_warn(battmgr->dev,
> +				 "invalid payload length for %#x request: %zd\n",
> +				 property, payload_len);
> +			battmgr->error = -ENODATA;
> +			return;
> +		}

We can do something like:

if (opcode == BATTMGR_USB2_PROPERTY_GET)
	usb_info = &battmgr->usb2;
else if (opcode == BATTMGR_USB3_PROPERTY_GET)
	usb_info = &battmgr->usb3;
else
	usb_info = &battmgr->usb;

and then have a shared case for all 3 in the switch-statement

[...]

> +static char *qcom_battmgr_battery[] = { "battery" };
> +
>  static void qcom_battmgr_enable_worker(struct work_struct *work)
>  {
>  	struct qcom_battmgr *battmgr = container_of(work, struct qcom_battmgr, enable_work);
> @@ -1591,11 +1989,53 @@ static void qcom_battmgr_enable_worker(struct work_struct *work)
>  		.hdr.type = cpu_to_le32(PMIC_GLINK_NOTIFY),
>  		.hdr.opcode = cpu_to_le32(BATTMGR_REQUEST_NOTIFICATION),
>  	};
> +	struct power_supply *psy;
>  	int ret;
> +	int num_ports_fw = 0;
>  
>  	ret = qcom_battmgr_request(battmgr, &req, sizeof(req));
>  	if (ret)
>  		dev_err(battmgr->dev, "failed to request power notifications\n");
> +
> +	if (battmgr->variant == QCOM_BATTMGR_X1E80100) {
> +		mutex_lock(&battmgr->lock);
> +		ret = qcom_battmgr_request_property(battmgr, BATTMGR_USB_PROPERTY_GET,
> +						     USB_NUM_PORTS, 0);
> +		mutex_unlock(&battmgr->lock);
> +		if (ret < 0) {
> +			dev_dbg(battmgr->dev, "Failed to read USB_NUM_PORTS from SoCCP, rc=%d\n",
> +				ret);

The pmic-glink drivers are supposed not to care what's on the other end,
let's just drop the "from SoCCP" part.

Also, the existing way this driver prints errors is:

"A problem occured: %d\n", without a 'rc=' prefix

Konrad