| Message ID | 20191031184134.30621-4-matheus@castello.eng.br (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | power: supply: MAX17040: Add IRQ for low level and alert SOC changes | expand |
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 >
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 >>
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 >>>
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
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
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"); } }
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(-) -- 2.24.0.rc2