Re: [PATCH 1/2] power: supply: Add support for RN5T618/RC5T619 charger and fuel gauge
Hi,
On Wed, 26 Aug 2020 23:59:50 +0200
Sebastian Reichel wrote:
> Hi,
>
> On Wed, Aug 26, 2020 at 08:28:34PM +0200, Andreas Kemnade wrote:
> > On Wed, 26 Aug 2020 19:48:17 +0200
> > Sebastian Reichel wrote:
> > > 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);
>
> Btw. I think sign_extend32() can be used here?
>
> > > > + /* 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?
> > >
> > Well, I have searched for documentation, found nothing and used the
> > bq27xxx driver as reference which I am used to from the GTA04/GTA02,
> > so things behave equal. That are the devices where a was most
> > intensively looking at those values.
> > I thought that there would be some unwritten rule about that.
>
> The mess is mostly due to lacking reviewing from my side
> (and possibly my predecessors). I just went through a dozen of
> drivers and it looks like most either do not support signed current
> (and use negative values as error code :() or use negative current
> for discharge. I could not find any other driver using negative
> numbers for charging. I think it's best to negative = discharge as
> official correct value and will send an update patch for the
> documentation later.
>
ok, I will remove that sign-change and keep/update the comment
so that you know that the driver does it the official way
and send a v2 with the other things you mentioned.
Regards,
Andreas
pgp5ljy2PRtrW.pgp
Description: OpenPGP digital signature
Re: [PATCH 1/2] power: supply: Add support for RN5T618/RC5T619 charger and fuel gauge
Hi,
On Wed, Aug 26, 2020 at 08:28:34PM +0200, Andreas Kemnade wrote:
> On Wed, 26 Aug 2020 19:48:17 +0200
> Sebastian Reichel wrote:
> > 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);
Btw. I think sign_extend32() can be used here?
> > > + /* 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?
> >
> Well, I have searched for documentation, found nothing and used the
> bq27xxx driver as reference which I am used to from the GTA04/GTA02,
> so things behave equal. That are the devices where a was most
> intensively looking at those values.
> I thought that there would be some unwritten rule about that.
The mess is mostly due to lacking reviewing from my side
(and possibly my predecessors). I just went through a dozen of
drivers and it looks like most either do not support signed current
(and use negative values as error code :() or use negative current
for discharge. I could not find any other driver using negative
numbers for charging. I think it's best to negative = discharge as
official correct value and will send an update patch for the
documentation later.
-- Sebastian
signature.asc
Description: PGP signature
Re: [PATCH 1/2] power: supply: Add support for RN5T618/RC5T619 charger and fuel gauge
On Wed, 26 Aug 2020 19:48:17 +0200
Sebastian Reichel wrote:
> 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?
>
Well, I have searched for documentation, found nothing and used the
bq27xxx driver as reference which I am used to from the GTA04/GTA02,
so things behave equal. That are the devices where a was most
intensively looking at those values.
I thought that there would be some unwritten rule about that.
Regards,
Andreas
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
signature.asc
Description: PGP signature
[PATCH 1/2] power: supply: Add support for RN5T618/RC5T619 charger and fuel gauge
Both chips have charger and a fuel gauge.
This adds basic support for displaying the state of the battery and the
input power, settings are not modified. There are some defaults set via
OTP.
Charging also starts after plugging USB.
Known issues of the fuel gauge: There are drivers in the wild which disable
the fuel gauge at shutdown. If a kernel is booted without fuel gauge
support, after such a driver has been used, the fuel gauge will stay off
and decalibrate.
If this driver is used after that, it might display wrong values for charge
level.
Signed-off-by: Andreas Kemnade
---
drivers/power/supply/Kconfig | 8 +
drivers/power/supply/Makefile| 1 +
drivers/power/supply/rn5t618_power.c | 565 +++
3 files changed, 574 insertions(+)
create mode 100644 drivers/power/supply/rn5t618_power.c
diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index 44d3c8512fb8..28cea178f6f1 100644
--- a/drivers/power/supply/Kconfig
+++ b/drivers/power/supply/Kconfig
@@ -739,4 +739,12 @@ config CHARGER_WILCO
information can be found in
Documentation/ABI/testing/sysfs-class-power-wilco
+config RN5T618_POWER
+ tristate "RN5T618 charger/fuel gauge support"
+ depends on MFD_RN5T618
+ help
+ Say Y here to have support for RN5T618 PMIC family fuel gauge and
charger
+ This driver can also be built as a module. If so, the module will be
+ called rn5t618_power.
+
endif # POWER_SUPPLY
diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
index b9644663e435..23866b6ccdae 100644
--- a/drivers/power/supply/Makefile
+++ b/drivers/power/supply/Makefile
@@ -95,3 +95,4 @@ obj-$(CONFIG_CHARGER_UCS1002) += ucs1002_power.o
obj-$(CONFIG_CHARGER_BD70528) += bd70528-charger.o
obj-$(CONFIG_CHARGER_BD99954) += bd99954-charger.o
obj-$(CONFIG_CHARGER_WILCO)+= wilco-charger.o
+obj-$(CONFIG_RN5T618_POWER)+= rn5t618_power.o
diff --git a/drivers/power/supply/rn5t618_power.c
b/drivers/power/supply/rn5t618_power.c
new file mode 100644
index ..f62bcb00f714
--- /dev/null
+++ b/drivers/power/supply/rn5t618_power.c
@@ -0,0 +1,565 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Power supply driver for the RICOH RN5T618 power management chip family
+ *
+ * Copyright (C) 2020 Andreas Kemnade
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define CHG_STATE_ADP_INPUT 0x40
+#define CHG_STATE_USB_INPUT 0x80
+#define CHG_STATE_MASK 0x1f
+#define CHG_STATE_CHG_OFF 0
+#define CHG_STATE_CHG_READY_VADP 1
+#define CHG_STATE_CHG_TRICKLE 2
+#define CHG_STATE_CHG_RAPID3
+#define CHG_STATE_CHG_COMPLETE 4
+#define CHG_STATE_SUSPEND 5
+#define CHG_STATE_VCHG_OVER_VOL6
+#define CHG_STATE_BAT_ERROR7
+#define CHG_STATE_NO_BAT 8
+#define CHG_STATE_BAT_OVER_VOL 9
+#define CHG_STATE_BAT_TEMP_ERR 10
+#define CHG_STATE_DIE_ERR 11
+#define CHG_STATE_DIE_SHUTDOWN 12
+#define CHG_STATE_NO_BAT2 13
+#define CHG_STATE_CHG_READY_VUSB 14
+
+#define FG_ENABLE 1
+
+struct rn5t618_power_info {
+ struct rn5t618 *rn5t618;
+ struct platform_device *pdev;
+ struct power_supply *battery;
+ struct power_supply *usb;
+ struct power_supply *adp;
+ int irq;
+};
+
+static enum power_supply_property rn5t618_usb_props[] = {
+ POWER_SUPPLY_PROP_STATUS,
+ POWER_SUPPLY_PROP_ONLINE,
+};
+
+static enum power_supply_property rn5t618_adp_props[] = {
+ POWER_SUPPLY_PROP_STATUS,
+ POWER_SUPPLY_PROP_ONLINE,
+};
+
+
+static enum power_supply_property rn5t618_battery_props[] = {
+ POWER_SUPPLY_PROP_STATUS,
+ POWER_SUPPLY_PROP_PRESENT,
+ POWER_SUPPLY_PROP_VOLTAGE_NOW,
+ POWER_SUPPLY_PROP_CURRENT_NOW,
+ POWER_SUPPLY_PROP_CAPACITY,
+ POWER_SUPPLY_PROP_TEMP,
+ POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW,
+ POWER_SUPPLY_PROP_TIME_TO_FULL_NOW,
+ POWER_SUPPLY_PROP_TECHNOLOGY,
+ POWER_SUPPLY_PROP_CHARGE_FULL,
+ POWER_SUPPLY_PROP_CHARGE_NOW,
+};
+
+static int rn5t618_battery_read_doublereg(struct rn5t618_power_info *info,
+ u8 reg, u16 *result)
+{
+ int ret, i;
+ u8 data[2];
+ u16 old, new;
+
+ old = 0;
+ /* Prevent races when registers are changing. */
+ for (i = 0; i < 3; i++) {
+ ret = regmap_bulk_read(info->rn5t618->regmap,
+ reg, data, sizeof(data));
+ if (ret)
+ return ret;
+
+ new = data[0] << 8;
+ new |= data[1];
+ if (new == old)
+ break;
+
+ old = new;
+ }
+
+ *result = new;
+
+ return 0;
+}
+
+static int rn5t618_decode_status(unsigned int status)
+{
+ switch (status & CHG_STATE_MASK) {
+ case CHG_STATE_CHG_OFF:
+ case
