| Message ID | 20190527022258.32748-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 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
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 >
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
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);
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(-) -- 2.20.1