linux-kernel - Re: [PATCH 1/2] power: supply: Add support for RN5T618/RC5T619 charger and fuel gauge

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200826174817.vhus3j4i4t7u7jc4@earth.universe>
Date:   Wed, 26 Aug 2020 19:48:17 +0200
From:   Sebastian Reichel <sebastian.reichel@...labora.com>
To:     Andreas Kemnade <andreas@...nade.info>
Cc:     lee.jones@...aro.org, linux-kernel@...r.kernel.org,
        linux-pm@...r.kernel.org, b.galvani@...il.com, phh@....me,
        letux-kernel@...nphoenux.org
Subject: Re: [PATCH 1/2] power: supply: Add support for RN5T618/RC5T619
 charger and fuel gauge

Hi,

Driver looks mostly good.

On Sat, Aug 15, 2020 at 06:56:09PM +0200, Andreas Kemnade wrote:
> [...]
> +static int rn5t618_battery_current_now(struct rn5t618_power_info *info,
> +				       union power_supply_propval *val)
> +{
> +	u16 res;
> +	int ret;
> +
> +	ret = rn5t618_battery_read_doublereg(info, RN5T618_CC_AVEREG1, &res);
> +	if (ret)
> +		return ret;
> +
> +	val->intval = res;
> +	/* 2's complement */
> +	if (val->intval & (1 << 13))
> +		val->intval = val->intval - (1 << 14);
> +
> +	/* negate current to be positive when discharging */
> +	val->intval *= -1000;

mh, the sign is not documented (which should be fixed). At least
sbs-battery does it the other way around (negative current when
discharging, positive otherwise). Some drivers do not support
signed current and always report positive values (e.g. ACPI driver).

What did you use as reference for swapping the sign?

> +	return 0;
> +}
> [...]
> +static const struct power_supply_desc rn5t618_adp_desc = {
> +	.name                   = "rn5t618-adp",
> +	.type                   = POWER_SUPPLY_TYPE_MAINS,
> +	.properties             = rn5t618_usb_props,

wrong property array, works by accident since usb and adp property
lists are the same.

> +	.num_properties         = ARRAY_SIZE(rn5t618_adp_props),
> +	.get_property           = rn5t618_adp_get_property,
> +};
> [...]

-- Sebastian

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)