| 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
| ||
|
Date: Wed, 29 May 2024 18:41:36 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
cc: Sebastian Reichel <sre@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, Bjorn Andersson <andersson@...nel.org>,
Hans de Goede <hdegoede@...hat.com>,
Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Konrad Dybcio <konrad.dybcio@...aro.org>, linux-pm@...r.kernel.org,
devicetree@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>,
platform-driver-x86@...r.kernel.org, linux-usb@...r.kernel.org,
linux-arm-msm@...r.kernel.org, Nikita Travkin <nikita@...n.ru>
Subject: Re: [PATCH v4 4/6] power: supply: lenovo_yoga_c630_battery: add
Lenovo C630 driver
On Tue, 28 May 2024, Dmitry Baryshkov wrote:
> On the Lenovo Yoga C630 WOS laptop the EC provides access to the adapter
> and battery status. Add the driver to read power supply status on the
> laptop.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
> ---
> drivers/power/supply/Kconfig | 9 +
> drivers/power/supply/Makefile | 1 +
> drivers/power/supply/lenovo_yoga_c630_battery.c | 479 ++++++++++++++++++++++++
> 3 files changed, 489 insertions(+)
>
> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> index 3e31375491d5..55ab8e90747d 100644
> --- a/drivers/power/supply/Kconfig
> +++ b/drivers/power/supply/Kconfig
> @@ -167,6 +167,15 @@ config BATTERY_LEGO_EV3
> help
> Say Y here to enable support for the LEGO MINDSTORMS EV3 battery.
>
> +config BATTERY_LENOVO_YOGA_C630
> + tristate "Lenovo Yoga C630 battery"
> + depends on OF && EC_LENOVO_YOGA_C630
> + help
> + This driver enables battery support on the Lenovo Yoga C630 laptop.
> +
> + To compile the driver as a module, choose M here: the module will be
> + called lenovo_yoga_c630_battery.
> +
> config BATTERY_PMU
> tristate "Apple PMU battery"
> depends on PPC32 && ADB_PMU
> diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
> index 58b567278034..8ebbdcf92dac 100644
> --- a/drivers/power/supply/Makefile
> +++ b/drivers/power/supply/Makefile
> @@ -32,6 +32,7 @@ obj-$(CONFIG_BATTERY_DS2782) += ds2782_battery.o
> obj-$(CONFIG_BATTERY_GAUGE_LTC2941) += ltc2941-battery-gauge.o
> obj-$(CONFIG_BATTERY_GOLDFISH) += goldfish_battery.o
> obj-$(CONFIG_BATTERY_LEGO_EV3) += lego_ev3_battery.o
> +obj-$(CONFIG_BATTERY_LENOVO_YOGA_C630) += lenovo_yoga_c630_battery.o
> obj-$(CONFIG_BATTERY_PMU) += pmu_battery.o
> obj-$(CONFIG_BATTERY_QCOM_BATTMGR) += qcom_battmgr.o
> obj-$(CONFIG_BATTERY_OLPC) += olpc_battery.o
> diff --git a/drivers/power/supply/lenovo_yoga_c630_battery.c b/drivers/power/supply/lenovo_yoga_c630_battery.c
> new file mode 100644
> index 000000000000..76152ad38d46
> --- /dev/null
> +++ b/drivers/power/supply/lenovo_yoga_c630_battery.c
> @@ -0,0 +1,479 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2022-2024, Linaro Ltd
> + * Authors:
> + * Bjorn Andersson
> + * Dmitry Baryshkov
> + */
> +#include <linux/auxiliary_bus.h>
> +#include <linux/delay.h>
> +#include <linux/module.h>
> +#include <linux/platform_data/lenovo-yoga-c630.h>
> +#include <linux/power_supply.h>
> +
> +struct yoga_c630_psy {
> + struct yoga_c630_ec *ec;
> + struct device *dev;
> + struct device_node *of_node;
> + struct notifier_block nb;
> + struct mutex lock;
Missing a few includes, please check them here as well.
> + struct power_supply *adp_psy;
> + struct power_supply *bat_psy;
> +
> + unsigned long last_status_update;
> +
> + bool adapter_online;
> +
> + bool unit_mA;
> +
> + bool bat_present;
> + unsigned int bat_status;
> + unsigned int design_capacity;
> + unsigned int design_voltage;
> + unsigned int full_charge_capacity;
> +
> + unsigned int capacity_now;
> + unsigned int voltage_now;
> +
> + int current_now;
> + int rate_now;
> +};
> +
> +#define LENOVO_EC_CACHE_TIME (10 * HZ)
> +
> +#define LENOVO_EC_ADPT_STATUS 0xa3
> +#define LENOVO_EC_ADPT_PRESENT BIT(7)
Add include for BIT()
> +#define LENOVO_EC_BAT_ATTRIBUTES 0xc0
> +#define LENOVO_EC_BAT_ATTR_UNIT_IS_MA BIT(1)
> +#define LENOVO_EC_BAT_STATUS 0xc1
> +#define LENOVO_EC_BAT_REMAIN_CAPACITY 0xc2
> +#define LENOVO_EC_BAT_VOLTAGE 0xc6
> +#define LENOVO_EC_BAT_DESIGN_VOLTAGE 0xc8
> +#define LENOVO_EC_BAT_DESIGN_CAPACITY 0xca
> +#define LENOVO_EC_BAT_FULL_CAPACITY 0xcc
> +#define LENOVO_EC_BAT_CURRENT 0xd2
> +#define LENOVO_EC_BAT_FULL_FACTORY 0xd6
> +#define LENOVO_EC_BAT_PRESENT 0xda
> +#define LENOVO_EC_BAT_FULL_REGISTER 0xdb
> +#define LENOVO_EC_BAT_FULL_IS_FACTORY BIT(0)
> +
> +/* the mutex should already be locked */
> +static int yoga_c630_psy_update_bat_info(struct yoga_c630_psy *ecbat)
> +{
> + struct yoga_c630_ec *ec = ecbat->ec;
> + int val;
> +
> + val = yoga_c630_ec_read8(ec, LENOVO_EC_BAT_PRESENT);
> + if (val < 0)
> + return val;
> + ecbat->bat_present = !!(val & BIT(0));
Name the bit with a define.
> + if (!ecbat->bat_present)
> + return val;
> +
> + val = yoga_c630_ec_read8(ec, LENOVO_EC_BAT_ATTRIBUTES);
> + if (val < 0)
> + return val;
> + ecbat->unit_mA = val & LENOVO_EC_BAT_ATTR_UNIT_IS_MA;
> +
> + val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_DESIGN_CAPACITY);
> + if (val < 0)
> + return val;
> + ecbat->design_capacity = val * 1000;
Check linux/units.h if some WATT related one matches to that literal 1000.
> + msleep(50);
> +
> + val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_DESIGN_VOLTAGE);
> + if (val < 0)
> + return val;
> + ecbat->design_voltage = val;
> +
> + msleep(50);
> +
> + val = yoga_c630_ec_read8(ec, LENOVO_EC_BAT_FULL_REGISTER);
> + if (val < 0)
> + return val;
> + if (val & LENOVO_EC_BAT_FULL_IS_FACTORY)
> + val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_FULL_FACTORY);
> + else
> + val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_FULL_CAPACITY);
You could consider doing this instead of branching:
val = yoga_c630_ec_read16(ec, val & LENOVO_EC_BAT_FULL_IS_FACTORY ?
LENOVO_EC_BAT_FULL_FACTORY :
LENOVO_EC_BAT_FULL_CAPACITY);
> + if (val < 0)
> + return val;
> +
> + ecbat->full_charge_capacity = val * 1000;
Something from linux/units.h ?
> + if (!ecbat->unit_mA) {
> + ecbat->design_capacity *= 10;
> + ecbat->full_charge_capacity *= 10;
> + }
> +
> + return 0;
> +}
> +
> +/* the mutex should already be locked */
> +static int yoga_c630_psy_maybe_update_bat_status(struct yoga_c630_psy *ecbat)
> +{
> + struct yoga_c630_ec *ec = ecbat->ec;
> + int current_mA;
> + int val;
> +
> + if (time_before(jiffies, ecbat->last_status_update + LENOVO_EC_CACHE_TIME))
> + return 0;
> +
> + val = yoga_c630_ec_read8(ec, LENOVO_EC_BAT_STATUS);
> + if (val < 0)
> + return val;
> + ecbat->bat_status = val;
> +
> + msleep(50);
> +
> + val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_REMAIN_CAPACITY);
> + if (val < 0)
> + return val;
> + ecbat->capacity_now = val * 1000;
units.h ?
> + msleep(50);
> +
> + val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_VOLTAGE);
> + if (val < 0)
> + return val;
> + ecbat->voltage_now = val * 1000;
Ditto.
> + msleep(50);
> +
> + val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_CURRENT);
> + if (val < 0)
> + return val;
> + current_mA = sign_extend32(val, 15);
> + ecbat->current_now = current_mA * 1000;
> + ecbat->rate_now = current_mA * (ecbat->voltage_now / 1000);
Ditto.
> + msleep(50);
> +
> + if (!ecbat->unit_mA)
> + ecbat->capacity_now *= 10;
> +
> + ecbat->last_status_update = jiffies;
Add jiffies.h
> + return 0;
> +}
> +
> +static int yoga_c630_psy_update_adapter_status(struct yoga_c630_psy *ecbat)
> +{
> + struct yoga_c630_ec *ec = ecbat->ec;
> + int val;
> +
> + mutex_lock(&ecbat->lock);
This kind of functions coul use guard to take the mutex so unlock will be
handled for you by the cleanup automation.
> + val = yoga_c630_ec_read8(ec, LENOVO_EC_ADPT_STATUS);
> + if (val > 0)
> + ecbat->adapter_online = FIELD_GET(LENOVO_EC_ADPT_PRESENT, val);
> +
> + mutex_unlock(&ecbat->lock);
> +
> + return val;
> +}
> +
> +static bool yoga_c630_psy_is_charged(struct yoga_c630_psy *ecbat)
> +{
> + if (ecbat->bat_status != 0)
> + return false;
> +
> + if (ecbat->full_charge_capacity <= ecbat->capacity_now)
> + return true;
> +
> + if (ecbat->design_capacity <= ecbat->capacity_now)
> + return true;
> +
> + return false;
> +}
> +
> +static int yoga_c630_psy_bat_get_property(struct power_supply *psy,
> + enum power_supply_property psp,
> + union power_supply_propval *val)
> +{
> + struct yoga_c630_psy *ecbat = power_supply_get_drvdata(psy);
> + int rc = 0;
> +
> + if (!ecbat->bat_present &&
> + psp != POWER_SUPPLY_PROP_PRESENT)
Fits to one line.
> + return -ENODEV;
> +
> + mutex_lock(&ecbat->lock);
> + rc = yoga_c630_psy_maybe_update_bat_status(ecbat);
> + mutex_unlock(&ecbat->lock);
> +
> + if (rc)
Remove empty line in between since this is the error handling.
> + return rc;
> +
> + switch (psp) {
> + case POWER_SUPPLY_PROP_STATUS:
> + if (ecbat->bat_status & BIT(0))
Name bits with defines.
> + val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
> + else if (ecbat->bat_status & BIT(1))
Ditto.
> + val->intval = POWER_SUPPLY_STATUS_CHARGING;
> + else if (yoga_c630_psy_is_charged(ecbat))
> + val->intval = POWER_SUPPLY_STATUS_FULL;
> + else
> + val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
> + break;
> + case POWER_SUPPLY_PROP_PRESENT:
> + val->intval = ecbat->bat_present;
> + break;
> + case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
> + val->intval = ecbat->design_voltage;
> + break;
> + case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
> + case POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN:
> + val->intval = ecbat->design_capacity;
> + break;
> + case POWER_SUPPLY_PROP_CHARGE_FULL:
> + case POWER_SUPPLY_PROP_ENERGY_FULL:
> + val->intval = ecbat->full_charge_capacity;
> + break;
> + case POWER_SUPPLY_PROP_CHARGE_NOW:
> + case POWER_SUPPLY_PROP_ENERGY_NOW:
> + val->intval = ecbat->capacity_now;
> + break;
> + case POWER_SUPPLY_PROP_CURRENT_NOW:
> + val->intval = ecbat->current_now;
> + break;
> + case POWER_SUPPLY_PROP_POWER_NOW:
> + val->intval = ecbat->rate_now;
> + break;
> + case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> + val->intval = ecbat->voltage_now;
> + break;
> + case POWER_SUPPLY_PROP_TECHNOLOGY:
> + val->intval = POWER_SUPPLY_TECHNOLOGY_LION;
> + break;
> + case POWER_SUPPLY_PROP_MODEL_NAME:
> + val->strval = "PABAS0241231";
> + break;
> + case POWER_SUPPLY_PROP_MANUFACTURER:
> + val->strval = "Compal";
> + break;
> + default:
> + rc = -EINVAL;
> + break;
> + }
> +
> + return rc;
> +}
> +static int yoga_c630_psy_register_bat_psy(struct yoga_c630_psy *ecbat)
> +{
> + struct power_supply_config bat_cfg = {};
> +
> + bat_cfg.drv_data = ecbat;
> + bat_cfg.of_node = ecbat->of_node;
> + if (ecbat->unit_mA)
> + ecbat->bat_psy = power_supply_register_no_ws(ecbat->dev, &yoga_c630_psy_bat_psy_desc_mA, &bat_cfg);
> + else
> + ecbat->bat_psy = power_supply_register_no_ws(ecbat->dev, &yoga_c630_psy_bat_psy_desc_mWh, &bat_cfg);
Again, it might be easier to see what's different in here if the relevant
parameter just uses ?: instead of full blown if/else.
> + if (IS_ERR(ecbat->bat_psy)) {
> + dev_err(ecbat->dev, "failed to register battery supply\n");
> + return PTR_ERR(ecbat->bat_psy);
> + }
> +
> + return 0;
> +}
--
i.
Powered by blists - more mailing lists