* [PATCH 0/4] power: supply: MAX17040: Add IRQ for low level and alert SOC changes @ 2018-07-23 4:08 Matheus Castello 2018-07-23 4:08 ` [PATCH 1/4] power: supply: max17040: Add IRQ handler for low SOC alert Matheus Castello ` (4 more replies) 0 siblings, 5 replies; 82+ messages in thread From: Matheus Castello @ 2018-07-23 4:08 UTC (permalink / raw) To: sre Cc: robh+dt, mark.rutland, linux-pm, devicetree, linux-kernel, Matheus Castello This series add IRQ handler for low level SOC alert, define a devicetree binding attribute to configure the alert level threshold and check for changes in SOC for send uevents. Max17040 have a pin for alert host about low level state of charge and this alert can be configured in a threshold from 1% up to 32% of SOC. Tested on Raspberry Pi Zero W, with a SparkFun Lipo Fuel Gauge module based on MAXIM MAX17043. Matheus Castello (4): power: supply: max17040: Add IRQ handler for low SOC alert power: supply: max17040: Config alert SOC low level threshold from FDT dt-bindings: power: supply: Max17040: Add low level SOC alert threshold power: supply: max17040: Send uevent in SOC changes .../bindings/power/supply/max17040_battery.txt | 24 ++++++ drivers/power/supply/max17040_battery.c | 95 ++++++++++++++++++++++ 2 files changed, 119 insertions(+) create mode 100644 Documentation/devicetree/bindings/power/supply/max17040_battery.txt -- 2.13.3 ^ permalink raw reply [flat|nested] 82+ messages in thread
* [PATCH 1/4] power: supply: max17040: Add IRQ handler for low SOC alert 2018-07-23 4:08 [PATCH 0/4] power: supply: MAX17040: Add IRQ for low level and alert SOC changes Matheus Castello @ 2018-07-23 4:08 ` Matheus Castello 2018-07-25 10:27 ` Krzysztof Kozlowski 2018-07-23 4:08 ` [PATCH 2/4] power: supply: max17040: Config alert SOC low level threshold from FDT Matheus Castello ` (3 subsequent siblings) 4 siblings, 1 reply; 82+ messages in thread From: Matheus Castello @ 2018-07-23 4:08 UTC (permalink / raw) To: sre Cc: robh+dt, mark.rutland, linux-pm, devicetree, linux-kernel, Matheus Castello According datasheet max17040 has a pin for alert host for low SOC. This pin can be used as external interrupt, so we need to check for interrupts assigned for device and handle it. In hadler we are checking and storing fuel gauge registers values and send an uevent to notificate user space, so user space can decide save work or turn off since the alert demonstrate that the battery may no have the power to keep the system turned on for much longer. Signed-off-by: Matheus Castello <matheus@castello.eng.br> --- drivers/power/supply/max17040_battery.c | 51 +++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c index 33c40f79d23d..6e54e58814a9 100644 --- a/drivers/power/supply/max17040_battery.c +++ b/drivers/power/supply/max17040_battery.c @@ -17,6 +17,7 @@ #include <linux/err.h> #include <linux/i2c.h> #include <linux/delay.h> +#include <linux/interrupt.h> #include <linux/power_supply.h> #include <linux/max17040_battery.h> #include <linux/slab.h> @@ -179,6 +180,24 @@ static void max17040_work(struct work_struct *work) MAX17040_DELAY); } +static irqreturn_t max17040_thread_handler(int id, void *dev) +{ + struct max17040_chip *chip = dev; + struct i2c_client *client = chip->client; + + dev_warn(&client->dev, "IRQ: Alert battery low level"); + /* read registers */ + max17040_get_vcell(chip->client); + max17040_get_soc(chip->client); + max17040_get_online(chip->client); + max17040_get_status(chip->client); + + /* send uevent */ + power_supply_changed(chip->battery); + + return IRQ_HANDLED; +} + static enum power_supply_property max17040_battery_props[] = { POWER_SUPPLY_PROP_STATUS, POWER_SUPPLY_PROP_ONLINE, @@ -200,6 +219,8 @@ static int max17040_probe(struct i2c_client *client, struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent); struct power_supply_config psy_cfg = {}; struct max17040_chip *chip; + int ret; + unsigned int flags; if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE)) return -EIO; @@ -221,6 +242,24 @@ static int max17040_probe(struct i2c_client *client, return PTR_ERR(chip->battery); } + /* check interrupt */ + if (client->irq) { + dev_info(&client->dev, "IRQ: enabled\n"); + flags = IRQF_TRIGGER_FALLING | IRQF_ONESHOT; + + ret = devm_request_threaded_irq(&client->dev, client->irq, + NULL, + max17040_thread_handler, flags, + chip->battery->desc->name, + chip); + if (ret) { + client->irq = 0; + if (ret != -EBUSY) + dev_warn(&client->dev, + "Failed to get IRQ err %d \n", ret); + } + } + max17040_reset(client); max17040_get_version(client); @@ -248,6 +287,12 @@ static int max17040_suspend(struct device *dev) struct max17040_chip *chip = i2c_get_clientdata(client); cancel_delayed_work(&chip->work); + + if (chip->client->irq) { + disable_irq(chip->client->irq); + enable_irq_wake(chip->client->irq); + } + return 0; } @@ -258,6 +303,12 @@ static int max17040_resume(struct device *dev) queue_delayed_work(system_power_efficient_wq, &chip->work, MAX17040_DELAY); + + if (chip->client->irq) { + disable_irq_wake(chip->client->irq); + enable_irq(chip->client->irq); + } + return 0; } -- 2.13.3 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* Re: [PATCH 1/4] power: supply: max17040: Add IRQ handler for low SOC alert 2018-07-23 4:08 ` [PATCH 1/4] power: supply: max17040: Add IRQ handler for low SOC alert Matheus Castello @ 2018-07-25 10:27 ` Krzysztof Kozlowski 0 siblings, 0 replies; 82+ messages in thread From: Krzysztof Kozlowski @ 2018-07-25 10:27 UTC (permalink / raw) To: Matheus Castello Cc: sre, robh+dt, mark.rutland, linux-pm, devicetree, linux-kernel On 23 July 2018 at 06:08, Matheus Castello <matheus@castello.eng.br> wrote: > According datasheet max17040 has a pin for alert host for low SOC. > This pin can be used as external interrupt, so we need to check for > interrupts assigned for device and handle it. > > In hadler we are checking and storing fuel gauge registers values > and send an uevent to notificate user space, so user space can decide > save work or turn off since the alert demonstrate that the battery may > no have the power to keep the system turned on for much longer. > > Signed-off-by: Matheus Castello <matheus@castello.eng.br> > --- > drivers/power/supply/max17040_battery.c | 51 +++++++++++++++++++++++++++++++++ > 1 file changed, 51 insertions(+) > > diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c > index 33c40f79d23d..6e54e58814a9 100644 > --- a/drivers/power/supply/max17040_battery.c > +++ b/drivers/power/supply/max17040_battery.c > @@ -17,6 +17,7 @@ > #include <linux/err.h> > #include <linux/i2c.h> > #include <linux/delay.h> > +#include <linux/interrupt.h> > #include <linux/power_supply.h> > #include <linux/max17040_battery.h> > #include <linux/slab.h> > @@ -179,6 +180,24 @@ static void max17040_work(struct work_struct *work) > MAX17040_DELAY); > } > > +static irqreturn_t max17040_thread_handler(int id, void *dev) > +{ > + struct max17040_chip *chip = dev; > + struct i2c_client *client = chip->client; > + > + dev_warn(&client->dev, "IRQ: Alert battery low level"); > + /* read registers */ > + max17040_get_vcell(chip->client); You just stored chip->client in client... > + max17040_get_soc(chip->client); > + max17040_get_online(chip->client); > + max17040_get_status(chip->client); This duplicates max17040_work(). Can you split common part? > + > + /* send uevent */ > + power_supply_changed(chip->battery); > + > + return IRQ_HANDLED; > +} > + > static enum power_supply_property max17040_battery_props[] = { > POWER_SUPPLY_PROP_STATUS, > POWER_SUPPLY_PROP_ONLINE, > @@ -200,6 +219,8 @@ static int max17040_probe(struct i2c_client *client, > struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent); > struct power_supply_config psy_cfg = {}; > struct max17040_chip *chip; > + int ret; > + unsigned int flags; Define them in scope using them. > > if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE)) > return -EIO; > @@ -221,6 +242,24 @@ static int max17040_probe(struct i2c_client *client, > return PTR_ERR(chip->battery); > } > > + /* check interrupt */ > + if (client->irq) { > + dev_info(&client->dev, "IRQ: enabled\n"); > + flags = IRQF_TRIGGER_FALLING | IRQF_ONESHOT; > + > + ret = devm_request_threaded_irq(&client->dev, client->irq, > + NULL, > + max17040_thread_handler, flags, > + chip->battery->desc->name, > + chip); Please indent it to parenthesis. > + if (ret) { > + client->irq = 0; > + if (ret != -EBUSY) > + dev_warn(&client->dev, > + "Failed to get IRQ err %d \n", ret); > + } > + } > + > max17040_reset(client); > max17040_get_version(client); > > @@ -248,6 +287,12 @@ static int max17040_suspend(struct device *dev) > struct max17040_chip *chip = i2c_get_clientdata(client); > > cancel_delayed_work(&chip->work); > + > + if (chip->client->irq) { I think this should use device wakeup properties (e.g. device_init_wakeup(), device_may_wakeup()) coming from pdata. It would be nice to CC users of this driver. We have it on some of boards. Unfortunately get_maintainer will not point it automatically so: scripts/get_maintainer.pl -f drivers/mfd/max14577.c Best regards, Krzysztof > + disable_irq(chip->client->irq); > + enable_irq_wake(chip->client->irq); > + } > + > return 0; > } > > @@ -258,6 +303,12 @@ static int max17040_resume(struct device *dev) > > queue_delayed_work(system_power_efficient_wq, &chip->work, > MAX17040_DELAY); > + > + if (chip->client->irq) { > + disable_irq_wake(chip->client->irq); > + enable_irq(chip->client->irq); > + } > + > return 0; > } > > -- > 2.13.3 > ^ permalink raw reply [flat|nested] 82+ messages in thread
* [PATCH 2/4] power: supply: max17040: Config alert SOC low level threshold from FDT 2018-07-23 4:08 [PATCH 0/4] power: supply: MAX17040: Add IRQ for low level and alert SOC changes Matheus Castello 2018-07-23 4:08 ` [PATCH 1/4] power: supply: max17040: Add IRQ handler for low SOC alert Matheus Castello @ 2018-07-23 4:08 ` Matheus Castello 2018-07-25 10:42 ` Krzysztof Kozlowski 2018-07-23 4:08 ` [PATCH 3/4] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold Matheus Castello ` (2 subsequent siblings) 4 siblings, 1 reply; 82+ messages in thread From: Matheus Castello @ 2018-07-23 4:08 UTC (permalink / raw) To: sre Cc: robh+dt, mark.rutland, linux-pm, devicetree, linux-kernel, Matheus Castello For configuration of fuel gauge alert for a low level state of charge interrupt we add a function to config level threshold and a device tree binding property to set it in flatned device tree node. Now we can use "maxim,alert-soc-level" property with the values from 1 up to 32 to configure alert interrupt threshold. Signed-off-by: Matheus Castello <matheus@castello.eng.br> --- drivers/power/supply/max17040_battery.c | 36 +++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c index 6e54e58814a9..3efa52d32b44 100644 --- a/drivers/power/supply/max17040_battery.c +++ b/drivers/power/supply/max17040_battery.c @@ -47,6 +47,8 @@ struct max17040_chip { int soc; /* State Of Charge */ int status; + /* Alert threshold */ + int alert_threshold; }; static int max17040_get_property(struct power_supply *psy, @@ -123,6 +125,28 @@ static void max17040_get_soc(struct i2c_client *client) chip->soc = (soc >> 8); } +static int max17040_set_soc_threshold(struct i2c_client *client, u8 level) +{ + int ret; + u16 data; + + /* check if level is between 1% and 32% */ + if (level > 0 && level < 33) { + /* alert threshold use LSb 5 bits from RCOMP */ + /* two's-complements form: 00000 = 32% and 11111 = 1% */ + level = 32 - level; + data = max17040_read_reg(client, MAX17040_RCOMP); + data &= 0xFFE0; + data |= level; + max17040_write_reg(client, MAX17040_RCOMP, data); + ret = 0; + } else { + ret = -EINVAL; + } + + return ret; +} + static void max17040_get_version(struct i2c_client *client) { u16 version; @@ -165,6 +189,16 @@ static void max17040_get_status(struct i2c_client *client) chip->status = POWER_SUPPLY_STATUS_FULL; } +static void max17040_get_of_data(struct max17040_chip *chip) +{ + struct device *dev = &chip->client->dev; + struct device_node *np = dev->of_node; + + if (of_property_read_s32(np, "maxim,alert-soc-level", + &chip->alert_threshold)) + chip->alert_threshold = 4; +} + static void max17040_work(struct work_struct *work) { struct max17040_chip *chip; @@ -231,6 +265,7 @@ static int max17040_probe(struct i2c_client *client, chip->client = client; chip->pdata = client->dev.platform_data; + max17040_get_of_data(chip); i2c_set_clientdata(client, chip); psy_cfg.drv_data = chip; @@ -262,6 +297,7 @@ static int max17040_probe(struct i2c_client *client, max17040_reset(client); max17040_get_version(client); + max17040_set_soc_threshold(client, chip->alert_threshold); INIT_DEFERRABLE_WORK(&chip->work, max17040_work); queue_delayed_work(system_power_efficient_wq, &chip->work, -- 2.13.3 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* Re: [PATCH 2/4] power: supply: max17040: Config alert SOC low level threshold from FDT 2018-07-23 4:08 ` [PATCH 2/4] power: supply: max17040: Config alert SOC low level threshold from FDT Matheus Castello @ 2018-07-25 10:42 ` Krzysztof Kozlowski 0 siblings, 0 replies; 82+ messages in thread From: Krzysztof Kozlowski @ 2018-07-25 10:42 UTC (permalink / raw) To: Matheus Castello Cc: sre, robh+dt, mark.rutland, linux-pm, devicetree, linux-kernel On 23 July 2018 at 06:08, Matheus Castello <matheus@castello.eng.br> wrote: > For configuration of fuel gauge alert for a low level state of charge > interrupt we add a function to config level threshold and a device tree > binding property to set it in flatned device tree node. > > Now we can use "maxim,alert-soc-level" property with the values from > 1 up to 32 to configure alert interrupt threshold. > > Signed-off-by: Matheus Castello <matheus@castello.eng.br> > --- > drivers/power/supply/max17040_battery.c | 36 +++++++++++++++++++++++++++++++++ > 1 file changed, 36 insertions(+) Hi Matheus, In series, bindings describing new DT property should go before introducing them in the driver. > > diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c > index 6e54e58814a9..3efa52d32b44 100644 > --- a/drivers/power/supply/max17040_battery.c > +++ b/drivers/power/supply/max17040_battery.c > @@ -47,6 +47,8 @@ struct max17040_chip { > int soc; > /* State Of Charge */ > int status; > + /* Alert threshold */ Threshold in what units? > + int alert_threshold; > }; > > static int max17040_get_property(struct power_supply *psy, > @@ -123,6 +125,28 @@ static void max17040_get_soc(struct i2c_client *client) > chip->soc = (soc >> 8); > } > > +static int max17040_set_soc_threshold(struct i2c_client *client, u8 level) > +{ > + int ret; > + u16 data; > + > + /* check if level is between 1% and 32% */ > + if (level > 0 && level < 33) { > + /* alert threshold use LSb 5 bits from RCOMP */ > + /* two's-complements form: 00000 = 32% and 11111 = 1% */ Please use Linux style comments. > + level = 32 - level; > + data = max17040_read_reg(client, MAX17040_RCOMP); > + data &= 0xFFE0; Please define the mask. > + data |= level; > + max17040_write_reg(client, MAX17040_RCOMP, data); > + ret = 0; > + } else { > + ret = -EINVAL; > + } > + > + return ret; > +} > + > static void max17040_get_version(struct i2c_client *client) > { > u16 version; > @@ -165,6 +189,16 @@ static void max17040_get_status(struct i2c_client *client) > chip->status = POWER_SUPPLY_STATUS_FULL; > } > > +static void max17040_get_of_data(struct max17040_chip *chip) > +{ > + struct device *dev = &chip->client->dev; > + struct device_node *np = dev->of_node; > + > + if (of_property_read_s32(np, "maxim,alert-soc-level", > + &chip->alert_threshold)) > + chip->alert_threshold = 4; 1. You read s32 and later cast it to u8. Either some validation of possible values or of_property_read_u8(). 2. You have hard-coded default value - please put it in some define. There are already defines, e.g.: MAX17040_BATTERY_FULL 3. Also the bindings say something about power up value... not 4. 4, I think that default - missing - value should mean "no interrupt warnings". > +} > + > static void max17040_work(struct work_struct *work) > { > struct max17040_chip *chip; > @@ -231,6 +265,7 @@ static int max17040_probe(struct i2c_client *client, > > chip->client = client; > chip->pdata = client->dev.platform_data; > + max17040_get_of_data(chip); > > i2c_set_clientdata(client, chip); > psy_cfg.drv_data = chip; > @@ -262,6 +297,7 @@ static int max17040_probe(struct i2c_client *client, > > max17040_reset(client); > max17040_get_version(client); > + max17040_set_soc_threshold(client, chip->alert_threshold); 1. Return value ignored. 2. First you enable interrupts for whatever default value of alerts and then you set the alerts threshold. You might generate false warning in such case so this should be in reverse order. Best regards, Krzysztof > > INIT_DEFERRABLE_WORK(&chip->work, max17040_work); > queue_delayed_work(system_power_efficient_wq, &chip->work, > -- > 2.13.3 > ^ permalink raw reply [flat|nested] 82+ messages in thread
* [PATCH 3/4] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold 2018-07-23 4:08 [PATCH 0/4] power: supply: MAX17040: Add IRQ for low level and alert SOC changes Matheus Castello 2018-07-23 4:08 ` [PATCH 1/4] power: supply: max17040: Add IRQ handler for low SOC alert Matheus Castello 2018-07-23 4:08 ` [PATCH 2/4] power: supply: max17040: Config alert SOC low level threshold from FDT Matheus Castello @ 2018-07-23 4:08 ` Matheus Castello 2018-07-25 10:45 ` Krzysztof Kozlowski 2018-07-23 4:08 ` [PATCH 4/4] power: supply: max17040: Send uevent in SOC changes Matheus Castello 2018-09-16 11:45 ` [PATCH 0/4] power: supply: MAX17040: Add IRQ for low level and alert " Sebastian Reichel 4 siblings, 1 reply; 82+ messages in thread From: Matheus Castello @ 2018-07-23 4:08 UTC (permalink / raw) To: sre Cc: robh+dt, mark.rutland, linux-pm, devicetree, linux-kernel, Matheus Castello For configure low level state of charge threshold alert signaled from max17040 we add "maxim,alert-soc-level" property. Signed-off-by: Matheus Castello <matheus@castello.eng.br> --- .../bindings/power/supply/max17040_battery.txt | 24 ++++++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 Documentation/devicetree/bindings/power/supply/max17040_battery.txt diff --git a/Documentation/devicetree/bindings/power/supply/max17040_battery.txt b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt new file mode 100644 index 000000000000..e6e4e46c03c4 --- /dev/null +++ b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt @@ -0,0 +1,24 @@ +max17040_battery +~~~~~~~~~~~~~~~~ + +Required properties : + - compatible : "maxim,max17040" + +Optional threshold properties : + If skipped the power up default value will be used + - maxim,alert-soc-level : The alert threshold that sets the state of + charge level where an interrupt is generated. + max17040 can be configured from 1 up to 32. + +Remembering that for the interrupt to be handled it must also be described the +information of the interruption in the node. + +Example: + + battery-charger@36 { + compatible = "maxim,max17040"; + reg = <0x36>; + maxim,alert-soc-level = <10>; + interrupt-parent = <&gpio>; + interrupts = <18 IRQ_TYPE_EDGE_FALLING>; + }; -- 2.13.3 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* Re: [PATCH 3/4] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold 2018-07-23 4:08 ` [PATCH 3/4] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold Matheus Castello @ 2018-07-25 10:45 ` Krzysztof Kozlowski 0 siblings, 0 replies; 82+ messages in thread From: Krzysztof Kozlowski @ 2018-07-25 10:45 UTC (permalink / raw) To: Matheus Castello Cc: sre, robh+dt, mark.rutland, linux-pm, devicetree, linux-kernel On 23 July 2018 at 06:08, Matheus Castello <matheus@castello.eng.br> wrote: > For configure low level state of charge threshold alert signaled from > max17040 we add "maxim,alert-soc-level" property. > > Signed-off-by: Matheus Castello <matheus@castello.eng.br> > --- > .../bindings/power/supply/max17040_battery.txt | 24 ++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > create mode 100644 Documentation/devicetree/bindings/power/supply/max17040_battery.txt > > diff --git a/Documentation/devicetree/bindings/power/supply/max17040_battery.txt b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt > new file mode 100644 > index 000000000000..e6e4e46c03c4 > --- /dev/null > +++ b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt > @@ -0,0 +1,24 @@ > +max17040_battery > +~~~~~~~~~~~~~~~~ > + > +Required properties : > + - compatible : "maxim,max17040" Why you skipped "maxim,max77836-battery"? > + > +Optional threshold properties : > + If skipped the power up default value will be used > + - maxim,alert-soc-level : The alert threshold that sets the state of > + charge level where an interrupt is generated. > + max17040 can be configured from 1 up to 32. > + > +Remembering that for the interrupt to be handled it must also be described the > +information of the interruption in the node. Just mention interrupts in optional properties, including the flags. BTW, the driver hard-codes the flags which is in contrast with DT here. Best regards, Krzysztof > + > +Example: > + > + battery-charger@36 { > + compatible = "maxim,max17040"; > + reg = <0x36>; > + maxim,alert-soc-level = <10>; > + interrupt-parent = <&gpio>; > + interrupts = <18 IRQ_TYPE_EDGE_FALLING>; > + }; > -- > 2.13.3 > > -- > To unsubscribe from this list: send the line "unsubscribe devicetree" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 82+ messages in thread
* [PATCH 4/4] power: supply: max17040: Send uevent in SOC changes 2018-07-23 4:08 [PATCH 0/4] power: supply: MAX17040: Add IRQ for low level and alert SOC changes Matheus Castello ` (2 preceding siblings ...) 2018-07-23 4:08 ` [PATCH 3/4] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold Matheus Castello @ 2018-07-23 4:08 ` Matheus Castello 2018-07-25 10:52 ` Krzysztof Kozlowski 2018-09-16 11:45 ` [PATCH 0/4] power: supply: MAX17040: Add IRQ for low level and alert " Sebastian Reichel 4 siblings, 1 reply; 82+ messages in thread From: Matheus Castello @ 2018-07-23 4:08 UTC (permalink / raw) To: sre Cc: robh+dt, mark.rutland, linux-pm, devicetree, linux-kernel, Matheus Castello Add check for changes in state of charge from delayed work, so in case of changes we call power_supply_changed to send an uevent. power_supply_changed send an uevent for alert user space about changes, is useful for example to user space apps made changes in UI battery level or made other decisions. Signed-off-by: Matheus Castello <matheus@castello.eng.br> --- drivers/power/supply/max17040_battery.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c index 3efa52d32b44..72915ac9e13b 100644 --- a/drivers/power/supply/max17040_battery.c +++ b/drivers/power/supply/max17040_battery.c @@ -202,14 +202,22 @@ static void max17040_get_of_data(struct max17040_chip *chip) static void max17040_work(struct work_struct *work) { struct max17040_chip *chip; + u16 last_soc; chip = container_of(work, struct max17040_chip, work.work); + /* store SOC for check change */ + last_soc = chip->soc; + max17040_get_vcell(chip->client); max17040_get_soc(chip->client); max17040_get_online(chip->client); max17040_get_status(chip->client); + /* check changes and send uevent */ + if (last_soc != chip->soc) + power_supply_changed(chip->battery); + queue_delayed_work(system_power_efficient_wq, &chip->work, MAX17040_DELAY); } -- 2.13.3 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* Re: [PATCH 4/4] power: supply: max17040: Send uevent in SOC changes 2018-07-23 4:08 ` [PATCH 4/4] power: supply: max17040: Send uevent in SOC changes Matheus Castello @ 2018-07-25 10:52 ` Krzysztof Kozlowski 0 siblings, 0 replies; 82+ messages in thread From: Krzysztof Kozlowski @ 2018-07-25 10:52 UTC (permalink / raw) To: Matheus Castello Cc: sre, robh+dt, mark.rutland, linux-pm, devicetree, linux-kernel On 23 July 2018 at 06:08, Matheus Castello <matheus@castello.eng.br> wrote: > Add check for changes in state of charge from delayed work, so > in case of changes we call power_supply_changed to send an uevent. > > power_supply_changed send an uevent for alert user space about > changes, is useful for example to user space apps made changes in > UI battery level or made other decisions. Hi, Too many "changes". How about: Notify core through power_supply_changed() in case of changes in state of charge. This is useful for user-space to efficiently update current battery level. Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org> Best regards, Krzysztof > Signed-off-by: Matheus Castello <matheus@castello.eng.br> > --- > drivers/power/supply/max17040_battery.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c > index 3efa52d32b44..72915ac9e13b 100644 > --- a/drivers/power/supply/max17040_battery.c > +++ b/drivers/power/supply/max17040_battery.c > @@ -202,14 +202,22 @@ static void max17040_get_of_data(struct max17040_chip *chip) > static void max17040_work(struct work_struct *work) > { > struct max17040_chip *chip; > + u16 last_soc; > > chip = container_of(work, struct max17040_chip, work.work); > > + /* store SOC for check change */ > + last_soc = chip->soc; > + > max17040_get_vcell(chip->client); > max17040_get_soc(chip->client); > max17040_get_online(chip->client); > max17040_get_status(chip->client); > > + /* check changes and send uevent */ > + if (last_soc != chip->soc) > + power_supply_changed(chip->battery); > + > queue_delayed_work(system_power_efficient_wq, &chip->work, > MAX17040_DELAY); > } > -- > 2.13.3 > > -- > To unsubscribe from this list: send the line "unsubscribe devicetree" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH 0/4] power: supply: MAX17040: Add IRQ for low level and alert SOC changes 2018-07-23 4:08 [PATCH 0/4] power: supply: MAX17040: Add IRQ for low level and alert SOC changes Matheus Castello ` (3 preceding siblings ...) 2018-07-23 4:08 ` [PATCH 4/4] power: supply: max17040: Send uevent in SOC changes Matheus Castello @ 2018-09-16 11:45 ` Sebastian Reichel 2018-09-17 11:32 ` Krzysztof Kozlowski 4 siblings, 1 reply; 82+ messages in thread From: Sebastian Reichel @ 2018-09-16 11:45 UTC (permalink / raw) To: Matheus Castello Cc: robh+dt, mark.rutland, linux-pm, devicetree, linux-kernel [-- Attachment #1: Type: text/plain, Size: 1235 bytes --] Hi Matheus, Did I miss a v2 of this patchset, that solves the issues found by Krzysztof? -- Sebastian On Mon, Jul 23, 2018 at 12:08:12AM -0400, Matheus Castello wrote: > This series add IRQ handler for low level SOC alert, define a devicetree > binding attribute to configure the alert level threshold and check for changes > in SOC for send uevents. > > Max17040 have a pin for alert host about low level state of charge and this > alert can be configured in a threshold from 1% up to 32% of SOC. > > Tested on Raspberry Pi Zero W, with a SparkFun Lipo Fuel Gauge module based on > MAXIM MAX17043. > > Matheus Castello (4): > power: supply: max17040: Add IRQ handler for low SOC alert > power: supply: max17040: Config alert SOC low level threshold from FDT > dt-bindings: power: supply: Max17040: Add low level SOC alert threshold > power: supply: max17040: Send uevent in SOC changes > > .../bindings/power/supply/max17040_battery.txt | 24 ++++++ > drivers/power/supply/max17040_battery.c | 95 ++++++++++++++++++++++ > 2 files changed, 119 insertions(+) > create mode 100644 Documentation/devicetree/bindings/power/supply/max17040_battery.txt > > -- > 2.13.3 > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH 0/4] power: supply: MAX17040: Add IRQ for low level and alert SOC changes 2018-09-16 11:45 ` [PATCH 0/4] power: supply: MAX17040: Add IRQ for low level and alert " Sebastian Reichel @ 2018-09-17 11:32 ` Krzysztof Kozlowski 2018-09-18 3:45 ` Matheus Castello 2019-04-15 1:26 ` [PATCH v2 " Matheus Castello 0 siblings, 2 replies; 82+ messages in thread From: Krzysztof Kozlowski @ 2018-09-17 11:32 UTC (permalink / raw) To: sebastian.reichel, matheus Cc: robh+dt, mark.rutland, linux-pm, devicetree, linux-kernel On Sun, 16 Sep 2018 at 22:03, Sebastian Reichel <sebastian.reichel@collabora.com> wrote: > > Hi Matheus, > > Did I miss a v2 of this patchset, that solves the issues > found by Krzysztof? I did not see v2. This patchset brings nice feature so it would be a pity if it were discontinued. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH 0/4] power: supply: MAX17040: Add IRQ for low level and alert SOC changes 2018-09-17 11:32 ` Krzysztof Kozlowski @ 2018-09-18 3:45 ` Matheus Castello 2019-04-15 1:26 ` [PATCH v2 " Matheus Castello 1 sibling, 0 replies; 82+ messages in thread From: Matheus Castello @ 2018-09-18 3:45 UTC (permalink / raw) To: Krzysztof Kozlowski, sebastian.reichel Cc: robh+dt, mark.rutland, linux-pm, devicetree, linux-kernel Hi Krzysztof and Sebastian, please forgive me for the delay in working with this patch. I've been having some personal issues these months, but leaving the excuses I still intend to send a v2 for this. Thanks Krzysztof for review and tips I'll work on it. Best Regards, Matheus Castello On 09/17/2018 08:32 AM, Krzysztof Kozlowski wrote: > On Sun, 16 Sep 2018 at 22:03, Sebastian Reichel > <sebastian.reichel@collabora.com> wrote: >> >> Hi Matheus, >> >> Did I miss a v2 of this patchset, that solves the issues >> found by Krzysztof? > > I did not see v2. This patchset brings nice feature so it would be a > pity if it were discontinued. > > Best regards, > Krzysztof > ^ permalink raw reply [flat|nested] 82+ messages in thread
* [PATCH v2 0/4] power: supply: MAX17040: Add IRQ for low level and alert SOC changes 2018-09-17 11:32 ` Krzysztof Kozlowski 2018-09-18 3:45 ` Matheus Castello @ 2019-04-15 1:26 ` Matheus Castello 2019-04-15 1:26 ` Matheus Castello ` (4 more replies) 1 sibling, 5 replies; 82+ messages in thread From: Matheus Castello @ 2019-04-15 1:26 UTC (permalink / raw) To: sre, krzk, robh+dt Cc: mark.rutland, cw00.choi, b.zolnierkie, lee.jones, linux-pm, devicetree, linux-kernel, Matheus Castello This series add IRQ handler for low level SOC alert, define a devicetree binding attribute to configure the alert level threshold and check for changes in SOC for send uevents. Max17040 have a pin for alert host about low level state of charge and this alert can be configured in a threshold from 1% up to 32% of SOC. Tested on Toradex Colibri iMX7D, with a SparkFun Lipo Fuel Gauge module based on MAXIM MAX17043. Thanks Krzysztof Kozlowski for your time reviewing it, and forgive me for the delay in working on it, now I'm back to the patchs. Let me know what you think about the fixes and I'm open to maintainers suggestions. Changes since v1: (Suggested by Krzysztof Kozlowski) - Put common code from max17040_work and max17040_thread_handler in a function - Code style fixes - Define mask and low level threshold alert default - Check return value from max17040_set_soc_threshold - Set low level state of charge threshold before IRQ - CC maintainers from drivers/mfd/max14577 - Use flags from FDT client->flags instead hard coded it - Mention interrupts on DT Documentation - Fix "maxim,max77836-battery" missed from DT Documentation - Fix commit description Matheus Castello (4): power: supply: max17040: Add IRQ handler for low SOC alert dt-bindings: power: supply: Max17040: Add low level SOC alert threshold power: supply: max17040: Config alert SOC low level threshold from FDT power: supply: max17040: Send uevent in SOC changes .../power/supply/max17040_battery.txt | 24 ++++ drivers/power/supply/max17040_battery.c | 118 +++++++++++++++++- 2 files changed, 138 insertions(+), 4 deletions(-) create mode 100644 Documentation/devicetree/bindings/power/supply/max17040_battery.txt -- 2.17.0 ^ permalink raw reply [flat|nested] 82+ messages in thread
* [PATCH v2 0/4] power: supply: MAX17040: Add IRQ for low level and alert SOC changes 2019-04-15 1:26 ` [PATCH v2 " Matheus Castello @ 2019-04-15 1:26 ` Matheus Castello 2019-04-15 1:26 ` [PATCH v2 1/4] power: supply: max17040: Add IRQ handler for low SOC alert Matheus Castello ` (3 subsequent siblings) 4 siblings, 0 replies; 82+ messages in thread From: Matheus Castello @ 2019-04-15 1:26 UTC (permalink / raw) To: sre, krzk, robh+dt Cc: mark.rutland, cw00.choi, b.zolnierkie, lee.jones, linux-pm, devicetree, linux-kernel, Matheus Castello This series add IRQ handler for low level SOC alert, define a devicetree binding attribute to configure the alert level threshold and check for changes in SOC for send uevents. Max17040 have a pin for alert host about low level state of charge and this alert can be configured in a threshold from 1% up to 32% of SOC. Tested on Toradex Colibri iMX7D, with a SparkFun Lipo Fuel Gauge module based on MAXIM MAX17043. Thanks Krzysztof Kozlowski for your time reviewing it, and forgive me for the delay in working on it, now I'm back to the patchs. Let me know what you think about the fixes and I'm open to maintainers suggestions. Changes since v1: (Suggested by Krzysztof Kozlowski) - Put common code from max17040_work and max17040_thread_handler in a function - Code style fixes - Define mask and low level threshold alert default - Check return value from max17040_set_soc_threshold - Set low level state of charge threshold before IRQ - CC maintainers from drivers/mfd/max14577 - Use flags from FDT client->flags instead hard coded it - Mention interrupts on DT Documentation - Fix "maxim,max77836-battery" missed from DT Documentation - Fix commit description Matheus Castello (4): power: supply: max17040: Add IRQ handler for low SOC alert dt-bindings: power: supply: Max17040: Add low level SOC alert threshold power: supply: max17040: Config alert SOC low level threshold from FDT power: supply: max17040: Send uevent in SOC changes .../power/supply/max17040_battery.txt | 24 ++++ drivers/power/supply/max17040_battery.c | 118 +++++++++++++++++- 2 files changed, 138 insertions(+), 4 deletions(-) create mode 100644 Documentation/devicetree/bindings/power/supply/max17040_battery.txt -- 2.17.0 ^ permalink raw reply [flat|nested] 82+ messages in thread
* [PATCH v2 1/4] power: supply: max17040: Add IRQ handler for low SOC alert 2019-04-15 1:26 ` [PATCH v2 " Matheus Castello 2019-04-15 1:26 ` Matheus Castello @ 2019-04-15 1:26 ` Matheus Castello 2019-04-15 1:26 ` Matheus Castello 2019-04-15 7:10 ` Krzysztof Kozlowski 2019-04-15 1:26 ` [PATCH v2 2/4] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold Matheus Castello ` (2 subsequent siblings) 4 siblings, 2 replies; 82+ messages in thread From: Matheus Castello @ 2019-04-15 1:26 UTC (permalink / raw) To: sre, krzk, robh+dt Cc: mark.rutland, cw00.choi, b.zolnierkie, lee.jones, linux-pm, devicetree, linux-kernel, Matheus Castello According datasheet max17040 has a pin for alert host for low SOC. This pin can be used as external interrupt, so we need to check for interrupts assigned for device and handle it. In hadler we are checking and storing fuel gauge registers values and send an uevent to notificate user space, so user space can decide save work or turn off since the alert demonstrate that the battery may no have the power to keep the system turned on for much longer. Signed-off-by: Matheus Castello <matheus@castello.eng.br> --- drivers/power/supply/max17040_battery.c | 69 +++++++++++++++++++++++-- 1 file changed, 64 insertions(+), 5 deletions(-) diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c index 91cafc7bed30..8d2f8ed3f44c 100644 --- a/drivers/power/supply/max17040_battery.c +++ b/drivers/power/supply/max17040_battery.c @@ -13,6 +13,7 @@ #include <linux/err.h> #include <linux/i2c.h> #include <linux/delay.h> +#include <linux/interrupt.h> #include <linux/power_supply.h> #include <linux/max17040_battery.h> #include <linux/slab.h> @@ -160,21 +161,40 @@ static void max17040_get_status(struct i2c_client *client) chip->status = POWER_SUPPLY_STATUS_FULL; } +static void max17040_check_changes(struct i2c_client *client) +{ + max17040_get_vcell(client); + max17040_get_soc(client); + max17040_get_online(client); + max17040_get_status(client); +} + static void max17040_work(struct work_struct *work) { struct max17040_chip *chip; chip = container_of(work, struct max17040_chip, work.work); - - max17040_get_vcell(chip->client); - max17040_get_soc(chip->client); - max17040_get_online(chip->client); - max17040_get_status(chip->client); + max17040_check_changes(chip->client); queue_delayed_work(system_power_efficient_wq, &chip->work, MAX17040_DELAY); } +static irqreturn_t max17040_thread_handler(int id, void *dev) +{ + struct max17040_chip *chip = dev; + struct i2c_client *client = chip->client; + + dev_warn(&client->dev, "IRQ: Alert battery low level"); + /* read registers */ + max17040_check_changes(chip->client); + + /* send uevent */ + power_supply_changed(chip->battery); + + return IRQ_HANDLED; +} + static enum power_supply_property max17040_battery_props[] = { POWER_SUPPLY_PROP_STATUS, POWER_SUPPLY_PROP_ONLINE, @@ -217,6 +237,27 @@ static int max17040_probe(struct i2c_client *client, return PTR_ERR(chip->battery); } + /* check interrupt */ + if (client->irq) { + int ret; + unsigned int flags; + + dev_info(&client->dev, "IRQ: enabled\n"); + flags = IRQF_TRIGGER_FALLING | IRQF_ONESHOT; + + ret = devm_request_threaded_irq(&client->dev, client->irq, NULL, + max17040_thread_handler, flags, + chip->battery->desc->name, + chip); + + if (ret) { + client->irq = 0; + if (ret != -EBUSY) + dev_warn(&client->dev, + "Failed to get IRQ err %d\n", ret); + } + } + max17040_reset(client); max17040_get_version(client); @@ -224,6 +265,8 @@ static int max17040_probe(struct i2c_client *client, queue_delayed_work(system_power_efficient_wq, &chip->work, MAX17040_DELAY); + device_init_wakeup(&client->dev, 1); + return 0; } @@ -244,6 +287,14 @@ static int max17040_suspend(struct device *dev) struct max17040_chip *chip = i2c_get_clientdata(client); cancel_delayed_work(&chip->work); + + if (client->irq) { + if (device_may_wakeup(dev)) + enable_irq_wake(client->irq); + else + disable_irq_wake(client->irq); + } + return 0; } @@ -254,6 +305,14 @@ static int max17040_resume(struct device *dev) queue_delayed_work(system_power_efficient_wq, &chip->work, MAX17040_DELAY); + + if (client->irq) { + if (device_may_wakeup(dev)) + disable_irq_wake(client->irq); + else + enable_irq_wake(client->irq); + } + return 0; } -- 2.17.0 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* [PATCH v2 1/4] power: supply: max17040: Add IRQ handler for low SOC alert 2019-04-15 1:26 ` [PATCH v2 1/4] power: supply: max17040: Add IRQ handler for low SOC alert Matheus Castello @ 2019-04-15 1:26 ` Matheus Castello 2019-04-15 7:10 ` Krzysztof Kozlowski 1 sibling, 0 replies; 82+ messages in thread From: Matheus Castello @ 2019-04-15 1:26 UTC (permalink / raw) To: sre, krzk, robh+dt Cc: mark.rutland, cw00.choi, b.zolnierkie, lee.jones, linux-pm, devicetree, linux-kernel, Matheus Castello According datasheet max17040 has a pin for alert host for low SOC. This pin can be used as external interrupt, so we need to check for interrupts assigned for device and handle it. In hadler we are checking and storing fuel gauge registers values and send an uevent to notificate user space, so user space can decide save work or turn off since the alert demonstrate that the battery may no have the power to keep the system turned on for much longer. Signed-off-by: Matheus Castello <matheus@castello.eng.br> --- drivers/power/supply/max17040_battery.c | 69 +++++++++++++++++++++++-- 1 file changed, 64 insertions(+), 5 deletions(-) diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c index 91cafc7bed30..8d2f8ed3f44c 100644 --- a/drivers/power/supply/max17040_battery.c +++ b/drivers/power/supply/max17040_battery.c @@ -13,6 +13,7 @@ #include <linux/err.h> #include <linux/i2c.h> #include <linux/delay.h> +#include <linux/interrupt.h> #include <linux/power_supply.h> #include <linux/max17040_battery.h> #include <linux/slab.h> @@ -160,21 +161,40 @@ static void max17040_get_status(struct i2c_client *client) chip->status = POWER_SUPPLY_STATUS_FULL; } +static void max17040_check_changes(struct i2c_client *client) +{ + max17040_get_vcell(client); + max17040_get_soc(client); + max17040_get_online(client); + max17040_get_status(client); +} + static void max17040_work(struct work_struct *work) { struct max17040_chip *chip; chip = container_of(work, struct max17040_chip, work.work); - - max17040_get_vcell(chip->client); - max17040_get_soc(chip->client); - max17040_get_online(chip->client); - max17040_get_status(chip->client); + max17040_check_changes(chip->client); queue_delayed_work(system_power_efficient_wq, &chip->work, MAX17040_DELAY); } +static irqreturn_t max17040_thread_handler(int id, void *dev) +{ + struct max17040_chip *chip = dev; + struct i2c_client *client = chip->client; + + dev_warn(&client->dev, "IRQ: Alert battery low level"); + /* read registers */ + max17040_check_changes(chip->client); + + /* send uevent */ + power_supply_changed(chip->battery); + + return IRQ_HANDLED; +} + static enum power_supply_property max17040_battery_props[] = { POWER_SUPPLY_PROP_STATUS, POWER_SUPPLY_PROP_ONLINE, @@ -217,6 +237,27 @@ static int max17040_probe(struct i2c_client *client, return PTR_ERR(chip->battery); } + /* check interrupt */ + if (client->irq) { + int ret; + unsigned int flags; + + dev_info(&client->dev, "IRQ: enabled\n"); + flags = IRQF_TRIGGER_FALLING | IRQF_ONESHOT; + + ret = devm_request_threaded_irq(&client->dev, client->irq, NULL, + max17040_thread_handler, flags, + chip->battery->desc->name, + chip); + + if (ret) { + client->irq = 0; + if (ret != -EBUSY) + dev_warn(&client->dev, + "Failed to get IRQ err %d\n", ret); + } + } + max17040_reset(client); max17040_get_version(client); @@ -224,6 +265,8 @@ static int max17040_probe(struct i2c_client *client, queue_delayed_work(system_power_efficient_wq, &chip->work, MAX17040_DELAY); + device_init_wakeup(&client->dev, 1); + return 0; } @@ -244,6 +287,14 @@ static int max17040_suspend(struct device *dev) struct max17040_chip *chip = i2c_get_clientdata(client); cancel_delayed_work(&chip->work); + + if (client->irq) { + if (device_may_wakeup(dev)) + enable_irq_wake(client->irq); + else + disable_irq_wake(client->irq); + } + return 0; } @@ -254,6 +305,14 @@ static int max17040_resume(struct device *dev) queue_delayed_work(system_power_efficient_wq, &chip->work, MAX17040_DELAY); + + if (client->irq) { + if (device_may_wakeup(dev)) + disable_irq_wake(client->irq); + else + enable_irq_wake(client->irq); + } + return 0; } -- 2.17.0 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* Re: [PATCH v2 1/4] power: supply: max17040: Add IRQ handler for low SOC alert 2019-04-15 1:26 ` [PATCH v2 1/4] power: supply: max17040: Add IRQ handler for low SOC alert Matheus Castello 2019-04-15 1:26 ` Matheus Castello @ 2019-04-15 7:10 ` Krzysztof Kozlowski 2019-04-15 7:10 ` Krzysztof Kozlowski 2019-04-19 18:12 ` Matheus Castello 1 sibling, 2 replies; 82+ messages in thread From: Krzysztof Kozlowski @ 2019-04-15 7:10 UTC (permalink / raw) To: Matheus Castello Cc: sre, robh+dt, mark.rutland, Chanwoo Choi, Bartłomiej Żołnierkiewicz, lee.jones, linux-pm, devicetree, linux-kernel On Mon, 15 Apr 2019 at 03:49, Matheus Castello <matheus@castello.eng.br> wrote: > > According datasheet max17040 has a pin for alert host for low SOC. > This pin can be used as external interrupt, so we need to check for > interrupts assigned for device and handle it. > > In hadler we are checking and storing fuel gauge registers values > and send an uevent to notificate user space, so user space can decide > save work or turn off since the alert demonstrate that the battery may > no have the power to keep the system turned on for much longer. > > Signed-off-by: Matheus Castello <matheus@castello.eng.br> > --- > drivers/power/supply/max17040_battery.c | 69 +++++++++++++++++++++++-- > 1 file changed, 64 insertions(+), 5 deletions(-) > > diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c > index 91cafc7bed30..8d2f8ed3f44c 100644 > --- a/drivers/power/supply/max17040_battery.c > +++ b/drivers/power/supply/max17040_battery.c > @@ -13,6 +13,7 @@ > #include <linux/err.h> > #include <linux/i2c.h> > #include <linux/delay.h> > +#include <linux/interrupt.h> > #include <linux/power_supply.h> > #include <linux/max17040_battery.h> > #include <linux/slab.h> > @@ -160,21 +161,40 @@ static void max17040_get_status(struct i2c_client *client) > chip->status = POWER_SUPPLY_STATUS_FULL; > } > > +static void max17040_check_changes(struct i2c_client *client) > +{ > + max17040_get_vcell(client); > + max17040_get_soc(client); > + max17040_get_online(client); > + max17040_get_status(client); > +} > + > static void max17040_work(struct work_struct *work) > { > struct max17040_chip *chip; > > chip = container_of(work, struct max17040_chip, work.work); > - > - max17040_get_vcell(chip->client); > - max17040_get_soc(chip->client); > - max17040_get_online(chip->client); > - max17040_get_status(chip->client); > + max17040_check_changes(chip->client); > > queue_delayed_work(system_power_efficient_wq, &chip->work, > MAX17040_DELAY); > } > > +static irqreturn_t max17040_thread_handler(int id, void *dev) > +{ > + struct max17040_chip *chip = dev; > + struct i2c_client *client = chip->client; > + > + dev_warn(&client->dev, "IRQ: Alert battery low level"); > + /* read registers */ > + max17040_check_changes(chip->client); > + > + /* send uevent */ > + power_supply_changed(chip->battery); > + > + return IRQ_HANDLED; > +} > + > static enum power_supply_property max17040_battery_props[] = { > POWER_SUPPLY_PROP_STATUS, > POWER_SUPPLY_PROP_ONLINE, > @@ -217,6 +237,27 @@ static int max17040_probe(struct i2c_client *client, > return PTR_ERR(chip->battery); > } > > + /* check interrupt */ > + if (client->irq) { > + int ret; > + unsigned int flags; > + > + dev_info(&client->dev, "IRQ: enabled\n"); > + flags = IRQF_TRIGGER_FALLING | IRQF_ONESHOT; > + > + ret = devm_request_threaded_irq(&client->dev, client->irq, NULL, > + max17040_thread_handler, flags, > + chip->battery->desc->name, > + chip); > + > + if (ret) { > + client->irq = 0; > + if (ret != -EBUSY) Why not on EBUSY? > + dev_warn(&client->dev, > + "Failed to get IRQ err %d\n", ret); > + } > + } > + > max17040_reset(client); > max17040_get_version(client); > > @@ -224,6 +265,8 @@ static int max17040_probe(struct i2c_client *client, > queue_delayed_work(system_power_efficient_wq, &chip->work, > MAX17040_DELAY); > > + device_init_wakeup(&client->dev, 1); Either you parse DT for wakeup-source property and use it... or you unconditionally enable wakeup. In the second case - there is no point to check later for device_may_wakeup(). Instead check the return value of device_init_wakeup(). > + > return 0; > } > > @@ -244,6 +287,14 @@ static int max17040_suspend(struct device *dev) > struct max17040_chip *chip = i2c_get_clientdata(client); > > cancel_delayed_work(&chip->work); > + > + if (client->irq) { > + if (device_may_wakeup(dev)) > + enable_irq_wake(client->irq); > + else > + disable_irq_wake(client->irq); Did you try the wakeup from suspend? You do not have a disable_irq() here which usually was needed for interrupts to work properly on suspend. Maybe this was fixed? Best regards, Krzysztof ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH v2 1/4] power: supply: max17040: Add IRQ handler for low SOC alert 2019-04-15 7:10 ` Krzysztof Kozlowski @ 2019-04-15 7:10 ` Krzysztof Kozlowski 2019-04-19 18:12 ` Matheus Castello 1 sibling, 0 replies; 82+ messages in thread From: Krzysztof Kozlowski @ 2019-04-15 7:10 UTC (permalink / raw) To: Matheus Castello Cc: sre, robh+dt, mark.rutland, Chanwoo Choi, Bartłomiej Żołnierkiewicz, lee.jones, linux-pm, devicetree, linux-kernel On Mon, 15 Apr 2019 at 03:49, Matheus Castello <matheus@castello.eng.br> wrote: > > According datasheet max17040 has a pin for alert host for low SOC. > This pin can be used as external interrupt, so we need to check for > interrupts assigned for device and handle it. > > In hadler we are checking and storing fuel gauge registers values > and send an uevent to notificate user space, so user space can decide > save work or turn off since the alert demonstrate that the battery may > no have the power to keep the system turned on for much longer. > > Signed-off-by: Matheus Castello <matheus@castello.eng.br> > --- > drivers/power/supply/max17040_battery.c | 69 +++++++++++++++++++++++-- > 1 file changed, 64 insertions(+), 5 deletions(-) > > diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c > index 91cafc7bed30..8d2f8ed3f44c 100644 > --- a/drivers/power/supply/max17040_battery.c > +++ b/drivers/power/supply/max17040_battery.c > @@ -13,6 +13,7 @@ > #include <linux/err.h> > #include <linux/i2c.h> > #include <linux/delay.h> > +#include <linux/interrupt.h> > #include <linux/power_supply.h> > #include <linux/max17040_battery.h> > #include <linux/slab.h> > @@ -160,21 +161,40 @@ static void max17040_get_status(struct i2c_client *client) > chip->status = POWER_SUPPLY_STATUS_FULL; > } > > +static void max17040_check_changes(struct i2c_client *client) > +{ > + max17040_get_vcell(client); > + max17040_get_soc(client); > + max17040_get_online(client); > + max17040_get_status(client); > +} > + > static void max17040_work(struct work_struct *work) > { > struct max17040_chip *chip; > > chip = container_of(work, struct max17040_chip, work.work); > - > - max17040_get_vcell(chip->client); > - max17040_get_soc(chip->client); > - max17040_get_online(chip->client); > - max17040_get_status(chip->client); > + max17040_check_changes(chip->client); > > queue_delayed_work(system_power_efficient_wq, &chip->work, > MAX17040_DELAY); > } > > +static irqreturn_t max17040_thread_handler(int id, void *dev) > +{ > + struct max17040_chip *chip = dev; > + struct i2c_client *client = chip->client; > + > + dev_warn(&client->dev, "IRQ: Alert battery low level"); > + /* read registers */ > + max17040_check_changes(chip->client); > + > + /* send uevent */ > + power_supply_changed(chip->battery); > + > + return IRQ_HANDLED; > +} > + > static enum power_supply_property max17040_battery_props[] = { > POWER_SUPPLY_PROP_STATUS, > POWER_SUPPLY_PROP_ONLINE, > @@ -217,6 +237,27 @@ static int max17040_probe(struct i2c_client *client, > return PTR_ERR(chip->battery); > } > > + /* check interrupt */ > + if (client->irq) { > + int ret; > + unsigned int flags; > + > + dev_info(&client->dev, "IRQ: enabled\n"); > + flags = IRQF_TRIGGER_FALLING | IRQF_ONESHOT; > + > + ret = devm_request_threaded_irq(&client->dev, client->irq, NULL, > + max17040_thread_handler, flags, > + chip->battery->desc->name, > + chip); > + > + if (ret) { > + client->irq = 0; > + if (ret != -EBUSY) Why not on EBUSY? > + dev_warn(&client->dev, > + "Failed to get IRQ err %d\n", ret); > + } > + } > + > max17040_reset(client); > max17040_get_version(client); > > @@ -224,6 +265,8 @@ static int max17040_probe(struct i2c_client *client, > queue_delayed_work(system_power_efficient_wq, &chip->work, > MAX17040_DELAY); > > + device_init_wakeup(&client->dev, 1); Either you parse DT for wakeup-source property and use it... or you unconditionally enable wakeup. In the second case - there is no point to check later for device_may_wakeup(). Instead check the return value of device_init_wakeup(). > + > return 0; > } > > @@ -244,6 +287,14 @@ static int max17040_suspend(struct device *dev) > struct max17040_chip *chip = i2c_get_clientdata(client); > > cancel_delayed_work(&chip->work); > + > + if (client->irq) { > + if (device_may_wakeup(dev)) > + enable_irq_wake(client->irq); > + else > + disable_irq_wake(client->irq); Did you try the wakeup from suspend? You do not have a disable_irq() here which usually was needed for interrupts to work properly on suspend. Maybe this was fixed? Best regards, Krzysztof ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH v2 1/4] power: supply: max17040: Add IRQ handler for low SOC alert 2019-04-15 7:10 ` Krzysztof Kozlowski 2019-04-15 7:10 ` Krzysztof Kozlowski @ 2019-04-19 18:12 ` Matheus Castello 2019-04-19 18:12 ` Matheus Castello 1 sibling, 1 reply; 82+ messages in thread From: Matheus Castello @ 2019-04-19 18:12 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: sre, robh+dt, mark.rutland, Chanwoo Choi, Bartłomiej Żołnierkiewicz, lee.jones, linux-pm, devicetree, linux-kernel Em 4/15/19 4:10 AM, Krzysztof Kozlowski escreveu: > On Mon, 15 Apr 2019 at 03:49, Matheus Castello <matheus@castello.eng.br> wrote: >> >> According datasheet max17040 has a pin for alert host for low SOC. >> This pin can be used as external interrupt, so we need to check for >> interrupts assigned for device and handle it. >> >> In hadler we are checking and storing fuel gauge registers values >> and send an uevent to notificate user space, so user space can decide >> save work or turn off since the alert demonstrate that the battery may >> no have the power to keep the system turned on for much longer. >> >> Signed-off-by: Matheus Castello <matheus@castello.eng.br> >> --- >> drivers/power/supply/max17040_battery.c | 69 +++++++++++++++++++++++-- >> 1 file changed, 64 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c >> index 91cafc7bed30..8d2f8ed3f44c 100644 >> --- a/drivers/power/supply/max17040_battery.c >> +++ b/drivers/power/supply/max17040_battery.c >> @@ -13,6 +13,7 @@ >> #include <linux/err.h> >> #include <linux/i2c.h> >> #include <linux/delay.h> >> +#include <linux/interrupt.h> >> #include <linux/power_supply.h> >> #include <linux/max17040_battery.h> >> #include <linux/slab.h> >> @@ -160,21 +161,40 @@ static void max17040_get_status(struct i2c_client *client) >> chip->status = POWER_SUPPLY_STATUS_FULL; >> } >> >> +static void max17040_check_changes(struct i2c_client *client) >> +{ >> + max17040_get_vcell(client); >> + max17040_get_soc(client); >> + max17040_get_online(client); >> + max17040_get_status(client); >> +} >> + >> static void max17040_work(struct work_struct *work) >> { >> struct max17040_chip *chip; >> >> chip = container_of(work, struct max17040_chip, work.work); >> - >> - max17040_get_vcell(chip->client); >> - max17040_get_soc(chip->client); >> - max17040_get_online(chip->client); >> - max17040_get_status(chip->client); >> + max17040_check_changes(chip->client); >> >> queue_delayed_work(system_power_efficient_wq, &chip->work, >> MAX17040_DELAY); >> } >> >> +static irqreturn_t max17040_thread_handler(int id, void *dev) >> +{ >> + struct max17040_chip *chip = dev; >> + struct i2c_client *client = chip->client; >> + >> + dev_warn(&client->dev, "IRQ: Alert battery low level"); >> + /* read registers */ >> + max17040_check_changes(chip->client); >> + >> + /* send uevent */ >> + power_supply_changed(chip->battery); >> + >> + return IRQ_HANDLED; >> +} >> + >> static enum power_supply_property max17040_battery_props[] = { >> POWER_SUPPLY_PROP_STATUS, >> POWER_SUPPLY_PROP_ONLINE, >> @@ -217,6 +237,27 @@ static int max17040_probe(struct i2c_client *client, >> return PTR_ERR(chip->battery); >> } >> >> + /* check interrupt */ >> + if (client->irq) { >> + int ret; >> + unsigned int flags; >> + >> + dev_info(&client->dev, "IRQ: enabled\n"); >> + flags = IRQF_TRIGGER_FALLING | IRQF_ONESHOT; >> + >> + ret = devm_request_threaded_irq(&client->dev, client->irq, NULL, >> + max17040_thread_handler, flags, >> + chip->battery->desc->name, >> + chip); >> + >> + if (ret) { >> + client->irq = 0; >> + if (ret != -EBUSY) > > Why not on EBUSY? > >> + dev_warn(&client->dev, >> + "Failed to get IRQ err %d\n", ret); >> + } >> + } >> + >> max17040_reset(client); >> max17040_get_version(client); >> >> @@ -224,6 +265,8 @@ static int max17040_probe(struct i2c_client *client, >> queue_delayed_work(system_power_efficient_wq, &chip->work, >> MAX17040_DELAY); >> >> + device_init_wakeup(&client->dev, 1); > > Either you parse DT for wakeup-source property and use it... or you > unconditionally enable wakeup. In the second case - there is no point > to check later for device_may_wakeup(). Instead check the return value > of device_init_wakeup(). > >> + >> return 0; >> } >> >> @@ -244,6 +287,14 @@ static int max17040_suspend(struct device *dev) >> struct max17040_chip *chip = i2c_get_clientdata(client); >> >> cancel_delayed_work(&chip->work); >> + >> + if (client->irq) { >> + if (device_may_wakeup(dev)) >> + enable_irq_wake(client->irq); >> + else >> + disable_irq_wake(client->irq); > > Did you try the wakeup from suspend? You do not have a disable_irq() > here which usually was needed for interrupts to work properly on > suspend. Maybe this was fixed? > > Best regards, > Krzysztof > Hi Krzysztof, I test it using mem state and suspend, resuming seems to have worked correctly ... Thanks for the review, I'm working in your suggestions and I expect to send v3 this weekend. Best Regards, Matheus Castello ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH v2 1/4] power: supply: max17040: Add IRQ handler for low SOC alert 2019-04-19 18:12 ` Matheus Castello @ 2019-04-19 18:12 ` Matheus Castello 0 siblings, 0 replies; 82+ messages in thread From: Matheus Castello @ 2019-04-19 18:12 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: sre, robh+dt, mark.rutland, Chanwoo Choi, Bartłomiej Żołnierkiewicz, lee.jones, linux-pm, devicetree, linux-kernel Em 4/15/19 4:10 AM, Krzysztof Kozlowski escreveu: > On Mon, 15 Apr 2019 at 03:49, Matheus Castello <matheus@castello.eng.br> wrote: >> >> According datasheet max17040 has a pin for alert host for low SOC. >> This pin can be used as external interrupt, so we need to check for >> interrupts assigned for device and handle it. >> >> In hadler we are checking and storing fuel gauge registers values >> and send an uevent to notificate user space, so user space can decide >> save work or turn off since the alert demonstrate that the battery may >> no have the power to keep the system turned on for much longer. >> >> Signed-off-by: Matheus Castello <matheus@castello.eng.br> >> --- >> drivers/power/supply/max17040_battery.c | 69 +++++++++++++++++++++++-- >> 1 file changed, 64 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c >> index 91cafc7bed30..8d2f8ed3f44c 100644 >> --- a/drivers/power/supply/max17040_battery.c >> +++ b/drivers/power/supply/max17040_battery.c >> @@ -13,6 +13,7 @@ >> #include <linux/err.h> >> #include <linux/i2c.h> >> #include <linux/delay.h> >> +#include <linux/interrupt.h> >> #include <linux/power_supply.h> >> #include <linux/max17040_battery.h> >> #include <linux/slab.h> >> @@ -160,21 +161,40 @@ static void max17040_get_status(struct i2c_client *client) >> chip->status = POWER_SUPPLY_STATUS_FULL; >> } >> >> +static void max17040_check_changes(struct i2c_client *client) >> +{ >> + max17040_get_vcell(client); >> + max17040_get_soc(client); >> + max17040_get_online(client); >> + max17040_get_status(client); >> +} >> + >> static void max17040_work(struct work_struct *work) >> { >> struct max17040_chip *chip; >> >> chip = container_of(work, struct max17040_chip, work.work); >> - >> - max17040_get_vcell(chip->client); >> - max17040_get_soc(chip->client); >> - max17040_get_online(chip->client); >> - max17040_get_status(chip->client); >> + max17040_check_changes(chip->client); >> >> queue_delayed_work(system_power_efficient_wq, &chip->work, >> MAX17040_DELAY); >> } >> >> +static irqreturn_t max17040_thread_handler(int id, void *dev) >> +{ >> + struct max17040_chip *chip = dev; >> + struct i2c_client *client = chip->client; >> + >> + dev_warn(&client->dev, "IRQ: Alert battery low level"); >> + /* read registers */ >> + max17040_check_changes(chip->client); >> + >> + /* send uevent */ >> + power_supply_changed(chip->battery); >> + >> + return IRQ_HANDLED; >> +} >> + >> static enum power_supply_property max17040_battery_props[] = { >> POWER_SUPPLY_PROP_STATUS, >> POWER_SUPPLY_PROP_ONLINE, >> @@ -217,6 +237,27 @@ static int max17040_probe(struct i2c_client *client, >> return PTR_ERR(chip->battery); >> } >> >> + /* check interrupt */ >> + if (client->irq) { >> + int ret; >> + unsigned int flags; >> + >> + dev_info(&client->dev, "IRQ: enabled\n"); >> + flags = IRQF_TRIGGER_FALLING | IRQF_ONESHOT; >> + >> + ret = devm_request_threaded_irq(&client->dev, client->irq, NULL, >> + max17040_thread_handler, flags, >> + chip->battery->desc->name, >> + chip); >> + >> + if (ret) { >> + client->irq = 0; >> + if (ret != -EBUSY) > > Why not on EBUSY? > >> + dev_warn(&client->dev, >> + "Failed to get IRQ err %d\n", ret); >> + } >> + } >> + >> max17040_reset(client); >> max17040_get_version(client); >> >> @@ -224,6 +265,8 @@ static int max17040_probe(struct i2c_client *client, >> queue_delayed_work(system_power_efficient_wq, &chip->work, >> MAX17040_DELAY); >> >> + device_init_wakeup(&client->dev, 1); > > Either you parse DT for wakeup-source property and use it... or you > unconditionally enable wakeup. In the second case - there is no point > to check later for device_may_wakeup(). Instead check the return value > of device_init_wakeup(). > >> + >> return 0; >> } >> >> @@ -244,6 +287,14 @@ static int max17040_suspend(struct device *dev) >> struct max17040_chip *chip = i2c_get_clientdata(client); >> >> cancel_delayed_work(&chip->work); >> + >> + if (client->irq) { >> + if (device_may_wakeup(dev)) >> + enable_irq_wake(client->irq); >> + else >> + disable_irq_wake(client->irq); > > Did you try the wakeup from suspend? You do not have a disable_irq() > here which usually was needed for interrupts to work properly on > suspend. Maybe this was fixed? > > Best regards, > Krzysztof > Hi Krzysztof, I test it using mem state and suspend, resuming seems to have worked correctly ... Thanks for the review, I'm working in your suggestions and I expect to send v3 this weekend. Best Regards, Matheus Castello ^ permalink raw reply [flat|nested] 82+ messages in thread
* [PATCH v2 2/4] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold 2019-04-15 1:26 ` [PATCH v2 " Matheus Castello 2019-04-15 1:26 ` Matheus Castello 2019-04-15 1:26 ` [PATCH v2 1/4] power: supply: max17040: Add IRQ handler for low SOC alert Matheus Castello @ 2019-04-15 1:26 ` Matheus Castello 2019-04-15 1:26 ` Matheus Castello ` (2 more replies) 2019-04-15 1:26 ` [PATCH v2 3/4] power: supply: max17040: Config alert SOC low level threshold from FDT Matheus Castello 2019-04-15 1:26 ` [PATCH v2 4/4] power: supply: max17040: Send uevent in SOC changes Matheus Castello 4 siblings, 3 replies; 82+ messages in thread From: Matheus Castello @ 2019-04-15 1:26 UTC (permalink / raw) To: sre, krzk, robh+dt Cc: mark.rutland, cw00.choi, b.zolnierkie, lee.jones, linux-pm, devicetree, linux-kernel, Matheus Castello For configure low level state of charge threshold alert signaled from max17040 we add "maxim,alert-soc-level" property. Signed-off-by: Matheus Castello <matheus@castello.eng.br> --- .../power/supply/max17040_battery.txt | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 Documentation/devicetree/bindings/power/supply/max17040_battery.txt diff --git a/Documentation/devicetree/bindings/power/supply/max17040_battery.txt b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt new file mode 100644 index 000000000000..9b2cc67d556f --- /dev/null +++ b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt @@ -0,0 +1,24 @@ +max17040_battery +~~~~~~~~~~~~~~~~ + +Required properties : + - compatible : "maxim,max17040" or "maxim,max77836-battery" + +Optional properties : +- maxim,alert-soc-level : The alert threshold that sets the state of + charge level where an interrupt is generated. + Can be configured from 1 up to 32. If skipped + the power up default value of 4 will be used. +- interrupt-parent : The GPIO bank from the interrupt line. +- interrupts : Interrupt line see Documentation/devicetree/ + bindings/interrupt-controller/interrupts.txt + +Example: + + battery-charger@36 { + compatible = "maxim,max17040"; + reg = <0x36>; + maxim,alert-soc-level = <10>; + interrupt-parent = <&gpio7>; + interrupts = <2 IRQ_TYPE_EDGE_FALLING>; + }; -- 2.17.0 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* [PATCH v2 2/4] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold 2019-04-15 1:26 ` [PATCH v2 2/4] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold Matheus Castello @ 2019-04-15 1:26 ` Matheus Castello 2019-04-15 7:13 ` Krzysztof Kozlowski 2019-04-29 22:13 ` Rob Herring 2 siblings, 0 replies; 82+ messages in thread From: Matheus Castello @ 2019-04-15 1:26 UTC (permalink / raw) To: sre, krzk, robh+dt Cc: mark.rutland, cw00.choi, b.zolnierkie, lee.jones, linux-pm, devicetree, linux-kernel, Matheus Castello For configure low level state of charge threshold alert signaled from max17040 we add "maxim,alert-soc-level" property. Signed-off-by: Matheus Castello <matheus@castello.eng.br> --- .../power/supply/max17040_battery.txt | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 Documentation/devicetree/bindings/power/supply/max17040_battery.txt diff --git a/Documentation/devicetree/bindings/power/supply/max17040_battery.txt b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt new file mode 100644 index 000000000000..9b2cc67d556f --- /dev/null +++ b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt @@ -0,0 +1,24 @@ +max17040_battery +~~~~~~~~~~~~~~~~ + +Required properties : + - compatible : "maxim,max17040" or "maxim,max77836-battery" + +Optional properties : +- maxim,alert-soc-level : The alert threshold that sets the state of + charge level where an interrupt is generated. + Can be configured from 1 up to 32. If skipped + the power up default value of 4 will be used. +- interrupt-parent : The GPIO bank from the interrupt line. +- interrupts : Interrupt line see Documentation/devicetree/ + bindings/interrupt-controller/interrupts.txt + +Example: + + battery-charger@36 { + compatible = "maxim,max17040"; + reg = <0x36>; + maxim,alert-soc-level = <10>; + interrupt-parent = <&gpio7>; + interrupts = <2 IRQ_TYPE_EDGE_FALLING>; + }; -- 2.17.0 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* Re: [PATCH v2 2/4] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold 2019-04-15 1:26 ` [PATCH v2 2/4] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold Matheus Castello 2019-04-15 1:26 ` Matheus Castello @ 2019-04-15 7:13 ` Krzysztof Kozlowski 2019-04-15 7:13 ` Krzysztof Kozlowski 2019-04-29 22:13 ` Rob Herring 2 siblings, 1 reply; 82+ messages in thread From: Krzysztof Kozlowski @ 2019-04-15 7:13 UTC (permalink / raw) To: Matheus Castello Cc: sre, robh+dt, mark.rutland, Chanwoo Choi, Bartłomiej Żołnierkiewicz, lee.jones, linux-pm, devicetree, linux-kernel On Mon, 15 Apr 2019 at 03:50, Matheus Castello <matheus@castello.eng.br> wrote: > > For configure low level state of charge threshold alert signaled from > max17040 we add "maxim,alert-soc-level" property. > > Signed-off-by: Matheus Castello <matheus@castello.eng.br> > --- > .../power/supply/max17040_battery.txt | 24 +++++++++++++++++++ > 1 file changed, 24 insertions(+) > create mode 100644 Documentation/devicetree/bindings/power/supply/max17040_battery.txt > > diff --git a/Documentation/devicetree/bindings/power/supply/max17040_battery.txt b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt > new file mode 100644 > index 000000000000..9b2cc67d556f > --- /dev/null > +++ b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt > @@ -0,0 +1,24 @@ > +max17040_battery > +~~~~~~~~~~~~~~~~ > + > +Required properties : > + - compatible : "maxim,max17040" or "maxim,max77836-battery" > + > +Optional properties : > +- maxim,alert-soc-level : The alert threshold that sets the state of > + charge level where an interrupt is generated. > + Can be configured from 1 up to 32. If skipped > + the power up default value of 4 will be used. > +- interrupt-parent : The GPIO bank from the interrupt line. It does not have to be GPIO so: "phandle of the parent interrupt controller" > +- interrupts : Interrupt line see Documentation/devicetree/ > + bindings/interrupt-controller/interrupts.txt > + > +Example: > + > + battery-charger@36 { It is a fuel gauge, not battery charger. Best regards, Krzysztof > + compatible = "maxim,max17040"; > + reg = <0x36>; > + maxim,alert-soc-level = <10>; > + interrupt-parent = <&gpio7>; > + interrupts = <2 IRQ_TYPE_EDGE_FALLING>; > + }; > -- > 2.17.0 > ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH v2 2/4] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold 2019-04-15 7:13 ` Krzysztof Kozlowski @ 2019-04-15 7:13 ` Krzysztof Kozlowski 0 siblings, 0 replies; 82+ messages in thread From: Krzysztof Kozlowski @ 2019-04-15 7:13 UTC (permalink / raw) To: Matheus Castello Cc: sre, robh+dt, mark.rutland, Chanwoo Choi, Bartłomiej Żołnierkiewicz, lee.jones, linux-pm, devicetree, linux-kernel On Mon, 15 Apr 2019 at 03:50, Matheus Castello <matheus@castello.eng.br> wrote: > > For configure low level state of charge threshold alert signaled from > max17040 we add "maxim,alert-soc-level" property. > > Signed-off-by: Matheus Castello <matheus@castello.eng.br> > --- > .../power/supply/max17040_battery.txt | 24 +++++++++++++++++++ > 1 file changed, 24 insertions(+) > create mode 100644 Documentation/devicetree/bindings/power/supply/max17040_battery.txt > > diff --git a/Documentation/devicetree/bindings/power/supply/max17040_battery.txt b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt > new file mode 100644 > index 000000000000..9b2cc67d556f > --- /dev/null > +++ b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt > @@ -0,0 +1,24 @@ > +max17040_battery > +~~~~~~~~~~~~~~~~ > + > +Required properties : > + - compatible : "maxim,max17040" or "maxim,max77836-battery" > + > +Optional properties : > +- maxim,alert-soc-level : The alert threshold that sets the state of > + charge level where an interrupt is generated. > + Can be configured from 1 up to 32. If skipped > + the power up default value of 4 will be used. > +- interrupt-parent : The GPIO bank from the interrupt line. It does not have to be GPIO so: "phandle of the parent interrupt controller" > +- interrupts : Interrupt line see Documentation/devicetree/ > + bindings/interrupt-controller/interrupts.txt > + > +Example: > + > + battery-charger@36 { It is a fuel gauge, not battery charger. Best regards, Krzysztof > + compatible = "maxim,max17040"; > + reg = <0x36>; > + maxim,alert-soc-level = <10>; > + interrupt-parent = <&gpio7>; > + interrupts = <2 IRQ_TYPE_EDGE_FALLING>; > + }; > -- > 2.17.0 > ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH v2 2/4] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold 2019-04-15 1:26 ` [PATCH v2 2/4] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold Matheus Castello 2019-04-15 1:26 ` Matheus Castello 2019-04-15 7:13 ` Krzysztof Kozlowski @ 2019-04-29 22:13 ` Rob Herring 2019-04-29 22:13 ` Rob Herring 2019-05-26 23:13 ` Matheus Castello 2 siblings, 2 replies; 82+ messages in thread From: Rob Herring @ 2019-04-29 22:13 UTC (permalink / raw) To: Matheus Castello Cc: sre, krzk, mark.rutland, cw00.choi, b.zolnierkie, lee.jones, linux-pm, devicetree, linux-kernel On Sun, Apr 14, 2019 at 10:26:33PM -0300, Matheus Castello wrote: > For configure low level state of charge threshold alert signaled from > max17040 we add "maxim,alert-soc-level" property. > > Signed-off-by: Matheus Castello <matheus@castello.eng.br> > --- > .../power/supply/max17040_battery.txt | 24 +++++++++++++++++++ > 1 file changed, 24 insertions(+) > create mode 100644 Documentation/devicetree/bindings/power/supply/max17040_battery.txt > > diff --git a/Documentation/devicetree/bindings/power/supply/max17040_battery.txt b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt > new file mode 100644 > index 000000000000..9b2cc67d556f > --- /dev/null > +++ b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt > @@ -0,0 +1,24 @@ > +max17040_battery > +~~~~~~~~~~~~~~~~ > + > +Required properties : > + - compatible : "maxim,max17040" or "maxim,max77836-battery" This is really a charger, not a battery. > + > +Optional properties : > +- maxim,alert-soc-level : The alert threshold that sets the state of > + charge level where an interrupt is generated. > + Can be configured from 1 up to 32. If skipped > + the power up default value of 4 will be used. Units? This is a low or high alert? Does a common property make sense here? > +- interrupt-parent : The GPIO bank from the interrupt line. Drop this. interrupt-parent is implied. > +- interrupts : Interrupt line see Documentation/devicetree/ > + bindings/interrupt-controller/interrupts.txt > + > +Example: > + > + battery-charger@36 { > + compatible = "maxim,max17040"; > + reg = <0x36>; > + maxim,alert-soc-level = <10>; > + interrupt-parent = <&gpio7>; > + interrupts = <2 IRQ_TYPE_EDGE_FALLING>; Usually there are battery properties that need to be described too... > + }; > -- > 2.17.0 > ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH v2 2/4] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold 2019-04-29 22:13 ` Rob Herring @ 2019-04-29 22:13 ` Rob Herring 2019-05-26 23:13 ` Matheus Castello 1 sibling, 0 replies; 82+ messages in thread From: Rob Herring @ 2019-04-29 22:13 UTC (permalink / raw) To: Matheus Castello Cc: sre, krzk, mark.rutland, cw00.choi, b.zolnierkie, lee.jones, linux-pm, devicetree, linux-kernel On Sun, Apr 14, 2019 at 10:26:33PM -0300, Matheus Castello wrote: > For configure low level state of charge threshold alert signaled from > max17040 we add "maxim,alert-soc-level" property. > > Signed-off-by: Matheus Castello <matheus@castello.eng.br> > --- > .../power/supply/max17040_battery.txt | 24 +++++++++++++++++++ > 1 file changed, 24 insertions(+) > create mode 100644 Documentation/devicetree/bindings/power/supply/max17040_battery.txt > > diff --git a/Documentation/devicetree/bindings/power/supply/max17040_battery.txt b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt > new file mode 100644 > index 000000000000..9b2cc67d556f > --- /dev/null > +++ b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt > @@ -0,0 +1,24 @@ > +max17040_battery > +~~~~~~~~~~~~~~~~ > + > +Required properties : > + - compatible : "maxim,max17040" or "maxim,max77836-battery" This is really a charger, not a battery. > + > +Optional properties : > +- maxim,alert-soc-level : The alert threshold that sets the state of > + charge level where an interrupt is generated. > + Can be configured from 1 up to 32. If skipped > + the power up default value of 4 will be used. Units? This is a low or high alert? Does a common property make sense here? > +- interrupt-parent : The GPIO bank from the interrupt line. Drop this. interrupt-parent is implied. > +- interrupts : Interrupt line see Documentation/devicetree/ > + bindings/interrupt-controller/interrupts.txt > + > +Example: > + > + battery-charger@36 { > + compatible = "maxim,max17040"; > + reg = <0x36>; > + maxim,alert-soc-level = <10>; > + interrupt-parent = <&gpio7>; > + interrupts = <2 IRQ_TYPE_EDGE_FALLING>; Usually there are battery properties that need to be described too... > + }; > -- > 2.17.0 > ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH v2 2/4] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold 2019-04-29 22:13 ` Rob Herring 2019-04-29 22:13 ` Rob Herring @ 2019-05-26 23:13 ` Matheus Castello 1 sibling, 0 replies; 82+ messages in thread From: Matheus Castello @ 2019-05-26 23:13 UTC (permalink / raw) To: Rob Herring Cc: sre, krzk, mark.rutland, cw00.choi, b.zolnierkie, lee.jones, linux-pm, devicetree, linux-kernel On 4/29/19 7:13 PM, Rob Herring wrote: > On Sun, Apr 14, 2019 at 10:26:33PM -0300, Matheus Castello wrote: >> For configure low level state of charge threshold alert signaled from >> max17040 we add "maxim,alert-soc-level" property. >> >> Signed-off-by: Matheus Castello <matheus@castello.eng.br> >> --- >> .../power/supply/max17040_battery.txt | 24 +++++++++++++++++++ >> 1 file changed, 24 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/power/supply/max17040_battery.txt >> >> diff --git a/Documentation/devicetree/bindings/power/supply/max17040_battery.txt b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt >> new file mode 100644 >> index 000000000000..9b2cc67d556f >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt >> @@ -0,0 +1,24 @@ >> +max17040_battery >> +~~~~~~~~~~~~~~~~ >> + >> +Required properties : >> + - compatible : "maxim,max17040" or "maxim,max77836-battery" > > This is really a charger, not a battery. > max17040 is a fuel gauge, max77836 MUIC has it integrated. Because of this we use it in the compatible list. >> + >> +Optional properties : >> +- maxim,alert-soc-level : The alert threshold that sets the state of >> + charge level where an interrupt is generated. >> + Can be configured from 1 up to 32. If skipped >> + the power up default value of 4 will be used. > > Units? This is a low or high alert? Does a common property make sense > here? > It is a low level alert. I will change the name of the property to "maxim,alert-low-soc-level" to make this clear and I will put the percent unit in the description. I do not find any common property that I can use here, if I am wrong let me know. >> +- interrupt-parent : The GPIO bank from the interrupt line. > > Drop this. interrupt-parent is implied. Ok, I will do. > >> +- interrupts : Interrupt line see Documentation/devicetree/ >> + bindings/interrupt-controller/interrupts.txt >> + >> +Example: >> + >> + battery-charger@36 { >> + compatible = "maxim,max17040"; >> + reg = <0x36>; >> + maxim,alert-soc-level = <10>; >> + interrupt-parent = <&gpio7>; >> + interrupts = <2 IRQ_TYPE_EDGE_FALLING>; > > Usually there are battery properties that need to be described too... > I will fix this for "battery-fuel-gauge@36". I will also add the description for wake-source as optional property. Thanks. Best Regards, Matheus Castello >> + }; >> -- >> 2.17.0 >> ^ permalink raw reply [flat|nested] 82+ messages in thread
* [PATCH v2 3/4] power: supply: max17040: Config alert SOC low level threshold from FDT 2019-04-15 1:26 ` [PATCH v2 " Matheus Castello ` (2 preceding siblings ...) 2019-04-15 1:26 ` [PATCH v2 2/4] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold Matheus Castello @ 2019-04-15 1:26 ` Matheus Castello 2019-04-15 1:26 ` Matheus Castello 2019-04-15 7:27 ` Krzysztof Kozlowski 2019-04-15 1:26 ` [PATCH v2 4/4] power: supply: max17040: Send uevent in SOC changes Matheus Castello 4 siblings, 2 replies; 82+ messages in thread From: Matheus Castello @ 2019-04-15 1:26 UTC (permalink / raw) To: sre, krzk, robh+dt Cc: mark.rutland, cw00.choi, b.zolnierkie, lee.jones, linux-pm, devicetree, linux-kernel, Matheus Castello For configuration of fuel gauge alert for a low level state of charge interrupt we add a function to config level threshold and a device tree binding property to set it in flatned device tree node. Now we can use "maxim,alert-soc-level" property with the values from 1 up to 32 to configure alert interrupt threshold. Signed-off-by: Matheus Castello <matheus@castello.eng.br> --- drivers/power/supply/max17040_battery.c | 56 ++++++++++++++++++++++--- 1 file changed, 50 insertions(+), 6 deletions(-) diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c index 8d2f8ed3f44c..f036f272d52f 100644 --- a/drivers/power/supply/max17040_battery.c +++ b/drivers/power/supply/max17040_battery.c @@ -29,6 +29,9 @@ #define MAX17040_DELAY 1000 #define MAX17040_BATTERY_FULL 95 +#define MAX17040_ATHD_MASK 0xFFE0 +#define MAX17040_ATHD_DEFAULT_POWER_UP 4 + struct max17040_chip { struct i2c_client *client; struct delayed_work work; @@ -43,6 +46,8 @@ struct max17040_chip { int soc; /* State Of Charge */ int status; + /* Alert threshold from 32% to 1% of the State of Charge */ + u32 alert_threshold; }; static int max17040_get_property(struct power_supply *psy, @@ -119,6 +124,27 @@ static void max17040_get_soc(struct i2c_client *client) chip->soc = (soc >> 8); } +static int max17040_set_soc_threshold(struct i2c_client *client, u32 level) +{ + int ret; + u16 data; + + /* check if level is between 1% and 32% */ + if (level > 0 && level < 33) { + /* alert threshold use LSb 5 bits from RCOMP */ + level = 32 - level; + data = max17040_read_reg(client, MAX17040_RCOMP); + data &= MAX17040_ATHD_MASK; + data |= level; + max17040_write_reg(client, MAX17040_RCOMP, data); + ret = 0; + } else { + ret = -EINVAL; + } + + return ret; +} + static void max17040_get_version(struct i2c_client *client) { u16 version; @@ -161,6 +187,16 @@ static void max17040_get_status(struct i2c_client *client) chip->status = POWER_SUPPLY_STATUS_FULL; } +static void max17040_get_of_data(struct max17040_chip *chip) +{ + struct device *dev = &chip->client->dev; + struct device_node *np = dev->of_node; + + if (of_property_read_u32(np, "maxim,alert-soc-level", + &chip->alert_threshold)) + chip->alert_threshold = MAX17040_ATHD_DEFAULT_POWER_UP; +} + static void max17040_check_changes(struct i2c_client *client) { max17040_get_vcell(client); @@ -226,6 +262,7 @@ static int max17040_probe(struct i2c_client *client, chip->client = client; chip->pdata = client->dev.platform_data; + max17040_get_of_data(chip); i2c_set_clientdata(client, chip); psy_cfg.drv_data = chip; @@ -237,16 +274,26 @@ static int max17040_probe(struct i2c_client *client, return PTR_ERR(chip->battery); } + max17040_reset(client); + max17040_get_version(client); + /* check interrupt */ if (client->irq) { int ret; - unsigned int flags; + + ret = max17040_set_soc_threshold(client, chip->alert_threshold); + if (ret) { + dev_err(&client->dev, + "Failed to set SOC alert threshold: err %d\n", + ret); + return ret; + } dev_info(&client->dev, "IRQ: enabled\n"); - flags = IRQF_TRIGGER_FALLING | IRQF_ONESHOT; ret = devm_request_threaded_irq(&client->dev, client->irq, NULL, - max17040_thread_handler, flags, + max17040_thread_handler, + (client->flags | IRQF_ONESHOT), chip->battery->desc->name, chip); @@ -258,9 +305,6 @@ static int max17040_probe(struct i2c_client *client, } } - max17040_reset(client); - max17040_get_version(client); - INIT_DEFERRABLE_WORK(&chip->work, max17040_work); queue_delayed_work(system_power_efficient_wq, &chip->work, MAX17040_DELAY); -- 2.17.0 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* [PATCH v2 3/4] power: supply: max17040: Config alert SOC low level threshold from FDT 2019-04-15 1:26 ` [PATCH v2 3/4] power: supply: max17040: Config alert SOC low level threshold from FDT Matheus Castello @ 2019-04-15 1:26 ` Matheus Castello 2019-04-15 7:27 ` Krzysztof Kozlowski 1 sibling, 0 replies; 82+ messages in thread From: Matheus Castello @ 2019-04-15 1:26 UTC (permalink / raw) To: sre, krzk, robh+dt Cc: mark.rutland, cw00.choi, b.zolnierkie, lee.jones, linux-pm, devicetree, linux-kernel, Matheus Castello For configuration of fuel gauge alert for a low level state of charge interrupt we add a function to config level threshold and a device tree binding property to set it in flatned device tree node. Now we can use "maxim,alert-soc-level" property with the values from 1 up to 32 to configure alert interrupt threshold. Signed-off-by: Matheus Castello <matheus@castello.eng.br> --- drivers/power/supply/max17040_battery.c | 56 ++++++++++++++++++++++--- 1 file changed, 50 insertions(+), 6 deletions(-) diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c index 8d2f8ed3f44c..f036f272d52f 100644 --- a/drivers/power/supply/max17040_battery.c +++ b/drivers/power/supply/max17040_battery.c @@ -29,6 +29,9 @@ #define MAX17040_DELAY 1000 #define MAX17040_BATTERY_FULL 95 +#define MAX17040_ATHD_MASK 0xFFE0 +#define MAX17040_ATHD_DEFAULT_POWER_UP 4 + struct max17040_chip { struct i2c_client *client; struct delayed_work work; @@ -43,6 +46,8 @@ struct max17040_chip { int soc; /* State Of Charge */ int status; + /* Alert threshold from 32% to 1% of the State of Charge */ + u32 alert_threshold; }; static int max17040_get_property(struct power_supply *psy, @@ -119,6 +124,27 @@ static void max17040_get_soc(struct i2c_client *client) chip->soc = (soc >> 8); } +static int max17040_set_soc_threshold(struct i2c_client *client, u32 level) +{ + int ret; + u16 data; + + /* check if level is between 1% and 32% */ + if (level > 0 && level < 33) { + /* alert threshold use LSb 5 bits from RCOMP */ + level = 32 - level; + data = max17040_read_reg(client, MAX17040_RCOMP); + data &= MAX17040_ATHD_MASK; + data |= level; + max17040_write_reg(client, MAX17040_RCOMP, data); + ret = 0; + } else { + ret = -EINVAL; + } + + return ret; +} + static void max17040_get_version(struct i2c_client *client) { u16 version; @@ -161,6 +187,16 @@ static void max17040_get_status(struct i2c_client *client) chip->status = POWER_SUPPLY_STATUS_FULL; } +static void max17040_get_of_data(struct max17040_chip *chip) +{ + struct device *dev = &chip->client->dev; + struct device_node *np = dev->of_node; + + if (of_property_read_u32(np, "maxim,alert-soc-level", + &chip->alert_threshold)) + chip->alert_threshold = MAX17040_ATHD_DEFAULT_POWER_UP; +} + static void max17040_check_changes(struct i2c_client *client) { max17040_get_vcell(client); @@ -226,6 +262,7 @@ static int max17040_probe(struct i2c_client *client, chip->client = client; chip->pdata = client->dev.platform_data; + max17040_get_of_data(chip); i2c_set_clientdata(client, chip); psy_cfg.drv_data = chip; @@ -237,16 +274,26 @@ static int max17040_probe(struct i2c_client *client, return PTR_ERR(chip->battery); } + max17040_reset(client); + max17040_get_version(client); + /* check interrupt */ if (client->irq) { int ret; - unsigned int flags; + + ret = max17040_set_soc_threshold(client, chip->alert_threshold); + if (ret) { + dev_err(&client->dev, + "Failed to set SOC alert threshold: err %d\n", + ret); + return ret; + } dev_info(&client->dev, "IRQ: enabled\n"); - flags = IRQF_TRIGGER_FALLING | IRQF_ONESHOT; ret = devm_request_threaded_irq(&client->dev, client->irq, NULL, - max17040_thread_handler, flags, + max17040_thread_handler, + (client->flags | IRQF_ONESHOT), chip->battery->desc->name, chip); @@ -258,9 +305,6 @@ static int max17040_probe(struct i2c_client *client, } } - max17040_reset(client); - max17040_get_version(client); - INIT_DEFERRABLE_WORK(&chip->work, max17040_work); queue_delayed_work(system_power_efficient_wq, &chip->work, MAX17040_DELAY); -- 2.17.0 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* Re: [PATCH v2 3/4] power: supply: max17040: Config alert SOC low level threshold from FDT 2019-04-15 1:26 ` [PATCH v2 3/4] power: supply: max17040: Config alert SOC low level threshold from FDT Matheus Castello 2019-04-15 1:26 ` Matheus Castello @ 2019-04-15 7:27 ` Krzysztof Kozlowski 2019-04-15 7:27 ` Krzysztof Kozlowski 1 sibling, 1 reply; 82+ messages in thread From: Krzysztof Kozlowski @ 2019-04-15 7:27 UTC (permalink / raw) To: Matheus Castello Cc: sre, robh+dt, mark.rutland, Chanwoo Choi, Bartłomiej Żołnierkiewicz, lee.jones, linux-pm, devicetree, linux-kernel On Mon, 15 Apr 2019 at 03:51, Matheus Castello <matheus@castello.eng.br> wrote: > > For configuration of fuel gauge alert for a low level state of charge > interrupt we add a function to config level threshold and a device tree > binding property to set it in flatned device tree node. > > Now we can use "maxim,alert-soc-level" property with the values from > 1 up to 32 to configure alert interrupt threshold. > > Signed-off-by: Matheus Castello <matheus@castello.eng.br> > --- > drivers/power/supply/max17040_battery.c | 56 ++++++++++++++++++++++--- > 1 file changed, 50 insertions(+), 6 deletions(-) > > diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c > index 8d2f8ed3f44c..f036f272d52f 100644 > --- a/drivers/power/supply/max17040_battery.c > +++ b/drivers/power/supply/max17040_battery.c > @@ -29,6 +29,9 @@ > #define MAX17040_DELAY 1000 > #define MAX17040_BATTERY_FULL 95 > > +#define MAX17040_ATHD_MASK 0xFFE0 > +#define MAX17040_ATHD_DEFAULT_POWER_UP 4 > + > struct max17040_chip { > struct i2c_client *client; > struct delayed_work work; > @@ -43,6 +46,8 @@ struct max17040_chip { > int soc; > /* State Of Charge */ > int status; > + /* Alert threshold from 32% to 1% of the State of Charge */ > + u32 alert_threshold; > }; > > static int max17040_get_property(struct power_supply *psy, > @@ -119,6 +124,27 @@ static void max17040_get_soc(struct i2c_client *client) > chip->soc = (soc >> 8); > } > > +static int max17040_set_soc_threshold(struct i2c_client *client, u32 level) > +{ > + int ret; > + u16 data; > + > + /* check if level is between 1% and 32% */ > + if (level > 0 && level < 33) { > + /* alert threshold use LSb 5 bits from RCOMP */ > + level = 32 - level; > + data = max17040_read_reg(client, MAX17040_RCOMP); > + data &= MAX17040_ATHD_MASK; > + data |= level; > + max17040_write_reg(client, MAX17040_RCOMP, data); > + ret = 0; > + } else { > + ret = -EINVAL; > + } > + > + return ret; > +} > + > static void max17040_get_version(struct i2c_client *client) > { > u16 version; > @@ -161,6 +187,16 @@ static void max17040_get_status(struct i2c_client *client) > chip->status = POWER_SUPPLY_STATUS_FULL; > } > > +static void max17040_get_of_data(struct max17040_chip *chip) > +{ > + struct device *dev = &chip->client->dev; > + struct device_node *np = dev->of_node; > + > + if (of_property_read_u32(np, "maxim,alert-soc-level", > + &chip->alert_threshold)) > + chip->alert_threshold = MAX17040_ATHD_DEFAULT_POWER_UP; > +} > + > static void max17040_check_changes(struct i2c_client *client) > { > max17040_get_vcell(client); > @@ -226,6 +262,7 @@ static int max17040_probe(struct i2c_client *client, > > chip->client = client; > chip->pdata = client->dev.platform_data; > + max17040_get_of_data(chip); > > i2c_set_clientdata(client, chip); > psy_cfg.drv_data = chip; > @@ -237,16 +274,26 @@ static int max17040_probe(struct i2c_client *client, > return PTR_ERR(chip->battery); > } > > + max17040_reset(client); > + max17040_get_version(client); > + > /* check interrupt */ > if (client->irq) { > int ret; > - unsigned int flags; > + > + ret = max17040_set_soc_threshold(client, chip->alert_threshold); > + if (ret) { > + dev_err(&client->dev, > + "Failed to set SOC alert threshold: err %d\n", > + ret); > + return ret; > + } > > dev_info(&client->dev, "IRQ: enabled\n"); > - flags = IRQF_TRIGGER_FALLING | IRQF_ONESHOT; > > ret = devm_request_threaded_irq(&client->dev, client->irq, NULL, > - max17040_thread_handler, flags, > + max17040_thread_handler, > + (client->flags | IRQF_ONESHOT), > chip->battery->desc->name, > chip); Now I noticed that you are not clearing the ALRT status bit. Therefore alert interrupt will be generated only once. Even if SoC goes up (charged) and then down to alert level it will not be generated second time. At least documentation for max77836 is saying that clearing this bit is required. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH v2 3/4] power: supply: max17040: Config alert SOC low level threshold from FDT 2019-04-15 7:27 ` Krzysztof Kozlowski @ 2019-04-15 7:27 ` Krzysztof Kozlowski 0 siblings, 0 replies; 82+ messages in thread From: Krzysztof Kozlowski @ 2019-04-15 7:27 UTC (permalink / raw) To: Matheus Castello Cc: sre, robh+dt, mark.rutland, Chanwoo Choi, Bartłomiej Żołnierkiewicz, lee.jones, linux-pm, devicetree, linux-kernel On Mon, 15 Apr 2019 at 03:51, Matheus Castello <matheus@castello.eng.br> wrote: > > For configuration of fuel gauge alert for a low level state of charge > interrupt we add a function to config level threshold and a device tree > binding property to set it in flatned device tree node. > > Now we can use "maxim,alert-soc-level" property with the values from > 1 up to 32 to configure alert interrupt threshold. > > Signed-off-by: Matheus Castello <matheus@castello.eng.br> > --- > drivers/power/supply/max17040_battery.c | 56 ++++++++++++++++++++++--- > 1 file changed, 50 insertions(+), 6 deletions(-) > > diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c > index 8d2f8ed3f44c..f036f272d52f 100644 > --- a/drivers/power/supply/max17040_battery.c > +++ b/drivers/power/supply/max17040_battery.c > @@ -29,6 +29,9 @@ > #define MAX17040_DELAY 1000 > #define MAX17040_BATTERY_FULL 95 > > +#define MAX17040_ATHD_MASK 0xFFE0 > +#define MAX17040_ATHD_DEFAULT_POWER_UP 4 > + > struct max17040_chip { > struct i2c_client *client; > struct delayed_work work; > @@ -43,6 +46,8 @@ struct max17040_chip { > int soc; > /* State Of Charge */ > int status; > + /* Alert threshold from 32% to 1% of the State of Charge */ > + u32 alert_threshold; > }; > > static int max17040_get_property(struct power_supply *psy, > @@ -119,6 +124,27 @@ static void max17040_get_soc(struct i2c_client *client) > chip->soc = (soc >> 8); > } > > +static int max17040_set_soc_threshold(struct i2c_client *client, u32 level) > +{ > + int ret; > + u16 data; > + > + /* check if level is between 1% and 32% */ > + if (level > 0 && level < 33) { > + /* alert threshold use LSb 5 bits from RCOMP */ > + level = 32 - level; > + data = max17040_read_reg(client, MAX17040_RCOMP); > + data &= MAX17040_ATHD_MASK; > + data |= level; > + max17040_write_reg(client, MAX17040_RCOMP, data); > + ret = 0; > + } else { > + ret = -EINVAL; > + } > + > + return ret; > +} > + > static void max17040_get_version(struct i2c_client *client) > { > u16 version; > @@ -161,6 +187,16 @@ static void max17040_get_status(struct i2c_client *client) > chip->status = POWER_SUPPLY_STATUS_FULL; > } > > +static void max17040_get_of_data(struct max17040_chip *chip) > +{ > + struct device *dev = &chip->client->dev; > + struct device_node *np = dev->of_node; > + > + if (of_property_read_u32(np, "maxim,alert-soc-level", > + &chip->alert_threshold)) > + chip->alert_threshold = MAX17040_ATHD_DEFAULT_POWER_UP; > +} > + > static void max17040_check_changes(struct i2c_client *client) > { > max17040_get_vcell(client); > @@ -226,6 +262,7 @@ static int max17040_probe(struct i2c_client *client, > > chip->client = client; > chip->pdata = client->dev.platform_data; > + max17040_get_of_data(chip); > > i2c_set_clientdata(client, chip); > psy_cfg.drv_data = chip; > @@ -237,16 +274,26 @@ static int max17040_probe(struct i2c_client *client, > return PTR_ERR(chip->battery); > } > > + max17040_reset(client); > + max17040_get_version(client); > + > /* check interrupt */ > if (client->irq) { > int ret; > - unsigned int flags; > + > + ret = max17040_set_soc_threshold(client, chip->alert_threshold); > + if (ret) { > + dev_err(&client->dev, > + "Failed to set SOC alert threshold: err %d\n", > + ret); > + return ret; > + } > > dev_info(&client->dev, "IRQ: enabled\n"); > - flags = IRQF_TRIGGER_FALLING | IRQF_ONESHOT; > > ret = devm_request_threaded_irq(&client->dev, client->irq, NULL, > - max17040_thread_handler, flags, > + max17040_thread_handler, > + (client->flags | IRQF_ONESHOT), > chip->battery->desc->name, > chip); Now I noticed that you are not clearing the ALRT status bit. Therefore alert interrupt will be generated only once. Even if SoC goes up (charged) and then down to alert level it will not be generated second time. At least documentation for max77836 is saying that clearing this bit is required. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 82+ messages in thread
* [PATCH v2 4/4] power: supply: max17040: Send uevent in SOC changes 2019-04-15 1:26 ` [PATCH v2 " Matheus Castello ` (3 preceding siblings ...) 2019-04-15 1:26 ` [PATCH v2 3/4] power: supply: max17040: Config alert SOC low level threshold from FDT Matheus Castello @ 2019-04-15 1:26 ` Matheus Castello 2019-04-15 1:26 ` Matheus Castello 2019-04-15 7:30 ` Krzysztof Kozlowski 4 siblings, 2 replies; 82+ messages in thread From: Matheus Castello @ 2019-04-15 1:26 UTC (permalink / raw) To: sre, krzk, robh+dt Cc: mark.rutland, cw00.choi, b.zolnierkie, lee.jones, linux-pm, devicetree, linux-kernel, Matheus Castello Notify core through power_supply_changed() in case of changes in state of charge. This is useful for user-space to efficiently update current battery level. Signed-off-by: Matheus Castello <matheus@castello.eng.br> --- drivers/power/supply/max17040_battery.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c index f036f272d52f..db901ebf495d 100644 --- a/drivers/power/supply/max17040_battery.c +++ b/drivers/power/supply/max17040_battery.c @@ -208,10 +208,17 @@ static void max17040_check_changes(struct i2c_client *client) static void max17040_work(struct work_struct *work) { struct max17040_chip *chip; + int last_soc; chip = container_of(work, struct max17040_chip, work.work); + /* store SOC for check change */ + last_soc = chip->soc; max17040_check_changes(chip->client); + /* check changes and send uevent */ + if (last_soc != chip->soc) + power_supply_changed(chip->battery); + queue_delayed_work(system_power_efficient_wq, &chip->work, MAX17040_DELAY); } -- 2.17.0 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* [PATCH v2 4/4] power: supply: max17040: Send uevent in SOC changes 2019-04-15 1:26 ` [PATCH v2 4/4] power: supply: max17040: Send uevent in SOC changes Matheus Castello @ 2019-04-15 1:26 ` Matheus Castello 2019-04-15 7:30 ` Krzysztof Kozlowski 1 sibling, 0 replies; 82+ messages in thread From: Matheus Castello @ 2019-04-15 1:26 UTC (permalink / raw) To: sre, krzk, robh+dt Cc: mark.rutland, cw00.choi, b.zolnierkie, lee.jones, linux-pm, devicetree, linux-kernel, Matheus Castello Notify core through power_supply_changed() in case of changes in state of charge. This is useful for user-space to efficiently update current battery level. Signed-off-by: Matheus Castello <matheus@castello.eng.br> --- drivers/power/supply/max17040_battery.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c index f036f272d52f..db901ebf495d 100644 --- a/drivers/power/supply/max17040_battery.c +++ b/drivers/power/supply/max17040_battery.c @@ -208,10 +208,17 @@ static void max17040_check_changes(struct i2c_client *client) static void max17040_work(struct work_struct *work) { struct max17040_chip *chip; + int last_soc; chip = container_of(work, struct max17040_chip, work.work); + /* store SOC for check change */ + last_soc = chip->soc; max17040_check_changes(chip->client); + /* check changes and send uevent */ + if (last_soc != chip->soc) + power_supply_changed(chip->battery); + queue_delayed_work(system_power_efficient_wq, &chip->work, MAX17040_DELAY); } -- 2.17.0 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* Re: [PATCH v2 4/4] power: supply: max17040: Send uevent in SOC changes 2019-04-15 1:26 ` [PATCH v2 4/4] power: supply: max17040: Send uevent in SOC changes Matheus Castello 2019-04-15 1:26 ` Matheus Castello @ 2019-04-15 7:30 ` Krzysztof Kozlowski 2019-04-15 7:30 ` Krzysztof Kozlowski 2019-05-27 2:22 ` [PATCH v3 0/5] power: supply: MAX17040: Add IRQ for low level and alert " Matheus Castello 1 sibling, 2 replies; 82+ messages in thread From: Krzysztof Kozlowski @ 2019-04-15 7:30 UTC (permalink / raw) To: Matheus Castello Cc: sre, robh+dt, mark.rutland, Chanwoo Choi, Bartłomiej Żołnierkiewicz, lee.jones, linux-pm, devicetree, linux-kernel On Mon, 15 Apr 2019 at 03:48, Matheus Castello <matheus@castello.eng.br> wrote: > > Notify core through power_supply_changed() in case of changes in state > of charge. This is useful for user-space to efficiently update current > battery level. > > Signed-off-by: Matheus Castello <matheus@castello.eng.br> > --- > drivers/power/supply/max17040_battery.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c > index f036f272d52f..db901ebf495d 100644 > --- a/drivers/power/supply/max17040_battery.c > +++ b/drivers/power/supply/max17040_battery.c > @@ -208,10 +208,17 @@ static void max17040_check_changes(struct i2c_client *client) > static void max17040_work(struct work_struct *work) > { > struct max17040_chip *chip; > + int last_soc; > > chip = container_of(work, struct max17040_chip, work.work); > + /* store SOC for check change */ > + last_soc = chip->soc; > max17040_check_changes(chip->client); > > + /* check changes and send uevent */ > + if (last_soc != chip->soc) chip->soc could be negative ERRNO so in such case I think user-space should not be notified. > + power_supply_changed(chip->battery); > + You should also notify on online and status change (e.g. started charging). User-space also wants to know that, e.g. to show the charging icon or battery health status. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH v2 4/4] power: supply: max17040: Send uevent in SOC changes 2019-04-15 7:30 ` Krzysztof Kozlowski @ 2019-04-15 7:30 ` Krzysztof Kozlowski 2019-05-27 2:22 ` [PATCH v3 0/5] power: supply: MAX17040: Add IRQ for low level and alert " Matheus Castello 1 sibling, 0 replies; 82+ messages in thread From: Krzysztof Kozlowski @ 2019-04-15 7:30 UTC (permalink / raw) To: Matheus Castello Cc: sre, robh+dt, mark.rutland, Chanwoo Choi, Bartłomiej Żołnierkiewicz, lee.jones, linux-pm, devicetree, linux-kernel On Mon, 15 Apr 2019 at 03:48, Matheus Castello <matheus@castello.eng.br> wrote: > > Notify core through power_supply_changed() in case of changes in state > of charge. This is useful for user-space to efficiently update current > battery level. > > Signed-off-by: Matheus Castello <matheus@castello.eng.br> > --- > drivers/power/supply/max17040_battery.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c > index f036f272d52f..db901ebf495d 100644 > --- a/drivers/power/supply/max17040_battery.c > +++ b/drivers/power/supply/max17040_battery.c > @@ -208,10 +208,17 @@ static void max17040_check_changes(struct i2c_client *client) > static void max17040_work(struct work_struct *work) > { > struct max17040_chip *chip; > + int last_soc; > > chip = container_of(work, struct max17040_chip, work.work); > + /* store SOC for check change */ > + last_soc = chip->soc; > max17040_check_changes(chip->client); > > + /* check changes and send uevent */ > + if (last_soc != chip->soc) chip->soc could be negative ERRNO so in such case I think user-space should not be notified. > + power_supply_changed(chip->battery); > + You should also notify on online and status change (e.g. started charging). User-space also wants to know that, e.g. to show the charging icon or battery health status. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 82+ messages in thread
* [PATCH v3 0/5] power: supply: MAX17040: Add IRQ for low level and alert SOC changes 2019-04-15 7:30 ` Krzysztof Kozlowski 2019-04-15 7:30 ` Krzysztof Kozlowski @ 2019-05-27 2:22 ` Matheus Castello 2019-05-27 2:22 ` [PATCH v3 1/5] power: supply: max17040: Add IRQ handler for low SOC alert Matheus Castello ` (4 more replies) 1 sibling, 5 replies; 82+ messages in thread From: Matheus Castello @ 2019-05-27 2:22 UTC (permalink / raw) To: sre, krzk, robh+dt Cc: mark.rutland, cw00.choi, b.zolnierkie, lee.jones, linux-pm, devicetree, linux-kernel, Matheus Castello This series add IRQ handler for low level SOC alert, define a devicetree binding attribute to configure the alert level threshold and check for changes in SOC and power supply status for send uevents. Max17040 have a pin for alert host about low level state of charge and this alert can be configured in a threshold from 1% up to 32% of SOC. Tested on Toradex Colibri iMX7D, with a SparkFun Lipo Fuel Gauge module based on MAXIM MAX17043. Thanks Krzysztof and Rob for yours time reviewing it. Let me know what you think about the fixes. Krzysztof I added a new patch to the series to check the battery charge up and clear ALRT bit when the SOC is above alert threshold, so not generating duplicate interrupts. Changes since v1: (Suggested by Krzysztof Kozlowski) - Put common code from max17040_work and max17040_thread_handler in a function - Code style fixes - Define mask and low level threshold alert default - Check return value from max17040_set_soc_threshold - Set low level state of charge threshold before IRQ - CC maintainers from drivers/mfd/max14577 - Use flags from FDT client->flags instead hard coded it - Mention interrupts on DT Documentation - Fix "maxim,max77836-battery" missed from DT Documentation - Fix commit description Changes since v2: (Suggested by Krzysztof Kozlowski) - Fix ebusy exception - Remove device_init_wakeup from probe, let wakeup-source from DT decide that - Fix the use of "charger" to fuel-gauge on device tree description - Clear ALRT bit while setting the SOC threshold - Check SOC and clear ALRT bit when the SOC are above alert threshold - Fix unnecessary uevent when SOC is an ERRNO - Notify user space when power supply status change (Suggested by Rob Herring) - Fix the use of "charger" to fuel-gauge on device tree description - Add the (%) units on the description of property - Drop interrupt-parent - Fix name of property to let clear that is a low level SOC alert Matheus Castello (5): power: supply: max17040: Add IRQ handler for low SOC alert dt-bindings: power: supply: Max17040: Add low level SOC alert threshold power: supply: max17040: Config alert SOC low level threshold from FDT power: supply: max17040: Clear ALRT bit when the SOC are above threshold power: supply: max17040: Send uevent in SOC and status change .../power/supply/max17040_battery.txt | 28 ++++ drivers/power/supply/max17040_battery.c | 134 +++++++++++++++++- 2 files changed, 158 insertions(+), 4 deletions(-) create mode 100644 Documentation/devicetree/bindings/power/supply/max17040_battery.txt -- 2.20.1 ^ permalink raw reply [flat|nested] 82+ messages in thread
* [PATCH v3 1/5] power: supply: max17040: Add IRQ handler for low SOC alert 2019-05-27 2:22 ` [PATCH v3 0/5] power: supply: MAX17040: Add IRQ for low level and alert " Matheus Castello @ 2019-05-27 2:22 ` Matheus Castello 2019-05-29 14:26 ` Krzysztof Kozlowski 2019-05-27 2:22 ` [PATCH v3 2/5] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold Matheus Castello ` (3 subsequent siblings) 4 siblings, 1 reply; 82+ messages in thread From: Matheus Castello @ 2019-05-27 2:22 UTC (permalink / raw) To: sre, krzk, robh+dt Cc: mark.rutland, cw00.choi, b.zolnierkie, lee.jones, linux-pm, devicetree, linux-kernel, Matheus Castello According datasheet max17040 has a pin for alert host for low SOC. This pin can be used as external interrupt, so we need to check for interrupts assigned for device and handle it. In handler we are checking and storing fuel gauge registers values and send an uevent to notificate user space, so user space can decide save work or turn off since the alert demonstrate that the battery may no have the power to keep the system turned on for much longer. Signed-off-by: Matheus Castello <matheus@castello.eng.br> --- drivers/power/supply/max17040_battery.c | 65 +++++++++++++++++++++++-- 1 file changed, 60 insertions(+), 5 deletions(-) diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c index 91cafc7bed30..b7433e9ca7c2 100644 --- a/drivers/power/supply/max17040_battery.c +++ b/drivers/power/supply/max17040_battery.c @@ -13,6 +13,7 @@ #include <linux/err.h> #include <linux/i2c.h> #include <linux/delay.h> +#include <linux/interrupt.h> #include <linux/power_supply.h> #include <linux/max17040_battery.h> #include <linux/slab.h> @@ -160,21 +161,40 @@ static void max17040_get_status(struct i2c_client *client) chip->status = POWER_SUPPLY_STATUS_FULL; } +static void max17040_check_changes(struct i2c_client *client) +{ + max17040_get_vcell(client); + max17040_get_soc(client); + max17040_get_online(client); + max17040_get_status(client); +} + static void max17040_work(struct work_struct *work) { struct max17040_chip *chip; chip = container_of(work, struct max17040_chip, work.work); - - max17040_get_vcell(chip->client); - max17040_get_soc(chip->client); - max17040_get_online(chip->client); - max17040_get_status(chip->client); + max17040_check_changes(chip->client); queue_delayed_work(system_power_efficient_wq, &chip->work, MAX17040_DELAY); } +static irqreturn_t max17040_thread_handler(int id, void *dev) +{ + struct max17040_chip *chip = dev; + struct i2c_client *client = chip->client; + + dev_warn(&client->dev, "IRQ: Alert battery low level"); + /* read registers */ + max17040_check_changes(chip->client); + + /* send uevent */ + power_supply_changed(chip->battery); + + return IRQ_HANDLED; +} + static enum power_supply_property max17040_battery_props[] = { POWER_SUPPLY_PROP_STATUS, POWER_SUPPLY_PROP_ONLINE, @@ -220,6 +240,25 @@ static int max17040_probe(struct i2c_client *client, max17040_reset(client); max17040_get_version(client); + /* check interrupt */ + if (client->irq) { + int ret; + unsigned int flags; + + dev_info(&client->dev, "IRQ: enabled\n"); + flags = IRQF_TRIGGER_FALLING | IRQF_ONESHOT; + ret = devm_request_threaded_irq(&client->dev, client->irq, NULL, + max17040_thread_handler, flags, + chip->battery->desc->name, + chip); + + if (ret) { + client->irq = 0; + dev_warn(&client->dev, + "Failed to get IRQ err %d\n", ret); + } + } + INIT_DEFERRABLE_WORK(&chip->work, max17040_work); queue_delayed_work(system_power_efficient_wq, &chip->work, MAX17040_DELAY); @@ -244,6 +283,14 @@ static int max17040_suspend(struct device *dev) struct max17040_chip *chip = i2c_get_clientdata(client); cancel_delayed_work(&chip->work); + + if (client->irq) { + if (device_may_wakeup(dev)) + enable_irq_wake(client->irq); + else + disable_irq_wake(client->irq); + } + return 0; } @@ -254,6 +301,14 @@ static int max17040_resume(struct device *dev) queue_delayed_work(system_power_efficient_wq, &chip->work, MAX17040_DELAY); + + if (client->irq) { + if (device_may_wakeup(dev)) + disable_irq_wake(client->irq); + else + enable_irq_wake(client->irq); + } + return 0; } -- 2.20.1 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* Re: [PATCH v3 1/5] power: supply: max17040: Add IRQ handler for low SOC alert 2019-05-27 2:22 ` [PATCH v3 1/5] power: supply: max17040: Add IRQ handler for low SOC alert Matheus Castello @ 2019-05-29 14:26 ` Krzysztof Kozlowski 0 siblings, 0 replies; 82+ messages in thread From: Krzysztof Kozlowski @ 2019-05-29 14:26 UTC (permalink / raw) To: Matheus Castello Cc: sre, robh+dt, mark.rutland, Chanwoo Choi, Bartłomiej Żołnierkiewicz, lee.jones, linux-pm, devicetree, linux-kernel On Mon, 27 May 2019 at 04:45, Matheus Castello <matheus@castello.eng.br> wrote: > > According datasheet max17040 has a pin for alert host for low SOC. > This pin can be used as external interrupt, so we need to check for > interrupts assigned for device and handle it. > > In handler we are checking and storing fuel gauge registers values > and send an uevent to notificate user space, so user space can decide > save work or turn off since the alert demonstrate that the battery may > no have the power to keep the system turned on for much longer. > > Signed-off-by: Matheus Castello <matheus@castello.eng.br> > --- > drivers/power/supply/max17040_battery.c | 65 +++++++++++++++++++++++-- > 1 file changed, 60 insertions(+), 5 deletions(-) Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org> Best regards, Krzysztof ^ permalink raw reply [flat|nested] 82+ messages in thread
* [PATCH v3 2/5] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold 2019-05-27 2:22 ` [PATCH v3 0/5] power: supply: MAX17040: Add IRQ for low level and alert " Matheus Castello 2019-05-27 2:22 ` [PATCH v3 1/5] power: supply: max17040: Add IRQ handler for low SOC alert Matheus Castello @ 2019-05-27 2:22 ` Matheus Castello 2019-05-29 14:40 ` Krzysztof Kozlowski 2019-05-29 14:57 ` Krzysztof Kozlowski 2019-05-27 2:22 ` [PATCH v3 3/5] power: supply: max17040: Config alert SOC low level threshold from FDT Matheus Castello ` (2 subsequent siblings) 4 siblings, 2 replies; 82+ messages in thread From: Matheus Castello @ 2019-05-27 2:22 UTC (permalink / raw) To: sre, krzk, robh+dt Cc: mark.rutland, cw00.choi, b.zolnierkie, lee.jones, linux-pm, devicetree, linux-kernel, Matheus Castello For configure low level state of charge threshold alert signaled from max17040 we add "maxim,alert-low-soc-level" property. Signed-off-by: Matheus Castello <matheus@castello.eng.br> --- .../power/supply/max17040_battery.txt | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 Documentation/devicetree/bindings/power/supply/max17040_battery.txt diff --git a/Documentation/devicetree/bindings/power/supply/max17040_battery.txt b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt new file mode 100644 index 000000000000..a13e8d50ff7b --- /dev/null +++ b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt @@ -0,0 +1,28 @@ +max17040_battery +~~~~~~~~~~~~~~~~ + +Required properties : + - compatible : "maxim,max17040" or "maxim,max77836-battery" + +Optional properties : +- maxim,alert-low-soc-level : The alert threshold that sets the state of + charge level (%) where an interrupt is + generated. Can be configured from 1 up to 32 + (%). If skipped the power up default value of + 4 (%) will be used. +- interrupts : Interrupt line see Documentation/devicetree/ + bindings/interrupt-controller/interrupts.txt +- wakeup-source : This device has wakeup capabilities. Use this + property to use alert low SOC level interrupt + as wake up source. + +Example: + + battery-fuel-gauge@36 { + compatible = "maxim,max17040"; + reg = <0x36>; + maxim,alert-low-soc-level = <10>; + interrupt-parent = <&gpio7>; + interrupts = <2 IRQ_TYPE_EDGE_FALLING>; + wakeup-source; + }; -- 2.20.1 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* Re: [PATCH v3 2/5] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold 2019-05-27 2:22 ` [PATCH v3 2/5] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold Matheus Castello @ 2019-05-29 14:40 ` Krzysztof Kozlowski 2019-05-29 14:57 ` Krzysztof Kozlowski 1 sibling, 0 replies; 82+ messages in thread From: Krzysztof Kozlowski @ 2019-05-29 14:40 UTC (permalink / raw) To: Matheus Castello Cc: sre, robh+dt, mark.rutland, Chanwoo Choi, Bartłomiej Żołnierkiewicz, lee.jones, linux-pm, devicetree, linux-kernel On Mon, 27 May 2019 at 04:45, Matheus Castello <matheus@castello.eng.br> wrote: > > For configure low level state of charge threshold alert signaled from > max17040 we add "maxim,alert-low-soc-level" property. > > Signed-off-by: Matheus Castello <matheus@castello.eng.br> > --- > .../power/supply/max17040_battery.txt | 28 +++++++++++++++++++ > 1 file changed, 28 insertions(+) > create mode 100644 Documentation/devicetree/bindings/power/supply/max17040_battery.txt > > diff --git a/Documentation/devicetree/bindings/power/supply/max17040_battery.txt b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt > new file mode 100644 > index 000000000000..a13e8d50ff7b > --- /dev/null > +++ b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt > @@ -0,0 +1,28 @@ > +max17040_battery > +~~~~~~~~~~~~~~~~ > + > +Required properties : > + - compatible : "maxim,max17040" or "maxim,max77836-battery" > + > +Optional properties : > +- maxim,alert-low-soc-level : The alert threshold that sets the state of > + charge level (%) where an interrupt is > + generated. Can be configured from 1 up to 32 > + (%). If skipped the power up default value of > + 4 (%) will be used. > +- interrupts : Interrupt line see Documentation/devicetree/ > + bindings/interrupt-controller/interrupts.txt Based on driver's behavior, I understand that these two properties come in pair so maxim,alert-low-soc-level (or its default value) will be used if and only if interrupts property is present. Maybe mention this? In general looks fine to me: Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org> Best regards, Krzysztof > +- wakeup-source : This device has wakeup capabilities. Use this > + property to use alert low SOC level interrupt > + as wake up source. > + > +Example: > + > + battery-fuel-gauge@36 { > + compatible = "maxim,max17040"; > + reg = <0x36>; > + maxim,alert-low-soc-level = <10>; > + interrupt-parent = <&gpio7>; > + interrupts = <2 IRQ_TYPE_EDGE_FALLING>; > + wakeup-source; > + }; > -- > 2.20.1 > ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH v3 2/5] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold 2019-05-27 2:22 ` [PATCH v3 2/5] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold Matheus Castello 2019-05-29 14:40 ` Krzysztof Kozlowski @ 2019-05-29 14:57 ` Krzysztof Kozlowski 2019-06-02 21:38 ` Matheus Castello 1 sibling, 1 reply; 82+ messages in thread From: Krzysztof Kozlowski @ 2019-05-29 14:57 UTC (permalink / raw) To: Matheus Castello Cc: sre, robh+dt, mark.rutland, Chanwoo Choi, Bartłomiej Żołnierkiewicz, lee.jones, linux-pm, devicetree, linux-kernel On Mon, 27 May 2019 at 04:45, Matheus Castello <matheus@castello.eng.br> wrote: > > For configure low level state of charge threshold alert signaled from > max17040 we add "maxim,alert-low-soc-level" property. > > Signed-off-by: Matheus Castello <matheus@castello.eng.br> > --- > .../power/supply/max17040_battery.txt | 28 +++++++++++++++++++ > 1 file changed, 28 insertions(+) > create mode 100644 Documentation/devicetree/bindings/power/supply/max17040_battery.txt > > diff --git a/Documentation/devicetree/bindings/power/supply/max17040_battery.txt b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt > new file mode 100644 > index 000000000000..a13e8d50ff7b > --- /dev/null > +++ b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt > @@ -0,0 +1,28 @@ > +max17040_battery > +~~~~~~~~~~~~~~~~ > + > +Required properties : > + - compatible : "maxim,max17040" or "maxim,max77836-battery" One more comment. The datasheet for max17040 says that there is on ALERT pin and ALERT bits in RCOMP register. Which device are you using? If it turns out that max17040 does not support it, then the driver and bindings should reflect this - interrupts should not be set on max17040. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH v3 2/5] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold 2019-05-29 14:57 ` Krzysztof Kozlowski @ 2019-06-02 21:38 ` Matheus Castello 2019-06-05 12:04 ` Krzysztof Kozlowski 0 siblings, 1 reply; 82+ messages in thread From: Matheus Castello @ 2019-06-02 21:38 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: sre, robh+dt, mark.rutland, Chanwoo Choi, Bartłomiej Żołnierkiewicz, lee.jones, linux-pm, devicetree, linux-kernel > On Mon, 27 May 2019 at 04:45, Matheus Castello <matheus@castello.eng.br> wrote: >> >> For configure low level state of charge threshold alert signaled from >> max17040 we add "maxim,alert-low-soc-level" property. >> >> Signed-off-by: Matheus Castello <matheus@castello.eng.br> >> --- >> .../power/supply/max17040_battery.txt | 28 +++++++++++++++++++ >> 1 file changed, 28 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/power/supply/max17040_battery.txt >> >> diff --git a/Documentation/devicetree/bindings/power/supply/max17040_battery.txt b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt >> new file mode 100644 >> index 000000000000..a13e8d50ff7b >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt >> @@ -0,0 +1,28 @@ >> +max17040_battery >> +~~~~~~~~~~~~~~~~ >> + >> +Required properties : >> + - compatible : "maxim,max17040" or "maxim,max77836-battery" > > One more comment. The datasheet for max17040 says that there is on > ALERT pin and ALERT bits in RCOMP register. Which device are you > using? If it turns out that max17040 does not support it, then the > driver and bindings should reflect this - interrupts should not be set > on max17040. > Yes you are right, max17040 have no ALERT pin. I am using max17043. Let me know what you think would be best, put a note about it in the description, add a compatibles like "maxim,max17043" and "maxim,max17044"? What do you think? Best Regards, Matheus Castello > Best regards, > Krzysztof > ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH v3 2/5] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold 2019-06-02 21:38 ` Matheus Castello @ 2019-06-05 12:04 ` Krzysztof Kozlowski 0 siblings, 0 replies; 82+ messages in thread From: Krzysztof Kozlowski @ 2019-06-05 12:04 UTC (permalink / raw) To: Matheus Castello Cc: sre, robh+dt, mark.rutland, Chanwoo Choi, Bartłomiej Żołnierkiewicz, lee.jones, linux-pm, devicetree, linux-kernel On Sun, 2 Jun 2019 at 23:42, Matheus Castello <matheus@castello.eng.br> wrote: > > > On Mon, 27 May 2019 at 04:45, Matheus Castello <matheus@castello.eng.br> wrote: > >> > >> For configure low level state of charge threshold alert signaled from > >> max17040 we add "maxim,alert-low-soc-level" property. > >> > >> Signed-off-by: Matheus Castello <matheus@castello.eng.br> > >> --- > >> .../power/supply/max17040_battery.txt | 28 +++++++++++++++++++ > >> 1 file changed, 28 insertions(+) > >> create mode 100644 Documentation/devicetree/bindings/power/supply/max17040_battery.txt > >> > >> diff --git a/Documentation/devicetree/bindings/power/supply/max17040_battery.txt b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt > >> new file mode 100644 > >> index 000000000000..a13e8d50ff7b > >> --- /dev/null > >> +++ b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt > >> @@ -0,0 +1,28 @@ > >> +max17040_battery > >> +~~~~~~~~~~~~~~~~ > >> + > >> +Required properties : > >> + - compatible : "maxim,max17040" or "maxim,max77836-battery" > > > > One more comment. The datasheet for max17040 says that there is on > > ALERT pin and ALERT bits in RCOMP register. Which device are you > > using? If it turns out that max17040 does not support it, then the > > driver and bindings should reflect this - interrupts should not be set > > on max17040. > > > > Yes you are right, max17040 have no ALERT pin. I am using max17043. Let > me know what you think would be best, put a note about it in the > description, add a compatibles like "maxim,max17043" and > "maxim,max17044"? What do you think? Usually in such case driver should behave differently for different devices. This difference is chosen by compatible. Since there is already max77836 compatible - which has the ALERT pin (probably it just includes 17043 fuel gauge) - you can customize it. No need for new compatible, unless it stops working on max77836... Anyway, configuring interrupts on max17040 would be probably harmless but still it is not really correct. The registers should not be touched in such case. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 82+ messages in thread
* [PATCH v3 3/5] power: supply: max17040: Config alert SOC low level threshold from FDT 2019-05-27 2:22 ` [PATCH v3 0/5] power: supply: MAX17040: Add IRQ for low level and alert " Matheus Castello 2019-05-27 2:22 ` [PATCH v3 1/5] power: supply: max17040: Add IRQ handler for low SOC alert Matheus Castello 2019-05-27 2:22 ` [PATCH v3 2/5] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold Matheus Castello @ 2019-05-27 2:22 ` Matheus Castello 2019-05-29 14:46 ` Krzysztof Kozlowski 2019-05-27 2:22 ` [PATCH v3 4/5] power: supply: max17040: Clear ALRT bit when the SOC are above threshold Matheus Castello 2019-05-27 2:22 ` [PATCH v3 5/5] power: supply: max17040: Send uevent in SOC and status change Matheus Castello 4 siblings, 1 reply; 82+ messages in thread From: Matheus Castello @ 2019-05-27 2:22 UTC (permalink / raw) To: sre, krzk, robh+dt Cc: mark.rutland, cw00.choi, b.zolnierkie, lee.jones, linux-pm, devicetree, linux-kernel, Matheus Castello For configuration of fuel gauge alert for a low level state of charge interrupt we add a function to config level threshold and a device tree binding property to set it in flatned device tree node. Now we can use "maxim,alert-low-soc-level" property with the values from 1% up to 32% to configure alert interrupt threshold. Signed-off-by: Matheus Castello <matheus@castello.eng.br> --- drivers/power/supply/max17040_battery.c | 52 +++++++++++++++++++++++-- 1 file changed, 49 insertions(+), 3 deletions(-) diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c index b7433e9ca7c2..2f4851608cfe 100644 --- a/drivers/power/supply/max17040_battery.c +++ b/drivers/power/supply/max17040_battery.c @@ -29,6 +29,9 @@ #define MAX17040_DELAY 1000 #define MAX17040_BATTERY_FULL 95 +#define MAX17040_ATHD_MASK 0xFFC0 +#define MAX17040_ATHD_DEFAULT_POWER_UP 4 + struct max17040_chip { struct i2c_client *client; struct delayed_work work; @@ -43,6 +46,8 @@ struct max17040_chip { int soc; /* State Of Charge */ int status; + /* Low alert threshold from 32% to 1% of the State of Charge */ + u32 low_soc_alert_threshold; }; static int max17040_get_property(struct power_supply *psy, @@ -99,6 +104,28 @@ static void max17040_reset(struct i2c_client *client) max17040_write_reg(client, MAX17040_CMD, 0x0054); } +static int max17040_set_low_soc_threshold_alert(struct i2c_client *client, + u32 level) +{ + int ret; + u16 data; + + /* check if level is between 1% and 32% */ + if (level > 0 && level < 33) { + level = 32 - level; + data = max17040_read_reg(client, MAX17040_RCOMP); + /* clear the alrt bit and set LSb 5 bits */ + data &= MAX17040_ATHD_MASK; + data |= level; + max17040_write_reg(client, MAX17040_RCOMP, data); + ret = 0; + } else { + ret = -EINVAL; + } + + return ret; +} + static void max17040_get_vcell(struct i2c_client *client) { struct max17040_chip *chip = i2c_get_clientdata(client); @@ -161,6 +188,16 @@ static void max17040_get_status(struct i2c_client *client) chip->status = POWER_SUPPLY_STATUS_FULL; } +static void max17040_get_of_data(struct max17040_chip *chip) +{ + struct device *dev = &chip->client->dev; + struct device_node *np = dev->of_node; + + if (of_property_read_u32(np, "maxim,alert-low-soc-level", + &chip->low_soc_alert_threshold)) + chip->low_soc_alert_threshold = MAX17040_ATHD_DEFAULT_POWER_UP; +} + static void max17040_check_changes(struct i2c_client *client) { max17040_get_vcell(client); @@ -226,6 +263,7 @@ static int max17040_probe(struct i2c_client *client, chip->client = client; chip->pdata = client->dev.platform_data; + max17040_get_of_data(chip); i2c_set_clientdata(client, chip); psy_cfg.drv_data = chip; @@ -243,12 +281,20 @@ static int max17040_probe(struct i2c_client *client, /* check interrupt */ if (client->irq) { int ret; - unsigned int flags; + + ret = max17040_set_low_soc_threshold_alert(client, + chip->low_soc_alert_threshold); + if (ret) { + dev_err(&client->dev, + "Failed to set low SOC alert: err %d\n", + ret); + return ret; + } dev_info(&client->dev, "IRQ: enabled\n"); - flags = IRQF_TRIGGER_FALLING | IRQF_ONESHOT; ret = devm_request_threaded_irq(&client->dev, client->irq, NULL, - max17040_thread_handler, flags, + max17040_thread_handler, + (client->flags | IRQF_ONESHOT), chip->battery->desc->name, chip); -- 2.20.1 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* Re: [PATCH v3 3/5] power: supply: max17040: Config alert SOC low level threshold from FDT 2019-05-27 2:22 ` [PATCH v3 3/5] power: supply: max17040: Config alert SOC low level threshold from FDT Matheus Castello @ 2019-05-29 14:46 ` Krzysztof Kozlowski 2019-06-02 22:26 ` Matheus Castello 0 siblings, 1 reply; 82+ messages in thread From: Krzysztof Kozlowski @ 2019-05-29 14:46 UTC (permalink / raw) To: Matheus Castello Cc: sre, robh+dt, mark.rutland, Chanwoo Choi, Bartłomiej Żołnierkiewicz, lee.jones, linux-pm, devicetree, linux-kernel On Mon, 27 May 2019 at 04:46, Matheus Castello <matheus@castello.eng.br> wrote: > > For configuration of fuel gauge alert for a low level state of charge > interrupt we add a function to config level threshold and a device tree > binding property to set it in flatned device tree node. > > Now we can use "maxim,alert-low-soc-level" property with the values from > 1% up to 32% to configure alert interrupt threshold. > > Signed-off-by: Matheus Castello <matheus@castello.eng.br> > --- > drivers/power/supply/max17040_battery.c | 52 +++++++++++++++++++++++-- > 1 file changed, 49 insertions(+), 3 deletions(-) > > diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c > index b7433e9ca7c2..2f4851608cfe 100644 > --- a/drivers/power/supply/max17040_battery.c > +++ b/drivers/power/supply/max17040_battery.c > @@ -29,6 +29,9 @@ > #define MAX17040_DELAY 1000 > #define MAX17040_BATTERY_FULL 95 > > +#define MAX17040_ATHD_MASK 0xFFC0 > +#define MAX17040_ATHD_DEFAULT_POWER_UP 4 > + > struct max17040_chip { > struct i2c_client *client; > struct delayed_work work; > @@ -43,6 +46,8 @@ struct max17040_chip { > int soc; > /* State Of Charge */ > int status; > + /* Low alert threshold from 32% to 1% of the State of Charge */ > + u32 low_soc_alert_threshold; > }; > > static int max17040_get_property(struct power_supply *psy, > @@ -99,6 +104,28 @@ static void max17040_reset(struct i2c_client *client) > max17040_write_reg(client, MAX17040_CMD, 0x0054); > } > > +static int max17040_set_low_soc_threshold_alert(struct i2c_client *client, > + u32 level) > +{ > + int ret; > + u16 data; > + > + /* check if level is between 1% and 32% */ > + if (level > 0 && level < 33) { > + level = 32 - level; > + data = max17040_read_reg(client, MAX17040_RCOMP); > + /* clear the alrt bit and set LSb 5 bits */ > + data &= MAX17040_ATHD_MASK; > + data |= level; > + max17040_write_reg(client, MAX17040_RCOMP, data); > + ret = 0; > + } else { > + ret = -EINVAL; > + } This is unusual way of handling error... when you parse DTS, you accept any value for "level" (even incorrect one). You validate the value later when setting it and show an error... however you ignore the error of max17040_write_reg() here... This is correct but looks unusual. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH v3 3/5] power: supply: max17040: Config alert SOC low level threshold from FDT 2019-05-29 14:46 ` Krzysztof Kozlowski @ 2019-06-02 22:26 ` Matheus Castello 2019-06-05 12:05 ` Krzysztof Kozlowski 0 siblings, 1 reply; 82+ messages in thread From: Matheus Castello @ 2019-06-02 22:26 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: sre, robh+dt, mark.rutland, Chanwoo Choi, Bartłomiej Żołnierkiewicz, lee.jones, linux-pm, devicetree, linux-kernel On 5/29/19 11:46 AM, Krzysztof Kozlowski wrote: > On Mon, 27 May 2019 at 04:46, Matheus Castello <matheus@castello.eng.br> wrote: >> >> For configuration of fuel gauge alert for a low level state of charge >> interrupt we add a function to config level threshold and a device tree >> binding property to set it in flatned device tree node. >> >> Now we can use "maxim,alert-low-soc-level" property with the values from >> 1% up to 32% to configure alert interrupt threshold. >> >> Signed-off-by: Matheus Castello <matheus@castello.eng.br> >> --- >> drivers/power/supply/max17040_battery.c | 52 +++++++++++++++++++++++-- >> 1 file changed, 49 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c >> index b7433e9ca7c2..2f4851608cfe 100644 >> --- a/drivers/power/supply/max17040_battery.c >> +++ b/drivers/power/supply/max17040_battery.c >> @@ -29,6 +29,9 @@ >> #define MAX17040_DELAY 1000 >> #define MAX17040_BATTERY_FULL 95 >> >> +#define MAX17040_ATHD_MASK 0xFFC0 >> +#define MAX17040_ATHD_DEFAULT_POWER_UP 4 >> + >> struct max17040_chip { >> struct i2c_client *client; >> struct delayed_work work; >> @@ -43,6 +46,8 @@ struct max17040_chip { >> int soc; >> /* State Of Charge */ >> int status; >> + /* Low alert threshold from 32% to 1% of the State of Charge */ >> + u32 low_soc_alert_threshold; >> }; >> >> static int max17040_get_property(struct power_supply *psy, >> @@ -99,6 +104,28 @@ static void max17040_reset(struct i2c_client *client) >> max17040_write_reg(client, MAX17040_CMD, 0x0054); >> } >> >> +static int max17040_set_low_soc_threshold_alert(struct i2c_client *client, >> + u32 level) >> +{ >> + int ret; >> + u16 data; >> + >> + /* check if level is between 1% and 32% */ >> + if (level > 0 && level < 33) { >> + level = 32 - level; >> + data = max17040_read_reg(client, MAX17040_RCOMP); >> + /* clear the alrt bit and set LSb 5 bits */ >> + data &= MAX17040_ATHD_MASK; >> + data |= level; >> + max17040_write_reg(client, MAX17040_RCOMP, data); >> + ret = 0; I will put the return of max17040_write_reg on ret, instead of ret = 0. >> + } else { >> + ret = -EINVAL; >> + } > > This is unusual way of handling error... when you parse DTS, you > accept any value for "level" (even incorrect one). You validate the > value later when setting it and show an error... however you ignore > the error of max17040_write_reg() here... This is correct but looks > unusual. > Ok, so would it be better to check the level value in "max17040_get_of_data" and return an error there if the input is wrong? Best Regards, Matheus Castello > Best regards, > Krzysztof > ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH v3 3/5] power: supply: max17040: Config alert SOC low level threshold from FDT 2019-06-02 22:26 ` Matheus Castello @ 2019-06-05 12:05 ` Krzysztof Kozlowski 0 siblings, 0 replies; 82+ messages in thread From: Krzysztof Kozlowski @ 2019-06-05 12:05 UTC (permalink / raw) To: Matheus Castello Cc: sre, robh+dt, mark.rutland, Chanwoo Choi, Bartłomiej Żołnierkiewicz, lee.jones, linux-pm, devicetree, linux-kernel On Mon, 3 Jun 2019 at 00:42, Matheus Castello <matheus@castello.eng.br> wrote: > > > > On 5/29/19 11:46 AM, Krzysztof Kozlowski wrote: > > On Mon, 27 May 2019 at 04:46, Matheus Castello <matheus@castello.eng.br> wrote: > >> > >> For configuration of fuel gauge alert for a low level state of charge > >> interrupt we add a function to config level threshold and a device tree > >> binding property to set it in flatned device tree node. > >> > >> Now we can use "maxim,alert-low-soc-level" property with the values from > >> 1% up to 32% to configure alert interrupt threshold. > >> > >> Signed-off-by: Matheus Castello <matheus@castello.eng.br> > >> --- > >> drivers/power/supply/max17040_battery.c | 52 +++++++++++++++++++++++-- > >> 1 file changed, 49 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c > >> index b7433e9ca7c2..2f4851608cfe 100644 > >> --- a/drivers/power/supply/max17040_battery.c > >> +++ b/drivers/power/supply/max17040_battery.c > >> @@ -29,6 +29,9 @@ > >> #define MAX17040_DELAY 1000 > >> #define MAX17040_BATTERY_FULL 95 > >> > >> +#define MAX17040_ATHD_MASK 0xFFC0 > >> +#define MAX17040_ATHD_DEFAULT_POWER_UP 4 > >> + > >> struct max17040_chip { > >> struct i2c_client *client; > >> struct delayed_work work; > >> @@ -43,6 +46,8 @@ struct max17040_chip { > >> int soc; > >> /* State Of Charge */ > >> int status; > >> + /* Low alert threshold from 32% to 1% of the State of Charge */ > >> + u32 low_soc_alert_threshold; > >> }; > >> > >> static int max17040_get_property(struct power_supply *psy, > >> @@ -99,6 +104,28 @@ static void max17040_reset(struct i2c_client *client) > >> max17040_write_reg(client, MAX17040_CMD, 0x0054); > >> } > >> > >> +static int max17040_set_low_soc_threshold_alert(struct i2c_client *client, > >> + u32 level) > >> +{ > >> + int ret; > >> + u16 data; > >> + > >> + /* check if level is between 1% and 32% */ > >> + if (level > 0 && level < 33) { > >> + level = 32 - level; > >> + data = max17040_read_reg(client, MAX17040_RCOMP); > >> + /* clear the alrt bit and set LSb 5 bits */ > >> + data &= MAX17040_ATHD_MASK; > >> + data |= level; > >> + max17040_write_reg(client, MAX17040_RCOMP, data); > >> + ret = 0; > > I will put the return of max17040_write_reg on ret, instead of ret = 0. > > >> + } else { > >> + ret = -EINVAL; > >> + } > > > > This is unusual way of handling error... when you parse DTS, you > > accept any value for "level" (even incorrect one). You validate the > > value later when setting it and show an error... however you ignore > > the error of max17040_write_reg() here... This is correct but looks > > unusual. > > > > Ok, so would it be better to check the level value in > "max17040_get_of_data" and return an error there if the input is wrong? I think yes. It looks more natural - validate the value as early as possible and fail the probe which gives the information about point of failure. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 82+ messages in thread
* [PATCH v3 4/5] power: supply: max17040: Clear ALRT bit when the SOC are above threshold 2019-05-27 2:22 ` [PATCH v3 0/5] power: supply: MAX17040: Add IRQ for low level and alert " Matheus Castello ` (2 preceding siblings ...) 2019-05-27 2:22 ` [PATCH v3 3/5] power: supply: max17040: Config alert SOC low level threshold from FDT Matheus Castello @ 2019-05-27 2:22 ` Matheus Castello 2019-05-29 14:54 ` Krzysztof Kozlowski 2019-05-27 2:22 ` [PATCH v3 5/5] power: supply: max17040: Send uevent in SOC and status change Matheus Castello 4 siblings, 1 reply; 82+ messages in thread From: Matheus Castello @ 2019-05-27 2:22 UTC (permalink / raw) To: sre, krzk, robh+dt Cc: mark.rutland, cw00.choi, b.zolnierkie, lee.jones, linux-pm, devicetree, linux-kernel, Matheus Castello In order to not generate duplicate interrupts we clear the ALRT bit when the SOC is in a state that shows that the battery is charged above the set threshold for the SOC low level alert. Signed-off-by: Matheus Castello <matheus@castello.eng.br> --- drivers/power/supply/max17040_battery.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c index 2f4851608cfe..61e6fcfea8a1 100644 --- a/drivers/power/supply/max17040_battery.c +++ b/drivers/power/supply/max17040_battery.c @@ -48,6 +48,7 @@ struct max17040_chip { int status; /* Low alert threshold from 32% to 1% of the State of Charge */ u32 low_soc_alert_threshold; + int alert_bit; }; static int max17040_get_property(struct power_supply *psy, @@ -107,6 +108,7 @@ static void max17040_reset(struct i2c_client *client) static int max17040_set_low_soc_threshold_alert(struct i2c_client *client, u32 level) { + struct max17040_chip *chip = i2c_get_clientdata(client); int ret; u16 data; @@ -118,6 +120,7 @@ static int max17040_set_low_soc_threshold_alert(struct i2c_client *client, data &= MAX17040_ATHD_MASK; data |= level; max17040_write_reg(client, MAX17040_RCOMP, data); + chip->alert_bit = 0; ret = 0; } else { ret = -EINVAL; @@ -144,6 +147,11 @@ static void max17040_get_soc(struct i2c_client *client) soc = max17040_read_reg(client, MAX17040_SOC); chip->soc = (soc >> 8); + + /* check SOC level to clear ALRT bit */ + if (chip->soc > chip->low_soc_alert_threshold && chip->alert_bit) + max17040_set_low_soc_threshold_alert(client, + chip->low_soc_alert_threshold); } static void max17040_get_version(struct i2c_client *client) @@ -229,6 +237,9 @@ static irqreturn_t max17040_thread_handler(int id, void *dev) /* send uevent */ power_supply_changed(chip->battery); + /* ALRT bit is seted */ + chip->alert_bit = 1; + return IRQ_HANDLED; } -- 2.20.1 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* Re: [PATCH v3 4/5] power: supply: max17040: Clear ALRT bit when the SOC are above threshold 2019-05-27 2:22 ` [PATCH v3 4/5] power: supply: max17040: Clear ALRT bit when the SOC are above threshold Matheus Castello @ 2019-05-29 14:54 ` Krzysztof Kozlowski 0 siblings, 0 replies; 82+ messages in thread From: Krzysztof Kozlowski @ 2019-05-29 14:54 UTC (permalink / raw) To: Matheus Castello Cc: sre, robh+dt, mark.rutland, Chanwoo Choi, Bartłomiej Żołnierkiewicz, lee.jones, linux-pm, devicetree, linux-kernel On Mon, 27 May 2019 at 04:45, Matheus Castello <matheus@castello.eng.br> wrote: > > In order to not generate duplicate interrupts we clear the ALRT bit when > the SOC is in a state that shows that the battery is charged above the set > threshold for the SOC low level alert. I think interrupt/alert bit should be cleared while handling interrupt, not later because: 1. It is logical to clear it when servicing it, 2. It is simpler - no need for "chip->alert_bit", 3. The alert threshold is understood as alert/warning so every interrupt should generate uevent. I understand you wanted to remove "duplicate interrupts" but in fact there are no duplicates. Every next interrupt comes from change of SoC while being below the critical level. Therefore on each such change user-space should be woken up and notified (e.g. to show the message to the user). I also think this should be squashed with previous patch as it does not make sense as standalone commit. > > Signed-off-by: Matheus Castello <matheus@castello.eng.br> > --- > drivers/power/supply/max17040_battery.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c > index 2f4851608cfe..61e6fcfea8a1 100644 > --- a/drivers/power/supply/max17040_battery.c > +++ b/drivers/power/supply/max17040_battery.c > @@ -48,6 +48,7 @@ struct max17040_chip { > int status; > /* Low alert threshold from 32% to 1% of the State of Charge */ > u32 low_soc_alert_threshold; > + int alert_bit; > }; > > static int max17040_get_property(struct power_supply *psy, > @@ -107,6 +108,7 @@ static void max17040_reset(struct i2c_client *client) > static int max17040_set_low_soc_threshold_alert(struct i2c_client *client, > u32 level) > { > + struct max17040_chip *chip = i2c_get_clientdata(client); > int ret; > u16 data; > > @@ -118,6 +120,7 @@ static int max17040_set_low_soc_threshold_alert(struct i2c_client *client, > data &= MAX17040_ATHD_MASK; > data |= level; > max17040_write_reg(client, MAX17040_RCOMP, data); > + chip->alert_bit = 0; > ret = 0; > } else { > ret = -EINVAL; > @@ -144,6 +147,11 @@ static void max17040_get_soc(struct i2c_client *client) > soc = max17040_read_reg(client, MAX17040_SOC); > > chip->soc = (soc >> 8); > + > + /* check SOC level to clear ALRT bit */ > + if (chip->soc > chip->low_soc_alert_threshold && chip->alert_bit) > + max17040_set_low_soc_threshold_alert(client, > + chip->low_soc_alert_threshold); > } > > static void max17040_get_version(struct i2c_client *client) > @@ -229,6 +237,9 @@ static irqreturn_t max17040_thread_handler(int id, void *dev) > /* send uevent */ > power_supply_changed(chip->battery); > > + /* ALRT bit is seted */ s/seted/set/ Best regards, Krzysztof > + chip->alert_bit = 1; > + > return IRQ_HANDLED; > } > > -- > 2.20.1 > ^ permalink raw reply [flat|nested] 82+ messages in thread
* [PATCH v3 5/5] power: supply: max17040: Send uevent in SOC and status change 2019-05-27 2:22 ` [PATCH v3 0/5] power: supply: MAX17040: Add IRQ for low level and alert " Matheus Castello ` (3 preceding siblings ...) 2019-05-27 2:22 ` [PATCH v3 4/5] power: supply: max17040: Clear ALRT bit when the SOC are above threshold Matheus Castello @ 2019-05-27 2:22 ` Matheus Castello 2019-05-29 15:00 ` Krzysztof Kozlowski 4 siblings, 1 reply; 82+ messages in thread From: Matheus Castello @ 2019-05-27 2:22 UTC (permalink / raw) To: sre, krzk, robh+dt Cc: mark.rutland, cw00.choi, b.zolnierkie, lee.jones, linux-pm, devicetree, linux-kernel, Matheus Castello Notify core through power_supply_changed() in case of changes in state of charge and power supply status. This is useful for user-space to efficiently update current battery level. Signed-off-by: Matheus Castello <matheus@castello.eng.br> --- drivers/power/supply/max17040_battery.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c index 61e6fcfea8a1..34278845cfe5 100644 --- a/drivers/power/supply/max17040_battery.c +++ b/drivers/power/supply/max17040_battery.c @@ -176,6 +176,9 @@ static void max17040_get_online(struct i2c_client *client) static void max17040_get_status(struct i2c_client *client) { struct max17040_chip *chip = i2c_get_clientdata(client); + int last_status; + + last_status = chip->status; if (!chip->pdata || !chip->pdata->charger_online || !chip->pdata->charger_enable) { @@ -194,6 +197,9 @@ static void max17040_get_status(struct i2c_client *client) if (chip->soc > MAX17040_BATTERY_FULL) chip->status = POWER_SUPPLY_STATUS_FULL; + + if (last_status != chip->status) + power_supply_changed(chip->battery); } static void max17040_get_of_data(struct max17040_chip *chip) @@ -217,10 +223,18 @@ static void max17040_check_changes(struct i2c_client *client) static void max17040_work(struct work_struct *work) { struct max17040_chip *chip; + int last_soc; chip = container_of(work, struct max17040_chip, work.work); + + /* store SOC for check change */ + last_soc = chip->soc; max17040_check_changes(chip->client); + /* check changes and send uevent */ + if (chip->soc >= 0 && last_soc != chip->soc) + power_supply_changed(chip->battery); + queue_delayed_work(system_power_efficient_wq, &chip->work, MAX17040_DELAY); } -- 2.20.1 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* Re: [PATCH v3 5/5] power: supply: max17040: Send uevent in SOC and status change 2019-05-27 2:22 ` [PATCH v3 5/5] power: supply: max17040: Send uevent in SOC and status change Matheus Castello @ 2019-05-29 15:00 ` Krzysztof Kozlowski 2019-10-31 18:41 ` [PATCH v4 0/4] power: supply: MAX17040: Add IRQ for low level and alert SOC changes Matheus Castello 0 siblings, 1 reply; 82+ messages in thread From: Krzysztof Kozlowski @ 2019-05-29 15:00 UTC (permalink / raw) To: Matheus Castello Cc: sre, robh+dt, mark.rutland, Chanwoo Choi, Bartłomiej Żołnierkiewicz, lee.jones, linux-pm, devicetree, linux-kernel On Mon, 27 May 2019 at 04:23, Matheus Castello <matheus@castello.eng.br> wrote: > > Notify core through power_supply_changed() in case of changes in state > of charge and power supply status. This is useful for user-space to > efficiently update current battery level. > > Signed-off-by: Matheus Castello <matheus@castello.eng.br> > --- > drivers/power/supply/max17040_battery.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c > index 61e6fcfea8a1..34278845cfe5 100644 > --- a/drivers/power/supply/max17040_battery.c > +++ b/drivers/power/supply/max17040_battery.c > @@ -176,6 +176,9 @@ static void max17040_get_online(struct i2c_client *client) > static void max17040_get_status(struct i2c_client *client) > { > struct max17040_chip *chip = i2c_get_clientdata(client); > + int last_status; > + > + last_status = chip->status; > > if (!chip->pdata || !chip->pdata->charger_online > || !chip->pdata->charger_enable) { > @@ -194,6 +197,9 @@ static void max17040_get_status(struct i2c_client *client) > > if (chip->soc > MAX17040_BATTERY_FULL) > chip->status = POWER_SUPPLY_STATUS_FULL; > + > + if (last_status != chip->status) > + power_supply_changed(chip->battery); Why splitting it from max17040_work()? It seems logical to check soc and status at the same time. Best regards, Krzysztof > } > > static void max17040_get_of_data(struct max17040_chip *chip) > @@ -217,10 +223,18 @@ static void max17040_check_changes(struct i2c_client *client) > static void max17040_work(struct work_struct *work) > { > struct max17040_chip *chip; > + int last_soc; > > chip = container_of(work, struct max17040_chip, work.work); > + > + /* store SOC for check change */ > + last_soc = chip->soc; > max17040_check_changes(chip->client); > > + /* check changes and send uevent */ > + if (chip->soc >= 0 && last_soc != chip->soc) > + power_supply_changed(chip->battery); > + > queue_delayed_work(system_power_efficient_wq, &chip->work, > MAX17040_DELAY); > } > -- > 2.20.1 > ^ permalink raw reply [flat|nested] 82+ messages in thread
* [PATCH v4 0/4] power: supply: MAX17040: Add IRQ for low level and alert SOC changes 2019-05-29 15:00 ` Krzysztof Kozlowski @ 2019-10-31 18:41 ` Matheus Castello 2019-10-31 18:41 ` [PATCH v4 1/4] power: supply: max17040: Add IRQ handler for low SOC alert Matheus Castello ` (3 more replies) 0 siblings, 4 replies; 82+ messages in thread From: Matheus Castello @ 2019-10-31 18:41 UTC (permalink / raw) To: sre, krzk, robh+dt Cc: mark.rutland, cw00.choi, b.zolnierkie, lee.jones, linux-pm, devicetree, linux-kernel, Matheus Castello This series add IRQ handler for low level SOC alert, define a devicetree binding attribute to configure the alert level threshold and check for changes in SOC and power supply status for send uevents. Max17043/17044 have a pin for alert host about low level state of charge and this alert can be configured in a threshold from 1% up to 32% of SOC. Tested on Toradex Colibri iMX7D, with a SparkFun Lipo Fuel Gauge module based on MAXIM MAX17043. Thanks Krzysztof for your time reviewing it. Let me know what you think about the fixes. Changes since v3: (Suggested by Krzysztof Kozlowski) - Validate values of the maxim,alert-low-soc-level early as possible - Clear the alert bit while handling IRQ - Remove the "chip->alert_bit" - Check the SOC and status at the same time on max17040_work - Add note about the compatible of devices with ALERT feature - Only set alert register and IRQ when compatible is max77836 Changes since v2: (Suggested by Krzysztof Kozlowski) - Fix ebusy exception - Remove device_init_wakeup from probe, let wakeup-source from DT decide that - Fix the use of "charger" to fuel-gauge on device tree description - Clear ALRT bit while setting the SOC threshold - Check SOC and clear ALRT bit when the SOC are above alert threshold - Fix unnecessary uevent when SOC is an ERRNO - Notify user space when power supply status change (Suggested by Rob Herring) - Fix the use of "charger" to fuel-gauge on device tree description - Add the (%) units on the description of property - Drop interrupt-parent - Fix name of property to let clear that is a low level SOC alert Changes since v1: (Suggested by Krzysztof Kozlowski) - Put common code from max17040_work and max17040_thread_handler in a function - Code style fixes - Define mask and low level threshold alert default - Check return value from max17040_set_soc_threshold - Set low level state of charge threshold before IRQ - CC maintainers from drivers/mfd/max14577 - Use flags from FDT client->flags instead hard coded it - Mention interrupts on DT Documentation - Fix "maxim,max77836-battery" missed from DT Documentation - Fix commit description Matheus Castello (4): power: supply: max17040: Add IRQ handler for low SOC alert dt-bindings: power: supply: Max17040: Add low level SOC alert threshold power: supply: max17040: Config alert SOC low level threshold from FDT power: supply: max17040: Send uevent in SOC and status change .../power/supply/max17040_battery.txt | 33 +++++ drivers/power/supply/max17040_battery.c | 134 +++++++++++++++++- 2 files changed, 162 insertions(+), 5 deletions(-) create mode 100644 Documentation/devicetree/bindings/power/supply/max17040_battery.txt -- 2.24.0.rc2 ^ permalink raw reply [flat|nested] 82+ messages in thread
* [PATCH v4 1/4] power: supply: max17040: Add IRQ handler for low SOC alert 2019-10-31 18:41 ` [PATCH v4 0/4] power: supply: MAX17040: Add IRQ for low level and alert SOC changes Matheus Castello @ 2019-10-31 18:41 ` Matheus Castello 2019-11-01 15:08 ` Krzysztof Kozlowski 2019-10-31 18:41 ` [PATCH v4 2/4] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold Matheus Castello ` (2 subsequent siblings) 3 siblings, 1 reply; 82+ messages in thread From: Matheus Castello @ 2019-10-31 18:41 UTC (permalink / raw) To: sre, krzk, robh+dt Cc: mark.rutland, cw00.choi, b.zolnierkie, lee.jones, linux-pm, devicetree, linux-kernel, Matheus Castello According datasheet max17040 has a pin for alert host for low SOC. This pin can be used as external interrupt, so we need to check for interrupts assigned for device and handle it. In handler we are checking and storing fuel gauge registers values and send an uevent to notificate user space, so user space can decide save work or turn off since the alert demonstrate that the battery may no have the power to keep the system turned on for much longer. Signed-off-by: Matheus Castello <matheus@castello.eng.br> --- drivers/power/supply/max17040_battery.c | 65 +++++++++++++++++++++++-- 1 file changed, 60 insertions(+), 5 deletions(-) diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c index 62499018e68b..75459f76d02c 100644 --- a/drivers/power/supply/max17040_battery.c +++ b/drivers/power/supply/max17040_battery.c @@ -13,6 +13,7 @@ #include <linux/err.h> #include <linux/i2c.h> #include <linux/delay.h> +#include <linux/interrupt.h> #include <linux/power_supply.h> #include <linux/max17040_battery.h> #include <linux/slab.h> @@ -160,21 +161,40 @@ static void max17040_get_status(struct i2c_client *client) chip->status = POWER_SUPPLY_STATUS_FULL; } +static void max17040_check_changes(struct i2c_client *client) +{ + max17040_get_vcell(client); + max17040_get_soc(client); + max17040_get_online(client); + max17040_get_status(client); +} + static void max17040_work(struct work_struct *work) { struct max17040_chip *chip; chip = container_of(work, struct max17040_chip, work.work); - - max17040_get_vcell(chip->client); - max17040_get_soc(chip->client); - max17040_get_online(chip->client); - max17040_get_status(chip->client); + max17040_check_changes(chip->client); queue_delayed_work(system_power_efficient_wq, &chip->work, MAX17040_DELAY); } +static irqreturn_t max17040_thread_handler(int id, void *dev) +{ + struct max17040_chip *chip = dev; + struct i2c_client *client = chip->client; + + dev_warn(&client->dev, "IRQ: Alert battery low level"); + /* read registers */ + max17040_check_changes(chip->client); + + /* send uevent */ + power_supply_changed(chip->battery); + + return IRQ_HANDLED; +} + static enum power_supply_property max17040_battery_props[] = { POWER_SUPPLY_PROP_STATUS, POWER_SUPPLY_PROP_ONLINE, @@ -220,6 +240,25 @@ static int max17040_probe(struct i2c_client *client, max17040_reset(client); max17040_get_version(client); + /* check interrupt */ + if (client->irq) { + int ret; + unsigned int flags; + + dev_info(&client->dev, "IRQ: enabled\n"); + flags = IRQF_TRIGGER_FALLING | IRQF_ONESHOT; + ret = devm_request_threaded_irq(&client->dev, client->irq, NULL, + max17040_thread_handler, flags, + chip->battery->desc->name, + chip); + + if (ret) { + client->irq = 0; + dev_warn(&client->dev, + "Failed to get IRQ err %d\n", ret); + } + } + INIT_DEFERRABLE_WORK(&chip->work, max17040_work); queue_delayed_work(system_power_efficient_wq, &chip->work, MAX17040_DELAY); @@ -244,6 +283,14 @@ static int max17040_suspend(struct device *dev) struct max17040_chip *chip = i2c_get_clientdata(client); cancel_delayed_work(&chip->work); + + if (client->irq) { + if (device_may_wakeup(dev)) + enable_irq_wake(client->irq); + else + disable_irq_wake(client->irq); + } + return 0; } @@ -254,6 +301,14 @@ static int max17040_resume(struct device *dev) queue_delayed_work(system_power_efficient_wq, &chip->work, MAX17040_DELAY); + + if (client->irq) { + if (device_may_wakeup(dev)) + disable_irq_wake(client->irq); + else + enable_irq_wake(client->irq); + } + return 0; } -- 2.24.0.rc2 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* Re: [PATCH v4 1/4] power: supply: max17040: Add IRQ handler for low SOC alert 2019-10-31 18:41 ` [PATCH v4 1/4] power: supply: max17040: Add IRQ handler for low SOC alert Matheus Castello @ 2019-11-01 15:08 ` Krzysztof Kozlowski 0 siblings, 0 replies; 82+ messages in thread From: Krzysztof Kozlowski @ 2019-11-01 15:08 UTC (permalink / raw) To: Matheus Castello Cc: sre, robh+dt, mark.rutland, cw00.choi, b.zolnierkie, lee.jones, linux-pm, devicetree, linux-kernel On Thu, Oct 31, 2019 at 03:41:31PM -0300, Matheus Castello wrote: > According datasheet max17040 has a pin for alert host for low SOC. > This pin can be used as external interrupt, so we need to check for > interrupts assigned for device and handle it. > > In handler we are checking and storing fuel gauge registers values > and send an uevent to notificate user space, so user space can decide > save work or turn off since the alert demonstrate that the battery may > no have the power to keep the system turned on for much longer. > > Signed-off-by: Matheus Castello <matheus@castello.eng.br> > --- > drivers/power/supply/max17040_battery.c | 65 +++++++++++++++++++++++-- > 1 file changed, 60 insertions(+), 5 deletions(-) This was already reviewed by me. Where's my tag? Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org> Best regards, Krzysztof ^ permalink raw reply [flat|nested] 82+ messages in thread
* [PATCH v4 2/4] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold 2019-10-31 18:41 ` [PATCH v4 0/4] power: supply: MAX17040: Add IRQ for low level and alert SOC changes Matheus Castello 2019-10-31 18:41 ` [PATCH v4 1/4] power: supply: max17040: Add IRQ handler for low SOC alert Matheus Castello @ 2019-10-31 18:41 ` Matheus Castello 2019-11-01 15:10 ` Krzysztof Kozlowski 2019-11-05 1:58 ` Rob Herring 2019-10-31 18:41 ` [PATCH v4 3/4] power: supply: max17040: Config alert SOC low level threshold from FDT Matheus Castello 2019-10-31 18:41 ` [PATCH v4 4/4] power: supply: max17040: Send uevent in SOC and status change Matheus Castello 3 siblings, 2 replies; 82+ messages in thread From: Matheus Castello @ 2019-10-31 18:41 UTC (permalink / raw) To: sre, krzk, robh+dt Cc: mark.rutland, cw00.choi, b.zolnierkie, lee.jones, linux-pm, devicetree, linux-kernel, Matheus Castello For configure low level state of charge threshold alert signaled from max17040 we add "maxim,alert-low-soc-level" property. Signed-off-by: Matheus Castello <matheus@castello.eng.br> --- .../power/supply/max17040_battery.txt | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) create mode 100644 Documentation/devicetree/bindings/power/supply/max17040_battery.txt diff --git a/Documentation/devicetree/bindings/power/supply/max17040_battery.txt b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt new file mode 100644 index 000000000000..f2d0b22b5f79 --- /dev/null +++ b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt @@ -0,0 +1,33 @@ +max17040_battery +~~~~~~~~~~~~~~~~ + +Required properties : + - compatible : "maxim,max17040" or "maxim,max77836-battery" + - reg: i2c slave address + +Optional properties : +- maxim,alert-low-soc-level : The alert threshold that sets the state of + charge level (%) where an interrupt is + generated. Can be configured from 1 up to 32 + (%). If skipped the power up default value of + 4 (%) will be used. +- interrupts : Interrupt line see Documentation/devicetree/ + bindings/interrupt-controller/interrupts.txt +- wakeup-source : This device has wakeup capabilities. Use this + property to use alert low SOC level interrupt + as wake up source. + +Optional properties support interrupt functionality for alert low state of +charge level, present in some ICs in the same family, and should be used with +compatible "maxim,max77836-battery". + +Example: + + battery-fuel-gauge@36 { + compatible = "maxim,max77836-battery"; + reg = <0x36>; + maxim,alert-low-soc-level = <10>; + interrupt-parent = <&gpio7>; + interrupts = <2 IRQ_TYPE_EDGE_FALLING>; + wakeup-source; + }; -- 2.24.0.rc2 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* Re: [PATCH v4 2/4] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold 2019-10-31 18:41 ` [PATCH v4 2/4] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold Matheus Castello @ 2019-11-01 15:10 ` Krzysztof Kozlowski 2019-11-05 1:58 ` Rob Herring 1 sibling, 0 replies; 82+ messages in thread From: Krzysztof Kozlowski @ 2019-11-01 15:10 UTC (permalink / raw) To: Matheus Castello Cc: sre, robh+dt, mark.rutland, cw00.choi, b.zolnierkie, lee.jones, linux-pm, devicetree, linux-kernel On Thu, Oct 31, 2019 at 03:41:32PM -0300, Matheus Castello wrote: > For configure low level state of charge threshold alert signaled from > max17040 we add "maxim,alert-low-soc-level" property. > > Signed-off-by: Matheus Castello <matheus@castello.eng.br> > --- > .../power/supply/max17040_battery.txt | 33 +++++++++++++++++++ > 1 file changed, 33 insertions(+) > create mode 100644 Documentation/devicetree/bindings/power/supply/max17040_battery.txt Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org> Best regards, Krzysztof ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH v4 2/4] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold 2019-10-31 18:41 ` [PATCH v4 2/4] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold Matheus Castello 2019-11-01 15:10 ` Krzysztof Kozlowski @ 2019-11-05 1:58 ` Rob Herring 2019-11-05 5:42 ` [PATCH v5 0/5] power: supply: MAX17040: Add IRQ for low level and alert SOC changes Matheus Castello 1 sibling, 1 reply; 82+ messages in thread From: Rob Herring @ 2019-11-05 1:58 UTC (permalink / raw) To: Matheus Castello Cc: sre, krzk, mark.rutland, cw00.choi, b.zolnierkie, lee.jones, linux-pm, devicetree, linux-kernel On Thu, Oct 31, 2019 at 03:41:32PM -0300, Matheus Castello wrote: > For configure low level state of charge threshold alert signaled from > max17040 we add "maxim,alert-low-soc-level" property. This doesn't match the change. You're adding a whole new device binding. > Signed-off-by: Matheus Castello <matheus@castello.eng.br> > --- > .../power/supply/max17040_battery.txt | 33 +++++++++++++++++++ Part of an MFD? The MFD binding should reference this file then. > 1 file changed, 33 insertions(+) > create mode 100644 Documentation/devicetree/bindings/power/supply/max17040_battery.txt > > diff --git a/Documentation/devicetree/bindings/power/supply/max17040_battery.txt b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt > new file mode 100644 > index 000000000000..f2d0b22b5f79 > --- /dev/null > +++ b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt > @@ -0,0 +1,33 @@ > +max17040_battery > +~~~~~~~~~~~~~~~~ > + > +Required properties : > + - compatible : "maxim,max17040" or "maxim,max77836-battery" > + - reg: i2c slave address > + > +Optional properties : > +- maxim,alert-low-soc-level : The alert threshold that sets the state of > + charge level (%) where an interrupt is > + generated. Can be configured from 1 up to 32 > + (%). If skipped the power up default value of > + 4 (%) will be used. Seems like something that should be a common battery property. > +- interrupts : Interrupt line see Documentation/devicetree/ > + bindings/interrupt-controller/interrupts.txt > +- wakeup-source : This device has wakeup capabilities. Use this > + property to use alert low SOC level interrupt > + as wake up source. > + > +Optional properties support interrupt functionality for alert low state of > +charge level, present in some ICs in the same family, and should be used with > +compatible "maxim,max77836-battery". > + > +Example: > + > + battery-fuel-gauge@36 { > + compatible = "maxim,max77836-battery"; > + reg = <0x36>; > + maxim,alert-low-soc-level = <10>; > + interrupt-parent = <&gpio7>; > + interrupts = <2 IRQ_TYPE_EDGE_FALLING>; > + wakeup-source; > + }; > -- > 2.24.0.rc2 > ^ permalink raw reply [flat|nested] 82+ messages in thread
* [PATCH v5 0/5] power: supply: MAX17040: Add IRQ for low level and alert SOC changes 2019-11-05 1:58 ` Rob Herring @ 2019-11-05 5:42 ` Matheus Castello 2019-11-05 5:42 ` [PATCH v5 1/5] power: supply: max17040: Add IRQ handler for low SOC alert Matheus Castello ` (4 more replies) 0 siblings, 5 replies; 82+ messages in thread From: Matheus Castello @ 2019-11-05 5:42 UTC (permalink / raw) To: sre, krzk, robh+dt Cc: mark.rutland, cw00.choi, b.zolnierkie, lee.jones, linux-pm, devicetree, linux-kernel, Matheus Castello This series add IRQ handler for low level SOC alert, define a devicetree binding attribute to configure the alert level threshold and check for changes in SOC and power supply status for send uevents. Max17043/17044 have a pin for alert host about low level state of charge and this alert can be configured in a threshold from 1% up to 32% of SOC. Tested on Toradex Colibri iMX7D, with a SparkFun Lipo Fuel Gauge module based on MAXIM MAX17043. Thanks Krzysztof for your time reviewing it. Let me know what you think about the fixes. Changes since v4: (Suggested by Krzysztof Kozlowski) - Fix code style and alignment issues - Keep IRQF_TRIGGER_FALLING | IRQF_ONESHOT instead client->flags (Suggested by Rob Herring) - Add reference to the MFD description - Fix the dt-bindings commit description Matheus Castello (5): power: supply: max17040: Add IRQ handler for low SOC alert dt-bindings: power: supply: Max17040: Add DT bindings for max17040 fuel gauge devicetree: mfd: max14577: Add reference to max14040_battery.txt descriptions power: supply: max17040: Config alert SOC low level threshold from FDT power: supply: max17040: Send uevent in SOC and status change .../devicetree/bindings/mfd/max14577.txt | 2 + .../power/supply/max17040_battery.txt | 33 ++++ drivers/power/supply/max17040_battery.c | 142 +++++++++++++++++- 3 files changed, 172 insertions(+), 5 deletions(-) create mode 100644 Documentation/devicetree/bindings/power/supply/max17040_battery.txt -- 2.24.0.rc2 ^ permalink raw reply [flat|nested] 82+ messages in thread
* [PATCH v5 1/5] power: supply: max17040: Add IRQ handler for low SOC alert 2019-11-05 5:42 ` [PATCH v5 0/5] power: supply: MAX17040: Add IRQ for low level and alert SOC changes Matheus Castello @ 2019-11-05 5:42 ` Matheus Castello 2019-11-05 5:42 ` [PATCH v5 2/5] dt-bindings: power: supply: Max17040: Add DT bindings for max17040 fuel gauge Matheus Castello ` (3 subsequent siblings) 4 siblings, 0 replies; 82+ messages in thread From: Matheus Castello @ 2019-11-05 5:42 UTC (permalink / raw) To: sre, krzk, robh+dt Cc: mark.rutland, cw00.choi, b.zolnierkie, lee.jones, linux-pm, devicetree, linux-kernel, Matheus Castello According datasheet max17040 has a pin for alert host for low SOC. This pin can be used as external interrupt, so we need to check for interrupts assigned for device and handle it. In handler we are checking and storing fuel gauge registers values and send an uevent to notificate user space, so user space can decide save work or turn off since the alert demonstrate that the battery may no have the power to keep the system turned on for much longer. Signed-off-by: Matheus Castello <matheus@castello.eng.br> Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org> --- drivers/power/supply/max17040_battery.c | 65 +++++++++++++++++++++++-- 1 file changed, 60 insertions(+), 5 deletions(-) diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c index 62499018e68b..75459f76d02c 100644 --- a/drivers/power/supply/max17040_battery.c +++ b/drivers/power/supply/max17040_battery.c @@ -13,6 +13,7 @@ #include <linux/err.h> #include <linux/i2c.h> #include <linux/delay.h> +#include <linux/interrupt.h> #include <linux/power_supply.h> #include <linux/max17040_battery.h> #include <linux/slab.h> @@ -160,21 +161,40 @@ static void max17040_get_status(struct i2c_client *client) chip->status = POWER_SUPPLY_STATUS_FULL; } +static void max17040_check_changes(struct i2c_client *client) +{ + max17040_get_vcell(client); + max17040_get_soc(client); + max17040_get_online(client); + max17040_get_status(client); +} + static void max17040_work(struct work_struct *work) { struct max17040_chip *chip; chip = container_of(work, struct max17040_chip, work.work); - - max17040_get_vcell(chip->client); - max17040_get_soc(chip->client); - max17040_get_online(chip->client); - max17040_get_status(chip->client); + max17040_check_changes(chip->client); queue_delayed_work(system_power_efficient_wq, &chip->work, MAX17040_DELAY); } +static irqreturn_t max17040_thread_handler(int id, void *dev) +{ + struct max17040_chip *chip = dev; + struct i2c_client *client = chip->client; + + dev_warn(&client->dev, "IRQ: Alert battery low level"); + /* read registers */ + max17040_check_changes(chip->client); + + /* send uevent */ + power_supply_changed(chip->battery); + + return IRQ_HANDLED; +} + static enum power_supply_property max17040_battery_props[] = { POWER_SUPPLY_PROP_STATUS, POWER_SUPPLY_PROP_ONLINE, @@ -220,6 +240,25 @@ static int max17040_probe(struct i2c_client *client, max17040_reset(client); max17040_get_version(client); + /* check interrupt */ + if (client->irq) { + int ret; + unsigned int flags; + + dev_info(&client->dev, "IRQ: enabled\n"); + flags = IRQF_TRIGGER_FALLING | IRQF_ONESHOT; + ret = devm_request_threaded_irq(&client->dev, client->irq, NULL, + max17040_thread_handler, flags, + chip->battery->desc->name, + chip); + + if (ret) { + client->irq = 0; + dev_warn(&client->dev, + "Failed to get IRQ err %d\n", ret); + } + } + INIT_DEFERRABLE_WORK(&chip->work, max17040_work); queue_delayed_work(system_power_efficient_wq, &chip->work, MAX17040_DELAY); @@ -244,6 +283,14 @@ static int max17040_suspend(struct device *dev) struct max17040_chip *chip = i2c_get_clientdata(client); cancel_delayed_work(&chip->work); + + if (client->irq) { + if (device_may_wakeup(dev)) + enable_irq_wake(client->irq); + else + disable_irq_wake(client->irq); + } + return 0; } @@ -254,6 +301,14 @@ static int max17040_resume(struct device *dev) queue_delayed_work(system_power_efficient_wq, &chip->work, MAX17040_DELAY); + + if (client->irq) { + if (device_may_wakeup(dev)) + disable_irq_wake(client->irq); + else + enable_irq_wake(client->irq); + } + return 0; } -- 2.24.0.rc2 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* [PATCH v5 2/5] dt-bindings: power: supply: Max17040: Add DT bindings for max17040 fuel gauge 2019-11-05 5:42 ` [PATCH v5 0/5] power: supply: MAX17040: Add IRQ for low level and alert SOC changes Matheus Castello 2019-11-05 5:42 ` [PATCH v5 1/5] power: supply: max17040: Add IRQ handler for low SOC alert Matheus Castello @ 2019-11-05 5:42 ` Matheus Castello 2019-11-05 5:42 ` [PATCH v5 3/5] devicetree: mfd: max14577: Add reference to max14040_battery.txt descriptions Matheus Castello ` (2 subsequent siblings) 4 siblings, 0 replies; 82+ messages in thread From: Matheus Castello @ 2019-11-05 5:42 UTC (permalink / raw) To: sre, krzk, robh+dt Cc: mark.rutland, cw00.choi, b.zolnierkie, lee.jones, linux-pm, devicetree, linux-kernel, Matheus Castello Documentation of max17040 based fuel gauge characteristics. For configure low level state of charge threshold alert signaled from max17043/max17044 we add "maxim,alert-low-soc-level" property. Signed-off-by: Matheus Castello <matheus@castello.eng.br> Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org> --- .../power/supply/max17040_battery.txt | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) create mode 100644 Documentation/devicetree/bindings/power/supply/max17040_battery.txt diff --git a/Documentation/devicetree/bindings/power/supply/max17040_battery.txt b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt new file mode 100644 index 000000000000..f2d0b22b5f79 --- /dev/null +++ b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt @@ -0,0 +1,33 @@ +max17040_battery +~~~~~~~~~~~~~~~~ + +Required properties : + - compatible : "maxim,max17040" or "maxim,max77836-battery" + - reg: i2c slave address + +Optional properties : +- maxim,alert-low-soc-level : The alert threshold that sets the state of + charge level (%) where an interrupt is + generated. Can be configured from 1 up to 32 + (%). If skipped the power up default value of + 4 (%) will be used. +- interrupts : Interrupt line see Documentation/devicetree/ + bindings/interrupt-controller/interrupts.txt +- wakeup-source : This device has wakeup capabilities. Use this + property to use alert low SOC level interrupt + as wake up source. + +Optional properties support interrupt functionality for alert low state of +charge level, present in some ICs in the same family, and should be used with +compatible "maxim,max77836-battery". + +Example: + + battery-fuel-gauge@36 { + compatible = "maxim,max77836-battery"; + reg = <0x36>; + maxim,alert-low-soc-level = <10>; + interrupt-parent = <&gpio7>; + interrupts = <2 IRQ_TYPE_EDGE_FALLING>; + wakeup-source; + }; -- 2.24.0.rc2 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* [PATCH v5 3/5] devicetree: mfd: max14577: Add reference to max14040_battery.txt descriptions 2019-11-05 5:42 ` [PATCH v5 0/5] power: supply: MAX17040: Add IRQ for low level and alert SOC changes Matheus Castello 2019-11-05 5:42 ` [PATCH v5 1/5] power: supply: max17040: Add IRQ handler for low SOC alert Matheus Castello 2019-11-05 5:42 ` [PATCH v5 2/5] dt-bindings: power: supply: Max17040: Add DT bindings for max17040 fuel gauge Matheus Castello @ 2019-11-05 5:42 ` Matheus Castello 2019-11-05 5:42 ` [PATCH v5 4/5] power: supply: max17040: Config alert SOC low level threshold from FDT Matheus Castello 2019-11-05 5:42 ` [PATCH v5 5/5] power: supply: max17040: Send uevent in SOC and status change Matheus Castello 4 siblings, 0 replies; 82+ messages in thread From: Matheus Castello @ 2019-11-05 5:42 UTC (permalink / raw) To: sre, krzk, robh+dt Cc: mark.rutland, cw00.choi, b.zolnierkie, lee.jones, linux-pm, devicetree, linux-kernel, Matheus Castello max77836 MFD has a fuel gauge that has a low SOC alert feature that is described in Documentation/devicetree/bindings/power/supply/max17040_battery.txt. Adding the reference to de documentation here. Signed-off-by: Matheus Castello <matheus@castello.eng.br> --- Documentation/devicetree/bindings/mfd/max14577.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/devicetree/bindings/mfd/max14577.txt b/Documentation/devicetree/bindings/mfd/max14577.txt index fc6f0f4e8beb..04d790b1513c 100644 --- a/Documentation/devicetree/bindings/mfd/max14577.txt +++ b/Documentation/devicetree/bindings/mfd/max14577.txt @@ -5,6 +5,8 @@ Battery Charger and SFOUT LDO output for powering USB devices. It is interfaced to host controller using I2C. MAX77836 additionally contains PMIC (with two LDO regulators) and Fuel Gauge. +For the description of Fuel Gauge low SOC alert interrupt see: +Documentation/devicetree/bindings/power/supply/max17040_battery.txt Required properties: -- 2.24.0.rc2 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* [PATCH v5 4/5] power: supply: max17040: Config alert SOC low level threshold from FDT 2019-11-05 5:42 ` [PATCH v5 0/5] power: supply: MAX17040: Add IRQ for low level and alert SOC changes Matheus Castello ` (2 preceding siblings ...) 2019-11-05 5:42 ` [PATCH v5 3/5] devicetree: mfd: max14577: Add reference to max14040_battery.txt descriptions Matheus Castello @ 2019-11-05 5:42 ` Matheus Castello 2019-11-05 9:59 ` Krzysztof Kozlowski 2019-11-05 5:42 ` [PATCH v5 5/5] power: supply: max17040: Send uevent in SOC and status change Matheus Castello 4 siblings, 1 reply; 82+ messages in thread From: Matheus Castello @ 2019-11-05 5:42 UTC (permalink / raw) To: sre, krzk, robh+dt Cc: mark.rutland, cw00.choi, b.zolnierkie, lee.jones, linux-pm, devicetree, linux-kernel, Matheus Castello For configuration of fuel gauge alert for a low level state of charge interrupt we add a function to config level threshold and a device tree binding property to set it in flatned device tree node. Now we can use "maxim,alert-low-soc-level" property with the values from 1% up to 32% to configure alert interrupt threshold. Signed-off-by: Matheus Castello <matheus@castello.eng.br> --- drivers/power/supply/max17040_battery.c | 96 +++++++++++++++++++++---- 1 file changed, 82 insertions(+), 14 deletions(-) diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c index 75459f76d02c..c48a691cbd7b 100644 --- a/drivers/power/supply/max17040_battery.c +++ b/drivers/power/supply/max17040_battery.c @@ -29,6 +29,9 @@ #define MAX17040_DELAY 1000 #define MAX17040_BATTERY_FULL 95 +#define MAX17040_ATHD_MASK 0xFFC0 +#define MAX17040_ATHD_DEFAULT_POWER_UP 4 + struct max17040_chip { struct i2c_client *client; struct delayed_work work; @@ -43,6 +46,8 @@ struct max17040_chip { int soc; /* State Of Charge */ int status; + /* Low alert threshold from 32% to 1% of the State of Charge */ + u32 low_soc_alert; }; static int max17040_get_property(struct power_supply *psy, @@ -99,6 +104,21 @@ static void max17040_reset(struct i2c_client *client) max17040_write_reg(client, MAX17040_CMD, 0x0054); } +static int max17040_set_low_soc_alert(struct i2c_client *client, u32 level) +{ + int ret; + u16 data; + + level = 32 - level; + data = max17040_read_reg(client, MAX17040_RCOMP); + /* clear the alrt bit and set LSb 5 bits */ + data &= MAX17040_ATHD_MASK; + data |= level; + ret = max17040_write_reg(client, MAX17040_RCOMP, data); + + return ret; +} + static void max17040_get_vcell(struct i2c_client *client) { struct max17040_chip *chip = i2c_get_clientdata(client); @@ -115,7 +135,6 @@ static void max17040_get_soc(struct i2c_client *client) u16 soc; soc = max17040_read_reg(client, MAX17040_SOC); - chip->soc = (soc >> 8); } @@ -161,6 +180,24 @@ static void max17040_get_status(struct i2c_client *client) chip->status = POWER_SUPPLY_STATUS_FULL; } +static int max17040_get_of_data(struct max17040_chip *chip) +{ + struct device *dev = &chip->client->dev; + struct device_node *np = dev->of_node; + int ret = 0; + + if (of_property_read_u32(np, "maxim,alert-low-soc-level", + &chip->low_soc_alert)) { + chip->low_soc_alert = MAX17040_ATHD_DEFAULT_POWER_UP; + } else if (chip->low_soc_alert <= 0 || + chip->low_soc_alert >= 33) { + /* low_soc_alert is not between 1% and 32% */ + ret = -EINVAL; + } + + return ret; +} + static void max17040_check_changes(struct i2c_client *client) { max17040_get_vcell(client); @@ -192,9 +229,27 @@ static irqreturn_t max17040_thread_handler(int id, void *dev) /* send uevent */ power_supply_changed(chip->battery); + /* reset alert bit */ + max17040_set_low_soc_alert(client, chip->low_soc_alert); + return IRQ_HANDLED; } +static int max17040_enable_alert_irq(struct max17040_chip *chip) +{ + struct i2c_client *client = chip->client; + unsigned int flags; + int ret; + + dev_info(&client->dev, "IRQ: enabled\n"); + flags = IRQF_TRIGGER_FALLING | IRQF_ONESHOT; + ret = devm_request_threaded_irq(&client->dev, client->irq, NULL, + max17040_thread_handler, flags, + chip->battery->desc->name, chip); + + return ret; +} + static enum power_supply_property max17040_battery_props[] = { POWER_SUPPLY_PROP_STATUS, POWER_SUPPLY_PROP_ONLINE, @@ -216,6 +271,7 @@ static int max17040_probe(struct i2c_client *client, struct i2c_adapter *adapter = client->adapter; struct power_supply_config psy_cfg = {}; struct max17040_chip *chip; + int ret; if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE)) return -EIO; @@ -226,6 +282,12 @@ static int max17040_probe(struct i2c_client *client, chip->client = client; chip->pdata = client->dev.platform_data; + ret = max17040_get_of_data(chip); + if (ret) { + dev_err(&client->dev, + "failed: low SOC alert OF data out of bounds\n"); + return ret; + } i2c_set_clientdata(client, chip); psy_cfg.drv_data = chip; @@ -242,20 +304,26 @@ static int max17040_probe(struct i2c_client *client, /* check interrupt */ if (client->irq) { - int ret; - unsigned int flags; - - dev_info(&client->dev, "IRQ: enabled\n"); - flags = IRQF_TRIGGER_FALLING | IRQF_ONESHOT; - ret = devm_request_threaded_irq(&client->dev, client->irq, NULL, - max17040_thread_handler, flags, - chip->battery->desc->name, - chip); - - if (ret) { - client->irq = 0; + if (of_device_is_compatible(client->dev.of_node, + "maxim,max77836-battery")) { + ret = max17040_set_low_soc_alert(client, + chip->low_soc_alert); + if (ret) { + dev_err(&client->dev, + "Failed to set low SOC alert: err %d\n", + ret); + return ret; + } + + ret = max17040_enable_alert_irq(chip); + if (ret) { + client->irq = 0; + dev_warn(&client->dev, + "Failed to get IRQ err %d\n", ret); + } + } else { dev_warn(&client->dev, - "Failed to get IRQ err %d\n", ret); + "Device not compatible for IRQ"); } } -- 2.24.0.rc2 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* Re: [PATCH v5 4/5] power: supply: max17040: Config alert SOC low level threshold from FDT 2019-11-05 5:42 ` [PATCH v5 4/5] power: supply: max17040: Config alert SOC low level threshold from FDT Matheus Castello @ 2019-11-05 9:59 ` Krzysztof Kozlowski 2019-11-07 3:17 ` [PATCH v6 0/5] power: supply: MAX17040: Add IRQ for low level and alert SOC changes Matheus Castello 0 siblings, 1 reply; 82+ messages in thread From: Krzysztof Kozlowski @ 2019-11-05 9:59 UTC (permalink / raw) To: Matheus Castello Cc: sre, robh+dt, mark.rutland, cw00.choi, b.zolnierkie, lee.jones, linux-pm, devicetree, linux-kernel On Tue, Nov 05, 2019 at 02:42:17AM -0300, Matheus Castello wrote: > For configuration of fuel gauge alert for a low level state of charge > interrupt we add a function to config level threshold and a device tree > binding property to set it in flatned device tree node. > > Now we can use "maxim,alert-low-soc-level" property with the values from > 1% up to 32% to configure alert interrupt threshold. > > Signed-off-by: Matheus Castello <matheus@castello.eng.br> > --- > drivers/power/supply/max17040_battery.c | 96 +++++++++++++++++++++---- > 1 file changed, 82 insertions(+), 14 deletions(-) > > diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c > index 75459f76d02c..c48a691cbd7b 100644 > --- a/drivers/power/supply/max17040_battery.c > +++ b/drivers/power/supply/max17040_battery.c > @@ -29,6 +29,9 @@ > #define MAX17040_DELAY 1000 > #define MAX17040_BATTERY_FULL 95 > > +#define MAX17040_ATHD_MASK 0xFFC0 > +#define MAX17040_ATHD_DEFAULT_POWER_UP 4 > + > struct max17040_chip { > struct i2c_client *client; > struct delayed_work work; > @@ -43,6 +46,8 @@ struct max17040_chip { > int soc; > /* State Of Charge */ > int status; > + /* Low alert threshold from 32% to 1% of the State of Charge */ > + u32 low_soc_alert; > }; > > static int max17040_get_property(struct power_supply *psy, > @@ -99,6 +104,21 @@ static void max17040_reset(struct i2c_client *client) > max17040_write_reg(client, MAX17040_CMD, 0x0054); > } > > +static int max17040_set_low_soc_alert(struct i2c_client *client, u32 level) > +{ > + int ret; > + u16 data; > + > + level = 32 - level; > + data = max17040_read_reg(client, MAX17040_RCOMP); > + /* clear the alrt bit and set LSb 5 bits */ > + data &= MAX17040_ATHD_MASK; > + data |= level; > + ret = max17040_write_reg(client, MAX17040_RCOMP, data); > + > + return ret; > +} > + > static void max17040_get_vcell(struct i2c_client *client) > { > struct max17040_chip *chip = i2c_get_clientdata(client); > @@ -115,7 +135,6 @@ static void max17040_get_soc(struct i2c_client *client) > u16 soc; > > soc = max17040_read_reg(client, MAX17040_SOC); > - > chip->soc = (soc >> 8); > } > > @@ -161,6 +180,24 @@ static void max17040_get_status(struct i2c_client *client) > chip->status = POWER_SUPPLY_STATUS_FULL; > } > > +static int max17040_get_of_data(struct max17040_chip *chip) > +{ > + struct device *dev = &chip->client->dev; > + struct device_node *np = dev->of_node; > + int ret = 0; > + > + if (of_property_read_u32(np, "maxim,alert-low-soc-level", > + &chip->low_soc_alert)) { > + chip->low_soc_alert = MAX17040_ATHD_DEFAULT_POWER_UP; > + } else if (chip->low_soc_alert <= 0 || > + chip->low_soc_alert >= 33) { > + /* low_soc_alert is not between 1% and 32% */ > + ret = -EINVAL; > + } > + > + return ret; > +} > + > static void max17040_check_changes(struct i2c_client *client) > { > max17040_get_vcell(client); > @@ -192,9 +229,27 @@ static irqreturn_t max17040_thread_handler(int id, void *dev) > /* send uevent */ > power_supply_changed(chip->battery); > > + /* reset alert bit */ > + max17040_set_low_soc_alert(client, chip->low_soc_alert); > + > return IRQ_HANDLED; > } > > +static int max17040_enable_alert_irq(struct max17040_chip *chip) > +{ It does not make really sense to move this code to separate function in this patch. This should be done in patch 1/5. Otherwise you add a code in patch 1 and later in patch 4 you immediately rearrange it. This raises eybrows and gives a hint that patchset is not well structured. > + struct i2c_client *client = chip->client; > + unsigned int flags; > + int ret; > + > + dev_info(&client->dev, "IRQ: enabled\n"); While at it, get rid of dev_info here. It does not bring any useful information. All this is available in /proc/interrupts. Best regards, Krzysztof > + flags = IRQF_TRIGGER_FALLING | IRQF_ONESHOT; > + ret = devm_request_threaded_irq(&client->dev, client->irq, NULL, > + max17040_thread_handler, flags, > + chip->battery->desc->name, chip); > + > + return ret; > +} > + > static enum power_supply_property max17040_battery_props[] = { > POWER_SUPPLY_PROP_STATUS, > POWER_SUPPLY_PROP_ONLINE, > @@ -216,6 +271,7 @@ static int max17040_probe(struct i2c_client *client, > struct i2c_adapter *adapter = client->adapter; > struct power_supply_config psy_cfg = {}; > struct max17040_chip *chip; > + int ret; > > if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE)) > return -EIO; > @@ -226,6 +282,12 @@ static int max17040_probe(struct i2c_client *client, > > chip->client = client; > chip->pdata = client->dev.platform_data; > + ret = max17040_get_of_data(chip); > + if (ret) { > + dev_err(&client->dev, > + "failed: low SOC alert OF data out of bounds\n"); > + return ret; > + } > > i2c_set_clientdata(client, chip); > psy_cfg.drv_data = chip; > @@ -242,20 +304,26 @@ static int max17040_probe(struct i2c_client *client, > > /* check interrupt */ > if (client->irq) { > - int ret; > - unsigned int flags; > - > - dev_info(&client->dev, "IRQ: enabled\n"); > - flags = IRQF_TRIGGER_FALLING | IRQF_ONESHOT; > - ret = devm_request_threaded_irq(&client->dev, client->irq, NULL, > - max17040_thread_handler, flags, > - chip->battery->desc->name, > - chip); > - > - if (ret) { > - client->irq = 0; > + if (of_device_is_compatible(client->dev.of_node, > + "maxim,max77836-battery")) { > + ret = max17040_set_low_soc_alert(client, > + chip->low_soc_alert); > + if (ret) { > + dev_err(&client->dev, > + "Failed to set low SOC alert: err %d\n", > + ret); > + return ret; > + } > + > + ret = max17040_enable_alert_irq(chip); > + if (ret) { > + client->irq = 0; > + dev_warn(&client->dev, > + "Failed to get IRQ err %d\n", ret); > + } > + } else { > dev_warn(&client->dev, > - "Failed to get IRQ err %d\n", ret); > + "Device not compatible for IRQ"); > } > } > > -- > 2.24.0.rc2 > ^ permalink raw reply [flat|nested] 82+ messages in thread
* [PATCH v6 0/5] power: supply: MAX17040: Add IRQ for low level and alert SOC changes 2019-11-05 9:59 ` Krzysztof Kozlowski @ 2019-11-07 3:17 ` Matheus Castello 2019-11-07 3:17 ` [PATCH v6 1/5] power: supply: max17040: Add IRQ handler for low SOC alert Matheus Castello ` (5 more replies) 0 siblings, 6 replies; 82+ messages in thread From: Matheus Castello @ 2019-11-07 3:17 UTC (permalink / raw) To: sre, krzk, robh+dt Cc: mark.rutland, cw00.choi, b.zolnierkie, lee.jones, linux-pm, devicetree, linux-kernel, Matheus Castello This series add IRQ handler for low level SOC alert, define a devicetree binding attribute to configure the alert level threshold and check for changes in SOC and power supply status for send uevents. Max17043/17044 have a pin for alert host about low level state of charge and this alert can be configured in a threshold from 1% up to 32% of SOC. Tested on Toradex Colibri iMX7D, with a SparkFun Lipo Fuel Gauge module based on MAXIM MAX17043. Thanks Krzysztof for your time reviewing it. Let me know what you think about the fixes. Changes since v5: (Suggested by Krzysztof Kozlowski) - Rearrange code and add max17040_enable_alert_irq on patch 1/5 - Remove useless dev_info Changes since v4: (Suggested by Krzysztof Kozlowski) - Fix code style and alignment issues - Keep IRQF_TRIGGER_FALLING | IRQF_ONESHOT instead client->flags (Suggested by Rob Herring) - Add reference to the MFD description - Fix the dt-bindings commit description Matheus Castello (5): power: supply: max17040: Add IRQ handler for low SOC alert dt-bindings: power: supply: Max17040: Add DT bindings for max17040 fuel gauge devicetree: mfd: max14577: Add reference to max14040_battery.txt descriptions power: supply: max17040: Config alert SOC low level threshold from FDT power: supply: max17040: Send uevent in SOC and status change .../devicetree/bindings/mfd/max14577.txt | 2 + .../power/supply/max17040_battery.txt | 33 ++++ drivers/power/supply/max17040_battery.c | 141 +++++++++++++++++- 3 files changed, 171 insertions(+), 5 deletions(-) create mode 100644 Documentation/devicetree/bindings/power/supply/max17040_battery.txt -- 2.24.0.rc2 ^ permalink raw reply [flat|nested] 82+ messages in thread
* [PATCH v6 1/5] power: supply: max17040: Add IRQ handler for low SOC alert 2019-11-07 3:17 ` [PATCH v6 0/5] power: supply: MAX17040: Add IRQ for low level and alert SOC changes Matheus Castello @ 2019-11-07 3:17 ` Matheus Castello 2019-11-07 3:17 ` [PATCH v6 2/5] dt-bindings: power: supply: Max17040: Add DT bindings for max17040 fuel gauge Matheus Castello ` (4 subsequent siblings) 5 siblings, 0 replies; 82+ messages in thread From: Matheus Castello @ 2019-11-07 3:17 UTC (permalink / raw) To: sre, krzk, robh+dt Cc: mark.rutland, cw00.choi, b.zolnierkie, lee.jones, linux-pm, devicetree, linux-kernel, Matheus Castello According datasheet max17040 has a pin for alert host for low SOC. This pin can be used as external interrupt, so we need to check for interrupts assigned for device and handle it. In handler we are checking and storing fuel gauge registers values and send an uevent to notificate user space, so user space can decide save work or turn off since the alert demonstrate that the battery may no have the power to keep the system turned on for much longer. Signed-off-by: Matheus Castello <matheus@castello.eng.br> Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org> --- drivers/power/supply/max17040_battery.c | 73 +++++++++++++++++++++++-- 1 file changed, 68 insertions(+), 5 deletions(-) diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c index 62499018e68b..9909f8cd7b5d 100644 --- a/drivers/power/supply/max17040_battery.c +++ b/drivers/power/supply/max17040_battery.c @@ -13,6 +13,7 @@ #include <linux/err.h> #include <linux/i2c.h> #include <linux/delay.h> +#include <linux/interrupt.h> #include <linux/power_supply.h> #include <linux/max17040_battery.h> #include <linux/slab.h> @@ -160,21 +161,54 @@ static void max17040_get_status(struct i2c_client *client) chip->status = POWER_SUPPLY_STATUS_FULL; } +static void max17040_check_changes(struct i2c_client *client) +{ + max17040_get_vcell(client); + max17040_get_soc(client); + max17040_get_online(client); + max17040_get_status(client); +} + static void max17040_work(struct work_struct *work) { struct max17040_chip *chip; chip = container_of(work, struct max17040_chip, work.work); - - max17040_get_vcell(chip->client); - max17040_get_soc(chip->client); - max17040_get_online(chip->client); - max17040_get_status(chip->client); + max17040_check_changes(chip->client); queue_delayed_work(system_power_efficient_wq, &chip->work, MAX17040_DELAY); } +static irqreturn_t max17040_thread_handler(int id, void *dev) +{ + struct max17040_chip *chip = dev; + struct i2c_client *client = chip->client; + + dev_warn(&client->dev, "IRQ: Alert battery low level"); + /* read registers */ + max17040_check_changes(chip->client); + + /* send uevent */ + power_supply_changed(chip->battery); + + return IRQ_HANDLED; +} + +static int max17040_enable_alert_irq(struct max17040_chip *chip) +{ + struct i2c_client *client = chip->client; + unsigned int flags; + int ret; + + flags = IRQF_TRIGGER_FALLING | IRQF_ONESHOT; + ret = devm_request_threaded_irq(&client->dev, client->irq, NULL, + max17040_thread_handler, flags, + chip->battery->desc->name, chip); + + return ret; +} + static enum power_supply_property max17040_battery_props[] = { POWER_SUPPLY_PROP_STATUS, POWER_SUPPLY_PROP_ONLINE, @@ -220,6 +254,19 @@ static int max17040_probe(struct i2c_client *client, max17040_reset(client); max17040_get_version(client); + /* check interrupt */ + if (client->irq) { + int ret; + + ret = max17040_enable_alert_irq(chip); + + if (ret) { + client->irq = 0; + dev_warn(&client->dev, + "Failed to get IRQ err %d\n", ret); + } + } + INIT_DEFERRABLE_WORK(&chip->work, max17040_work); queue_delayed_work(system_power_efficient_wq, &chip->work, MAX17040_DELAY); @@ -244,6 +291,14 @@ static int max17040_suspend(struct device *dev) struct max17040_chip *chip = i2c_get_clientdata(client); cancel_delayed_work(&chip->work); + + if (client->irq) { + if (device_may_wakeup(dev)) + enable_irq_wake(client->irq); + else + disable_irq_wake(client->irq); + } + return 0; } @@ -254,6 +309,14 @@ static int max17040_resume(struct device *dev) queue_delayed_work(system_power_efficient_wq, &chip->work, MAX17040_DELAY); + + if (client->irq) { + if (device_may_wakeup(dev)) + disable_irq_wake(client->irq); + else + enable_irq_wake(client->irq); + } + return 0; } -- 2.24.0.rc2 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* [PATCH v6 2/5] dt-bindings: power: supply: Max17040: Add DT bindings for max17040 fuel gauge 2019-11-07 3:17 ` [PATCH v6 0/5] power: supply: MAX17040: Add IRQ for low level and alert SOC changes Matheus Castello 2019-11-07 3:17 ` [PATCH v6 1/5] power: supply: max17040: Add IRQ handler for low SOC alert Matheus Castello @ 2019-11-07 3:17 ` Matheus Castello 2019-11-14 0:53 ` Rob Herring 2019-11-07 3:17 ` [PATCH v6 3/5] devicetree: mfd: max14577: Add reference to max14040_battery.txt descriptions Matheus Castello ` (3 subsequent siblings) 5 siblings, 1 reply; 82+ messages in thread From: Matheus Castello @ 2019-11-07 3:17 UTC (permalink / raw) To: sre, krzk, robh+dt Cc: mark.rutland, cw00.choi, b.zolnierkie, lee.jones, linux-pm, devicetree, linux-kernel, Matheus Castello Documentation of max17040 based fuel gauge characteristics. For configure low level state of charge threshold alert signaled from max17043/max17044 we add "maxim,alert-low-soc-level" property. Signed-off-by: Matheus Castello <matheus@castello.eng.br> Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org> --- .../power/supply/max17040_battery.txt | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) create mode 100644 Documentation/devicetree/bindings/power/supply/max17040_battery.txt diff --git a/Documentation/devicetree/bindings/power/supply/max17040_battery.txt b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt new file mode 100644 index 000000000000..f2d0b22b5f79 --- /dev/null +++ b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt @@ -0,0 +1,33 @@ +max17040_battery +~~~~~~~~~~~~~~~~ + +Required properties : + - compatible : "maxim,max17040" or "maxim,max77836-battery" + - reg: i2c slave address + +Optional properties : +- maxim,alert-low-soc-level : The alert threshold that sets the state of + charge level (%) where an interrupt is + generated. Can be configured from 1 up to 32 + (%). If skipped the power up default value of + 4 (%) will be used. +- interrupts : Interrupt line see Documentation/devicetree/ + bindings/interrupt-controller/interrupts.txt +- wakeup-source : This device has wakeup capabilities. Use this + property to use alert low SOC level interrupt + as wake up source. + +Optional properties support interrupt functionality for alert low state of +charge level, present in some ICs in the same family, and should be used with +compatible "maxim,max77836-battery". + +Example: + + battery-fuel-gauge@36 { + compatible = "maxim,max77836-battery"; + reg = <0x36>; + maxim,alert-low-soc-level = <10>; + interrupt-parent = <&gpio7>; + interrupts = <2 IRQ_TYPE_EDGE_FALLING>; + wakeup-source; + }; -- 2.24.0.rc2 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* Re: [PATCH v6 2/5] dt-bindings: power: supply: Max17040: Add DT bindings for max17040 fuel gauge 2019-11-07 3:17 ` [PATCH v6 2/5] dt-bindings: power: supply: Max17040: Add DT bindings for max17040 fuel gauge Matheus Castello @ 2019-11-14 0:53 ` Rob Herring 0 siblings, 0 replies; 82+ messages in thread From: Rob Herring @ 2019-11-14 0:53 UTC (permalink / raw) To: Matheus Castello Cc: sre, krzk, robh+dt, mark.rutland, cw00.choi, b.zolnierkie, lee.jones, linux-pm, devicetree, linux-kernel, Matheus Castello On Thu, 7 Nov 2019 00:17:07 -0300, Matheus Castello wrote: > Documentation of max17040 based fuel gauge characteristics. > For configure low level state of charge threshold alert signaled from > max17043/max17044 we add "maxim,alert-low-soc-level" property. > > Signed-off-by: Matheus Castello <matheus@castello.eng.br> > Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org> > --- > .../power/supply/max17040_battery.txt | 33 +++++++++++++++++++ > 1 file changed, 33 insertions(+) > create mode 100644 Documentation/devicetree/bindings/power/supply/max17040_battery.txt > Reviewed-by: Rob Herring <robh@kernel.org> ^ permalink raw reply [flat|nested] 82+ messages in thread
* [PATCH v6 3/5] devicetree: mfd: max14577: Add reference to max14040_battery.txt descriptions 2019-11-07 3:17 ` [PATCH v6 0/5] power: supply: MAX17040: Add IRQ for low level and alert SOC changes Matheus Castello 2019-11-07 3:17 ` [PATCH v6 1/5] power: supply: max17040: Add IRQ handler for low SOC alert Matheus Castello 2019-11-07 3:17 ` [PATCH v6 2/5] dt-bindings: power: supply: Max17040: Add DT bindings for max17040 fuel gauge Matheus Castello @ 2019-11-07 3:17 ` Matheus Castello 2019-11-11 10:09 ` Lee Jones 2019-11-14 0:54 ` Rob Herring 2019-11-07 3:17 ` [PATCH v6 4/5] power: supply: max17040: Config alert SOC low level threshold from FDT Matheus Castello ` (2 subsequent siblings) 5 siblings, 2 replies; 82+ messages in thread From: Matheus Castello @ 2019-11-07 3:17 UTC (permalink / raw) To: sre, krzk, robh+dt Cc: mark.rutland, cw00.choi, b.zolnierkie, lee.jones, linux-pm, devicetree, linux-kernel, Matheus Castello max77836 MFD has a fuel gauge that has a low SOC alert feature that is described in Documentation/devicetree/bindings/power/supply/max17040_battery.txt. Adding the reference to de documentation here. Signed-off-by: Matheus Castello <matheus@castello.eng.br> --- Documentation/devicetree/bindings/mfd/max14577.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/devicetree/bindings/mfd/max14577.txt b/Documentation/devicetree/bindings/mfd/max14577.txt index fc6f0f4e8beb..04d790b1513c 100644 --- a/Documentation/devicetree/bindings/mfd/max14577.txt +++ b/Documentation/devicetree/bindings/mfd/max14577.txt @@ -5,6 +5,8 @@ Battery Charger and SFOUT LDO output for powering USB devices. It is interfaced to host controller using I2C. MAX77836 additionally contains PMIC (with two LDO regulators) and Fuel Gauge. +For the description of Fuel Gauge low SOC alert interrupt see: +Documentation/devicetree/bindings/power/supply/max17040_battery.txt Required properties: -- 2.24.0.rc2 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* Re: [PATCH v6 3/5] devicetree: mfd: max14577: Add reference to max14040_battery.txt descriptions 2019-11-07 3:17 ` [PATCH v6 3/5] devicetree: mfd: max14577: Add reference to max14040_battery.txt descriptions Matheus Castello @ 2019-11-11 10:09 ` Lee Jones 2019-11-14 0:54 ` Rob Herring 1 sibling, 0 replies; 82+ messages in thread From: Lee Jones @ 2019-11-11 10:09 UTC (permalink / raw) To: Matheus Castello Cc: sre, krzk, robh+dt, mark.rutland, cw00.choi, b.zolnierkie, linux-pm, devicetree, linux-kernel On Thu, 07 Nov 2019, Matheus Castello wrote: > max77836 MFD has a fuel gauge that has a low SOC alert feature that is > described in Documentation/devicetree/bindings/power/supply/max17040_battery.txt. > Adding the reference to de documentation here. > > Signed-off-by: Matheus Castello <matheus@castello.eng.br> > --- > Documentation/devicetree/bindings/mfd/max14577.txt | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/Documentation/devicetree/bindings/mfd/max14577.txt b/Documentation/devicetree/bindings/mfd/max14577.txt > index fc6f0f4e8beb..04d790b1513c 100644 > --- a/Documentation/devicetree/bindings/mfd/max14577.txt > +++ b/Documentation/devicetree/bindings/mfd/max14577.txt > @@ -5,6 +5,8 @@ Battery Charger and SFOUT LDO output for powering USB devices. It is > interfaced to host controller using I2C. > > MAX77836 additionally contains PMIC (with two LDO regulators) and Fuel Gauge. > +For the description of Fuel Gauge low SOC alert interrupt see: > +Documentation/devicetree/bindings/power/supply/max17040_battery.txt I would prefer the use of relative paths in documentation. Once fixed, please apply my: Acked-by: Lee Jones <lee.jones@linaro.org> -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH v6 3/5] devicetree: mfd: max14577: Add reference to max14040_battery.txt descriptions 2019-11-07 3:17 ` [PATCH v6 3/5] devicetree: mfd: max14577: Add reference to max14040_battery.txt descriptions Matheus Castello 2019-11-11 10:09 ` Lee Jones @ 2019-11-14 0:54 ` Rob Herring 1 sibling, 0 replies; 82+ messages in thread From: Rob Herring @ 2019-11-14 0:54 UTC (permalink / raw) To: Matheus Castello Cc: sre, krzk, robh+dt, mark.rutland, cw00.choi, b.zolnierkie, lee.jones, linux-pm, devicetree, linux-kernel, Matheus Castello On Thu, 7 Nov 2019 00:17:08 -0300, Matheus Castello wrote: > max77836 MFD has a fuel gauge that has a low SOC alert feature that is > described in Documentation/devicetree/bindings/power/supply/max17040_battery.txt. > Adding the reference to de documentation here. > > Signed-off-by: Matheus Castello <matheus@castello.eng.br> > --- > Documentation/devicetree/bindings/mfd/max14577.txt | 2 ++ > 1 file changed, 2 insertions(+) > Acked-by: Rob Herring <robh@kernel.org> ^ permalink raw reply [flat|nested] 82+ messages in thread
* [PATCH v6 4/5] power: supply: max17040: Config alert SOC low level threshold from FDT 2019-11-07 3:17 ` [PATCH v6 0/5] power: supply: MAX17040: Add IRQ for low level and alert SOC changes Matheus Castello ` (2 preceding siblings ...) 2019-11-07 3:17 ` [PATCH v6 3/5] devicetree: mfd: max14577: Add reference to max14040_battery.txt descriptions Matheus Castello @ 2019-11-07 3:17 ` Matheus Castello 2019-11-07 3:17 ` [PATCH v6 5/5] power: supply: max17040: Send uevent in SOC and status change Matheus Castello 2019-11-11 9:59 ` [PATCH v6 0/5] power: supply: MAX17040: Add IRQ for low level and alert SOC changes Lee Jones 5 siblings, 0 replies; 82+ messages in thread From: Matheus Castello @ 2019-11-07 3:17 UTC (permalink / raw) To: sre, krzk, robh+dt Cc: mark.rutland, cw00.choi, b.zolnierkie, lee.jones, linux-pm, devicetree, linux-kernel, Matheus Castello For configuration of fuel gauge alert for a low level state of charge interrupt we add a function to config level threshold and a device tree binding property to set it in flatned device tree node. Now we can use "maxim,alert-low-soc-level" property with the values from 1% up to 32% to configure alert interrupt threshold. Signed-off-by: Matheus Castello <matheus@castello.eng.br> --- drivers/power/supply/max17040_battery.c | 75 ++++++++++++++++++++++--- 1 file changed, 67 insertions(+), 8 deletions(-) diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c index 9909f8cd7b5d..3fc9e1c7b257 100644 --- a/drivers/power/supply/max17040_battery.c +++ b/drivers/power/supply/max17040_battery.c @@ -29,6 +29,9 @@ #define MAX17040_DELAY 1000 #define MAX17040_BATTERY_FULL 95 +#define MAX17040_ATHD_MASK 0xFFC0 +#define MAX17040_ATHD_DEFAULT_POWER_UP 4 + struct max17040_chip { struct i2c_client *client; struct delayed_work work; @@ -43,6 +46,8 @@ struct max17040_chip { int soc; /* State Of Charge */ int status; + /* Low alert threshold from 32% to 1% of the State of Charge */ + u32 low_soc_alert; }; static int max17040_get_property(struct power_supply *psy, @@ -99,6 +104,21 @@ static void max17040_reset(struct i2c_client *client) max17040_write_reg(client, MAX17040_CMD, 0x0054); } +static int max17040_set_low_soc_alert(struct i2c_client *client, u32 level) +{ + int ret; + u16 data; + + level = 32 - level; + data = max17040_read_reg(client, MAX17040_RCOMP); + /* clear the alrt bit and set LSb 5 bits */ + data &= MAX17040_ATHD_MASK; + data |= level; + ret = max17040_write_reg(client, MAX17040_RCOMP, data); + + return ret; +} + static void max17040_get_vcell(struct i2c_client *client) { struct max17040_chip *chip = i2c_get_clientdata(client); @@ -115,7 +135,6 @@ static void max17040_get_soc(struct i2c_client *client) u16 soc; soc = max17040_read_reg(client, MAX17040_SOC); - chip->soc = (soc >> 8); } @@ -161,6 +180,24 @@ static void max17040_get_status(struct i2c_client *client) chip->status = POWER_SUPPLY_STATUS_FULL; } +static int max17040_get_of_data(struct max17040_chip *chip) +{ + struct device *dev = &chip->client->dev; + struct device_node *np = dev->of_node; + int ret = 0; + + if (of_property_read_u32(np, "maxim,alert-low-soc-level", + &chip->low_soc_alert)) { + chip->low_soc_alert = MAX17040_ATHD_DEFAULT_POWER_UP; + } else if (chip->low_soc_alert <= 0 || + chip->low_soc_alert >= 33) { + /* low_soc_alert is not between 1% and 32% */ + ret = -EINVAL; + } + + return ret; +} + static void max17040_check_changes(struct i2c_client *client) { max17040_get_vcell(client); @@ -192,6 +229,9 @@ static irqreturn_t max17040_thread_handler(int id, void *dev) /* send uevent */ power_supply_changed(chip->battery); + /* reset alert bit */ + max17040_set_low_soc_alert(client, chip->low_soc_alert); + return IRQ_HANDLED; } @@ -230,6 +270,7 @@ static int max17040_probe(struct i2c_client *client, struct i2c_adapter *adapter = client->adapter; struct power_supply_config psy_cfg = {}; struct max17040_chip *chip; + int ret; if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE)) return -EIO; @@ -240,6 +281,12 @@ static int max17040_probe(struct i2c_client *client, chip->client = client; chip->pdata = client->dev.platform_data; + ret = max17040_get_of_data(chip); + if (ret) { + dev_err(&client->dev, + "failed: low SOC alert OF data out of bounds\n"); + return ret; + } i2c_set_clientdata(client, chip); psy_cfg.drv_data = chip; @@ -256,14 +303,26 @@ static int max17040_probe(struct i2c_client *client, /* check interrupt */ if (client->irq) { - int ret; - - ret = max17040_enable_alert_irq(chip); - - if (ret) { - client->irq = 0; + if (of_device_is_compatible(client->dev.of_node, + "maxim,max77836-battery")) { + ret = max17040_set_low_soc_alert(client, + chip->low_soc_alert); + if (ret) { + dev_err(&client->dev, + "Failed to set low SOC alert: err %d\n", + ret); + return ret; + } + + ret = max17040_enable_alert_irq(chip); + if (ret) { + client->irq = 0; + dev_warn(&client->dev, + "Failed to get IRQ err %d\n", ret); + } + } else { dev_warn(&client->dev, - "Failed to get IRQ err %d\n", ret); + "Device not compatible for IRQ"); } } -- 2.24.0.rc2 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* [PATCH v6 5/5] power: supply: max17040: Send uevent in SOC and status change 2019-11-07 3:17 ` [PATCH v6 0/5] power: supply: MAX17040: Add IRQ for low level and alert SOC changes Matheus Castello ` (3 preceding siblings ...) 2019-11-07 3:17 ` [PATCH v6 4/5] power: supply: max17040: Config alert SOC low level threshold from FDT Matheus Castello @ 2019-11-07 3:17 ` Matheus Castello 2019-11-11 9:59 ` [PATCH v6 0/5] power: supply: MAX17040: Add IRQ for low level and alert SOC changes Lee Jones 5 siblings, 0 replies; 82+ messages in thread From: Matheus Castello @ 2019-11-07 3:17 UTC (permalink / raw) To: sre, krzk, robh+dt Cc: mark.rutland, cw00.choi, b.zolnierkie, lee.jones, linux-pm, devicetree, linux-kernel, Matheus Castello Notify core through power_supply_changed() in case of changes in state of charge and power supply status. This is useful for user-space to efficiently update current battery level. Signed-off-by: Matheus Castello <matheus@castello.eng.br> Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org> --- drivers/power/supply/max17040_battery.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c index 3fc9e1c7b257..1f5afabdbabc 100644 --- a/drivers/power/supply/max17040_battery.c +++ b/drivers/power/supply/max17040_battery.c @@ -209,10 +209,19 @@ static void max17040_check_changes(struct i2c_client *client) static void max17040_work(struct work_struct *work) { struct max17040_chip *chip; + int last_soc, last_status; chip = container_of(work, struct max17040_chip, work.work); + + /* store SOC and status to check changes */ + last_soc = chip->soc; + last_status = chip->status; max17040_check_changes(chip->client); + /* check changes and send uevent */ + if (last_soc != chip->soc || last_status != chip->status) + power_supply_changed(chip->battery); + queue_delayed_work(system_power_efficient_wq, &chip->work, MAX17040_DELAY); } -- 2.24.0.rc2 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* Re: [PATCH v6 0/5] power: supply: MAX17040: Add IRQ for low level and alert SOC changes 2019-11-07 3:17 ` [PATCH v6 0/5] power: supply: MAX17040: Add IRQ for low level and alert SOC changes Matheus Castello ` (4 preceding siblings ...) 2019-11-07 3:17 ` [PATCH v6 5/5] power: supply: max17040: Send uevent in SOC and status change Matheus Castello @ 2019-11-11 9:59 ` Lee Jones 5 siblings, 0 replies; 82+ messages in thread From: Lee Jones @ 2019-11-11 9:59 UTC (permalink / raw) To: Matheus Castello Cc: sre, krzk, robh+dt, mark.rutland, cw00.choi, b.zolnierkie, linux-pm, devicetree, linux-kernel On Thu, 07 Nov 2019, Matheus Castello wrote: > This series add IRQ handler for low level SOC alert, define a devicetree > binding attribute to configure the alert level threshold and check for > changes in SOC and power supply status for send uevents. > > Max17043/17044 have a pin for alert host about low level state of charge and > this alert can be configured in a threshold from 1% up to 32% of SOC. > > Tested on Toradex Colibri iMX7D, with a SparkFun Lipo Fuel Gauge module > based on MAXIM MAX17043. > > Thanks Krzysztof for your time reviewing it. Let me know what you think about > the fixes. > > Changes since v5: > (Suggested by Krzysztof Kozlowski) > - Rearrange code and add max17040_enable_alert_irq on patch 1/5 > - Remove useless dev_info Just something to bear in mind in the future: When submitting subsequent versions, could you please do so as separate sets? Attaching them to previous submissions gets confusing real quick. -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog ^ permalink raw reply [flat|nested] 82+ messages in thread
* [PATCH v5 5/5] power: supply: max17040: Send uevent in SOC and status change 2019-11-05 5:42 ` [PATCH v5 0/5] power: supply: MAX17040: Add IRQ for low level and alert SOC changes Matheus Castello ` (3 preceding siblings ...) 2019-11-05 5:42 ` [PATCH v5 4/5] power: supply: max17040: Config alert SOC low level threshold from FDT Matheus Castello @ 2019-11-05 5:42 ` Matheus Castello 4 siblings, 0 replies; 82+ messages in thread From: Matheus Castello @ 2019-11-05 5:42 UTC (permalink / raw) To: sre, krzk, robh+dt Cc: mark.rutland, cw00.choi, b.zolnierkie, lee.jones, linux-pm, devicetree, linux-kernel, Matheus Castello Notify core through power_supply_changed() in case of changes in state of charge and power supply status. This is useful for user-space to efficiently update current battery level. Signed-off-by: Matheus Castello <matheus@castello.eng.br> Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org> --- drivers/power/supply/max17040_battery.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c index c48a691cbd7b..d7405650cc38 100644 --- a/drivers/power/supply/max17040_battery.c +++ b/drivers/power/supply/max17040_battery.c @@ -209,10 +209,19 @@ static void max17040_check_changes(struct i2c_client *client) static void max17040_work(struct work_struct *work) { struct max17040_chip *chip; + int last_soc, last_status; chip = container_of(work, struct max17040_chip, work.work); + + /* store SOC and status to check changes */ + last_soc = chip->soc; + last_status = chip->status; max17040_check_changes(chip->client); + /* check changes and send uevent */ + if (last_soc != chip->soc || last_status != chip->status) + power_supply_changed(chip->battery); + queue_delayed_work(system_power_efficient_wq, &chip->work, MAX17040_DELAY); } -- 2.24.0.rc2 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* [PATCH v4 3/4] power: supply: max17040: Config alert SOC low level threshold from FDT 2019-10-31 18:41 ` [PATCH v4 0/4] power: supply: MAX17040: Add IRQ for low level and alert SOC changes Matheus Castello 2019-10-31 18:41 ` [PATCH v4 1/4] power: supply: max17040: Add IRQ handler for low SOC alert Matheus Castello 2019-10-31 18:41 ` [PATCH v4 2/4] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold Matheus Castello @ 2019-10-31 18:41 ` Matheus Castello 2019-11-01 15:27 ` Krzysztof Kozlowski 2019-10-31 18:41 ` [PATCH v4 4/4] power: supply: max17040: Send uevent in SOC and status change Matheus Castello 3 siblings, 1 reply; 82+ messages in thread From: Matheus Castello @ 2019-10-31 18:41 UTC (permalink / raw) To: sre, krzk, robh+dt Cc: mark.rutland, cw00.choi, b.zolnierkie, lee.jones, linux-pm, devicetree, linux-kernel, Matheus Castello For configuration of fuel gauge alert for a low level state of charge interrupt we add a function to config level threshold and a device tree binding property to set it in flatned device tree node. Now we can use "maxim,alert-low-soc-level" property with the values from 1% up to 32% to configure alert interrupt threshold. Signed-off-by: Matheus Castello <matheus@castello.eng.br> --- drivers/power/supply/max17040_battery.c | 88 +++++++++++++++++++++---- 1 file changed, 74 insertions(+), 14 deletions(-) diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c index 75459f76d02c..802575342c72 100644 --- a/drivers/power/supply/max17040_battery.c +++ b/drivers/power/supply/max17040_battery.c @@ -29,6 +29,9 @@ #define MAX17040_DELAY 1000 #define MAX17040_BATTERY_FULL 95 +#define MAX17040_ATHD_MASK 0xFFC0 +#define MAX17040_ATHD_DEFAULT_POWER_UP 4 + struct max17040_chip { struct i2c_client *client; struct delayed_work work; @@ -43,6 +46,8 @@ struct max17040_chip { int soc; /* State Of Charge */ int status; + /* Low alert threshold from 32% to 1% of the State of Charge */ + u32 low_soc_alert_threshold; }; static int max17040_get_property(struct power_supply *psy, @@ -99,6 +104,22 @@ static void max17040_reset(struct i2c_client *client) max17040_write_reg(client, MAX17040_CMD, 0x0054); } +static int max17040_set_low_soc_threshold_alert(struct i2c_client *client, + u32 level) +{ + int ret; + u16 data; + + level = 32 - level; + data = max17040_read_reg(client, MAX17040_RCOMP); + /* clear the alrt bit and set LSb 5 bits */ + data &= MAX17040_ATHD_MASK; + data |= level; + ret = max17040_write_reg(client, MAX17040_RCOMP, data); + + return ret; +} + static void max17040_get_vcell(struct i2c_client *client) { struct max17040_chip *chip = i2c_get_clientdata(client); @@ -115,7 +136,6 @@ static void max17040_get_soc(struct i2c_client *client) u16 soc; soc = max17040_read_reg(client, MAX17040_SOC); - chip->soc = (soc >> 8); } @@ -161,6 +181,24 @@ static void max17040_get_status(struct i2c_client *client) chip->status = POWER_SUPPLY_STATUS_FULL; } +static int max17040_get_of_data(struct max17040_chip *chip) +{ + struct device *dev = &chip->client->dev; + struct device_node *np = dev->of_node; + int ret = 0; + + if (of_property_read_u32(np, "maxim,alert-low-soc-level", + &chip->low_soc_alert_threshold)) { + chip->low_soc_alert_threshold = MAX17040_ATHD_DEFAULT_POWER_UP; + /* check if low_soc_alert_threshold is between 1% and 32% */ + } else if (chip->low_soc_alert_threshold <= 0 || + chip->low_soc_alert_threshold >= 33){ + ret = -EINVAL; + } + + return ret; +} + static void max17040_check_changes(struct i2c_client *client) { max17040_get_vcell(client); @@ -192,6 +230,10 @@ static irqreturn_t max17040_thread_handler(int id, void *dev) /* send uevent */ power_supply_changed(chip->battery); + /* reset alert bit */ + max17040_set_low_soc_threshold_alert(client, + chip->low_soc_alert_threshold); + return IRQ_HANDLED; } @@ -216,6 +258,7 @@ static int max17040_probe(struct i2c_client *client, struct i2c_adapter *adapter = client->adapter; struct power_supply_config psy_cfg = {}; struct max17040_chip *chip; + int ret; if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE)) return -EIO; @@ -226,6 +269,12 @@ static int max17040_probe(struct i2c_client *client, chip->client = client; chip->pdata = client->dev.platform_data; + ret = max17040_get_of_data(chip); + if (ret) { + dev_err(&client->dev, + "failed: low SOC alert OF data out of bounds\n"); + return ret; + } i2c_set_clientdata(client, chip); psy_cfg.drv_data = chip; @@ -242,20 +291,31 @@ static int max17040_probe(struct i2c_client *client, /* check interrupt */ if (client->irq) { - int ret; - unsigned int flags; - - dev_info(&client->dev, "IRQ: enabled\n"); - flags = IRQF_TRIGGER_FALLING | IRQF_ONESHOT; - ret = devm_request_threaded_irq(&client->dev, client->irq, NULL, - max17040_thread_handler, flags, - chip->battery->desc->name, - chip); - - if (ret) { - client->irq = 0; + if (of_device_is_compatible(client->dev.of_node, + "maxim,max77836-battery")) { + ret = max17040_set_low_soc_threshold_alert(client, + chip->low_soc_alert_threshold); + if (ret) { + dev_err(&client->dev, + "Failed to set low SOC alert: err %d\n", + ret); + return ret; + } + + dev_info(&client->dev, "IRQ: enabled\n"); + ret = devm_request_threaded_irq(&client->dev, + client->irq, NULL, max17040_thread_handler, + (client->flags | IRQF_ONESHOT), + chip->battery->desc->name, chip); + + if (ret) { + client->irq = 0; + dev_warn(&client->dev, + "Failed to get IRQ err %d\n", ret); + } + } else { dev_warn(&client->dev, - "Failed to get IRQ err %d\n", ret); + "Device not compatible for IRQ"); } } -- 2.24.0.rc2 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* Re: [PATCH v4 3/4] power: supply: max17040: Config alert SOC low level threshold from FDT 2019-10-31 18:41 ` [PATCH v4 3/4] power: supply: max17040: Config alert SOC low level threshold from FDT Matheus Castello @ 2019-11-01 15:27 ` Krzysztof Kozlowski 2019-11-01 16:52 ` Matheus Castello 0 siblings, 1 reply; 82+ messages in thread From: Krzysztof Kozlowski @ 2019-11-01 15:27 UTC (permalink / raw) To: Matheus Castello Cc: sre, robh+dt, mark.rutland, cw00.choi, b.zolnierkie, lee.jones, linux-pm, devicetree, linux-kernel On Thu, Oct 31, 2019 at 03:41:33PM -0300, Matheus Castello wrote: > For configuration of fuel gauge alert for a low level state of charge > interrupt we add a function to config level threshold and a device tree > binding property to set it in flatned device tree node. > > Now we can use "maxim,alert-low-soc-level" property with the values from > 1% up to 32% to configure alert interrupt threshold. > > Signed-off-by: Matheus Castello <matheus@castello.eng.br> > --- > drivers/power/supply/max17040_battery.c | 88 +++++++++++++++++++++---- > 1 file changed, 74 insertions(+), 14 deletions(-) > > diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c > index 75459f76d02c..802575342c72 100644 > --- a/drivers/power/supply/max17040_battery.c > +++ b/drivers/power/supply/max17040_battery.c > @@ -29,6 +29,9 @@ > #define MAX17040_DELAY 1000 > #define MAX17040_BATTERY_FULL 95 > > +#define MAX17040_ATHD_MASK 0xFFC0 > +#define MAX17040_ATHD_DEFAULT_POWER_UP 4 > + > struct max17040_chip { > struct i2c_client *client; > struct delayed_work work; > @@ -43,6 +46,8 @@ struct max17040_chip { > int soc; > /* State Of Charge */ > int status; > + /* Low alert threshold from 32% to 1% of the State of Charge */ > + u32 low_soc_alert_threshold; > }; > > static int max17040_get_property(struct power_supply *psy, > @@ -99,6 +104,22 @@ static void max17040_reset(struct i2c_client *client) > max17040_write_reg(client, MAX17040_CMD, 0x0054); > } > > +static int max17040_set_low_soc_threshold_alert(struct i2c_client *client, > + u32 level) > +{ > + int ret; > + u16 data; > + > + level = 32 - level; > + data = max17040_read_reg(client, MAX17040_RCOMP); > + /* clear the alrt bit and set LSb 5 bits */ > + data &= MAX17040_ATHD_MASK; > + data |= level; > + ret = max17040_write_reg(client, MAX17040_RCOMP, data); > + > + return ret; > +} > + > static void max17040_get_vcell(struct i2c_client *client) > { > struct max17040_chip *chip = i2c_get_clientdata(client); > @@ -115,7 +136,6 @@ static void max17040_get_soc(struct i2c_client *client) > u16 soc; > > soc = max17040_read_reg(client, MAX17040_SOC); > - > chip->soc = (soc >> 8); > } > > @@ -161,6 +181,24 @@ static void max17040_get_status(struct i2c_client *client) > chip->status = POWER_SUPPLY_STATUS_FULL; > } > > +static int max17040_get_of_data(struct max17040_chip *chip) > +{ > + struct device *dev = &chip->client->dev; > + struct device_node *np = dev->of_node; > + int ret = 0; > + > + if (of_property_read_u32(np, "maxim,alert-low-soc-level", > + &chip->low_soc_alert_threshold)) { Please align the line break with line above. checkpatch --strict might give you hints about this. > + chip->low_soc_alert_threshold = MAX17040_ATHD_DEFAULT_POWER_UP; > + /* check if low_soc_alert_threshold is between 1% and 32% */ The comment looks misleading here, like it belongs to previous block. Maybe put it inside else if {} block? > + } else if (chip->low_soc_alert_threshold <= 0 || > + chip->low_soc_alert_threshold >= 33){ Missing space before {. > + ret = -EINVAL; > + } > + > + return ret; > +} > + > static void max17040_check_changes(struct i2c_client *client) > { > max17040_get_vcell(client); > @@ -192,6 +230,10 @@ static irqreturn_t max17040_thread_handler(int id, void *dev) > /* send uevent */ > power_supply_changed(chip->battery); > > + /* reset alert bit */ > + max17040_set_low_soc_threshold_alert(client, > + chip->low_soc_alert_threshold); Unless the continuation exceeds 80 character limit, please align it with previous line. > + > return IRQ_HANDLED; > } > > @@ -216,6 +258,7 @@ static int max17040_probe(struct i2c_client *client, > struct i2c_adapter *adapter = client->adapter; > struct power_supply_config psy_cfg = {}; > struct max17040_chip *chip; > + int ret; > > if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE)) > return -EIO; > @@ -226,6 +269,12 @@ static int max17040_probe(struct i2c_client *client, > > chip->client = client; > chip->pdata = client->dev.platform_data; > + ret = max17040_get_of_data(chip); > + if (ret) { > + dev_err(&client->dev, > + "failed: low SOC alert OF data out of bounds\n"); > + return ret; > + } > > i2c_set_clientdata(client, chip); > psy_cfg.drv_data = chip; > @@ -242,20 +291,31 @@ static int max17040_probe(struct i2c_client *client, > > /* check interrupt */ > if (client->irq) { > - int ret; > - unsigned int flags; > - > - dev_info(&client->dev, "IRQ: enabled\n"); > - flags = IRQF_TRIGGER_FALLING | IRQF_ONESHOT; > - ret = devm_request_threaded_irq(&client->dev, client->irq, NULL, > - max17040_thread_handler, flags, > - chip->battery->desc->name, > - chip); > - > - if (ret) { > - client->irq = 0; > + if (of_device_is_compatible(client->dev.of_node, > + "maxim,max77836-battery")) { Alignment. > + ret = max17040_set_low_soc_threshold_alert(client, > + chip->low_soc_alert_threshold); Ditto. > + if (ret) { > + dev_err(&client->dev, > + "Failed to set low SOC alert: err %d\n", > + ret); > + return ret; > + } > + > + dev_info(&client->dev, "IRQ: enabled\n"); > + ret = devm_request_threaded_irq(&client->dev, > + client->irq, NULL, max17040_thread_handler, > + (client->flags | IRQF_ONESHOT), This looks unrelated. Befor ethis were IRQF_TRIGGER_FALLING | IRQF_ONESHOT, now you use client->flags. There is no reason why this commit should change it. > + chip->battery->desc->name, chip); This breaks alignment which was here before. Best regards, Krzysztof > + > + if (ret) { > + client->irq = 0; > + dev_warn(&client->dev, > + "Failed to get IRQ err %d\n", ret); > + } > + } else { > dev_warn(&client->dev, > - "Failed to get IRQ err %d\n", ret); > + "Device not compatible for IRQ"); > } > } > > -- > 2.24.0.rc2 > ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH v4 3/4] power: supply: max17040: Config alert SOC low level threshold from FDT 2019-11-01 15:27 ` Krzysztof Kozlowski @ 2019-11-01 16:52 ` Matheus Castello 2019-11-02 18:12 ` Matheus Castello 2019-11-04 10:59 ` Krzysztof Kozlowski 0 siblings, 2 replies; 82+ messages in thread From: Matheus Castello @ 2019-11-01 16:52 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: sre, robh+dt, mark.rutland, cw00.choi, b.zolnierkie, lee.jones, linux-pm, devicetree, linux-kernel Em 11/1/19 12:27 PM, Krzysztof Kozlowski escreveu: > On Thu, Oct 31, 2019 at 03:41:33PM -0300, Matheus Castello wrote: >> For configuration of fuel gauge alert for a low level state of charge >> interrupt we add a function to config level threshold and a device tree >> binding property to set it in flatned device tree node. >> >> Now we can use "maxim,alert-low-soc-level" property with the values from >> 1% up to 32% to configure alert interrupt threshold. >> >> Signed-off-by: Matheus Castello <matheus@castello.eng.br> >> --- >> drivers/power/supply/max17040_battery.c | 88 +++++++++++++++++++++---- >> 1 file changed, 74 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c >> index 75459f76d02c..802575342c72 100644 >> --- a/drivers/power/supply/max17040_battery.c >> +++ b/drivers/power/supply/max17040_battery.c >> @@ -29,6 +29,9 @@ >> #define MAX17040_DELAY 1000 >> #define MAX17040_BATTERY_FULL 95 >> >> +#define MAX17040_ATHD_MASK 0xFFC0 >> +#define MAX17040_ATHD_DEFAULT_POWER_UP 4 >> + >> struct max17040_chip { >> struct i2c_client *client; >> struct delayed_work work; >> @@ -43,6 +46,8 @@ struct max17040_chip { >> int soc; >> /* State Of Charge */ >> int status; >> + /* Low alert threshold from 32% to 1% of the State of Charge */ >> + u32 low_soc_alert_threshold; >> }; >> >> static int max17040_get_property(struct power_supply *psy, >> @@ -99,6 +104,22 @@ static void max17040_reset(struct i2c_client *client) >> max17040_write_reg(client, MAX17040_CMD, 0x0054); >> } >> >> +static int max17040_set_low_soc_threshold_alert(struct i2c_client *client, >> + u32 level) >> +{ >> + int ret; >> + u16 data; >> + >> + level = 32 - level; >> + data = max17040_read_reg(client, MAX17040_RCOMP); >> + /* clear the alrt bit and set LSb 5 bits */ >> + data &= MAX17040_ATHD_MASK; >> + data |= level; >> + ret = max17040_write_reg(client, MAX17040_RCOMP, data); >> + >> + return ret; >> +} >> + >> static void max17040_get_vcell(struct i2c_client *client) >> { >> struct max17040_chip *chip = i2c_get_clientdata(client); >> @@ -115,7 +136,6 @@ static void max17040_get_soc(struct i2c_client *client) >> u16 soc; >> >> soc = max17040_read_reg(client, MAX17040_SOC); >> - >> chip->soc = (soc >> 8); >> } >> >> @@ -161,6 +181,24 @@ static void max17040_get_status(struct i2c_client *client) >> chip->status = POWER_SUPPLY_STATUS_FULL; >> } >> >> +static int max17040_get_of_data(struct max17040_chip *chip) >> +{ >> + struct device *dev = &chip->client->dev; >> + struct device_node *np = dev->of_node; >> + int ret = 0; >> + >> + if (of_property_read_u32(np, "maxim,alert-low-soc-level", >> + &chip->low_soc_alert_threshold)) { > > Please align the line break with line above. checkpatch --strict might > give you hints about this. > >> + chip->low_soc_alert_threshold = MAX17040_ATHD_DEFAULT_POWER_UP; >> + /* check if low_soc_alert_threshold is between 1% and 32% */ > > The comment looks misleading here, like it belongs to previous block. > Maybe put it inside else if {} block? > >> + } else if (chip->low_soc_alert_threshold <= 0 || >> + chip->low_soc_alert_threshold >= 33){ > > Missing space before {. > >> + ret = -EINVAL; >> + } >> + >> + return ret; >> +} >> + >> static void max17040_check_changes(struct i2c_client *client) >> { >> max17040_get_vcell(client); >> @@ -192,6 +230,10 @@ static irqreturn_t max17040_thread_handler(int id, void *dev) >> /* send uevent */ >> power_supply_changed(chip->battery); >> >> + /* reset alert bit */ >> + max17040_set_low_soc_threshold_alert(client, >> + chip->low_soc_alert_threshold); > > Unless the continuation exceeds 80 character limit, please align it with > previous line. > >> + >> return IRQ_HANDLED; >> } >> >> @@ -216,6 +258,7 @@ static int max17040_probe(struct i2c_client *client, >> struct i2c_adapter *adapter = client->adapter; >> struct power_supply_config psy_cfg = {}; >> struct max17040_chip *chip; >> + int ret; >> >> if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE)) >> return -EIO; >> @@ -226,6 +269,12 @@ static int max17040_probe(struct i2c_client *client, >> >> chip->client = client; >> chip->pdata = client->dev.platform_data; >> + ret = max17040_get_of_data(chip); >> + if (ret) { >> + dev_err(&client->dev, >> + "failed: low SOC alert OF data out of bounds\n"); >> + return ret; >> + } >> >> i2c_set_clientdata(client, chip); >> psy_cfg.drv_data = chip; >> @@ -242,20 +291,31 @@ static int max17040_probe(struct i2c_client *client, >> >> /* check interrupt */ >> if (client->irq) { >> - int ret; >> - unsigned int flags; >> - >> - dev_info(&client->dev, "IRQ: enabled\n"); >> - flags = IRQF_TRIGGER_FALLING | IRQF_ONESHOT; >> - ret = devm_request_threaded_irq(&client->dev, client->irq, NULL, >> - max17040_thread_handler, flags, >> - chip->battery->desc->name, >> - chip); >> - >> - if (ret) { >> - client->irq = 0; >> + if (of_device_is_compatible(client->dev.of_node, >> + "maxim,max77836-battery")) { > > Alignment. > >> + ret = max17040_set_low_soc_threshold_alert(client, >> + chip->low_soc_alert_threshold); > > Ditto. > >> + if (ret) { >> + dev_err(&client->dev, >> + "Failed to set low SOC alert: err %d\n", >> + ret); >> + return ret; >> + } >> + >> + dev_info(&client->dev, "IRQ: enabled\n"); >> + ret = devm_request_threaded_irq(&client->dev, >> + client->irq, NULL, max17040_thread_handler, >> + (client->flags | IRQF_ONESHOT), > > This looks unrelated. Befor ethis were IRQF_TRIGGER_FALLING | > IRQF_ONESHOT, now you use client->flags. There is no reason why this > commit should change > I am using client->flags here to not overwrite the flag passed in device tree. Let me know what you think about it: if I should leave it as in the previous commit, or should I modify the previous commit too. >> + chip->battery->desc->name, chip); > > This breaks alignment which was here before. > > Best regards, > Krzysztof > > Thanks for the review i will work on it. >> + >> + if (ret) { >> + client->irq = 0; >> + dev_warn(&client->dev, >> + "Failed to get IRQ err %d\n", ret); >> + } >> + } else { >> dev_warn(&client->dev, >> - "Failed to get IRQ err %d\n", ret); >> + "Device not compatible for IRQ"); >> } >> } >> >> -- >> 2.24.0.rc2 >> ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH v4 3/4] power: supply: max17040: Config alert SOC low level threshold from FDT 2019-11-01 16:52 ` Matheus Castello @ 2019-11-02 18:12 ` Matheus Castello 2019-11-04 11:04 ` Krzysztof Kozlowski 2019-11-04 10:59 ` Krzysztof Kozlowski 1 sibling, 1 reply; 82+ messages in thread From: Matheus Castello @ 2019-11-02 18:12 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: sre, robh+dt, mark.rutland, cw00.choi, b.zolnierkie, lee.jones, linux-pm, devicetree, linux-kernel Em 11/1/19 1:52 PM, Matheus Castello escreveu: > > > Em 11/1/19 12:27 PM, Krzysztof Kozlowski escreveu: >> On Thu, Oct 31, 2019 at 03:41:33PM -0300, Matheus Castello wrote: >>> For configuration of fuel gauge alert for a low level state of charge >>> interrupt we add a function to config level threshold and a device tree >>> binding property to set it in flatned device tree node. >>> >>> Now we can use "maxim,alert-low-soc-level" property with the values from >>> 1% up to 32% to configure alert interrupt threshold. >>> >>> Signed-off-by: Matheus Castello <matheus@castello.eng.br> >>> --- >>> drivers/power/supply/max17040_battery.c | 88 +++++++++++++++++++++---- >>> 1 file changed, 74 insertions(+), 14 deletions(-) >>> >>> diff --git a/drivers/power/supply/max17040_battery.c >>> b/drivers/power/supply/max17040_battery.c >>> index 75459f76d02c..802575342c72 100644 >>> --- a/drivers/power/supply/max17040_battery.c >>> +++ b/drivers/power/supply/max17040_battery.c >>> @@ -29,6 +29,9 @@ >>> #define MAX17040_DELAY 1000 >>> #define MAX17040_BATTERY_FULL 95 >>> >>> +#define MAX17040_ATHD_MASK 0xFFC0 >>> +#define MAX17040_ATHD_DEFAULT_POWER_UP 4 >>> + >>> struct max17040_chip { >>> struct i2c_client *client; >>> struct delayed_work work; >>> @@ -43,6 +46,8 @@ struct max17040_chip { >>> int soc; >>> /* State Of Charge */ >>> int status; >>> + /* Low alert threshold from 32% to 1% of the State of Charge */ >>> + u32 low_soc_alert_threshold; >>> }; >>> >>> static int max17040_get_property(struct power_supply *psy, >>> @@ -99,6 +104,22 @@ static void max17040_reset(struct i2c_client >>> *client) >>> max17040_write_reg(client, MAX17040_CMD, 0x0054); >>> } >>> >>> +static int max17040_set_low_soc_threshold_alert(struct i2c_client >>> *client, >>> + u32 level) >>> +{ >>> + int ret; >>> + u16 data; >>> + >>> + level = 32 - level; >>> + data = max17040_read_reg(client, MAX17040_RCOMP); >>> + /* clear the alrt bit and set LSb 5 bits */ >>> + data &= MAX17040_ATHD_MASK; >>> + data |= level; >>> + ret = max17040_write_reg(client, MAX17040_RCOMP, data); >>> + >>> + return ret; >>> +} >>> + >>> static void max17040_get_vcell(struct i2c_client *client) >>> { >>> struct max17040_chip *chip = i2c_get_clientdata(client); >>> @@ -115,7 +136,6 @@ static void max17040_get_soc(struct i2c_client >>> *client) >>> u16 soc; >>> >>> soc = max17040_read_reg(client, MAX17040_SOC); >>> - >>> chip->soc = (soc >> 8); >>> } >>> >>> @@ -161,6 +181,24 @@ static void max17040_get_status(struct >>> i2c_client *client) >>> chip->status = POWER_SUPPLY_STATUS_FULL; >>> } >>> >>> +static int max17040_get_of_data(struct max17040_chip *chip) >>> +{ >>> + struct device *dev = &chip->client->dev; >>> + struct device_node *np = dev->of_node; >>> + int ret = 0; >>> + >>> + if (of_property_read_u32(np, "maxim,alert-low-soc-level", >>> + &chip->low_soc_alert_threshold)) { >> >> Please align the line break with line above. checkpatch --strict might >> give you hints about this. >> >> + chip->low_soc_alert_threshold = >> MAX17040_ATHD_DEFAULT_POWER_UP; >>> + /* check if low_soc_alert_threshold is between 1% and 32% */ >> >> The comment looks misleading here, like it belongs to previous block. >> Maybe put it inside else if {} block? >> >>> + } else if (chip->low_soc_alert_threshold <= 0 || >>> + chip->low_soc_alert_threshold >= 33){ >> >> Missing space before {. >> >>> + ret = -EINVAL; >>> + } >>> + >>> + return ret; >>> +} >>> + >>> static void max17040_check_changes(struct i2c_client *client) >>> { >>> max17040_get_vcell(client); >>> @@ -192,6 +230,10 @@ static irqreturn_t max17040_thread_handler(int >>> id, void *dev) >>> /* send uevent */ >>> power_supply_changed(chip->battery); >>> >>> + /* reset alert bit */ >>> + max17040_set_low_soc_threshold_alert(client, >>> + chip->low_soc_alert_threshold); >> >> Unless the continuation exceeds 80 character limit, please align it with >> previous line. >> >>> + >>> return IRQ_HANDLED; >>> } >>> >>> @@ -216,6 +258,7 @@ static int max17040_probe(struct i2c_client *client, >>> struct i2c_adapter *adapter = client->adapter; >>> struct power_supply_config psy_cfg = {}; >>> struct max17040_chip *chip; >>> + int ret; >>> >>> if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE)) >>> return -EIO; >>> @@ -226,6 +269,12 @@ static int max17040_probe(struct i2c_client >>> *client, >>> >>> chip->client = client; >>> chip->pdata = client->dev.platform_data; >>> + ret = max17040_get_of_data(chip); >>> + if (ret) { >>> + dev_err(&client->dev, >>> + "failed: low SOC alert OF data out of bounds\n"); >>> + return ret; >>> + } >>> >>> i2c_set_clientdata(client, chip); >>> psy_cfg.drv_data = chip; >>> @@ -242,20 +291,31 @@ static int max17040_probe(struct i2c_client >>> *client, >>> >>> /* check interrupt */ >>> if (client->irq) { >>> - int ret; >>> - unsigned int flags; >>> - >>> - dev_info(&client->dev, "IRQ: enabled\n"); >>> - flags = IRQF_TRIGGER_FALLING | IRQF_ONESHOT; >>> - ret = devm_request_threaded_irq(&client->dev, client->irq, >>> NULL, >>> - max17040_thread_handler, flags, >>> - chip->battery->desc->name, >>> - chip); >>> - >>> - if (ret) { >>> - client->irq = 0; >>> + if (of_device_is_compatible(client->dev.of_node, >>> + "maxim,max77836-battery")) { >> >> Alignment. >> >>> + ret = max17040_set_low_soc_threshold_alert(client, >>> + chip->low_soc_alert_threshold); >> >> Ditto. >> I am working to fix the alignments issues using the checkpath strict and I have a doubt. Here for example if I fix the check "Alignment should match open parenthesis" it will pass the 80 characters limit and will show me a warning. >>> + if (ret) { >>> + dev_err(&client->dev, >>> + "Failed to set low SOC alert: err %d\n", >>> + ret); >>> + return ret; >>> + } >>> + >>> + dev_info(&client->dev, "IRQ: enabled\n"); >>> + ret = devm_request_threaded_irq(&client->dev, >>> + client->irq, NULL, max17040_thread_handler, >>> + (client->flags | IRQF_ONESHOT), >> >> This looks unrelated. Befor ethis were IRQF_TRIGGER_FALLING | >> IRQF_ONESHOT, now you use client->flags. There is no reason why this >> commit should change > > > I am using client->flags here to not overwrite the flag passed in device > tree. Let me know what you think about it: if I should leave it as in > the previous commit, or should I modify the previous commit too. > >>> + chip->battery->desc->name, chip); >> >> This breaks alignment which was here before. >> The same here. How to proceed in this case? Best Regards, Matheus Castello >> Best regards, >> Krzysztof >> >> > > Thanks for the review i will work on it. > >>> + >>> + if (ret) { >>> + client->irq = 0; >>> + dev_warn(&client->dev, >>> + "Failed to get IRQ err %d\n", ret); >>> + } >>> + } else { >>> dev_warn(&client->dev, >>> - "Failed to get IRQ err %d\n", ret); >>> + "Device not compatible for IRQ"); >>> } >>> } >>> >>> -- >>> 2.24.0.rc2 >>> ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH v4 3/4] power: supply: max17040: Config alert SOC low level threshold from FDT 2019-11-02 18:12 ` Matheus Castello @ 2019-11-04 11:04 ` Krzysztof Kozlowski 0 siblings, 0 replies; 82+ messages in thread From: Krzysztof Kozlowski @ 2019-11-04 11:04 UTC (permalink / raw) To: Matheus Castello Cc: sre, robh+dt, mark.rutland, cw00.choi, b.zolnierkie, lee.jones, linux-pm, devicetree, linux-kernel On Sat, Nov 02, 2019 at 03:12:44PM -0300, Matheus Castello wrote: > > > Em 11/1/19 1:52 PM, Matheus Castello escreveu: > > > > > > Em 11/1/19 12:27 PM, Krzysztof Kozlowski escreveu: > > > On Thu, Oct 31, 2019 at 03:41:33PM -0300, Matheus Castello wrote: > > > > For configuration of fuel gauge alert for a low level state of charge > > > > interrupt we add a function to config level threshold and a device tree > > > > binding property to set it in flatned device tree node. > > > > > > > > Now we can use "maxim,alert-low-soc-level" property with the values from > > > > 1% up to 32% to configure alert interrupt threshold. > > > > > > > > Signed-off-by: Matheus Castello <matheus@castello.eng.br> > > > > --- > > > > drivers/power/supply/max17040_battery.c | 88 +++++++++++++++++++++---- > > > > 1 file changed, 74 insertions(+), 14 deletions(-) > > > > > > > > diff --git a/drivers/power/supply/max17040_battery.c > > > > b/drivers/power/supply/max17040_battery.c > > > > index 75459f76d02c..802575342c72 100644 > > > > --- a/drivers/power/supply/max17040_battery.c > > > > +++ b/drivers/power/supply/max17040_battery.c > > > > @@ -29,6 +29,9 @@ > > > > #define MAX17040_DELAY 1000 > > > > #define MAX17040_BATTERY_FULL 95 > > > > > > > > +#define MAX17040_ATHD_MASK 0xFFC0 > > > > +#define MAX17040_ATHD_DEFAULT_POWER_UP 4 > > > > + > > > > struct max17040_chip { > > > > struct i2c_client *client; > > > > struct delayed_work work; > > > > @@ -43,6 +46,8 @@ struct max17040_chip { > > > > int soc; > > > > /* State Of Charge */ > > > > int status; > > > > + /* Low alert threshold from 32% to 1% of the State of Charge */ > > > > + u32 low_soc_alert_threshold; > > > > }; > > > > > > > > static int max17040_get_property(struct power_supply *psy, > > > > @@ -99,6 +104,22 @@ static void max17040_reset(struct i2c_client > > > > *client) > > > > max17040_write_reg(client, MAX17040_CMD, 0x0054); > > > > } > > > > > > > > +static int max17040_set_low_soc_threshold_alert(struct > > > > i2c_client *client, > > > > + u32 level) > > > > +{ > > > > + int ret; > > > > + u16 data; > > > > + > > > > + level = 32 - level; > > > > + data = max17040_read_reg(client, MAX17040_RCOMP); > > > > + /* clear the alrt bit and set LSb 5 bits */ > > > > + data &= MAX17040_ATHD_MASK; > > > > + data |= level; > > > > + ret = max17040_write_reg(client, MAX17040_RCOMP, data); > > > > + > > > > + return ret; > > > > +} > > > > + > > > > static void max17040_get_vcell(struct i2c_client *client) > > > > { > > > > struct max17040_chip *chip = i2c_get_clientdata(client); > > > > @@ -115,7 +136,6 @@ static void max17040_get_soc(struct > > > > i2c_client *client) > > > > u16 soc; > > > > > > > > soc = max17040_read_reg(client, MAX17040_SOC); > > > > - > > > > chip->soc = (soc >> 8); > > > > } > > > > > > > > @@ -161,6 +181,24 @@ static void max17040_get_status(struct > > > > i2c_client *client) > > > > chip->status = POWER_SUPPLY_STATUS_FULL; > > > > } > > > > > > > > +static int max17040_get_of_data(struct max17040_chip *chip) > > > > +{ > > > > + struct device *dev = &chip->client->dev; > > > > + struct device_node *np = dev->of_node; > > > > + int ret = 0; > > > > + > > > > + if (of_property_read_u32(np, "maxim,alert-low-soc-level", > > > > + &chip->low_soc_alert_threshold)) { > > > > > > Please align the line break with line above. checkpatch --strict might > > > give you hints about this. > > > >> + chip->low_soc_alert_threshold = > > > MAX17040_ATHD_DEFAULT_POWER_UP; > > > > + /* check if low_soc_alert_threshold is between 1% and 32% */ > > > > > > The comment looks misleading here, like it belongs to previous block. > > > Maybe put it inside else if {} block? > > > > > > > + } else if (chip->low_soc_alert_threshold <= 0 || > > > > + chip->low_soc_alert_threshold >= 33){ > > > > > > Missing space before {. > > > > > > > + ret = -EINVAL; > > > > + } > > > > + > > > > + return ret; > > > > +} > > > > + > > > > static void max17040_check_changes(struct i2c_client *client) > > > > { > > > > max17040_get_vcell(client); > > > > @@ -192,6 +230,10 @@ static irqreturn_t > > > > max17040_thread_handler(int id, void *dev) > > > > /* send uevent */ > > > > power_supply_changed(chip->battery); > > > > > > > > + /* reset alert bit */ > > > > + max17040_set_low_soc_threshold_alert(client, > > > > + chip->low_soc_alert_threshold); > > > > > > Unless the continuation exceeds 80 character limit, please align it with > > > previous line. > > > > > > > + > > > > return IRQ_HANDLED; > > > > } > > > > > > > > @@ -216,6 +258,7 @@ static int max17040_probe(struct i2c_client *client, > > > > struct i2c_adapter *adapter = client->adapter; > > > > struct power_supply_config psy_cfg = {}; > > > > struct max17040_chip *chip; > > > > + int ret; > > > > > > > > if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE)) > > > > return -EIO; > > > > @@ -226,6 +269,12 @@ static int max17040_probe(struct i2c_client > > > > *client, > > > > > > > > chip->client = client; > > > > chip->pdata = client->dev.platform_data; > > > > + ret = max17040_get_of_data(chip); > > > > + if (ret) { > > > > + dev_err(&client->dev, > > > > + "failed: low SOC alert OF data out of bounds\n"); > > > > + return ret; > > > > + } > > > > > > > > i2c_set_clientdata(client, chip); > > > > psy_cfg.drv_data = chip; > > > > @@ -242,20 +291,31 @@ static int max17040_probe(struct > > > > i2c_client *client, > > > > > > > > /* check interrupt */ > > > > if (client->irq) { > > > > - int ret; > > > > - unsigned int flags; > > > > - > > > > - dev_info(&client->dev, "IRQ: enabled\n"); > > > > - flags = IRQF_TRIGGER_FALLING | IRQF_ONESHOT; > > > > - ret = devm_request_threaded_irq(&client->dev, > > > > client->irq, NULL, > > > > - max17040_thread_handler, flags, > > > > - chip->battery->desc->name, > > > > - chip); > > > > - > > > > - if (ret) { > > > > - client->irq = 0; > > > > + if (of_device_is_compatible(client->dev.of_node, > > > > + "maxim,max77836-battery")) { > > > > > > Alignment. > > > > > > > + ret = max17040_set_low_soc_threshold_alert(client, > > > > + chip->low_soc_alert_threshold); > > > > > > Ditto. > > > > > I am working to fix the alignments issues using the checkpath strict and I > have a doubt. Here for example if I fix the check "Alignment should match > open parenthesis" it will pass the 80 characters limit and will show me a > warning. Indeed which is a hint that the code readabiltiy is affected by long function name + two indentations + long variable name. You can split it to different function (covering the IRQ case) which will reduce one indentation. Alternatively do not align it but add few more tabs before chip->: ret = max17040_set_low_soc_threshold_alert(client, chip->low_soc_alert_threshold); Renaming low_soc_alert_threshold to something shorter can help as well (soc_alert seems enough descriptive). Best regards, Krzysztof ^ permalink raw reply [flat|nested] 82+ messages in thread
* Re: [PATCH v4 3/4] power: supply: max17040: Config alert SOC low level threshold from FDT 2019-11-01 16:52 ` Matheus Castello 2019-11-02 18:12 ` Matheus Castello @ 2019-11-04 10:59 ` Krzysztof Kozlowski 1 sibling, 0 replies; 82+ messages in thread From: Krzysztof Kozlowski @ 2019-11-04 10:59 UTC (permalink / raw) To: Matheus Castello Cc: sre, robh+dt, mark.rutland, cw00.choi, b.zolnierkie, lee.jones, linux-pm, devicetree, linux-kernel On Fri, Nov 01, 2019 at 01:52:13PM -0300, Matheus Castello wrote: > > > Em 11/1/19 12:27 PM, Krzysztof Kozlowski escreveu: > > On Thu, Oct 31, 2019 at 03:41:33PM -0300, Matheus Castello wrote: > > > For configuration of fuel gauge alert for a low level state of charge > > > interrupt we add a function to config level threshold and a device tree > > > binding property to set it in flatned device tree node. > > > > > > Now we can use "maxim,alert-low-soc-level" property with the values from > > > 1% up to 32% to configure alert interrupt threshold. > > > > > > Signed-off-by: Matheus Castello <matheus@castello.eng.br> > > > --- > > > drivers/power/supply/max17040_battery.c | 88 +++++++++++++++++++++---- > > > 1 file changed, 74 insertions(+), 14 deletions(-) > > > > > > diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c > > > index 75459f76d02c..802575342c72 100644 > > > --- a/drivers/power/supply/max17040_battery.c > > > +++ b/drivers/power/supply/max17040_battery.c > > > @@ -29,6 +29,9 @@ > > > #define MAX17040_DELAY 1000 > > > #define MAX17040_BATTERY_FULL 95 > > > > > > +#define MAX17040_ATHD_MASK 0xFFC0 > > > +#define MAX17040_ATHD_DEFAULT_POWER_UP 4 > > > + > > > struct max17040_chip { > > > struct i2c_client *client; > > > struct delayed_work work; > > > @@ -43,6 +46,8 @@ struct max17040_chip { > > > int soc; > > > /* State Of Charge */ > > > int status; > > > + /* Low alert threshold from 32% to 1% of the State of Charge */ > > > + u32 low_soc_alert_threshold; > > > }; > > > > > > static int max17040_get_property(struct power_supply *psy, > > > @@ -99,6 +104,22 @@ static void max17040_reset(struct i2c_client *client) > > > max17040_write_reg(client, MAX17040_CMD, 0x0054); > > > } > > > > > > +static int max17040_set_low_soc_threshold_alert(struct i2c_client *client, > > > + u32 level) > > > +{ > > > + int ret; > > > + u16 data; > > > + > > > + level = 32 - level; > > > + data = max17040_read_reg(client, MAX17040_RCOMP); > > > + /* clear the alrt bit and set LSb 5 bits */ > > > + data &= MAX17040_ATHD_MASK; > > > + data |= level; > > > + ret = max17040_write_reg(client, MAX17040_RCOMP, data); > > > + > > > + return ret; > > > +} > > > + > > > static void max17040_get_vcell(struct i2c_client *client) > > > { > > > struct max17040_chip *chip = i2c_get_clientdata(client); > > > @@ -115,7 +136,6 @@ static void max17040_get_soc(struct i2c_client *client) > > > u16 soc; > > > > > > soc = max17040_read_reg(client, MAX17040_SOC); > > > - > > > chip->soc = (soc >> 8); > > > } > > > > > > @@ -161,6 +181,24 @@ static void max17040_get_status(struct i2c_client *client) > > > chip->status = POWER_SUPPLY_STATUS_FULL; > > > } > > > > > > +static int max17040_get_of_data(struct max17040_chip *chip) > > > +{ > > > + struct device *dev = &chip->client->dev; > > > + struct device_node *np = dev->of_node; > > > + int ret = 0; > > > + > > > + if (of_property_read_u32(np, "maxim,alert-low-soc-level", > > > + &chip->low_soc_alert_threshold)) { > > > > Please align the line break with line above. checkpatch --strict might > > give you hints about this. > > >> + chip->low_soc_alert_threshold = MAX17040_ATHD_DEFAULT_POWER_UP; > > > + /* check if low_soc_alert_threshold is between 1% and 32% */ > > > > The comment looks misleading here, like it belongs to previous block. > > Maybe put it inside else if {} block? > > > > > + } else if (chip->low_soc_alert_threshold <= 0 || > > > + chip->low_soc_alert_threshold >= 33){ > > > > Missing space before {. > > > > > + ret = -EINVAL; > > > + } > > > + > > > + return ret; > > > +} > > > + > > > static void max17040_check_changes(struct i2c_client *client) > > > { > > > max17040_get_vcell(client); > > > @@ -192,6 +230,10 @@ static irqreturn_t max17040_thread_handler(int id, void *dev) > > > /* send uevent */ > > > power_supply_changed(chip->battery); > > > > > > + /* reset alert bit */ > > > + max17040_set_low_soc_threshold_alert(client, > > > + chip->low_soc_alert_threshold); > > > > Unless the continuation exceeds 80 character limit, please align it with > > previous line. > > > > > + > > > return IRQ_HANDLED; > > > } > > > > > > @@ -216,6 +258,7 @@ static int max17040_probe(struct i2c_client *client, > > > struct i2c_adapter *adapter = client->adapter; > > > struct power_supply_config psy_cfg = {}; > > > struct max17040_chip *chip; > > > + int ret; > > > > > > if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE)) > > > return -EIO; > > > @@ -226,6 +269,12 @@ static int max17040_probe(struct i2c_client *client, > > > > > > chip->client = client; > > > chip->pdata = client->dev.platform_data; > > > + ret = max17040_get_of_data(chip); > > > + if (ret) { > > > + dev_err(&client->dev, > > > + "failed: low SOC alert OF data out of bounds\n"); > > > + return ret; > > > + } > > > > > > i2c_set_clientdata(client, chip); > > > psy_cfg.drv_data = chip; > > > @@ -242,20 +291,31 @@ static int max17040_probe(struct i2c_client *client, > > > > > > /* check interrupt */ > > > if (client->irq) { > > > - int ret; > > > - unsigned int flags; > > > - > > > - dev_info(&client->dev, "IRQ: enabled\n"); > > > - flags = IRQF_TRIGGER_FALLING | IRQF_ONESHOT; > > > - ret = devm_request_threaded_irq(&client->dev, client->irq, NULL, > > > - max17040_thread_handler, flags, > > > - chip->battery->desc->name, > > > - chip); > > > - > > > - if (ret) { > > > - client->irq = 0; > > > + if (of_device_is_compatible(client->dev.of_node, > > > + "maxim,max77836-battery")) { > > > > Alignment. > > > > > + ret = max17040_set_low_soc_threshold_alert(client, > > > + chip->low_soc_alert_threshold); > > > > Ditto. > > > > > + if (ret) { > > > + dev_err(&client->dev, > > > + "Failed to set low SOC alert: err %d\n", > > > + ret); > > > + return ret; > > > + } > > > + > > > + dev_info(&client->dev, "IRQ: enabled\n"); > > > + ret = devm_request_threaded_irq(&client->dev, > > > + client->irq, NULL, max17040_thread_handler, > > > + (client->flags | IRQF_ONESHOT), > > > > This looks unrelated. Befor ethis were IRQF_TRIGGER_FALLING | > > IRQF_ONESHOT, now you use client->flags. There is no reason why this > > commit should change > > > I am using client->flags here to not overwrite the flag passed in device > tree. Let me know what you think about it: if I should leave it as in the > previous commit, or should I modify the previous commit too. I still do not get why this change is here and how is related to this commit. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 82+ messages in thread
* [PATCH v4 4/4] power: supply: max17040: Send uevent in SOC and status change 2019-10-31 18:41 ` [PATCH v4 0/4] power: supply: MAX17040: Add IRQ for low level and alert SOC changes Matheus Castello ` (2 preceding siblings ...) 2019-10-31 18:41 ` [PATCH v4 3/4] power: supply: max17040: Config alert SOC low level threshold from FDT Matheus Castello @ 2019-10-31 18:41 ` Matheus Castello 2019-11-01 15:30 ` Krzysztof Kozlowski 3 siblings, 1 reply; 82+ messages in thread From: Matheus Castello @ 2019-10-31 18:41 UTC (permalink / raw) To: sre, krzk, robh+dt Cc: mark.rutland, cw00.choi, b.zolnierkie, lee.jones, linux-pm, devicetree, linux-kernel, Matheus Castello Notify core through power_supply_changed() in case of changes in state of charge and power supply status. This is useful for user-space to efficiently update current battery level. Signed-off-by: Matheus Castello <matheus@castello.eng.br> --- drivers/power/supply/max17040_battery.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c index 802575342c72..0ed3d856e4f5 100644 --- a/drivers/power/supply/max17040_battery.c +++ b/drivers/power/supply/max17040_battery.c @@ -210,10 +210,19 @@ static void max17040_check_changes(struct i2c_client *client) static void max17040_work(struct work_struct *work) { struct max17040_chip *chip; + int last_soc, last_status; chip = container_of(work, struct max17040_chip, work.work); + + /* store SOC and status to check changes */ + last_soc = chip->soc; + last_status = chip->status; max17040_check_changes(chip->client); + /* check changes and send uevent */ + if (last_soc != chip->soc || last_status != chip->status) + power_supply_changed(chip->battery); + queue_delayed_work(system_power_efficient_wq, &chip->work, MAX17040_DELAY); } -- 2.24.0.rc2 ^ permalink raw reply related [flat|nested] 82+ messages in thread
* Re: [PATCH v4 4/4] power: supply: max17040: Send uevent in SOC and status change 2019-10-31 18:41 ` [PATCH v4 4/4] power: supply: max17040: Send uevent in SOC and status change Matheus Castello @ 2019-11-01 15:30 ` Krzysztof Kozlowski 0 siblings, 0 replies; 82+ messages in thread From: Krzysztof Kozlowski @ 2019-11-01 15:30 UTC (permalink / raw) To: Matheus Castello Cc: sre, robh+dt, mark.rutland, cw00.choi, b.zolnierkie, lee.jones, linux-pm, devicetree, linux-kernel On Thu, Oct 31, 2019 at 03:41:34PM -0300, Matheus Castello wrote: > Notify core through power_supply_changed() in case of changes in state > of charge and power supply status. This is useful for user-space to > efficiently update current battery level. > > Signed-off-by: Matheus Castello <matheus@castello.eng.br> > --- > drivers/power/supply/max17040_battery.c | 9 +++++++++ Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org> Best regards, Krzysztof ^ permalink raw reply [flat|nested] 82+ messages in thread
end of thread, other threads:[~2019-11-14 0:54 UTC | newest] Thread overview: 82+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-07-23 4:08 [PATCH 0/4] power: supply: MAX17040: Add IRQ for low level and alert SOC changes Matheus Castello 2018-07-23 4:08 ` [PATCH 1/4] power: supply: max17040: Add IRQ handler for low SOC alert Matheus Castello 2018-07-25 10:27 ` Krzysztof Kozlowski 2018-07-23 4:08 ` [PATCH 2/4] power: supply: max17040: Config alert SOC low level threshold from FDT Matheus Castello 2018-07-25 10:42 ` Krzysztof Kozlowski 2018-07-23 4:08 ` [PATCH 3/4] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold Matheus Castello 2018-07-25 10:45 ` Krzysztof Kozlowski 2018-07-23 4:08 ` [PATCH 4/4] power: supply: max17040: Send uevent in SOC changes Matheus Castello 2018-07-25 10:52 ` Krzysztof Kozlowski 2018-09-16 11:45 ` [PATCH 0/4] power: supply: MAX17040: Add IRQ for low level and alert " Sebastian Reichel 2018-09-17 11:32 ` Krzysztof Kozlowski 2018-09-18 3:45 ` Matheus Castello 2019-04-15 1:26 ` [PATCH v2 " Matheus Castello 2019-04-15 1:26 ` Matheus Castello 2019-04-15 1:26 ` [PATCH v2 1/4] power: supply: max17040: Add IRQ handler for low SOC alert Matheus Castello 2019-04-15 1:26 ` Matheus Castello 2019-04-15 7:10 ` Krzysztof Kozlowski 2019-04-15 7:10 ` Krzysztof Kozlowski 2019-04-19 18:12 ` Matheus Castello 2019-04-19 18:12 ` Matheus Castello 2019-04-15 1:26 ` [PATCH v2 2/4] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold Matheus Castello 2019-04-15 1:26 ` Matheus Castello 2019-04-15 7:13 ` Krzysztof Kozlowski 2019-04-15 7:13 ` Krzysztof Kozlowski 2019-04-29 22:13 ` Rob Herring 2019-04-29 22:13 ` Rob Herring 2019-05-26 23:13 ` Matheus Castello 2019-04-15 1:26 ` [PATCH v2 3/4] power: supply: max17040: Config alert SOC low level threshold from FDT Matheus Castello 2019-04-15 1:26 ` Matheus Castello 2019-04-15 7:27 ` Krzysztof Kozlowski 2019-04-15 7:27 ` Krzysztof Kozlowski 2019-04-15 1:26 ` [PATCH v2 4/4] power: supply: max17040: Send uevent in SOC changes Matheus Castello 2019-04-15 1:26 ` Matheus Castello 2019-04-15 7:30 ` Krzysztof Kozlowski 2019-04-15 7:30 ` Krzysztof Kozlowski 2019-05-27 2:22 ` [PATCH v3 0/5] power: supply: MAX17040: Add IRQ for low level and alert " Matheus Castello 2019-05-27 2:22 ` [PATCH v3 1/5] power: supply: max17040: Add IRQ handler for low SOC alert Matheus Castello 2019-05-29 14:26 ` Krzysztof Kozlowski 2019-05-27 2:22 ` [PATCH v3 2/5] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold Matheus Castello 2019-05-29 14:40 ` Krzysztof Kozlowski 2019-05-29 14:57 ` Krzysztof Kozlowski 2019-06-02 21:38 ` Matheus Castello 2019-06-05 12:04 ` Krzysztof Kozlowski 2019-05-27 2:22 ` [PATCH v3 3/5] power: supply: max17040: Config alert SOC low level threshold from FDT Matheus Castello 2019-05-29 14:46 ` Krzysztof Kozlowski 2019-06-02 22:26 ` Matheus Castello 2019-06-05 12:05 ` Krzysztof Kozlowski 2019-05-27 2:22 ` [PATCH v3 4/5] power: supply: max17040: Clear ALRT bit when the SOC are above threshold Matheus Castello 2019-05-29 14:54 ` Krzysztof Kozlowski 2019-05-27 2:22 ` [PATCH v3 5/5] power: supply: max17040: Send uevent in SOC and status change Matheus Castello 2019-05-29 15:00 ` Krzysztof Kozlowski 2019-10-31 18:41 ` [PATCH v4 0/4] power: supply: MAX17040: Add IRQ for low level and alert SOC changes Matheus Castello 2019-10-31 18:41 ` [PATCH v4 1/4] power: supply: max17040: Add IRQ handler for low SOC alert Matheus Castello 2019-11-01 15:08 ` Krzysztof Kozlowski 2019-10-31 18:41 ` [PATCH v4 2/4] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold Matheus Castello 2019-11-01 15:10 ` Krzysztof Kozlowski 2019-11-05 1:58 ` Rob Herring 2019-11-05 5:42 ` [PATCH v5 0/5] power: supply: MAX17040: Add IRQ for low level and alert SOC changes Matheus Castello 2019-11-05 5:42 ` [PATCH v5 1/5] power: supply: max17040: Add IRQ handler for low SOC alert Matheus Castello 2019-11-05 5:42 ` [PATCH v5 2/5] dt-bindings: power: supply: Max17040: Add DT bindings for max17040 fuel gauge Matheus Castello 2019-11-05 5:42 ` [PATCH v5 3/5] devicetree: mfd: max14577: Add reference to max14040_battery.txt descriptions Matheus Castello 2019-11-05 5:42 ` [PATCH v5 4/5] power: supply: max17040: Config alert SOC low level threshold from FDT Matheus Castello 2019-11-05 9:59 ` Krzysztof Kozlowski 2019-11-07 3:17 ` [PATCH v6 0/5] power: supply: MAX17040: Add IRQ for low level and alert SOC changes Matheus Castello 2019-11-07 3:17 ` [PATCH v6 1/5] power: supply: max17040: Add IRQ handler for low SOC alert Matheus Castello 2019-11-07 3:17 ` [PATCH v6 2/5] dt-bindings: power: supply: Max17040: Add DT bindings for max17040 fuel gauge Matheus Castello 2019-11-14 0:53 ` Rob Herring 2019-11-07 3:17 ` [PATCH v6 3/5] devicetree: mfd: max14577: Add reference to max14040_battery.txt descriptions Matheus Castello 2019-11-11 10:09 ` Lee Jones 2019-11-14 0:54 ` Rob Herring 2019-11-07 3:17 ` [PATCH v6 4/5] power: supply: max17040: Config alert SOC low level threshold from FDT Matheus Castello 2019-11-07 3:17 ` [PATCH v6 5/5] power: supply: max17040: Send uevent in SOC and status change Matheus Castello 2019-11-11 9:59 ` [PATCH v6 0/5] power: supply: MAX17040: Add IRQ for low level and alert SOC changes Lee Jones 2019-11-05 5:42 ` [PATCH v5 5/5] power: supply: max17040: Send uevent in SOC and status change Matheus Castello 2019-10-31 18:41 ` [PATCH v4 3/4] power: supply: max17040: Config alert SOC low level threshold from FDT Matheus Castello 2019-11-01 15:27 ` Krzysztof Kozlowski 2019-11-01 16:52 ` Matheus Castello 2019-11-02 18:12 ` Matheus Castello 2019-11-04 11:04 ` Krzysztof Kozlowski 2019-11-04 10:59 ` Krzysztof Kozlowski 2019-10-31 18:41 ` [PATCH v4 4/4] power: supply: max17040: Send uevent in SOC and status change Matheus Castello 2019-11-01 15:30 ` Krzysztof Kozlowski
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).