linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCHv1 1/2] power: supply: gpio-charger: add charge-current-limit feature
@ 2020-05-13 11:56 Sebastian Reichel
  2020-05-13 11:56 ` [PATCHv1 2/2] dt-bindings: power: supply: gpio-charger: convert to yaml Sebastian Reichel
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Sebastian Reichel @ 2020-05-13 11:56 UTC (permalink / raw)
  To: Sebastian Reichel, Rob Herring
  Cc: linux-pm, devicetree, linux-kernel, kernel, Sebastian Reichel

Add new charge-current-limit feature to gpio-charger. This also
makes the online status GPIO optional, since hardware might only
expose the charge-current-limit feature and there is no good reason
to have it mandatory now that different GPIOs are supported.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 .../bindings/power/supply/gpio-charger.txt    |  11 +-
 drivers/power/supply/gpio-charger.c           | 176 ++++++++++++++++--
 2 files changed, 174 insertions(+), 13 deletions(-)

diff --git a/Documentation/devicetree/bindings/power/supply/gpio-charger.txt b/Documentation/devicetree/bindings/power/supply/gpio-charger.txt
index 0fb33b2c62a6..dbfd29029f69 100644
--- a/Documentation/devicetree/bindings/power/supply/gpio-charger.txt
+++ b/Documentation/devicetree/bindings/power/supply/gpio-charger.txt
@@ -2,8 +2,6 @@ gpio-charger
 
 Required properties :
  - compatible : "gpio-charger"
- - gpios : GPIO indicating the charger presence.
-   See GPIO binding in bindings/gpio/gpio.txt .
  - charger-type : power supply type, one of
      unknown
      battery
@@ -15,7 +13,13 @@ Required properties :
      usb-aca (USB accessory charger adapter)
 
 Optional properties:
+ - gpios : GPIO indicating the charger presence.
+   See GPIO binding in bindings/gpio/gpio.txt .
  - charge-status-gpios: GPIO indicating whether a battery is charging.
+ - charge-current-limit-gpios: Output GPIOs specifiers for limiting the charge current
+ - charge-current-limit-mapping: List of touples with current in uA and a GPIO bitmap (in this order).
+                                The GPIOs are encoded in the same order as specified in charge-current-limit-gpios.
+				The touples must be provided in descending order of the current limit.
 
 Example:
 
@@ -24,6 +28,9 @@ Example:
 		charger-type = "usb-sdp";
 		gpios = <&gpd 28 GPIO_ACTIVE_LOW>;
 		charge-status-gpios = <&gpc 27 GPIO_ACTIVE_LOW>;
+
+		charge-current-limit-gpios = <&gpioA 11 GPIO_ACTIVE_HIGH>, <&gpioA 12 GPIO_ACTIVE_HIGH>;
+		charge-current-limit-mapping = <2500000 0x00>, <700000 0x01>, <0 0x02>;
 	};
 
 	battery {
diff --git a/drivers/power/supply/gpio-charger.c b/drivers/power/supply/gpio-charger.c
index 1b959c7f8b0e..4a5eac7cc36c 100644
--- a/drivers/power/supply/gpio-charger.c
+++ b/drivers/power/supply/gpio-charger.c
@@ -18,7 +18,13 @@
 
 #include <linux/power/gpio-charger.h>
 
+struct gpio_mapping {
+	u32 limit_ua;
+	u32 gpiodata;
+} __packed;
+
 struct gpio_charger {
+	struct device *dev;
 	unsigned int irq;
 	unsigned int charge_status_irq;
 	bool wakeup_enabled;
@@ -27,6 +33,11 @@ struct gpio_charger {
 	struct power_supply_desc charger_desc;
 	struct gpio_desc *gpiod;
 	struct gpio_desc *charge_status;
+
+	struct gpio_descs *current_limit_gpios;
+	struct gpio_mapping *current_limit_map;
+	u32 current_limit_map_size;
+	u32 charge_current_limit;
 };
 
 static irqreturn_t gpio_charger_irq(int irq, void *devid)
@@ -43,6 +54,35 @@ static inline struct gpio_charger *psy_to_gpio_charger(struct power_supply *psy)
 	return power_supply_get_drvdata(psy);
 }
 
+static int set_charge_current_limit(struct gpio_charger *gpio_charger, int val)
+{
+	struct gpio_mapping mapping;
+	int ndescs = gpio_charger->current_limit_gpios->ndescs;
+	struct gpio_desc **gpios = gpio_charger->current_limit_gpios->desc;
+	int i;
+
+	if (!gpio_charger->current_limit_map_size)
+		return -EINVAL;
+
+	for (i = 0; i < gpio_charger->current_limit_map_size; i++) {
+		if (gpio_charger->current_limit_map[i].limit_ua <= val)
+			break;
+	}
+	mapping = gpio_charger->current_limit_map[i];
+
+	for (i = 0; i < ndescs; i++) {
+		bool val = (mapping.gpiodata >> i) & 1;
+		gpiod_set_value_cansleep(gpios[ndescs-i-1], val);
+	}
+
+	gpio_charger->charge_current_limit = mapping.limit_ua;
+
+	dev_dbg(gpio_charger->dev, "set charge current limit to %d (requested: %d)\n",
+		gpio_charger->charge_current_limit, val);
+
+	return 0;
+}
+
 static int gpio_charger_get_property(struct power_supply *psy,
 		enum power_supply_property psp, union power_supply_propval *val)
 {
@@ -58,6 +98,9 @@ static int gpio_charger_get_property(struct power_supply *psy,
 		else
 			val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
 		break;
+	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
+		val->intval = gpio_charger->charge_current_limit;
+		break;
 	default:
 		return -EINVAL;
 	}
@@ -65,6 +108,34 @@ static int gpio_charger_get_property(struct power_supply *psy,
 	return 0;
 }
 
+static int gpio_charger_set_property(struct power_supply *psy,
+	enum power_supply_property psp, const union power_supply_propval *val)
+{
+	struct gpio_charger *gpio_charger = psy_to_gpio_charger(psy);
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
+		return set_charge_current_limit(gpio_charger, val->intval);
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int gpio_charger_property_is_writeable(struct power_supply *psy,
+					      enum power_supply_property psp)
+{
+	switch (psp) {
+	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
+		return 1;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
 static enum power_supply_type gpio_charger_get_type(struct device *dev)
 {
 	const char *chargetype;
@@ -112,9 +183,70 @@ static int gpio_charger_get_irq(struct device *dev, void *dev_id,
 	return irq;
 }
 
+static int init_charge_current_limit(struct device *dev,
+				    struct gpio_charger *gpio_charger)
+{
+	int i, len;
+	u32 cur_limit = U32_MAX;
+
+	gpio_charger->current_limit_gpios = devm_gpiod_get_array_optional(dev,
+		"charge-current-limit", GPIOD_OUT_LOW);
+	if (IS_ERR(gpio_charger->current_limit_gpios)) {
+		dev_err(dev, "error getting current-limit GPIOs\n");
+		return PTR_ERR(gpio_charger->current_limit_gpios);
+	}
+
+	if (!gpio_charger->current_limit_gpios)
+		return 0;
+
+	len = device_property_read_u32_array(dev, "charge-current-limit-mapping",
+		NULL, 0);
+	if (len < 0)
+		return len;
+
+	if (len % 2) {
+		dev_err(dev, "invalid charge-current-limit-mapping length\n");
+		return -EINVAL;
+	}
+
+	gpio_charger->current_limit_map = devm_kmalloc_array(dev,
+		len / 2, sizeof(*gpio_charger->current_limit_map), GFP_KERNEL);
+	if (!gpio_charger->current_limit_map)
+		return -ENOMEM;
+
+	gpio_charger->current_limit_map_size = len / 2;
+
+	len = device_property_read_u32_array(dev, "charge-current-limit-mapping",
+		(u32*) gpio_charger->current_limit_map, len);
+	if (len < 0)
+		return len;
+
+	for (i=0; i < gpio_charger->current_limit_map_size; i++) {
+		if (gpio_charger->current_limit_map[i].limit_ua > cur_limit) {
+			dev_err(dev, "invalid charge-current-limit-mapping\n");
+			return -EINVAL;
+		}
+
+		cur_limit = gpio_charger->current_limit_map[i].limit_ua;
+	}
+
+	/* default to smallest current limitation for safety reasons */
+	len = gpio_charger->current_limit_map_size - 1;
+	set_charge_current_limit(gpio_charger,
+		gpio_charger->current_limit_map[len].limit_ua);
+
+	return 0;
+}
+
+/*
+ * The entries will be overwritten by driver's probe routine depending
+ * on the available features. This list ensures, that the array is big
+ * enough for all optional features.
+ */
 static enum power_supply_property gpio_charger_properties[] = {
 	POWER_SUPPLY_PROP_ONLINE,
-	POWER_SUPPLY_PROP_STATUS /* Must always be last in the array. */
+	POWER_SUPPLY_PROP_STATUS,
+	POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX,
 };
 
 static int gpio_charger_probe(struct platform_device *pdev)
@@ -128,6 +260,7 @@ static int gpio_charger_probe(struct platform_device *pdev)
 	int charge_status_irq;
 	unsigned long flags;
 	int ret;
+	int num_props = 0;
 
 	if (!pdata && !dev->of_node) {
 		dev_err(dev, "No platform data\n");
@@ -137,18 +270,19 @@ static int gpio_charger_probe(struct platform_device *pdev)
 	gpio_charger = devm_kzalloc(dev, sizeof(*gpio_charger), GFP_KERNEL);
 	if (!gpio_charger)
 		return -ENOMEM;
+	gpio_charger->dev = dev;
 
 	/*
 	 * This will fetch a GPIO descriptor from device tree, ACPI or
 	 * boardfile descriptor tables. It's good to try this first.
 	 */
-	gpio_charger->gpiod = devm_gpiod_get(dev, NULL, GPIOD_IN);
+	gpio_charger->gpiod = devm_gpiod_get_optional(dev, NULL, GPIOD_IN);
 
 	/*
 	 * If this fails and we're not using device tree, try the
 	 * legacy platform data method.
 	 */
-	if (IS_ERR(gpio_charger->gpiod) && !dev->of_node) {
+	if (!gpio_charger->gpiod && !dev->of_node) {
 		/* Non-DT: use legacy GPIO numbers */
 		if (!gpio_is_valid(pdata->gpio)) {
 			dev_err(dev, "Invalid gpio pin in pdata\n");
@@ -173,18 +307,38 @@ static int gpio_charger_probe(struct platform_device *pdev)
 		return PTR_ERR(gpio_charger->gpiod);
 	}
 
+	if (gpio_charger->gpiod &&
+	    num_props < ARRAY_SIZE(gpio_charger_properties)) {
+		gpio_charger_properties[num_props] = POWER_SUPPLY_PROP_ONLINE;
+		num_props++;
+	}
+
 	charge_status = devm_gpiod_get_optional(dev, "charge-status", GPIOD_IN);
-	gpio_charger->charge_status = charge_status;
-	if (IS_ERR(gpio_charger->charge_status))
-		return PTR_ERR(gpio_charger->charge_status);
+	if (IS_ERR(charge_status))
+		return PTR_ERR(charge_status);
+	if (charge_status && num_props < ARRAY_SIZE(gpio_charger_properties)) {
+		gpio_charger->charge_status = charge_status;
+		gpio_charger_properties[num_props] = POWER_SUPPLY_PROP_STATUS;
+		num_props++;
+	}
+
+	ret = init_charge_current_limit(dev, gpio_charger);
+	if (ret < 0)
+		return ret;
+	if (gpio_charger->current_limit_map &&
+	    num_props < ARRAY_SIZE(gpio_charger_properties)) {
+		gpio_charger_properties[num_props] =
+			POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX;
+		num_props++;
+	}
 
 	charger_desc = &gpio_charger->charger_desc;
 	charger_desc->properties = gpio_charger_properties;
-	charger_desc->num_properties = ARRAY_SIZE(gpio_charger_properties);
-	/* Remove POWER_SUPPLY_PROP_STATUS from the supported properties. */
-	if (!gpio_charger->charge_status)
-		charger_desc->num_properties -= 1;
+	charger_desc->num_properties = num_props;
 	charger_desc->get_property = gpio_charger_get_property;
+	charger_desc->set_property = gpio_charger_set_property;
+	charger_desc->property_is_writeable =
+					gpio_charger_property_is_writeable;
 
 	psy_cfg.of_node = dev->of_node;
 	psy_cfg.drv_data = gpio_charger;
@@ -269,6 +423,6 @@ static struct platform_driver gpio_charger_driver = {
 module_platform_driver(gpio_charger_driver);
 
 MODULE_AUTHOR("Lars-Peter Clausen <lars@metafoo.de>");
-MODULE_DESCRIPTION("Driver for chargers which report their online status through a GPIO");
+MODULE_DESCRIPTION("Driver for chargers only communicating via GPIO(s)");
 MODULE_LICENSE("GPL");
 MODULE_ALIAS("platform:gpio-charger");
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCHv1 2/2] dt-bindings: power: supply: gpio-charger: convert to yaml
  2020-05-13 11:56 [PATCHv1 1/2] power: supply: gpio-charger: add charge-current-limit feature Sebastian Reichel
@ 2020-05-13 11:56 ` Sebastian Reichel
  2020-05-28  2:06   ` Rob Herring
  2020-05-15 13:24 ` [PATCHv1 1/2] power: supply: gpio-charger: add charge-current-limit feature Emil Velikov
  2020-05-28  1:56 ` Rob Herring
  2 siblings, 1 reply; 5+ messages in thread
From: Sebastian Reichel @ 2020-05-13 11:56 UTC (permalink / raw)
  To: Sebastian Reichel, Rob Herring
  Cc: linux-pm, devicetree, linux-kernel, kernel, Sebastian Reichel

Convert the gpio-charger bindings from text format to
new YAML based representation.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 .../bindings/power/supply/gpio-charger.txt    | 38 ----------
 .../bindings/power/supply/gpio-charger.yaml   | 75 +++++++++++++++++++
 2 files changed, 75 insertions(+), 38 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/power/supply/gpio-charger.txt
 create mode 100644 Documentation/devicetree/bindings/power/supply/gpio-charger.yaml

diff --git a/Documentation/devicetree/bindings/power/supply/gpio-charger.txt b/Documentation/devicetree/bindings/power/supply/gpio-charger.txt
deleted file mode 100644
index dbfd29029f69..000000000000
--- a/Documentation/devicetree/bindings/power/supply/gpio-charger.txt
+++ /dev/null
@@ -1,38 +0,0 @@
-gpio-charger
-
-Required properties :
- - compatible : "gpio-charger"
- - charger-type : power supply type, one of
-     unknown
-     battery
-     ups
-     mains
-     usb-sdp (USB standard downstream port)
-     usb-dcp (USB dedicated charging port)
-     usb-cdp (USB charging downstream port)
-     usb-aca (USB accessory charger adapter)
-
-Optional properties:
- - gpios : GPIO indicating the charger presence.
-   See GPIO binding in bindings/gpio/gpio.txt .
- - charge-status-gpios: GPIO indicating whether a battery is charging.
- - charge-current-limit-gpios: Output GPIOs specifiers for limiting the charge current
- - charge-current-limit-mapping: List of touples with current in uA and a GPIO bitmap (in this order).
-                                The GPIOs are encoded in the same order as specified in charge-current-limit-gpios.
-				The touples must be provided in descending order of the current limit.
-
-Example:
-
-	usb_charger: charger {
-		compatible = "gpio-charger";
-		charger-type = "usb-sdp";
-		gpios = <&gpd 28 GPIO_ACTIVE_LOW>;
-		charge-status-gpios = <&gpc 27 GPIO_ACTIVE_LOW>;
-
-		charge-current-limit-gpios = <&gpioA 11 GPIO_ACTIVE_HIGH>, <&gpioA 12 GPIO_ACTIVE_HIGH>;
-		charge-current-limit-mapping = <2500000 0x00>, <700000 0x01>, <0 0x02>;
-	};
-
-	battery {
-		power-supplies = <&usb_charger>;
-	};
diff --git a/Documentation/devicetree/bindings/power/supply/gpio-charger.yaml b/Documentation/devicetree/bindings/power/supply/gpio-charger.yaml
new file mode 100644
index 000000000000..14fb3e54f861
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/supply/gpio-charger.yaml
@@ -0,0 +1,75 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/power/supply/gpio-charger.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: simple battery chargers only communicating through GPIOs
+
+maintainers:
+  - Sebastian Reichel <sre@kernel.org>
+
+description: |
+  This binding is for all chargers, which are working more
+  or less autonomously, only providing some status GPIOs
+  and possibly some GPIOs for limited control over the
+  charging process.
+
+properties:
+  compatible:
+    const: gpio-charger
+
+  charger-type:
+    oneOf:
+      - const: unknown
+      - const: battery
+      - const: ups
+      - const: mains
+      - const: usb-sdp                   # USB standard downstream port
+      - const: usb-dcp                   # USB dedicated charging port
+      - const: usb-cdp                   # USB charging downstream port
+      - const: usb-aca                   # USB accessory charger adapter
+
+  gpios:
+    maxItems: 1
+    description: GPIO indicating the charger presence
+
+  charge-status-gpios:
+    maxItems: 1
+    description: GPIO indicating the charging status
+
+  charge-current-limit-gpios:
+    minItems: 1
+    maxItems: 32
+    description: GPIOs used for current limiting
+
+  charge-current-limit-mapping:
+    description: List of touples with current in uA and a GPIO bitmap (in
+      this order). The GPIOs are encoded in the same order as specified in
+      charge-current-limit-gpios. The touples must be provided in descending
+      order of the current limit.
+    $ref: "/meta-schemas/cell.yaml#array"
+
+required:
+  - compatible
+additionalProperties: false
+
+dependencies:
+  charge-current-limit-gpios: [ charge-current-limit-mapping ]
+  charge-current-limit-mapping: [ charge-current-limit-gpios ]
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    charger {
+      compatible = "gpio-charger";
+      charger-type = "usb-sdp";
+
+      gpios = <&gpd 28 GPIO_ACTIVE_LOW>;
+      charge-status-gpios = <&gpc 27 GPIO_ACTIVE_LOW>;
+
+      charge-current-limit-gpios = <&gpioA 11 GPIO_ACTIVE_HIGH>,
+                                   <&gpioA 12 GPIO_ACTIVE_HIGH>;
+      charge-current-limit-mapping = <2500000 0x00>, <700000 0x01>, <0 0x02>;
+    };
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCHv1 1/2] power: supply: gpio-charger: add charge-current-limit feature
  2020-05-13 11:56 [PATCHv1 1/2] power: supply: gpio-charger: add charge-current-limit feature Sebastian Reichel
  2020-05-13 11:56 ` [PATCHv1 2/2] dt-bindings: power: supply: gpio-charger: convert to yaml Sebastian Reichel
