[PATCH 1/3] power: supply: max17042: handle fails of reading status register

[PATCH 1/3] power: supply: max17042: handle fails of reading status register

[Krzysztof Kozlowski]

Reading status register can fail in the interrupt handler.  In such
case, the regmap_read() will not store anything useful under passed
'val' variable and random stack value will be used to determine type of
interrupt.

Handle the regmap_read() failure to avoid handling interrupt type and
triggering changed power supply event based on random stack value.

Fixes: 39e7213edc4f ("max17042_battery: Support regmap to access device's registers")
Cc: <stable@vger.kernel.org>
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
---
 drivers/power/supply/max17042_battery.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
index ce2041b30a06..858ae97600d4 100644
--- a/drivers/power/supply/max17042_battery.c
+++ b/drivers/power/supply/max17042_battery.c
@@ -869,8 +869,12 @@ static irqreturn_t max17042_thread_handler(int id, void *dev)
 {
 	struct max17042_chip *chip = dev;
 	u32 val;
+	int ret;
+
+	ret = regmap_read(chip->regmap, MAX17042_STATUS, &val);
+	if (ret)
+		return IRQ_HANDLED;
 
-	regmap_read(chip->regmap, MAX17042_STATUS, &val);
 	if ((val & STATUS_INTR_SOCMIN_BIT) ||
 		(val & STATUS_INTR_SOCMAX_BIT)) {
 		dev_info(&chip->client->dev, "SOC threshold INTR\n");
-- 
2.30.2

[PATCH 2/3] power: supply: max17042: remove duplicated STATUS bit defines

[Krzysztof Kozlowski]

All bits of STATUS register are already defined (see STATUS_SMN_BIT and
further) so there is no need to define status SoC threshold min/max
values one more time.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
---
 drivers/power/supply/max17042_battery.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
index 858ae97600d4..936e43fb710b 100644
--- a/drivers/power/supply/max17042_battery.c
+++ b/drivers/power/supply/max17042_battery.c
@@ -36,8 +36,6 @@
 
 /* Interrupt mask bits */
 #define CONFIG_ALRT_BIT_ENBL	(1 << 2)
-#define STATUS_INTR_SOCMIN_BIT	(1 << 10)
-#define STATUS_INTR_SOCMAX_BIT	(1 << 14)
 
 #define VFSOC0_LOCK		0x0000
 #define VFSOC0_UNLOCK		0x0080
@@ -875,8 +873,7 @@ static irqreturn_t max17042_thread_handler(int id, void *dev)
 	if (ret)
 		return IRQ_HANDLED;
 
-	if ((val & STATUS_INTR_SOCMIN_BIT) ||
-		(val & STATUS_INTR_SOCMAX_BIT)) {
+	if ((val & STATUS_SMN_BIT) || (val & STATUS_SMX_BIT)) {
 		dev_info(&chip->client->dev, "SOC threshold INTR\n");
 		max17042_set_soc_threshold(chip, 1);
 	}
-- 
2.30.2

[PATCH 3/3] dt-bindings: power: supply: max17042: describe interrupt

[Krzysztof Kozlowski]

The Maxim 17042-family of fuel gauges are often embedded in other Maxim
chips, e.g. in Maxim 77693 which is a companion power management IC.
In such designs there might be actually two interrupts:
 - INTB signaling change from charger, flash or MUIC,
 - ALERT signaling change from fuel gauge.

Describe the interrupt in bindings to make it clear it is about the fuel
gauge ALERT interrupt, not the INT.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
---
 .../devicetree/bindings/power/supply/maxim,max17042.yaml        | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/power/supply/maxim,max17042.yaml b/Documentation/devicetree/bindings/power/supply/maxim,max17042.yaml
index c70f05ea6d27..95beae958096 100644
--- a/Documentation/devicetree/bindings/power/supply/maxim,max17042.yaml
+++ b/Documentation/devicetree/bindings/power/supply/maxim,max17042.yaml
@@ -25,6 +25,8 @@ properties:
 
   interrupts:
     maxItems: 1
+    description: |
+      The ALRT pin, an open-drain interrupt.
 
   maxim,rsns-microohm:
     $ref: /schemas/types.yaml#/definitions/uint32
-- 
2.30.2

Re: [PATCH 3/3] dt-bindings: power: supply: max17042: describe interrupt

[Rob Herring]

On Mon, 16 Aug 2021 10:27:16 +0200, Krzysztof Kozlowski wrote:
> The Maxim 17042-family of fuel gauges are often embedded in other Maxim
> chips, e.g. in Maxim 77693 which is a companion power management IC.
> In such designs there might be actually two interrupts:
>  - INTB signaling change from charger, flash or MUIC,
>  - ALERT signaling change from fuel gauge.
> 
> Describe the interrupt in bindings to make it clear it is about the fuel
> gauge ALERT interrupt, not the INT.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> ---
>  .../devicetree/bindings/power/supply/maxim,max17042.yaml        | 2 ++
>  1 file changed, 2 insertions(+)
> 

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH 1/3] power: supply: max17042: handle fails of reading status register

[Hans de Goede]

Hi,

On 8/16/21 10:27 AM, Krzysztof Kozlowski wrote:
> Reading status register can fail in the interrupt handler.  In such
> case, the regmap_read() will not store anything useful under passed
> 'val' variable and random stack value will be used to determine type of
> interrupt.
> 
> Handle the regmap_read() failure to avoid handling interrupt type and
> triggering changed power supply event based on random stack value.
> 
> Fixes: 39e7213edc4f ("max17042_battery: Support regmap to access device's registers")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>

Thanks, the entire series looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

For the series.

Regards,

Hans

> ---
>  drivers/power/supply/max17042_battery.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
> index ce2041b30a06..858ae97600d4 100644
> --- a/drivers/power/supply/max17042_battery.c
> +++ b/drivers/power/supply/max17042_battery.c
> @@ -869,8 +869,12 @@ static irqreturn_t max17042_thread_handler(int id, void *dev)
>  {
>  	struct max17042_chip *chip = dev;
>  	u32 val;
> +	int ret;
> +
> +	ret = regmap_read(chip->regmap, MAX17042_STATUS, &val);
> +	if (ret)
> +		return IRQ_HANDLED;
>  
> -	regmap_read(chip->regmap, MAX17042_STATUS, &val);
>  	if ((val & STATUS_INTR_SOCMIN_BIT) ||
>  		(val & STATUS_INTR_SOCMAX_BIT)) {
>  		dev_info(&chip->client->dev, "SOC threshold INTR\n");
> 

Re: [PATCH 1/3] power: supply: max17042: handle fails of reading status register

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1816 bytes --]

Hi,

On Mon, Aug 16, 2021 at 10:42:01AM +0200, Hans de Goede wrote:
> Hi,
> 
> On 8/16/21 10:27 AM, Krzysztof Kozlowski wrote:
> > Reading status register can fail in the interrupt handler.  In such
> > case, the regmap_read() will not store anything useful under passed
> > 'val' variable and random stack value will be used to determine type of
> > interrupt.
> > 
> > Handle the regmap_read() failure to avoid handling interrupt type and
> > triggering changed power supply event based on random stack value.
> > 
> > Fixes: 39e7213edc4f ("max17042_battery: Support regmap to access device's registers")
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> 
> Thanks, the entire series looks good to me:
> 
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> 
> For the series.

Thanks, series queued.

-- Sebastian

> 
> Regards,
> 
> Hans
> 
> > ---
> >  drivers/power/supply/max17042_battery.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
> > index ce2041b30a06..858ae97600d4 100644
> > --- a/drivers/power/supply/max17042_battery.c
> > +++ b/drivers/power/supply/max17042_battery.c
> > @@ -869,8 +869,12 @@ static irqreturn_t max17042_thread_handler(int id, void *dev)
> >  {
> >  	struct max17042_chip *chip = dev;
> >  	u32 val;
> > +	int ret;
> > +
> > +	ret = regmap_read(chip->regmap, MAX17042_STATUS, &val);
> > +	if (ret)
> > +		return IRQ_HANDLED;
> >  
> > -	regmap_read(chip->regmap, MAX17042_STATUS, &val);
> >  	if ((val & STATUS_INTR_SOCMIN_BIT) ||
> >  		(val & STATUS_INTR_SOCMAX_BIT)) {
> >  		dev_info(&chip->client->dev, "SOC threshold INTR\n");
> > 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]