| 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: <22du3s2n3pcyivw7ktpqcvyvady24qggiqouz5hqzoca2tzyqd@vdi5qbtdkrgj>
Date: Tue, 29 Apr 2025 01:02:21 +0200
From: Sebastian Reichel <sebastian.reichel@...labora.com>
To: Svyatoslav Ryhel <clamor95@...il.com>
Cc: Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
Thierry Reding <thierry.reding@...il.com>, Jonathan Hunter <jonathanh@...dia.com>,
Neil Armstrong <neil.armstrong@...aro.org>, Jonathan Cameron <Jonathan.Cameron@...wei.com>,
Heiko Stuebner <heiko@...ech.de>, Rafał Miłecki <rafal@...ecki.pl>,
Aradhya Bhatia <a-bhatia1@...com>, linux-pm@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-tegra@...r.kernel.org
Subject: Re: [PATCH v2 3/4] power/supply: Add driver for Pegatron Chagall
battery
Hi,
On Sun, Apr 13, 2025 at 02:10:32PM +0300, Svyatoslav Ryhel wrote:
> The Pegatron Chagall is an Android tablet utilizing a customized Cypress
> CG7153AM microcontroller (MCU) as its battery fuel gauge. It supports a
> single-cell battery and features a dual-color charging LED.
>
> Signed-off-by: Svyatoslav Ryhel <clamor95@...il.com>
> ---
This looks mostly good to me, but I have some comments.
> drivers/power/supply/Kconfig | 12 +
> drivers/power/supply/Makefile | 1 +
> drivers/power/supply/chagall-battery.c | 308 +++++++++++++++++++++++++
> 3 files changed, 321 insertions(+)
> create mode 100644 drivers/power/supply/chagall-battery.c
[...]
> +static void chagall_leds_status_update(struct chagall_battery_data *cg, int state)
> +{
> + switch (state) {
> + case POWER_SUPPLY_STATUS_FULL:
> + led_set_brightness(&cg->amber_led, LED_OFF);
> + led_set_brightness(&cg->white_led, LED_ON);
> + break;
> +
> + case POWER_SUPPLY_STATUS_CHARGING:
> + led_set_brightness(&cg->white_led, LED_OFF);
> + led_set_brightness(&cg->amber_led, LED_ON);
> + break;
> +
> + default:
> + led_set_brightness(&cg->amber_led, LED_OFF);
> + led_set_brightness(&cg->white_led, LED_OFF);
> + break;
> + }
> +}
Instead of doing this, you should setup LED triggers when
registering the LEDs. The white LED can use power-supply's full_trig
and the orange LED can use power-supply's charging_trig, which
should have the same effect.
> +static const enum power_supply_property chagall_battery_properties[] = {
> + POWER_SUPPLY_PROP_STATUS,
> + POWER_SUPPLY_PROP_PRESENT,
> + POWER_SUPPLY_PROP_VOLTAGE_NOW,
> + POWER_SUPPLY_PROP_VOLTAGE_MAX,
> + POWER_SUPPLY_PROP_CURRENT_NOW,
> + POWER_SUPPLY_PROP_CURRENT_MAX,
> + POWER_SUPPLY_PROP_CAPACITY,
> + POWER_SUPPLY_PROP_TEMP,
> + POWER_SUPPLY_PROP_CHARGE_FULL,
> + POWER_SUPPLY_PROP_CHARGE_NOW,
> +};
> +
> +static const unsigned int chagall_battery_prop_offs[] = {
> + [POWER_SUPPLY_PROP_TEMP] = CHAGALL_REG_BATTERY_TEMPERATURE,
> + [POWER_SUPPLY_PROP_VOLTAGE_NOW] = CHAGALL_REG_BATTERY_VOLTAGE,
> + [POWER_SUPPLY_PROP_CURRENT_NOW] = CHAGALL_REG_BATTERY_CURRENT,
> + [POWER_SUPPLY_PROP_CAPACITY] = CHAGALL_REG_BATTERY_CAPACITY,
> + [POWER_SUPPLY_PROP_CURRENT_MAX] = CHAGALL_REG_BATTERY_CHARGING_CURRENT,
> + [POWER_SUPPLY_PROP_VOLTAGE_MAX] = CHAGALL_REG_BATTERY_CHARGING_VOLTAGE,
> + [POWER_SUPPLY_PROP_STATUS] = CHAGALL_REG_BATTERY_STATUS,
> + [POWER_SUPPLY_PROP_CHARGE_NOW] = CHAGALL_REG_BATTERY_REMAIN_CAPACITY,
> + [POWER_SUPPLY_PROP_CHARGE_FULL] = CHAGALL_REG_BATTERY_FULL_CAPACITY,
> +};
Please use the same order for chagall_battery_prop_offs and
chagall_battery_properties. Makes it a lot easier to see
that all options have been covered.
> +static int chagall_battery_get_value(struct chagall_battery_data *cg,
> + enum power_supply_property psp, u32 *val)
> +{
> + if (psp >= ARRAY_SIZE(chagall_battery_prop_offs))
> + return -EINVAL;
> + if (!chagall_battery_prop_offs[psp])
> + return -EINVAL;
> +
> + /* Battery data is stored in 2 consecutive registers with little-endian */
> + return regmap_bulk_read(cg->regmap, chagall_battery_prop_offs[psp], val, 2);
> +}
> +
> +static int chagall_battery_get_property(struct power_supply *psy,
> + enum power_supply_property psp,
> + union power_supply_propval *val)
> +{
> + struct chagall_battery_data *cg = power_supply_get_drvdata(psy);
> + int ret;
> +
> + switch (psp) {
> + case POWER_SUPPLY_PROP_PRESENT:
> + val->intval = 1;
> + break;
> +
> + default:
> + ret = chagall_battery_get_value(cg, psp, &val->intval);
> + if (ret)
> + return ret;
> +
> + switch (psp) {
> + case POWER_SUPPLY_PROP_TEMP:
> + val->intval -= TEMP_CELSIUS_OFFSET;
> + break;
> +
> + case POWER_SUPPLY_PROP_VOLTAGE_MAX:
> + case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> + case POWER_SUPPLY_PROP_CURRENT_MAX:
> + case POWER_SUPPLY_PROP_CURRENT_NOW:
> + case POWER_SUPPLY_PROP_CHARGE_FULL:
> + case POWER_SUPPLY_PROP_CHARGE_NOW:
> + val->intval *= 1000;
> + break;
> +
> + case POWER_SUPPLY_PROP_STATUS:
> + if (val->intval & BATTERY_FULL_CHARGED)
> + val->intval = POWER_SUPPLY_STATUS_FULL;
> + else if (val->intval & BATTERY_FULL_DISCHARGED)
> + val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
Have you tested this path? POWER_SUPPLY_STATUS_NOT_CHARGING is
intended to be used when the battery is neither charging nor
discharging. Does BATTERY_FULL_DISCHARGED mean, that the battery
is fully depleted and not providing any energy at all? Or is this
some kind of "battery level is criticial, you should attach a
power-supply now or the system will be turn off by itself soon"?
> + else if (val->intval & BATTERY_DISCHARGING)
> + val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
> + else
> + val->intval = POWER_SUPPLY_STATUS_CHARGING;
> + break;
> +
> + default:
> + break;
> + }
> +
> + break;
> + }
> +
> + return 0;
> +}
> +
> +static void chagall_battery_poll_work(struct work_struct *work)
> +{
> + struct chagall_battery_data *cg =
> + container_of(work, struct chagall_battery_data, poll_work.work);
> + u32 state;
> + int ret;
> +
> + ret = chagall_battery_get_value(cg, POWER_SUPPLY_PROP_STATUS, &state);
> + if (ret)
> + return;
> +
> + if (state & BATTERY_FULL_CHARGED)
> + state = POWER_SUPPLY_STATUS_FULL;
> + else if (state & BATTERY_DISCHARGING)
> + state = POWER_SUPPLY_STATUS_DISCHARGING;
> + else
> + state = POWER_SUPPLY_STATUS_CHARGING;
This basically duplicates the logic from chagall_battery_get_property(),
so put the translation logic into a helper function and use it in
both places.
Greetings,
-- Sebastian
Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)
Powered by blists - more mailing lists