@ 2020-05-15 13:24 ` Emil Velikov
  2020-05-28  1:56 ` Rob Herring
  2 siblings, 0 replies; 5+ messages in thread
From: Emil Velikov @ 2020-05-15 13:24 UTC (permalink / raw)
  To: Sebastian Reichel
  Cc: Sebastian Reichel, Rob Herring, linux-pm, devicetree,
	linux-kernel, kernel

Hi Sebastian,

I've left a few trivial suggestions, although I suspect only one of them
really matters. Namely - I think as-is the code changes the legacy behaviour
when OF is missing.

Mind you, this is my third time skimming through power/supply, so take it with
a grain of salt.

On 2020/05/13, Sebastian Reichel wrote:
> Add new charge-current-limit feature to gpio-charger. This also
> makes the online status GPIO optional, since hardware might only
> expose the charge-current-limit feature and there is no good reason
> to have it mandatory now that different GPIOs are supported.
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>  .../bindings/power/supply/gpio-charger.txt    |  11 +-
>  drivers/power/supply/gpio-charger.c           | 176 ++++++++++++++++--
>  2 files changed, 174 insertions(+), 13 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/power/supply/gpio-charger.txt b/Documentation/devicetree/bindings/power/supply/gpio-charger.txt
> index 0fb33b2c62a6..dbfd29029f69 100644
> --- a/Documentation/devicetree/bindings/power/supply/gpio-charger.txt
> +++ b/Documentation/devicetree/bindings/power/supply/gpio-charger.txt
> @@ -2,8 +2,6 @@ gpio-charger
>  
>  Required properties :
>   - compatible : "gpio-charger"
> - - gpios : GPIO indicating the charger presence.
> -   See GPIO binding in bindings/gpio/gpio.txt .
>   - charger-type : power supply type, one of
>       unknown
>       battery
> @@ -15,7 +13,13 @@ Required properties :
>       usb-aca (USB accessory charger adapter)
>  
>  Optional properties:
> + - gpios : GPIO indicating the charger presence.
> +   See GPIO binding in bindings/gpio/gpio.txt .
>   - charge-status-gpios: GPIO indicating whether a battery is charging.
> + - charge-current-limit-gpios: Output GPIOs specifiers for limiting the charge current
> + - charge-current-limit-mapping: List of touples with current in uA and a GPIO bitmap (in this order).
> +                                The GPIOs are encoded in the same order as specified in charge-current-limit-gpios.
> +				The touples must be provided in descending order of the current limit.

