[PATCH 0/2] power: supply: max8903: add GPIO-controlled input current-limit

[PATCH 0/2] power: supply: max8903: add GPIO-controlled input current-limit

[Herman van Hazendonk]

The MAX8903 charger exposes two hardware pins for input current control:
IDC (DC path, pin 11) and IUSB (USB path, pin 7). On many boards these
are driven by GPIO lines to a resistor mux or logic selector, letting
software select the input current limit. The driver currently has no
mechanism to set POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT at all.

This series adds that capability via two optional DT property groups:

  DC path: "dc-current-limit-gpios" + "dc-current-limit-mapping"
  A 1..4-GPIO array drives a resistor mux connected to the MAX8903 IDC
  pin. The IDC pin programs the step-down DC input current limit from
  0.5 A to 2 A (R_IDC = 6000V/I_DC_MAX) when the DCM mode pin is
  logic-high. The mapping table (uint32-matrix of {microamps,
  gpio_bit_pattern} pairs) lists every step the board supports. The
  driver picks the largest step at or below the requested limit.

  USB path: "usb-current-limit-gpios" + "usb-current-limit-values"
  A single GPIO drives the MAX8903 IUSB pin. Logic-low selects 100 mA,
  logic-high selects 500 mA per the MAX8903 spec. The two microamp
  values are described in DT so boards with non-standard IUSB thresholds
  can override them. Requests below the lower value are rejected with
  -EINVAL rather than silently programming a higher-than-requested limit.

The dispatch between the two paths is guarded by a new mutex (source_lock)
that is also taken in the IRQ handlers that update the "source online"
flags, preventing a race between set_property and a concurrent DOK/UOK
IRQ flip. The IRQs are already requested IRQF_ONESHOT so the sleepable
mutex is the appropriate primitive.

Motivation: the HP TouchPad carries a MAX8903B charger with two TLMM GPIO
lines feeding a four-step IDC resistor mux (500 mA, 1 A, 1.5 A, 2 A DC
input current limit) and a third TLMM line driving IUSB for the 100 mA /
500 mA USB limit. Without this series, the charger always draws its
hardware-default current irrespective of what the USB host negotiated.

Both patches are purely additive; platforms that don't provide the new DT
properties continue to behave exactly as before.

Herman van Hazendonk (2):
  dt-bindings: power: supply: maxim,max8903: add DC and USB input
    current-limit controls
  power: supply: max8903: add DC and USB input current-limit GPIO
    controls

 .../bindings/power/supply/maxim,max8903.yaml  |  58 +++
 drivers/power/supply/max8903_charger.c        | 369 +++++++++++++++++-
 2 files changed, 416 insertions(+), 11 deletions(-)

-- 
2.43.0

[PATCH 1/2] dt-bindings: power: supply: maxim,max8903: add DC and USB input current-limit controls

[Herman van Hazendonk]

Add four optional properties to the MAX8903 charger binding to
describe board-level GPIO control of the DC and USB input current
limits:

DC input (TA / DOK pin):
  - dc-current-limit-gpios (1..4 GPIOs): mux control lines feeding
    the MAX8903 IDC resistor mux;
  - dc-current-limit-mapping (uint32-matrix of {microamps,
    gpio_bit_pattern} pairs): the available current levels and the
    GPIO bit pattern that selects each level.

USB input (USB / UOK pin):
  - usb-current-limit-gpios: a single GPIO driving the IUSB
    tri-state pin (low / high);
  - usb-current-limit-values: the two microamp values that the IUSB
    pin selects.

These let userspace clamp the input draw via the standard
POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT power_supply attribute. The HP
TouchPad uses both: two TLMM lines select between 0.5 A, 1.0 A,
1.5 A and 2.0 A DC input current limits behind the MAX8903B
charger, and a third TLMM line picks the IUSB 100 mA / 500 mA
limit.

These are purely additive; existing platforms remain unaffected.

Signed-off-by: Herman van Hazendonk <github.com@herrie.org>
---
 .../bindings/power/supply/maxim,max8903.yaml  | 58 +++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/Documentation/devicetree/bindings/power/supply/maxim,max8903.yaml b/Documentation/devicetree/bindings/power/supply/maxim,max8903.yaml
index 86af38378999..5e970ebc08df 100644
--- a/Documentation/devicetree/bindings/power/supply/maxim,max8903.yaml
+++ b/Documentation/devicetree/bindings/power/supply/maxim,max8903.yaml
@@ -44,6 +44,41 @@ properties:
     maxItems: 1
     description: USB suspend pin (active high, output)
 
+  dc-current-limit-gpios:
+    minItems: 1
+    maxItems: 4
+    description:
+      GPIOs controlling DC input current limit via resistor mux.
+      Used with dc-current-limit-mapping to select charging current.
+
+  dc-current-limit-mapping:
+    $ref: /schemas/types.yaml#/definitions/uint32-matrix
+    minItems: 2
+    maxItems: 16
+    description: |
+      Array of (current_microamps, gpio_bit_pattern) pairs defining available
+      DC current limits. The gpio_bit_pattern is applied to dc-current-limit-gpios
+      to select that current level.
+    items:
+      items:
+        - description: Current limit in microamps
+        - description: GPIO bit pattern value
+
+  usb-current-limit-gpios:
+    maxItems: 1
+    description:
+      GPIO controlling USB input current limit.
+      Low = usb-current-limit-values[0], High = usb-current-limit-values[1].
+
+  usb-current-limit-values:
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    minItems: 2
+    maxItems: 2
+    default: [100000, 500000]
+    description:
+      USB current limits in microamps for GPIO low and high states.
+      Default is 100mA (low) and 500mA (high) per USB specification.
+
 required:
   - compatible
 
