| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <c5bb4962-10f7-0877-0c99-c2dad5bb53cf@castello.eng.br>
Date: Wed, 27 Nov 2019 22:06:47 -0300
From: Matheus Castello <matheus@...tello.eng.br>
To: Sebastian Reichel <sebastian.reichel@...labora.com>
Cc: krzk@...nel.org, robh+dt@...nel.org, mark.rutland@....com,
cw00.choi@...sung.com, b.zolnierkie@...sung.com,
lee.jones@...aro.org, linux-pm@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 4/5] power: supply: max17040: Config alert SOC low
level threshold from FDT
Hi Sebastian,
Em 11/26/19 11:52 AM, Sebastian Reichel escreveu:
> Hi,
>
> On Sun, Nov 17, 2019 at 11:13:34AM -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@...tello.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);
>> -
>
> unrelated change.
>
>> 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;
>> + }
>
> use device_property_read_u32(), which is not DT specific. Also
> code can be simplified a bit:
>
> chip->low_soc_alert = MAX17040_ATHD_DEFAULT_POWER_UP;
> device_property_read_u32(dev, "maxim,alert-low-soc-level", &chip->low_soc_alert);
> if (chip->low_soc_alert <= 0 || chip->low_soc_alert >= 33)
> return -EINVAL;
> return 0;
>
Thanks for the review, I will use this.
>> +
>> + 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");
>
> Something is odd here. Either this should be part of the first
> patch ("max17040: Add IRQ handler for low SOC alert"), or both
> device types support the IRQ and this check should be removed.
> > -- Sebastian
>
The first patch add the IRQ without the configuration of the low SoC
alert, using the default state of charge level. This patch is working
with registers to config the low state of charge level, so it was
proposed to just try to write registers in the models compatible with
that (maxim,max77836-battery).
Maybe join the first patch to this one, and let DT binding be the first
patch on the series so we can already test compatible here, let me know
what you think about it.
>> }
>> }
>>
>> --
>> 2.24.0.rc2
>>
Powered by blists - more mailing lists