| 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: <CAPVz0n0Qcu7NAsqiRRrUjZLhRhNp=rmtdM9XLnf2XTiTpDyJgg@mail.gmail.com>
Date: Tue, 29 Apr 2025 08:41:52 +0300
From: Svyatoslav Ryhel <clamor95@...il.com>
To: Sebastian Reichel <sebastian.reichel@...labora.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
вт, 29 квіт. 2025 р. о 02:02 Sebastian Reichel
<sebastian.reichel@...labora.com> пише:
>
> 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"?
>
This one is tricky. I have transferred this logic from downstream
kernel and couldn't trigger BATTERY_FULL_DISCHARGED ever. It might be
a brief state you describe as "battery level is criticial, you should
attach a power-supply now or the system will be turn off by itself
soon" but there is no such entry in POWER_SUPPLY_STATUS enum, so I
assumed that POWER_SUPPLY_STATUS_NOT_CHARGING co-responds to such
state. If this is not the case I will just remove this entry.
> > + 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
Powered by blists - more mailing lists