@@ -65,3 +100,26 @@ examples:
       chg-gpios = <&gpio3 15 GPIO_ACTIVE_LOW>;
       cen-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>;
     };
+  - |
+    /* Example with DC and USB current limit control */
+    #include <dt-bindings/gpio/gpio.h>
+    charger-with-current-limit {
+      compatible = "maxim,max8903";
+      dok-gpios = <&gpio2 3 GPIO_ACTIVE_LOW>;
+      flt-gpios = <&gpio2 2 GPIO_ACTIVE_LOW>;
+      chg-gpios = <&gpio3 15 GPIO_ACTIVE_LOW>;
+      cen-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>;
+      dcm-gpios = <&gpio2 6 GPIO_ACTIVE_HIGH>;
+
+      /* DC input current limit via IDC resistor mux */
+      dc-current-limit-gpios = <&gpio1 0 GPIO_ACTIVE_HIGH>,
+                               <&gpio1 1 GPIO_ACTIVE_HIGH>;
+      dc-current-limit-mapping = <750000 0>,  /* GPIO[1:0]=0b00 */
+                                 <900000 1>,  /* GPIO[1:0]=0b01 */
+                                 <1400000 3>, /* GPIO[1:0]=0b11 */
+                                 <2000000 2>; /* GPIO[1:0]=0b10 */
+
+      /* USB current control */
+      usb-current-limit-gpios = <&gpio1 2 GPIO_ACTIVE_HIGH>;
+      usb-current-limit-values = <100000 500000>; /* 100mA / 500mA */
+    };
-- 
2.43.0

[PATCH 2/2] power: supply: max8903: add DC and USB input current-limit GPIO controls

[Herman van Hazendonk]

Add two optional current-limit knobs surfaced through
POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT, selected by which input source
is currently online:

  - DC (DOK / TA-IN): a "dc-current-limit-gpios" array drives the
    GPIOs of an external resistor mux connected to the MAX8903 IDC pin
    (DC Current-Limit Set Input, pin 11). The IDC pin programs the
    step-down DC input current limit from 0.5 A to 2 A via R_IDC when
    the DCM mode pin is logic-high. The DT property
    "dc-current-limit-mapping" describes the (current_ua, gpio_value)
    pairs the board can program; the driver picks the largest entry
    whose limit is <= the requested limit. A 0 uA entry, used to
    stop drawing DC current, is selectable by issuing a 0 uA request
    (the selection uses a -1 "not found" sentinel rather than
    tracking best_limit > 0, so the all-zero entry can win).

  - USB (UOK / USB-IN): a single "usb-current-limit-gpio" drives the
    MAX8903 IUSB pin (USB Current-Limit Set Input, pin 7). Logic-low
    selects 100 mA; logic-high selects 500 mA per the MAX8903 spec.
    The DT property "usb-current-limit-values" overrides those limits
    for boards with non-standard IUSB thresholds. The requested limit
    selects the highest of the two configured values that does not
    exceed the cap: requests at or above the high value pick high,
    requests at or above the low value pick low, and a request below
    the low value is rejected with -EINVAL rather than silently
    programming a higher current that would violate the system power
    budget.

The dispatch in max8903_set_property() to the DC vs USB path needs to
match the active source flag set by the corresponding *_ok GPIO IRQ
handler; both update sites take a new struct mutex source_lock so the
check and the resulting hardware write cannot be torn by a concurrent
IRQ flipping the source-online flag mid-decision. The IRQs are
requested with IRQF_ONESHOT (threaded), so a sleepable lock is the
right primitive in both contexts. max8903_get_property() also takes
source_lock briefly to snapshot the source flags and current-limit
values so userspace never observes a torn pair of
(source-online flag, current-limit ua).

dc-current-limit-mapping gpio_value entries are validated at parse
against the GPIO array width so a malformed DT value is rejected
instead of being silently truncated by gpiod_set_array_value() and
selecting the wrong mux level. The mapping is additionally required
to contain a gpio_value=0 entry: devm_gpiod_get_array_optional()
asks for GPIOD_OUT_LOW, so the hardware mux starts at gpio_value 0,
and the driver seeds dc_current_limit_ua from the matching map entry.
A DT lacking the all-zero entry is rejected with -EINVAL because
otherwise the reported INPUT_CURRENT_LIMIT could disagree with the
mux state until a set_property write picks a real value.

Signed-off-by: Herman van Hazendonk <github.com@herrie.org>
---
 drivers/power/supply/max8903_charger.c | 369 ++++++++++++++++++++++++-
 1 file changed, 358 insertions(+), 11 deletions(-)