Minor tweaks:

	List of tuples with current in uA and a GPIO bitmap.
	Tuples must be sorted in descending order of the current limit.
	GPIOs are encoded in the order as specified in charge-current-limit-gpios.


> +static int init_charge_current_limit(struct device *dev,
> +				    struct gpio_charger *gpio_charger)
> +{
> +	int i, len;
> +	u32 cur_limit = U32_MAX;
> +
> +	gpio_charger->current_limit_gpios = devm_gpiod_get_array_optional(dev,
> +		"charge-current-limit", GPIOD_OUT_LOW);
> +	if (IS_ERR(gpio_charger->current_limit_gpios)) {
> +		dev_err(dev, "error getting current-limit GPIOs\n");
> +		return PTR_ERR(gpio_charger->current_limit_gpios);
> +	}
> +
> +	if (!gpio_charger->current_limit_gpios)
> +		return 0;
> +
> +	len = device_property_read_u32_array(dev, "charge-current-limit-mapping",
> +		NULL, 0);
> +	if (len < 0)

The properly is optional, although I'm not sure if having an 'empty' properly
(len == 0) should be considered an error as indicated by -ENOMEM below or not.

Worth documenting that, unless it's covered already.

> +		return len;
> +
> +	if (len % 2) {
> +		dev_err(dev, "invalid charge-current-limit-mapping length\n");
> +		return -EINVAL;
> +	}
> +
> +	gpio_charger->current_limit_map = devm_kmalloc_array(dev,
> +		len / 2, sizeof(*gpio_charger->current_limit_map), GFP_KERNEL);
> +	if (!gpio_charger->current_limit_map)
> +		return -ENOMEM;
> +
> +	gpio_charger->current_limit_map_size = len / 2;
> +
> +	len = device_property_read_u32_array(dev, "charge-current-limit-mapping",
> +		(u32*) gpio_charger->current_limit_map, len);
> +	if (len < 0)
> +		return len;
> +
> +	for (i=0; i < gpio_charger->current_limit_map_size; i++) {
> +		if (gpio_charger->current_limit_map[i].limit_ua > cur_limit) {
> +			dev_err(dev, "invalid charge-current-limit-mapping\n");
Would make sense to use something more descriptive than "invalid". Say "not
sorted by current descending order"?


> @@ -137,18 +270,19 @@ static int gpio_charger_probe(struct platform_device *pdev)

>  	/*
>  	 * If this fails and we're not using device tree, try the
>  	 * legacy platform data method.
>  	 */
> -	if (IS_ERR(gpio_charger->gpiod) && !dev->of_node) {
> +	if (!gpio_charger->gpiod && !dev->of_node) {
The original code will attempt the legacy code for ... (from the doc)

 * ..., -ENOENT if no GPIO has been assigned to the requested function, or
 * another IS_ERR() code if an error occurred while trying to acquire the GPIO.

While the new code will only consider -ENOENT.

Using IS_ERR_OR_NULL(gpio_charger->gpiod) should preserve the original
behaviour.


>  		/* Non-DT: use legacy GPIO numbers */
>  		if (!gpio_is_valid(pdata->gpio)) {
>  			dev_err(dev, "Invalid gpio pin in pdata\n");
> @@ -173,18 +307,38 @@ static int gpio_charger_probe(struct platform_device *pdev)
>  		return PTR_ERR(gpio_charger->gpiod);
>  	}
>  
> +	if (gpio_charger->gpiod &&
> +	    num_props < ARRAY_SIZE(gpio_charger_properties)) {
> +		gpio_charger_properties[num_props] = POWER_SUPPLY_PROP_ONLINE;
The ARRAY_SIZE() check here (and below) are always true, albeit not dead code.

IMHO the beefy comment above gpio_charger_properties, plus review process is
enough to catch these issues, so it can be dropped.


>  	charger_desc = &gpio_charger->charger_desc;
>  	charger_desc->properties = gpio_charger_properties;

Aside: any particular reason why power_supply_desc::properties isn't const?

-Emil

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCHv1 1/2] power: supply: gpio-charger: add charge-current-limit feature
  2020-05-13 11:56 [PATCHv1 1/2] power: supply: gpio-charger: add charge-current-limit feature Sebastian Reichel
  2020-05-13 11:56 ` [PATCHv1 2/2] dt-bindings: power: supply: gpio-charger: convert to yaml Sebastian Reichel
  2020-05-15 13:24 ` [PATCHv1 1/2] power: supply: gpio-charger: add charge-current-limit feature Emil Velikov
@ 2020-05-28  1:56 ` Rob Herring
  2 siblings, 0 replies; 5+ messages in thread
From: Rob Herring @ 2020-05-28  1:56 UTC (permalink / raw)
  To: Sebastian Reichel
  Cc: Sebastian Reichel, linux-pm, devicetree, linux-kernel, kernel

On Wed, May 13, 2020 at 01:56:00PM +0200, Sebastian Reichel wrote:
> Add new charge-current-limit feature to gpio-charger. This also
> makes the online status GPIO optional, since hardware might only
> expose the charge-current-limit feature and there is no good reason
> to have it mandatory now that different GPIOs are supported.
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>  .../bindings/power/supply/gpio-charger.txt    |  11 +-
>  drivers/power/supply/gpio-charger.c           | 176 ++++++++++++++++--
>  2 files changed, 174 insertions(+), 13 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/power/supply/gpio-charger.txt b/Documentation/devicetree/bindings/power/supply/gpio-charger.txt
> index 0fb33b2c62a6..dbfd29029f69 100644
> --- a/Documentation/devicetree/bindings/power/supply/gpio-charger.txt
> +++ b/Documentation/devicetree/bindings/power/supply/gpio-charger.txt
> @@ -2,8 +2,6 @@ gpio-charger
>  
>  Required properties :
>   - compatible : "gpio-charger"
> - - gpios : GPIO indicating the charger presence.
> -   See GPIO binding in bindings/gpio/gpio.txt .
>   - charger-type : power supply type, one of
>       unknown
>       battery
> @@ -15,7 +13,13 @@ Required properties :
>       usb-aca (USB accessory charger adapter)
>  
>  Optional properties:
> + - gpios : GPIO indicating the charger presence.
> +   See GPIO binding in bindings/gpio/gpio.txt .
>   - charge-status-gpios: GPIO indicating whether a battery is charging.
> + - charge-current-limit-gpios: Output GPIOs specifiers for limiting the charge current
> + - charge-current-limit-mapping: List of touples with current in uA and a GPIO bitmap (in this order).
> +                                The GPIOs are encoded in the same order as specified in charge-current-limit-gpios.

TBC, index 0 GPIO is bit 0?

> +				The touples must be provided in descending order of the current limit.

tuples

Wrap at 80 columns.

>  
>  Example:
>  
> @@ -24,6 +28,9 @@ Example:
>  		charger-type = "usb-sdp";
>  		gpios = <&gpd 28 GPIO_ACTIVE_LOW>;
>  		charge-status-gpios = <&gpc 27 GPIO_ACTIVE_LOW>;
> +
> +		charge-current-limit-gpios = <&gpioA 11 GPIO_ACTIVE_HIGH>, <&gpioA 12 GPIO_ACTIVE_HIGH>;
> +		charge-current-limit-mapping = <2500000 0x00>, <700000 0x01>, <0 0x02>;
>  	};
>  
>  	battery {
> diff --git a/drivers/power/supply/gpio-charger.c b/drivers/power/supply/gpio-charger.c
> index 1b959c7f8b0e..4a5eac7cc36c 100644
> --- a/drivers/power/supply/gpio-charger.c
> +++ b/drivers/power/supply/gpio-charger.c
> @@ -18,7 +18,13 @@
>  
>  #include <linux/power/gpio-charger.h>
>  
> +struct gpio_mapping {
> +	u32 limit_ua;
> +	u32 gpiodata;
> +} __packed;
> +
>  struct gpio_charger {
> +	struct device *dev;
>  	unsigned int irq;
>  	unsigned int charge_status_irq;
>  	bool wakeup_enabled;
> @@ -27,6 +33,11 @@ struct gpio_charger {
>  	struct power_supply_desc charger_desc;
>  	struct gpio_desc *gpiod;
>  	struct gpio_desc *charge_status;
> +
> +	struct gpio_descs *current_limit_gpios;
> +	struct gpio_mapping *current_limit_map;
> +	u32 current_limit_map_size;
> +	u32 charge_current_limit;
>  };
>  
>  static irqreturn_t gpio_charger_irq(int irq, void *devid)
> @@ -43,6 +54,35 @@ static inline struct gpio_charger *psy_to_gpio_charger(struct power_supply *psy)
>  	return power_supply_get_drvdata(psy);
>  }
>  
> +static int set_charge_current_limit(struct gpio_charger *gpio_charger, int val)
> +{
> +	struct gpio_mapping mapping;
> +	int ndescs = gpio_charger->current_limit_gpios->ndescs;
> +	struct gpio_desc **gpios = gpio_charger->current_limit_gpios->desc;
> +	int i;
> +
> +	if (!gpio_charger->current_limit_map_size)
> +		return -EINVAL;
> +
> +	for (i = 0; i < gpio_charger->current_limit_map_size; i++) {
> +		if (gpio_charger->current_limit_map[i].limit_ua <= val)
> +			break;
> +	}
> +	mapping = gpio_charger->current_limit_map[i];
> +
> +	for (i = 0; i < ndescs; i++) {
> +		bool val = (mapping.gpiodata >> i) & 1;
> +		gpiod_set_value_cansleep(gpios[ndescs-i-1], val);
> +	}
> +
> +	gpio_charger->charge_current_limit = mapping.limit_ua;
> +
> +	dev_dbg(gpio_charger->dev, "set charge current limit to %d (requested: %d)\n",
> +		gpio_charger->charge_current_limit, val);
> +
> +	return 0;
> +}
> +
>  static int gpio_charger_get_property(struct power_supply *psy,
>  		enum power_supply_property psp, union power_supply_propval *val)
>  {
> @@ -58,6 +98,9 @@ static int gpio_charger_get_property(struct power_supply *psy,
>  		else
>  			val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
>  		break;
> +	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
> +		val->intval = gpio_charger->charge_current_limit;
> +		break;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -65,6 +108,34 @@ static int gpio_charger_get_property(struct power_supply *psy,
>  	return 0;
>  }
>  
> +static int gpio_charger_set_property(struct power_supply *psy,
> +	enum power_supply_property psp, const union power_supply_propval *val)
> +{
> +	struct gpio_charger *gpio_charger = psy_to_gpio_charger(psy);
> +
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
> +		return set_charge_current_limit(gpio_charger, val->intval);
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int gpio_charger_property_is_writeable(struct power_supply *psy,
> +					      enum power_supply_property psp)
> +{
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
> +		return 1;
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
>  static enum power_supply_type gpio_charger_get_type(struct device *dev)
>  {
>  	const char *chargetype;
> @@ -112,9 +183,70 @@ static int gpio_charger_get_irq(struct device *dev, void *dev_id,
>  	return irq;
>  }
>  
> +static int init_charge_current_limit(struct device *dev,
> +				    struct gpio_charger *gpio_charger)
> +{
> +	int i, len;
> +	u32 cur_limit = U32_MAX;
> +
> +	gpio_charger->current_limit_gpios = devm_gpiod_get_array_optional(dev,
> +		"charge-current-limit", GPIOD_OUT_LOW);
> +	if (IS_ERR(gpio_charger->current_limit_gpios)) {
> +		dev_err(dev, "error getting current-limit GPIOs\n");
> +		return PTR_ERR(gpio_charger->current_limit_gpios);
> +	}
> +
> +	if (!gpio_charger->current_limit_gpios)
> +		return 0;
> +
> +	len = device_property_read_u32_array(dev, "charge-current-limit-mapping",
> +		NULL, 0);
> +	if (len < 0)
> +		return len;
> +
> +	if (len % 2) {
> +		dev_err(dev, "invalid charge-current-limit-mapping length\n");
> +		return -EINVAL;
> +	}
> +
> +	gpio_charger->current_limit_map = devm_kmalloc_array(dev,
> +		len / 2, sizeof(*gpio_charger->current_limit_map), GFP_KERNEL);
> +	if (!gpio_charger->current_limit_map)
> +		return -ENOMEM;
> +
> +	gpio_charger->current_limit_map_size = len / 2;
> +
> +	len = device_property_read_u32_array(dev, "charge-current-limit-mapping",
> +		(u32*) gpio_charger->current_limit_map, len);
> +	if (len < 0)
> +		return len;
> +
> +	for (i=0; i < gpio_charger->current_limit_map_size; i++) {
> +		if (gpio_charger->current_limit_map[i].limit_ua > cur_limit) {
> +			dev_err(dev, "invalid charge-current-limit-mapping\n");
> +			return -EINVAL;
> +		}
> +
> +		cur_limit = gpio_charger->current_limit_map[i].limit_ua;
> +	}
> +
> +	/* default to smallest current limitation for safety reasons */
> +	len = gpio_charger->current_limit_map_size - 1;
> +	set_charge_current_limit(gpio_charger,
> +		gpio_charger->current_limit_map[len].limit_ua);
> +
> +	return 0;
> +}
> +
> +/*
> + * The entries will be overwritten by driver's probe routine depending
> + * on the available features. This list ensures, that the array is big
> + * enough for all optional features.
> + */
>  static enum power_supply_property gpio_charger_properties[] = {
>  	POWER_SUPPLY_PROP_ONLINE,
> -	POWER_SUPPLY_PROP_STATUS /* Must always be last in the array. */
> +	POWER_SUPPLY_PROP_STATUS,
> +	POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX,
>  };
>  
>  static int gpio_charger_probe(struct platform_device *pdev)
> @@ -128,6 +260,7 @@ static int gpio_charger_probe(struct platform_device *pdev)
>  	int charge_status_irq;
>  	unsigned long flags;
>  	int ret;
> +	int num_props = 0;
>  
>  	if (!pdata && !dev->of_node) {
>  		dev_err(dev, "No platform data\n");
> @@ -137,18 +270,19 @@ static int gpio_charger_probe(struct platform_device *pdev)
>  	gpio_charger = devm_kzalloc(dev, sizeof(*gpio_charger), GFP_KERNEL);
>  	if (!gpio_charger)
>  		return -ENOMEM;
> +	gpio_charger->dev = dev;
>  
>  	/*
>  	 * This will fetch a GPIO descriptor from device tree, ACPI or
>  	 * boardfile descriptor tables. It's good to try this first.
>  	 */
> -	gpio_charger->gpiod = devm_gpiod_get(dev, NULL, GPIOD_IN);
> +	gpio_charger->gpiod = devm_gpiod_get_optional(dev, NULL, GPIOD_IN);
>  
>  	/*
>  	 * If this fails and we're not using device tree, try the
>  	 * legacy platform data method.
>  	 */
> -	if (IS_ERR(gpio_charger->gpiod) && !dev->of_node) {
> +	if (!gpio_charger->gpiod && !dev->of_node) {
>  		/* Non-DT: use legacy GPIO numbers */
>  		if (!gpio_is_valid(pdata->gpio)) {
>  			dev_err(dev, "Invalid gpio pin in pdata\n");
> @@ -173,18 +307,38 @@ static int gpio_charger_probe(struct platform_device *pdev)
>  		return PTR_ERR(gpio_charger->gpiod);
>  	}
>  
> +	if (gpio_charger->gpiod &&
> +	    num_props < ARRAY_SIZE(gpio_charger_properties)) {
> +		gpio_charger_properties[num_props] = POWER_SUPPLY_PROP_ONLINE;
> +		num_props++;
> +	}
> +
>  	charge_status = devm_gpiod_get_optional(dev, "charge-status", GPIOD_IN);
> -	gpio_charger->charge_status = charge_status;
> -	if (IS_ERR(gpio_charger->charge_status))
> -		return PTR_ERR(gpio_charger->charge_status);
> +	if (IS_ERR(charge_status))
> +		return PTR_ERR(charge_status);
> +	if (charge_status && num_props < ARRAY_SIZE(gpio_charger_properties)) {
> +		gpio_charger->charge_status = charge_status;
> +		gpio_charger_properties[num_props] = POWER_SUPPLY_PROP_STATUS;
> +		num_props++;
> +	}
> +
> +	ret = init_charge_current_limit(dev, gpio_charger);
> +	if (ret < 0)
> +		return ret;
> +	if (gpio_charger->current_limit_map &&
> +	    num_props < ARRAY_SIZE(gpio_charger_properties)) {
> +		gpio_charger_properties[num_props] =
> +			POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX;
> +		num_props++;
> +	}
>  
>  	charger_desc = &gpio_charger->charger_desc;
>  	charger_desc->properties = gpio_charger_properties;
> -	charger_desc->num_properties = ARRAY_SIZE(gpio_charger_properties);
> -	/* Remove POWER_SUPPLY_PROP_STATUS from the supported properties. */
> -	if (!gpio_charger->charge_status)
> -		charger_desc->num_properties -= 1;
> +	charger_desc->num_properties = num_props;
>  	charger_desc->get_property = gpio_charger_get_property;
> +	charger_desc->set_property = gpio_charger_set_property;
> +	charger_desc->property_is_writeable =
> +					gpio_charger_property_is_writeable;
>  
>  	psy_cfg.of_node = dev->of_node;
>  	psy_cfg.drv_data = gpio_charger;
> @@ -269,6 +423,6 @@ static struct platform_driver gpio_charger_driver = {
>  module_platform_driver(gpio_charger_driver);
>  
>  MODULE_AUTHOR("Lars-Peter Clausen <lars@metafoo.de>");
> -MODULE_DESCRIPTION("Driver for chargers which report their online status through a GPIO");
> +MODULE_DESCRIPTION("Driver for chargers only communicating via GPIO(s)");
>  MODULE_LICENSE("GPL");
>  MODULE_ALIAS("platform:gpio-charger");
> -- 
> 2.26.2
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCHv1 2/2] dt-bindings: power: supply: gpio-charger: convert to yaml
  2020-05-13 11:56 ` [PATCHv1 2/2] dt-bindings: power: supply: gpio-charger: convert to yaml Sebastian Reichel
@ 2020-05-28  2:06   ` Rob Herring
  0 siblings, 0 replies; 5+ messages in thread
From: Rob Herring @ 2020-05-28  2:06 UTC (permalink / raw)
  To: Sebastian Reichel
  Cc: Sebastian Reichel, linux-pm, devicetree, linux-kernel, kernel

On Wed, May 13, 2020 at 01:56:01PM +0200, Sebastian Reichel wrote:
> Convert the gpio-charger bindings from text format to
> new YAML based representation.
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>  .../bindings/power/supply/gpio-charger.txt    | 38 ----------
>  .../bindings/power/supply/gpio-charger.yaml   | 75 +++++++++++++++++++
>  2 files changed, 75 insertions(+), 38 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/power/supply/gpio-charger.txt
>  create mode 100644 Documentation/devicetree/bindings/power/supply/gpio-charger.yaml
> 
> diff --git a/Documentation/devicetree/bindings/power/supply/gpio-charger.txt b/Documentation/devicetree/bindings/power/supply/gpio-charger.txt
> deleted file mode 100644
> index dbfd29029f69..000000000000
> --- a/Documentation/devicetree/bindings/power/supply/gpio-charger.txt
> +++ /dev/null
> @@ -1,38 +0,0 @@
> -gpio-charger
> -
> -Required properties :
> - - compatible : "gpio-charger"
> - - charger-type : power supply type, one of
> -     unknown
> -     battery
> -     ups
> -     mains
> -     usb-sdp (USB standard downstream port)
> -     usb-dcp (USB dedicated charging port)
> -     usb-cdp (USB charging downstream port)
> -     usb-aca (USB accessory charger adapter)
> -
> -Optional properties:
> - - gpios : GPIO indicating the charger presence.
> -   See GPIO binding in bindings/gpio/gpio.txt .
> - - charge-status-gpios: GPIO indicating whether a battery is charging.
> - - charge-current-limit-gpios: Output GPIOs specifiers for limiting the charge current
> - - charge-current-limit-mapping: List of touples with current in uA and a GPIO bitmap (in this order).
> -                                The GPIOs are encoded in the same order as specified in charge-current-limit-gpios.
> -				The touples must be provided in descending order of the current limit.
> -
> -Example:
> -
> -	usb_charger: charger {
> -		compatible = "gpio-charger";
> -		charger-type = "usb-sdp";
> -		gpios = <&gpd 28 GPIO_ACTIVE_LOW>;
> -		charge-status-gpios = <&gpc 27 GPIO_ACTIVE_LOW>;
> -
> -		charge-current-limit-gpios = <&gpioA 11 GPIO_ACTIVE_HIGH>, <&gpioA 12 GPIO_ACTIVE_HIGH>;
> -		charge-current-limit-mapping = <2500000 0x00>, <700000 0x01>, <0 0x02>;
> -	};
> -
> -	battery {
> -		power-supplies = <&usb_charger>;
> -	};
> diff --git a/Documentation/devicetree/bindings/power/supply/gpio-charger.yaml b/Documentation/devicetree/bindings/power/supply/gpio-charger.yaml
> new file mode 100644
> index 000000000000..14fb3e54f861
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/supply/gpio-charger.yaml
> @@ -0,0 +1,75 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/power/supply/gpio-charger.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: simple battery chargers only communicating through GPIOs
> +
> +maintainers:
> +  - Sebastian Reichel <sre@kernel.org>
> +
> +description: |

Can drop '|' if formatting is not important.

> +  This binding is for all chargers, which are working more
> +  or less autonomously, only providing some status GPIOs
> +  and possibly some GPIOs for limited control over the
> +  charging process.
> +
> +properties:
> +  compatible:
> +    const: gpio-charger
> +
> +  charger-type:
> +    oneOf:
> +      - const: unknown
> +      - const: battery
> +      - const: ups
> +      - const: mains
> +      - const: usb-sdp                   # USB standard downstream port
> +      - const: usb-dcp                   # USB dedicated charging port
> +      - const: usb-cdp                   # USB charging downstream port
> +      - const: usb-aca                   # USB accessory charger adapter

Use enum rather than oneOf+const

Should have a description too.

> +
> +  gpios:
> +    maxItems: 1
> +    description: GPIO indicating the charger presence
> +
> +  charge-status-gpios:
> +    maxItems: 1
> +    description: GPIO indicating the charging status
> +
> +  charge-current-limit-gpios:
> +    minItems: 1
> +    maxItems: 32
> +    description: GPIOs used for current limiting
> +
> +  charge-current-limit-mapping:
> +    description: List of touples with current in uA and a GPIO bitmap (in
> +      this order). The GPIOs are encoded in the same order as specified in
> +      charge-current-limit-gpios. The touples must be provided in descending
> +      order of the current limit.
> +    $ref: "/meta-schemas/cell.yaml#array"

That's a first... A meta-schema is what checks this document. Not what 
you want. Should be like this:

$ref: /schemas/types.yaml#/definitions/uint32-matrix
items:
  items:
    - description: Current limit in uA
    - description: Encoded GPIO setting...

I guess there's not any more constraints we can add here.

> +
> +required:
> +  - compatible

blank line

At least 1 of the gpio properties is required, right? Can be expressed 
with required entries under a oneOf or anyOf.

> +additionalProperties: false
> +
> +dependencies:
> +  charge-current-limit-gpios: [ charge-current-limit-mapping ]
> +  charge-current-limit-mapping: [ charge-current-limit-gpios ]
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    charger {
> +      compatible = "gpio-charger";
> +      charger-type = "usb-sdp";
> +
> +      gpios = <&gpd 28 GPIO_ACTIVE_LOW>;
> +      charge-status-gpios = <&gpc 27 GPIO_ACTIVE_LOW>;
> +
> +      charge-current-limit-gpios = <&gpioA 11 GPIO_ACTIVE_HIGH>,
> +                                   <&gpioA 12 GPIO_ACTIVE_HIGH>;
> +      charge-current-limit-mapping = <2500000 0x00>, <700000 0x01>, <0 0x02>;
> +    };
> -- 
> 2.26.2
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-05-28  2:06 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-13 11:56 [PATCHv1 1/2] power: supply: gpio-charger: add charge-current-limit feature Sebastian Reichel
2020-05-13 11:56 ` [PATCHv1 2/2] dt-bindings: power: supply: gpio-charger: convert to yaml Sebastian Reichel
2020-05-28  2:06   ` Rob Herring
2020-05-15 13:24 ` [PATCHv1 1/2] power: supply: gpio-charger: add charge-current-limit feature Emil Velikov
2020-05-28  1:56 ` Rob Herring

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).