2024-04-17 13:46:23

by Herman van Hazendonk

[permalink] [raw]
Subject: [PATCH] power: supply: max8903: configure USUS as output

The USUS pin was mistakenly configured as an input, when it should be an
output that specifies whether the USB power input is suspended. In addition
to fixing the pin mode, this patch also suspends the USB input when a DC
charger is connected, which should result in a slight reduction in USB
quiescent current.

Signed-off-by: Ben Wolsieffer <[email protected]>
Signed-off-by: Herman van Hazendonk <[email protected]>
---
drivers/power/supply/max8903_charger.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/power/supply/max8903_charger.c b/drivers/power/supply/max8903_charger.c
index e65d0141f260..15dc3a5239e2 100644
--- a/drivers/power/supply/max8903_charger.c
+++ b/drivers/power/supply/max8903_charger.c
@@ -102,6 +102,10 @@ static irqreturn_t max8903_dcin(int irq, void *_data)
if (data->dcm)
gpiod_set_value(data->dcm, ta_in);

+ /* Set USB-Suspend 1:Suspended 0:Active */
+ if (data->usus)
+ gpiod_set_value(data->usus, ta_in);
+
/* Charger Enable / Disable */
if (data->cen) {
int val;
@@ -310,7 +314,15 @@ static int max8903_setup_gpios(struct platform_device *pdev)
"failed to get FLT GPIO");
gpiod_set_consumer_name(data->flt, data->psy_desc.name);

- data->usus = devm_gpiod_get_optional(dev, "usus", GPIOD_IN);
+ /*
+ * Suspend the USB input if the DC charger is connected.
+ *
+ * The USUS line should be marked GPIO_ACTIVE_HIGH in the
+ * device tree. Driving it low will enable the USB charger
+ * input.
+ */
+ flags = ta_in ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW;
+ data->usus = devm_gpiod_get_optional(dev, "usus", flags);
if (IS_ERR(data->usus))
return dev_err_probe(dev, PTR_ERR(data->usus),
"failed to get USUS GPIO");

---
base-commit: 96fca68c4fbf77a8185eb10f7557e23352732ea2
change-id: 20240417-max8903-ace97d7d3407

Best regards,
--
Herman van Hazendonk <[email protected]>



2024-04-30 14:43:59

by Sebastian Reichel

[permalink] [raw]
Subject: Re: [PATCH] power: supply: max8903: configure USUS as output

Hi,

On Wed, Apr 17, 2024 at 03:35:00PM +0200, Herman van Hazendonk wrote:
> The USUS pin was mistakenly configured as an input, when it should be an
> output that specifies whether the USB power input is suspended. In addition
> to fixing the pin mode, this patch also suspends the USB input when a DC
> charger is connected, which should result in a slight reduction in USB
> quiescent current.
>
> Signed-off-by: Ben Wolsieffer <[email protected]>
> Signed-off-by: Herman van Hazendonk <[email protected]>
> ---

Thanks for your patch. I think this should get

Fixes: 50da8d04ee52 ("power: supply: max8903: Convert to GPIO descriptors")

Also what's going on with the Signed-off-by? Did you forward the
patch from Ben Wolsieffer (i.e. the author is wrong) or did you
co-develop it (so there should be a "Co-developed-by: Ben Wolsieffer
<[email protected]>" tag)?

> drivers/power/supply/max8903_charger.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/power/supply/max8903_charger.c b/drivers/power/supply/max8903_charger.c
> index e65d0141f260..15dc3a5239e2 100644
> --- a/drivers/power/supply/max8903_charger.c
> +++ b/drivers/power/supply/max8903_charger.c
> @@ -102,6 +102,10 @@ static irqreturn_t max8903_dcin(int irq, void *_data)
> if (data->dcm)
> gpiod_set_value(data->dcm, ta_in);
>
> + /* Set USB-Suspend 1:Suspended 0:Active */
> + if (data->usus)
> + gpiod_set_value(data->usus, ta_in);
> +
> /* Charger Enable / Disable */
> if (data->cen) {
> int val;
> @@ -310,7 +314,15 @@ static int max8903_setup_gpios(struct platform_device *pdev)
> "failed to get FLT GPIO");
> gpiod_set_consumer_name(data->flt, data->psy_desc.name);
>
> - data->usus = devm_gpiod_get_optional(dev, "usus", GPIOD_IN);
> + /*
> + * Suspend the USB input if the DC charger is connected.
> + *
> + * The USUS line should be marked GPIO_ACTIVE_HIGH in the
> + * device tree. Driving it low will enable the USB charger
> + * input.

I think the above 3 lines should be dropped. They added more
confusion to me than actually helping and the information is
already in the DT binding file.

Greetings,

-- Sebastian

> + */
> + flags = ta_in ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW;
> + data->usus = devm_gpiod_get_optional(dev, "usus", flags);
> if (IS_ERR(data->usus))
> return dev_err_probe(dev, PTR_ERR(data->usus),
> "failed to get USUS GPIO");
>
> ---
> base-commit: 96fca68c4fbf77a8185eb10f7557e23352732ea2
> change-id: 20240417-max8903-ace97d7d3407
>
> Best regards,
> --
> Herman van Hazendonk <[email protected]>
>


Attachments:
(No filename) (2.64 kB)
signature.asc (849.00 B)
Download all attachments