diff --git a/drivers/power/supply/max8903_charger.c b/drivers/power/supply/max8903_charger.c
index 45fbaad6c647..19c2b348a045 100644
--- a/drivers/power/supply/max8903_charger.c
+++ b/drivers/power/supply/max8903_charger.c
@@ -9,11 +9,18 @@
 #include <linux/gpio/consumer.h>
 #include <linux/interrupt.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/of.h>
+#include <linux/property.h>
 #include <linux/slab.h>
 #include <linux/power_supply.h>
 #include <linux/platform_device.h>
 
+struct max8903_current_limit_mapping {
+	u32 limit_ua;		/* Current limit in microamps */
+	u32 gpio_value;		/* GPIO bit pattern */
+};
+
 struct max8903_data {
 	struct device *dev;
 	struct power_supply *psy;
@@ -31,6 +38,27 @@ struct max8903_data {
 	struct gpio_desc *flt; /* Fault output */
 	struct gpio_desc *dcm; /* Current-Limit Mode input (1: DC, 2: USB) */
 	struct gpio_desc *usus; /* USB Suspend Input (1: suspended) */
+
+	/* DC current limit control (ISET pins) */
+	struct gpio_descs *dc_current_limit_gpios;
+	struct max8903_current_limit_mapping *dc_current_limit_map;
+	u32 dc_current_limit_map_size;
+	u32 dc_current_limit_ua;	/* Current setting in uA */
+
+	/* USB current limit control (IUSB pin) */
+	struct gpio_desc *usb_current_limit_gpio;
+	u32 usb_current_limit_low_ua;	/* Current when GPIO low */
+	u32 usb_current_limit_high_ua;	/* Current when GPIO high */
+	u32 usb_current_limit_ua;	/* Current setting in uA */
+
+	/*
+	 * Serialises ta_in / usb_in updates against
+	 * max8903_set_property() which steers the current-limit write to
+	 * the DC or USB path based on which source is currently online.
+	 * The IRQ handlers are requested with IRQF_ONESHOT (threaded), so
+	 * a sleepable mutex is the right primitive in both contexts.
+	 */
+	struct mutex source_lock;
 	bool fault;
 	bool usb_in;
 	bool ta_in;
@@ -40,6 +68,7 @@ static enum power_supply_property max8903_charger_props[] = {
 	POWER_SUPPLY_PROP_STATUS, /* Charger status output */
 	POWER_SUPPLY_PROP_ONLINE, /* External power source */
 	POWER_SUPPLY_PROP_HEALTH, /* Fault or OK */
+	POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT, /* Input current limit */
 };
 
 static int max8903_get_property(struct power_supply *psy,
@@ -47,6 +76,24 @@ static int max8903_get_property(struct power_supply *psy,
 		union power_supply_propval *val)
 {
 	struct max8903_data *data = power_supply_get_drvdata(psy);
+	bool ta_in, usb_in;
+	u32 dc_limit, usb_limit;
+
+	/*
+	 * Snapshot the source flags and current-limit settings under the
+	 * source_lock that the IRQs (max8903_dcin / max8903_usbin) and
+	 * max8903_set_property() take when updating them, so we never
+	 * observe a torn pair of (source-online flag, current-limit ua).
+	 * The gpiod_get_value() reads further down deliberately stay
+	 * outside the lock — they hit the GPIO controller, not driver
+	 * state, and the IRQs do not touch them under the lock either.
+	 */
+	mutex_lock(&data->source_lock);
+	ta_in = data->ta_in;
+	usb_in = data->usb_in;
+	dc_limit = data->dc_current_limit_ua;
+	usb_limit = data->usb_current_limit_ua;
+	mutex_unlock(&data->source_lock);
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_STATUS:
@@ -55,21 +102,31 @@ static int max8903_get_property(struct power_supply *psy,
 			if (gpiod_get_value(data->chg))
 				/* CHG asserted */
 				val->intval = POWER_SUPPLY_STATUS_CHARGING;
-			else if (data->usb_in || data->ta_in)
+			else if (usb_in || ta_in)
 				val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
 			else
 				val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
 		}
 		break;
 	case POWER_SUPPLY_PROP_ONLINE:
-		val->intval = 0;
-		if (data->usb_in || data->ta_in)
-			val->intval = 1;
+		val->intval = (ta_in || usb_in) ? 1 : 0;
 		break;
 	case POWER_SUPPLY_PROP_HEALTH:
-		val->intval = POWER_SUPPLY_HEALTH_GOOD;
-		if (data->fault)
-			val->intval = POWER_SUPPLY_HEALTH_UNSPEC_FAILURE;
+		/*
+		 * data->fault is a single bool toggled from one IRQ
+		 * handler, so a torn read is not possible; no need to
+		 * extend source_lock coverage here.
+		 */
+		val->intval = data->fault ? POWER_SUPPLY_HEALTH_UNSPEC_FAILURE
+					  : POWER_SUPPLY_HEALTH_GOOD;
+		break;
+	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
+		if (ta_in && data->dc_current_limit_gpios)
+			val->intval = dc_limit;
+		else if (usb_in && data->usb_current_limit_gpio)
+			val->intval = usb_limit;
+		else
+			return -ENODATA;
 		break;
 	default:
 		return -EINVAL;
@@ -78,6 +135,130 @@ static int max8903_get_property(struct power_supply *psy,
 	return 0;
 }
 
+static int max8903_set_dc_current_limit(struct max8903_data *data, u32 limit_ua)
+{
+	int i, best_idx = -1;
+	u32 best_gpio_value;
+	/*
+	 * gpio_value is a u32 in the DT mapping and is parse-time
+	 * validated to fit in BIT(ndescs); size the bitmap to the full
+	 * width of the source u32 so a DT with up to 32 dc-current-limit
+	 * GPIOs cannot overflow this stack buffer.
+	 */
+	DECLARE_BITMAP(values, BITS_PER_TYPE(u32));
+
+	if (!data->dc_current_limit_gpios)
+		return -EOPNOTSUPP;
+
+	/*
+	 * Find the highest supported current <= requested. Use a -1
+	 * "not found" sentinel rather than tracking best_limit > 0 so
+	 * that a 0 uA entry (used to disable charging) can be selected
+	 * by a 0 uA request.
+	 */
+	for (i = 0; i < data->dc_current_limit_map_size; i++) {
+		if (data->dc_current_limit_map[i].limit_ua > limit_ua)
+			continue;
+		if (best_idx < 0 ||
+		    data->dc_current_limit_map[i].limit_ua >
+				data->dc_current_limit_map[best_idx].limit_ua)
+			best_idx = i;
+	}
+
+	if (best_idx < 0)
+		return -EINVAL;
+
+	best_gpio_value = data->dc_current_limit_map[best_idx].gpio_value;
+	bitmap_from_arr32(values, &best_gpio_value, BITS_PER_TYPE(u32));
+	gpiod_set_array_value_cansleep(data->dc_current_limit_gpios->ndescs,
+				       data->dc_current_limit_gpios->desc,
+				       data->dc_current_limit_gpios->info,
+				       values);
+
+	data->dc_current_limit_ua = data->dc_current_limit_map[best_idx].limit_ua;
+	dev_dbg(data->dev, "DC current limit set to %u uA\n",
+		data->dc_current_limit_ua);
+
+	return 0;
+}
+
+static int max8903_set_usb_current_limit(struct max8903_data *data, u32 limit_ua)
+{
+	u32 selected;
+	int gpio_val;
+
+	if (!data->usb_current_limit_gpio)
+		return -EOPNOTSUPP;
+
+	/*
+	 * Pick the highest of the two configured limits that does not
+	 * exceed the requested cap. Mirror the DC path's policy: if
+	 * neither value fits (the request is below even the low limit),
+	 * refuse the request rather than silently program a higher
+	 * current that violates the system power budget.
+	 */
+	if (limit_ua >= data->usb_current_limit_high_ua) {
+		selected = data->usb_current_limit_high_ua;
+		gpio_val = 1;
+	} else if (limit_ua >= data->usb_current_limit_low_ua) {
+		selected = data->usb_current_limit_low_ua;
+		gpio_val = 0;
+	} else {
+		return -EINVAL;
+	}
+
+	gpiod_set_value_cansleep(data->usb_current_limit_gpio, gpio_val);
+	data->usb_current_limit_ua = selected;
+
+	dev_dbg(data->dev, "USB current limit set to %u uA\n",
+		data->usb_current_limit_ua);
+
+	return 0;
+}
+
+static int max8903_set_property(struct power_supply *psy,
+		enum power_supply_property psp,
+		const union power_supply_propval *val)
+{
+	struct max8903_data *data = power_supply_get_drvdata(psy);
+	int ret;
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
+		/*
+		 * Hold source_lock across the source check and the
+		 * resulting hardware write so the IRQ handler cannot
+		 * flip ta_in/usb_in between them and have us program the
+		 * limit for a source that has just gone offline.
+		 */
+		mutex_lock(&data->source_lock);
+		if (data->ta_in && data->dc_current_limit_gpios)
+			ret = max8903_set_dc_current_limit(data, val->intval);
+		else if (data->usb_in && data->usb_current_limit_gpio)
+			ret = max8903_set_usb_current_limit(data, val->intval);
+		else
+			ret = -EINVAL;
+		mutex_unlock(&data->source_lock);
+		return ret;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int max8903_property_is_writeable(struct power_supply *psy,
+		enum power_supply_property psp)
+{
+	struct max8903_data *data = power_supply_get_drvdata(psy);
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
+		return data->dc_current_limit_gpios ||
+		       data->usb_current_limit_gpio;
+	default:
+		return 0;
+	}
+}
+
 static irqreturn_t max8903_dcin(int irq, void *_data)
 {
 	struct max8903_data *data = _data;
@@ -91,12 +272,21 @@ static irqreturn_t max8903_dcin(int irq, void *_data)
 	 * library as the line should be flagged GPIO_ACTIVE_LOW in the device
 	 * tree.
 	 */
+	/*
+	 * Sample the line under source_lock so a concurrent
+	 * max8903_set_property() observes either the old or the new
+	 * state consistently, never a torn read where the lock is held
+	 * but the cached flag is about to be updated.
+	 */
+	mutex_lock(&data->source_lock);
 	ta_in = gpiod_get_value(data->dok);
-
-	if (ta_in == data->ta_in)
+	if (ta_in == data->ta_in) {
+		mutex_unlock(&data->source_lock);
 		return IRQ_HANDLED;
+	}
 
 	data->ta_in = ta_in;
+	mutex_unlock(&data->source_lock);
 
 	/* Set Current-Limit-Mode 1:DC 0:USB */
 	if (data->dcm)
@@ -150,12 +340,16 @@ static irqreturn_t max8903_usbin(int irq, void *_data)
 	 * library as the line should be flagged GPIO_ACTIVE_LOW in the device
 	 * tree.
 	 */
+	/* See ta_in handler: sample the line under the lock. */
+	mutex_lock(&data->source_lock);
 	usb_in = gpiod_get_value(data->uok);
-
-	if (usb_in == data->usb_in)
+	if (usb_in == data->usb_in) {
+		mutex_unlock(&data->source_lock);
 		return IRQ_HANDLED;
+	}
 
 	data->usb_in = usb_in;
+	mutex_unlock(&data->source_lock);
 
 	/* Do not touch Current-Limit-Mode */
 
@@ -221,6 +415,148 @@ static irqreturn_t max8903_fault(int irq, void *_data)
 	return IRQ_HANDLED;
 }
 
+static int max8903_parse_dc_current_limit(struct platform_device *pdev,
+					  struct max8903_data *data)
+{
+	struct device *dev = &pdev->dev;
+	int ret, i, map_size;
+	u32 *map;
+
+	data->dc_current_limit_gpios = devm_gpiod_get_array_optional(dev,
+					"dc-current-limit", GPIOD_OUT_LOW);
+	if (IS_ERR(data->dc_current_limit_gpios))
+		return dev_err_probe(dev, PTR_ERR(data->dc_current_limit_gpios),
+				     "failed to get DC current limit GPIOs");
+
+	if (!data->dc_current_limit_gpios)
+		return 0;	/* Optional feature not present */
+
+	/* Parse mapping: pairs of (current_ua, gpio_value) */
+	map_size = device_property_count_u32(dev, "dc-current-limit-mapping");
+	if (map_size <= 0 || map_size % 2) {
+		dev_err(dev, "invalid dc-current-limit-mapping\n");
+		return -EINVAL;
+	}
+
+	map = devm_kcalloc(dev, map_size, sizeof(*map), GFP_KERNEL);
+	if (!map)
+		return -ENOMEM;
+
+	ret = device_property_read_u32_array(dev, "dc-current-limit-mapping",
+					     map, map_size);
+	if (ret) {
+		dev_err(dev, "failed to read dc-current-limit-mapping\n");
+		return ret;
+	}
+
+	data->dc_current_limit_map_size = map_size / 2;
+	data->dc_current_limit_map = devm_kcalloc(dev,
+					data->dc_current_limit_map_size,
+					sizeof(*data->dc_current_limit_map),
+					GFP_KERNEL);
+	if (!data->dc_current_limit_map)
+		return -ENOMEM;
+
+	for (i = 0; i < data->dc_current_limit_map_size; i++) {
+		u32 gpio_value = map[i * 2 + 1];
+
+		/*
+		 * gpio_value is the bitmap programmed across the
+		 * dc-current-limit GPIOs, so it cannot represent more
+		 * bits than the GPIO array width. A larger value would
+		 * be silently truncated by gpiod_set_array_value() and
+		 * select the wrong limit; reject it at parse time so
+		 * the bogus DT is visible to the integrator.
+		 */
+		if (gpio_value >= BIT(data->dc_current_limit_gpios->ndescs)) {
+			dev_err(dev,
+				"dc-current-limit-mapping entry %d: gpio_value 0x%x exceeds %u-GPIO range\n",
+				i, gpio_value,
+				data->dc_current_limit_gpios->ndescs);
+			return -EINVAL;
+		}
+		data->dc_current_limit_map[i].limit_ua = map[i * 2];
+		data->dc_current_limit_map[i].gpio_value = gpio_value;
+	}
+
+	/*
+	 * devm_gpiod_get_array_optional() above asked for GPIOD_OUT_LOW,
+	 * so the hardware mux starts at gpio_value 0. Require the DT
+	 * mapping to include a gpio_value=0 entry so the software
+	 * current-limit state has a definite initial value matching the
+	 * hardware. Without this entry we would have to guess and the
+	 * reported INPUT_CURRENT_LIMIT could disagree with what the
+	 * mux is actually wired to until a set_property write picks a
+	 * real value.
+	 */
+	for (i = 0; i < data->dc_current_limit_map_size; i++)
+		if (data->dc_current_limit_map[i].gpio_value == 0)
+			break;
+	if (i == data->dc_current_limit_map_size) {
+		dev_err(dev,
+			"dc-current-limit-mapping must include a gpio_value=0 entry to describe the boot-time mux state\n");
+		return -EINVAL;
+	}
+	data->dc_current_limit_ua = data->dc_current_limit_map[i].limit_ua;
+
+	dev_dbg(dev, "DC current limit control: %d levels available, initial %u uA\n",
+		data->dc_current_limit_map_size, data->dc_current_limit_ua);
+
+	return 0;
+}
+
+static int max8903_parse_usb_current_limit(struct platform_device *pdev,
+					   struct max8903_data *data)
+{
+	struct device *dev = &pdev->dev;
+	u32 limits[2];
+	int ret;
+
+	data->usb_current_limit_gpio = devm_gpiod_get_optional(dev,
+					"usb-current-limit", GPIOD_OUT_LOW);
+	if (IS_ERR(data->usb_current_limit_gpio))
+		return dev_err_probe(dev, PTR_ERR(data->usb_current_limit_gpio),
+				     "failed to get USB current limit GPIO");
+
+	if (!data->usb_current_limit_gpio)
+		return 0;	/* Optional feature not present */
+
+	/* Parse [low_ua, high_ua] values, default to USB spec values */
+	ret = device_property_read_u32_array(dev, "usb-current-limit-values",
+					     limits, 2);
+	if (ret) {
+		/* Default to USB spec values */
+		data->usb_current_limit_low_ua = 100000;   /* 100mA */
+		data->usb_current_limit_high_ua = 500000;  /* 500mA */
+	} else {
+		data->usb_current_limit_low_ua = limits[0];
+		data->usb_current_limit_high_ua = limits[1];
+	}
+
+	/*
+	 * max8903_set_usb_current_limit() picks the highest cap that
+	 * doesn't exceed the request by checking >=high first then
+	 * >=low; that policy only works when high > low. Reject DTs
+	 * that hand the property in the wrong order rather than
+	 * silently program a sub-optimal current limit.
+	 */
+	if (data->usb_current_limit_high_ua <= data->usb_current_limit_low_ua) {
+		dev_err(dev,
+			"usb-current-limit-values must be [low, high] with high > low (got low=%u uA, high=%u uA)\n",
+			data->usb_current_limit_low_ua,
+			data->usb_current_limit_high_ua);
+		return -EINVAL;
+	}
+
+	/* Start at low current for safety */
+	data->usb_current_limit_ua = data->usb_current_limit_low_ua;
+
+	dev_dbg(dev, "USB current limit control: %u/%u uA\n",
+		data->usb_current_limit_low_ua, data->usb_current_limit_high_ua);
+
+	return 0;
+}
+
 static int max8903_setup_gpios(struct platform_device *pdev)
 {
 	struct max8903_data *data = platform_get_drvdata(pdev);
@@ -335,17 +671,28 @@ static int max8903_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	data->dev = dev;
+	mutex_init(&data->source_lock);
 	platform_set_drvdata(pdev, data);
 
 	ret = max8903_setup_gpios(pdev);
 	if (ret)
 		return ret;
 
+	ret = max8903_parse_dc_current_limit(pdev, data);
+	if (ret)
+		return ret;
+
+	ret = max8903_parse_usb_current_limit(pdev, data);
+	if (ret)
+		return ret;
+
 	data->psy_desc.name = "max8903_charger";
 	data->psy_desc.type = (data->ta_in) ? POWER_SUPPLY_TYPE_MAINS :
 			((data->usb_in) ? POWER_SUPPLY_TYPE_USB :
 			 POWER_SUPPLY_TYPE_BATTERY);
 	data->psy_desc.get_property = max8903_get_property;
+	data->psy_desc.set_property = max8903_set_property;
+	data->psy_desc.property_is_writeable = max8903_property_is_writeable;
 	data->psy_desc.properties = max8903_charger_props;
 	data->psy_desc.num_properties = ARRAY_SIZE(max8903_charger_props);
 
-- 
2.43.0

Re: [PATCH 1/2] dt-bindings: power: supply: maxim,max8903: add DC and USB input current-limit controls

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The schema does not enforce mutual dependencies for functionally coupled properties (`dc-current-limit-gpios` with `dc-current-limit-mapping`, and `usb-current-limit-gpios` with `usb-current-limit-values`).
- [Low] The property `usb-current-limit-values` violates the standard devicetree unit suffix naming convention.
- [Low] The device tree node name in the added example violates standard Devicetree naming conventions.
--

commit 2e0e545e4d1ddb26950bce515555057419610f75
Author: Herman van Hazendonk <github.com@herrie.org>

dt-bindings: power: supply: maxim,max8903: add DC and USB input current-limit controls

Add four optional properties to the MAX8903 charger binding to describe
board-level GPIO control of the DC and USB input current limits.

> diff --git a/Documentation/devicetree/bindings/power/supply/maxim,max8903.yaml b/Documentation/devicetree/bindings/power/supply/maxim,max8903.yaml
> index 86af383789992..5e970ebc08df6 100644
> --- a/Documentation/devicetree/bindings/power/supply/maxim,max8903.yaml
> +++ b/Documentation/devicetree/bindings/power/supply/maxim,max8903.yaml
> @@ -44,6 +44,41 @@ properties:
>      maxItems: 1
>      description: USB suspend pin (active high, output)
>  
> +  dc-current-limit-gpios:
> +    minItems: 1
> +    maxItems: 4

[Severity: Medium]
Should the schema enforce mutual dependencies between these new properties at
the root level?

Defining a GPIO mux control without the mapping of bit patterns to current
limits is invalid, and conversely, providing a mapping without the GPIOs to
control it has no effect. Similarly, specifying usb-current-limit-values
without usb-current-limit-gpios doesn't seem meaningful.

Without a 'dependencies' or 'dependentRequired' block at the root of the
schema, would incomplete devicetree configurations silently pass
dt_binding_check validation?

> +    description:
> +      GPIOs controlling DC input current limit via resistor mux.
> +      Used with dc-current-limit-mapping to select charging current.
> +
> +  dc-current-limit-mapping:
> +    $ref: /schemas/types.yaml#/definitions/uint32-matrix

[ ... ]

> +  usb-current-limit-gpios:
> +    maxItems: 1
> +    description:
> +      GPIO controlling USB input current limit.
> +      Low = usb-current-limit-values[0], High = usb-current-limit-values[1].
> +
> +  usb-current-limit-values:
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    minItems: 2
> +    maxItems: 2
> +    default: [100000, 500000]

[Severity: Low]
Does this property name violate the standard devicetree unit suffix naming
conventions?

Properties that express a microamp value should generally end with the
-microamp suffix (e.g., usb-current-limit-microamp) to allow automatic unit
type validation by the dtschema tooling.

> +    description:
> +      USB current limits in microamps for GPIO low and high states.
> +      Default is 100mA (low) and 500mA (high) per USB specification.
> +
>  required:
>    - compatible

[ ... ]

> @@ -65,3 +100,26 @@ examples:
>        chg-gpios = <&gpio3 15 GPIO_ACTIVE_LOW>;
>        cen-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>;
>      };
> +  - |
> +    /* Example with DC and USB current limit control */
> +    #include <dt-bindings/gpio/gpio.h>
> +    charger-with-current-limit {
> +      compatible = "maxim,max8903";

[Severity: Low]
Could this trigger dtc compiler warnings or dt-schema validation errors when
generic node name patterns are strictly enforced?

The Devicetree Specification recommends node names be generic and reflect the
function of the device. Should this be renamed to a generic name like
'charger'?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603071519.807604-1-github.com@herrie.org?part=1

Re: [PATCH 2/2] power: supply: max8903: add DC and USB input current-limit GPIO controls

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

New issues:
- [High] Stack out-of-bounds read and endianness corruption due to `bitmap_from_arr32` with a 4-byte `u32`.
- [High] Missing upper-bound validation on `dc-current-limit-gpios` array length leads to out-of-bounds read and undefined behavior.
- [Medium] Incorrect source selection in get/set properties when DC is active but only USB has a configured GPIO.
- [Medium] Negative current limit requests bypass the minimum bounds check due to a signedness bug.
- [Low] Memory leak of temporary parsing buffer `map` due to devm_kcalloc.

Pre-existing issues:
- [High] TOCTOU data race between IRQ handlers due to prematurely dropped `source_lock`.
--

commit e36685a3d0ef54240804ea29cb948b482e9cc91e
Author: Herman van Hazendonk <github.com@herrie.org>

power: supply: max8903: add DC and USB input current-limit GPIO controls
    
This commit adds optional current-limit controls for both DC and USB power
inputs via the POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT property. It parses
device tree nodes for GPIO values and limits, then dynamically steers
requests to the currently active source (DC or USB) while introducing a
mutex to serialize source state changes.

> diff --git a/drivers/power/supply/max8903_charger.c b/drivers/power/supply/max8903_charger.c
> index 45fbaad6c6470..19c2b348a0456 100644
> --- a/drivers/power/supply/max8903_charger.c
> +++ b/drivers/power/supply/max8903_charger.c

[ ... ]

> @@ -47,6 +76,24 @@ static int max8903_get_property(struct power_supply *psy,
>  		union power_supply_propval *val)
>  {

[ ... ]

> +	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
> +		if (ta_in && data->dc_current_limit_gpios)
> +			val->intval = dc_limit;
> +		else if (usb_in && data->usb_current_limit_gpio)
> +			val->intval = usb_limit;
> +		else
> +			return -ENODATA;
> +		break;

[Severity: Medium]
If both sources are active (where hardware prioritizes DC) but the device
tree only configures USB GPIOs, won't this fall through to the USB branch?

This could result in reporting the inactive USB limit while the hardware
continues drawing unmanaged power from the DC input. Should the code reject
requests for the active source if its GPIOs are missing instead of falling
back to the inactive source?

[ ... ]

> +static int max8903_set_dc_current_limit(struct max8903_data *data, u32 limit_ua)
> +{
> +	int i, best_idx = -1;
> +	u32 best_gpio_value;
> +	/*
> +	 * gpio_value is a u32 in the DT mapping and is parse-time
> +	 * validated to fit in BIT(ndescs); size the bitmap to the full
> +	 * width of the source u32 so a DT with up to 32 dc-current-limit
> +	 * GPIOs cannot overflow this stack buffer.
> +	 */
> +	DECLARE_BITMAP(values, BITS_PER_TYPE(u32));

[ ... ]

> +	best_gpio_value = data->dc_current_limit_map[best_idx].gpio_value;
> +	bitmap_from_arr32(values, &best_gpio_value, BITS_PER_TYPE(u32));

[Severity: High]
Can this cause a stack out-of-bounds read and endianness corruption?

The bitmap_from_arr32() macro casts the 4-byte pointer to best_gpio_value
to an 8-byte unsigned long pointer on 64-bit systems. This would pull
4 bytes of adjacent uninitialized stack memory into the values bitmap.

On big-endian 64-bit systems, this could incorrectly interpret the value,
placing the intended value in the upper 32 bits and garbage in the lower.

> +	gpiod_set_array_value_cansleep(data->dc_current_limit_gpios->ndescs,
> +				       data->dc_current_limit_gpios->desc,
> +				       data->dc_current_limit_gpios->info,
> +				       values);

[ ... ]

> +static int max8903_set_property(struct power_supply *psy,
> +		enum power_supply_property psp,
> +		const union power_supply_propval *val)
> +{
> +	struct max8903_data *data = power_supply_get_drvdata(psy);
> +	int ret;
> +
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
> +		/*
> +		 * Hold source_lock across the source check and the
> +		 * resulting hardware write so the IRQ handler cannot
> +		 * flip ta_in/usb_in between them and have us program the
> +		 * limit for a source that has just gone offline.
> +		 */
> +		mutex_lock(&data->source_lock);
> +		if (data->ta_in && data->dc_current_limit_gpios)
> +			ret = max8903_set_dc_current_limit(data, val->intval);
> +		else if (data->usb_in && data->usb_current_limit_gpio)
> +			ret = max8903_set_usb_current_limit(data, val->intval);
> +		else
> +			ret = -EINVAL;
> +		mutex_unlock(&data->source_lock);
> +		return ret;

[Severity: Medium]
Could a negative requested limit bypass the minimum bounds check here?

The requested limit in val->intval is a signed integer, but it is passed
to functions like max8903_set_usb_current_limit() which expect an unsigned
u32 limit_ua. If a negative value is requested, the implicit cast to u32
would create a very large unsigned value. This would bypass the upper bounds
check and silently configure the maximum possible current instead of
returning -EINVAL.

[ ... ]

> @@ -91,12 +272,21 @@ static irqreturn_t max8903_dcin(int irq, void *_data)
>  	 * library as the line should be flagged GPIO_ACTIVE_LOW in the device
>  	 * tree.
>  	 */
> +	/*
> +	 * Sample the line under source_lock so a concurrent
> +	 * max8903_set_property() observes either the old or the new
> +	 * state consistently, never a torn read where the lock is held
> +	 * but the cached flag is about to be updated.
> +	 */
> +	mutex_lock(&data->source_lock);
>  	ta_in = gpiod_get_value(data->dok);
> -
> -	if (ta_in == data->ta_in)
> +	if (ta_in == data->ta_in) {
> +		mutex_unlock(&data->source_lock);
>  		return IRQ_HANDLED;
> +	}
>  
>  	data->ta_in = ta_in;
> +	mutex_unlock(&data->source_lock);
>  
>  	/* Set Current-Limit-Mode 1:DC 0:USB */

[Severity: High]
This is a pre-existing issue, but does dropping the lock before evaluating
data->usb_in and calculating the charger enable state expose a race condition?

If DC and USB states change simultaneously, the threads could interleave.
For example, a DC unplug could evaluate the charger enable state using a
stale data->usb_in = 0, while a concurrent USB plug updates usb_in = 1.
Could this result in writing the wrong enable state to the hardware?

Should the mutex coverage be extended over the entire state evaluation block
in both max8903_dcin() and max8903_usbin()?

[ ... ]

> +static int max8903_parse_dc_current_limit(struct platform_device *pdev,
> +					  struct max8903_data *data)
> +{

[ ... ]

> +	map = devm_kcalloc(dev, map_size, sizeof(*map), GFP_KERNEL);
> +	if (!map)
> +		return -ENOMEM;

[Severity: Low]
Does this leak the temporary map buffer?

The array map is allocated using devm_kcalloc() to temporarily hold device
tree properties for parsing. Once parsed, the buffer isn't explicitly freed,
meaning the devres allocation will persist and waste memory until the
device is unbound.

Would it be better to use kcalloc() and explicitly kfree() it at the end
of the function?

[ ... ]

> +	for (i = 0; i < data->dc_current_limit_map_size; i++) {
> +		u32 gpio_value = map[i * 2 + 1];
> +
> +		/*
> +		 * gpio_value is the bitmap programmed across the
> +		 * dc-current-limit GPIOs, so it cannot represent more
> +		 * bits than the GPIO array width. A larger value would
> +		 * be silently truncated by gpiod_set_array_value() and
> +		 * select the wrong limit; reject it at parse time so
> +		 * the bogus DT is visible to the integrator.
> +		 */
> +		if (gpio_value >= BIT(data->dc_current_limit_gpios->ndescs)) {

[Severity: High]
Does this missing bounds check on ndescs lead to undefined behavior?

The ndescs value is populated from the device tree, and the stack values
array is sized to 32 bits. If the device tree provides an ndescs value of
32 or greater, the BIT(ndescs) macro would trigger undefined behavior on
32-bit systems.

Additionally, passing an unvalidated ndescs to gpiod_set_array_value_cansleep()
could read out of bounds from the values stack array. Should we ensure
ndescs is strictly less than 32 during initialization?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603071519.807604-1-github.com@herrie.org?part=2