Introduce Embedded Controller driver for Acer A500

Acer Iconia Tab A500 is an Android tablet device, it has ENE KB930 Embedded Controller which provides battery-gauge, LED, GPIO and some other functions. The EC uses firmware that is specifically customized for Acer A500. This patch adds MFD driver for the Embedded Controller which allows to power-off / reboot the A500 device, it also provides a common register read/write API that will be used by the sub-devices.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/mfd/Kconfig              |  10 ++
 drivers/mfd/Makefile             |   1 +
 drivers/mfd/acer-ec-a500.c       | 196 +++++++++++++++++++++++++++++++
 include/linux/mfd/acer-ec-a500.h |  80 +++++++++++++
 4 files changed, 287 insertions(+)
 create mode 100644 drivers/mfd/acer-ec-a500.c
 create mode 100644 include/linux/mfd/acer-ec-a500.h

drivers/mfd/Kconfig +0 -0

index 33df0837ab41..9e5cf88a52d8 100644

drivers/mfd/Kconfig +7 -0

+	select MFD_CORE
+	help
+	  Support for Acer Iconia Tab A500 Embedded Controller.
+
+	  The controller itself is ENE KB930, it is running firmware
+	  customized for the specific needs of the Acer A500 hardware.
+
 menu "Multimedia Capabilities Port drivers"
 	depends on ARCH_SA1100
 

drivers/mfd/Makefile +1 -0

index a60e5f835283..6e3a6162ad94 100644
@@ -262,5 +262,6 @@ obj-$(CONFIG_MFD_ROHM_BD71828)	+= rohm-bd71828.o
 obj-$(CONFIG_MFD_ROHM_BD718XX)	+= rohm-bd718x7.o
 obj-$(CONFIG_MFD_STMFX) 	+= stmfx.o
 obj-$(CONFIG_MFD_KHADAS_MCU) 	+= khadas-mcu.o
+obj-$(CONFIG_MFD_ACER_A500_EC)	+= acer-ec-a500.o
 
 obj-$(CONFIG_SGI_MFD_IOC3)	+= ioc3.o

drivers/mfd/acer-ec-a500.c new file +17 -0

new file mode 100644
index 000000000000..f75bb60ab408
@@ -0,0 +1,196 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * MFD driver for Acer Iconia Tab A500 Embedded Controller.
+ *
+ * Copyright 2020 GRATE-driver project.
+ *
+ * Based on downstream driver from Acer Inc.
+ */
+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/irqflags.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/reboot.h>
+
+#include <linux/mfd/acer-ec-a500.h>

drivers/mfd/acer-ec-a500.c new file +66 -0

+{
+	struct i2c_client *client = ec_chip->client;
+	unsigned int retries = 5;
+	s32 ret = 0;
+
+	while (retries-- > 0) {
+		ret = i2c_smbus_read_word_data(client, cmd_desc->cmd);
+		if (ret >= 0)
+			break;
+
+		a500_ec_delay(A500_EC_I2C_ERR_TIMEOUT);
+	}
+
+	if (ret < 0) {
+		dev_err(&client->dev, "i2c read command 0x%x failed: %d\n",
+			cmd_desc->cmd, ret);
+		return ret;
+	}
+
+	a500_ec_delay(cmd_desc->post_delay);
+
+	return le16_to_cpu(ret);
+}
+EXPORT_SYMBOL_GPL(a500_ec_read_locked);
+
+int a500_ec_write_locked(struct a500_ec *ec_chip,
+			 const struct a500_ec_cmd *cmd_desc, u16 value)
+{
+	struct i2c_client *client = ec_chip->client;
+	unsigned int retries = 5;
+	s32 ret = 0;
+
+	while (retries-- > 0) {
+		ret = i2c_smbus_write_word_data(client, cmd_desc->cmd,
+						le16_to_cpu(value));
+		if (ret >= 0)
+			break;
+
+		a500_ec_delay(A500_EC_I2C_ERR_TIMEOUT);
+	}
+
+	if (ret < 0) {
+		dev_err(&client->dev, "i2c write command 0x%x failed: %d\n",
+			cmd_desc->cmd, ret);
+		return ret;
+	}
+
+	a500_ec_delay(cmd_desc->post_delay);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(a500_ec_write_locked);
+
+static void a500_ec_poweroff(void)
+{
+	struct a500_ec *ec_chip = a500_ec_scratch;
+
+	a500_ec_write_locked(ec_chip, SHUTDOWN, 0);
+}
+
+static int a500_ec_restart_notify(struct notifier_block *this,
+				  unsigned long reboot_mode, void *data)
+{
+	struct a500_ec *ec_chip = a500_ec_scratch;
+
+	if (reboot_mode == REBOOT_WARM)

drivers/mfd/acer-ec-a500.c new file +71 -0

+};
+
+static const struct mfd_cell a500_ec_cells[] = {
+	{ .name = "acer-a500-iconia-battery", },
+	{ .name = "acer-a500-iconia-leds", },
+};
+
+static int a500_ec_probe(struct i2c_client *client,
+			 const struct i2c_device_id *id)
+{
+	struct a500_ec *ec_chip;
+	int err;
+
+	ec_chip = devm_kzalloc(&client->dev, sizeof(*ec_chip), GFP_KERNEL);
+	if (!ec_chip)
+		return -ENOMEM;
+
+	ec_chip->client = client;
+	mutex_init(&ec_chip->lock);
+
+	i2c_set_clientdata(client, ec_chip);
+
+	/* register battery and LED sub-devices */
+	err = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_AUTO,
+				   a500_ec_cells, ARRAY_SIZE(a500_ec_cells),
+				   NULL, 0, NULL);
+	if (err) {
+		dev_err(&client->dev, "failed to add sub-devices: %d\n", err);
+		return err;
+	}
+
+	/* set up power management functions */
+	if (of_device_is_system_power_controller(client->dev.of_node)) {
+		a500_ec_scratch = ec_chip;
+
+		err = register_restart_handler(&a500_ec_restart_handler);
+		if (err) {
+			dev_err(&client->dev,
+				"unable to register restart handler: %d\n",
+				err);
+			return err;
+		}
+
+		if (!pm_power_off)
+			pm_power_off = a500_ec_poweroff;
+	}
+
+	return 0;
+}
+
+static int a500_ec_remove(struct i2c_client *client)
+{
+	if (of_device_is_system_power_controller(client->dev.of_node)) {
+		if (pm_power_off == a500_ec_poweroff)
+			pm_power_off = NULL;
+
+		unregister_restart_handler(&a500_ec_restart_handler);
+	}
+
+	return 0;
+}
+
+static const struct of_device_id a500_ec_match[] = {
+	{ .compatible = "acer,a500-iconia-ec" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, a500_ec_match);
+
+static struct i2c_driver a500_ec_driver = {
+	.driver = {
+		.name = "acer-a500-embedded-controller",

include/linux/mfd/acer-ec-a500.h new file +14 -0

new file mode 100644
index 000000000000..08bc681e6525
@@ -0,0 +1,80 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * API for Embedded Controller of Acer Iconia Tab A500.
+ *
+ * Copyright 2020 GRATE-driver project.
+ */
+
+#ifndef __LINUX_MFD_ACER_A500_EC_H
+#define __LINUX_MFD_ACER_A500_EC_H
+
+#include <linux/i2c.h>
+#include <linux/mutex.h>
+#include <linux/types.h>
+

include/linux/mfd/acer-ec-a500.h new file +56 -0

+static const __maybe_unused struct a500_ec_cmd *NAME = &A500_EC_##NAME
+
+struct a500_ec {
+	struct i2c_client *client;
+
+	/*
+	 * Some EC commands shall be executed sequentially and some commands
+	 * shall not be executed instantly after the other commands. Hence the
+	 * locking is needed in order to protect from conflicting accesses to
+	 * the EC.
+	 */
+	struct mutex lock;
+};
+
+int a500_ec_read_locked(struct a500_ec *ec_chip,
+			const struct a500_ec_cmd *cmd_desc);
+
+int a500_ec_write_locked(struct a500_ec *ec_chip,
+			 const struct a500_ec_cmd *cmd_desc, u16 value);
+
+static inline void a500_ec_lock(struct a500_ec *ec_chip)
+{
+	mutex_lock(&ec_chip->lock);
+}
+
+static inline void a500_ec_unlock(struct a500_ec *ec_chip)
+{
+	mutex_unlock(&ec_chip->lock);
+}
+
+static inline int a500_ec_read(struct a500_ec *ec_chip,
+			       const struct a500_ec_cmd *cmd_desc)
+{
+	s32 ret;
+
+	a500_ec_lock(ec_chip);
+	ret = a500_ec_read_locked(ec_chip, cmd_desc);
+	a500_ec_unlock(ec_chip);
+
+	return ret;
+}
+
+static inline int a500_ec_write(struct a500_ec *ec_chip,
+				const struct a500_ec_cmd *cmd_desc,
+				u16 value)
+{
+	s32 ret;
+
+	a500_ec_lock(ec_chip);
+	ret = a500_ec_write_locked(ec_chip, cmd_desc, value);
+	a500_ec_unlock(ec_chip);
+
+	return ret;
+}
+
+#endif /* __LINUX_MFD_ACER_A500_EC_H */

Signature · 1 line

2.27.0

Hello,

drivers/mfd/Kconfig config MFD_KHADAS_MCU · line 2067

Quoted 3 lines

drivers/mfd/Kconfig +3 -0

+config MFD_ACER_A500_EC
+	tristate "Embedded Controller driver for Acer Iconia Tab A500"
+	depends on (I2C_TEGRA && ARCH_TEGRA_2x_SOC) || COMPILE_TEST

This seems to also depend on I2C and OF. Perhaps I2C_TEGRA and ARCH_TEGRA_2x_SOC imply that, but it could lead to build failures with COMPILE_TEST=y.

Quoted 1 line

+static struct a500_ec *a500_ec_scratch;

If this is only used for power_off, please rename it. I've been told to do so in my driver: https://lore.kernel.org/lkml/20200519104933.GX271301@dell/

Review

Quoted 2 lines

(quoted diff) +2 -0

+int a500_ec_read_locked(struct a500_ec *ec_chip,
+			const struct a500_ec_cmd *cmd_desc)

Any reason you're exporting these to the cell drivers instead of using regmap?

I think regmap would also help you with the locking if you set .lock() and .unlock() callbacks in regmap_config.

drivers/mfd/acer-ec-a500.c line 115

Quoted 3 lines

drivers/mfd/acer-ec-a500.c new file +3 -0

+static struct notifier_block a500_ec_restart_handler = {
+	.notifier_call = a500_ec_restart_notify,
+	.priority = 200,

What would happend if you didn't set priority explicitly?

Quoted 1 line

+MODULE_LICENSE("GPL v2");

MODULE_LICENSE("GPL");

Your SPDX tag suggests newer versions of GPL than v2 are okay.

include/linux/mfd/acer-ec-a500.h line 24

Quoted 5 lines

include/linux/mfd/acer-ec-a500.h new file +5 -0

+#define A500_EC_COMMAND(NAME, CMD, DELAY_MS)				\
+static const struct a500_ec_cmd A500_EC_##NAME = {			\
+	.cmd = CMD,							\
+	.post_delay = DELAY_MS,						\
+};									\

I think that the mfd driver should decide about the delay, not the cell drivers.

23.08.2020 21:16, Lubomir Rintel пишет:

Quoted 1 line

Hello,

...

Quoted 7 lines

drivers/mfd/Kconfig config MFD_KHADAS_MCU · line 2067

Quoted 3 lines

drivers/mfd/Kconfig +3 -0

+config MFD_ACER_A500_EC
+	tristate "Embedded Controller driver for Acer Iconia Tab A500"
+	depends on (I2C_TEGRA && ARCH_TEGRA_2x_SOC) || COMPILE_TEST

This seems to also depend on I2C and OF. Perhaps I2C_TEGRA and ARCH_TEGRA_2x_SOC imply that, but it could lead to build failures with COMPILE_TEST=y.

Hello, Lubomir! You're right about the I2C because it could be compiled as a loadable module, good catch! The OF seems should fine as-is.

...

Quoted 2 lines

Quoted 1 line

+static struct a500_ec *a500_ec_scratch;

Quoted 2 lines

If this is only used for power_off, please rename it. I've been told to do so in my driver: https://lore.kernel.org/lkml/20200519104933.GX271301@dell/

I don't mind to rename the variable, but not sure whether it will be a worthwhile change since _scratch is also a common naming scheme among MFD drivers. Please see max77620_scratch for example, which I added about a year ago.

Quoted 10 lines

...

Quoted 4 lines

Quoted 1 line

+static struct a500_ec *a500_ec_scratch;

If this is only used for power_off, please rename it. I've been told to do so in my driver: https://lore.kernel.org/lkml/20200519104933.GX271301@dell/

I don't mind to rename the variable, but not sure whether it will be a worthwhile change since _scratch is also a common naming scheme among MFD drivers. Please see max77620_scratch for example, which I added about a year ago.

If something is used once, it does not make it 'common'.

Not sure how this slipped my notice before, but I don't like it.

Ensure any global struct used for power_off only includes items required for this purpose. It's unfortunate this API requires a global variable at all.

24.08.2020 10:33, Lee Jones пишет:

Quoted 11 lines

Quoted 10 lines

...

Quoted 4 lines

Quoted 1 line

+static struct a500_ec *a500_ec_scratch;

If this is only used for power_off, please rename it. I've been told to do so in my driver: https://lore.kernel.org/lkml/20200519104933.GX271301@dell/

I don't mind to rename the variable, but not sure whether it will be a worthwhile change since _scratch is also a common naming scheme among MFD drivers. Please see max77620_scratch for example, which I added about a year ago.

Quoted 8 lines

If something is used once, it does not make it 'common'.

Not sure how this slipped my notice before, but I don't like it.

Ensure any global struct used for power_off only includes items required for this purpose. It's unfortunate this API requires a global variable at all.

Okay! I'll change it in the v2, thanks!

Thierry Reding was working on the shutdown API which should replace the global variables, unfortunately he doesn't have enough time to finish that work yet.

This patch adds battery gauge driver for Acer Iconia Tab A500 device. The battery gauge function is provided via the Embedded Controller, which is found on the Acer A500.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/power/supply/Kconfig             |   6 +
 drivers/power/supply/Makefile            |   1 +
 drivers/power/supply/acer_a500_battery.c | 369 +++++++++++++++++++++++
 3 files changed, 376 insertions(+)
 create mode 100644 drivers/power/supply/acer_a500_battery.c

drivers/power/supply/Kconfig +6 -0

index faf2830aa152..dff5e5a7383f 100644
@@ -752,4 +752,10 @@ config CHARGER_WILCO
 	  information can be found in
 	  Documentation/ABI/testing/sysfs-class-power-wilco
 
+config BATTERY_ACER_A500
+	tristate "Acer Iconia Tab A500 battery driver"
+	depends on MFD_ACER_A500_EC
+	help
+	  Say Y to include support for Acer Iconia Tab A500 battery fuel gauge.
+
 endif # POWER_SUPPLY

drivers/power/supply/Makefile +1 -0

index b3c694a65114..08a5b49e2936 100644
@@ -96,3 +96,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_BATTERY_ACER_A500)	+= acer_a500_battery.o

drivers/power/supply/acer_a500_battery.c new file +164 -0

new file mode 100644
index 000000000000..933101e1261e
@@ -0,0 +1,369 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Battery driver for Acer Iconia Tab A500.
+ *
+ * Copyright 2020 GRATE-driver project.
+ *
+ * Based on downstream driver from Acer Inc.
+ * Based on NVIDIA Gas Gauge driver for SBS Compliant Batteries.
+ *
+ * Copyright (c) 2010, NVIDIA Corporation.
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/power_supply.h>
+#include <linux/sched.h>
+#include <linux/slab.h>
+#include <linux/workqueue.h>
+
+#include <linux/mfd/acer-ec-a500.h>
+
+#define BATTERY_SERIAL_LEN	22
+
+enum {
+	REG_CAPACITY,
+	REG_VOLTAGE,
+	REG_CURRENT,
+	REG_DESIGN_CAPACITY,
+	REG_TEMPERATURE,
+	REG_SERIAL_NUMBER,
+};
+
+#define EC_DATA(_cmd, _delay, _psp) {		\
+	.psp = POWER_SUPPLY_PROP_ ## _psp,	\
+	.cmd = {				\
+		.cmd = _cmd,			\
+		.post_delay = _delay		\
+	},					\
+}
+
+static const struct chip_data {
+	enum power_supply_property psp;
+	struct a500_ec_cmd cmd;
+} ec_data[] = {
+	[REG_CAPACITY]		= EC_DATA(0x00,  0, CAPACITY),
+	[REG_VOLTAGE]		= EC_DATA(0x01,  0, VOLTAGE_NOW),
+	[REG_CURRENT]		= EC_DATA(0x03, 10, CURRENT_NOW),
+	[REG_DESIGN_CAPACITY]	= EC_DATA(0x08,  0, CHARGE_FULL_DESIGN),
+	[REG_TEMPERATURE]	= EC_DATA(0x0a,  0, TEMP),
+	[REG_SERIAL_NUMBER]	= EC_DATA(0x6a,  0, SERIAL_NUMBER),
+};
+
+static const enum power_supply_property a500_battery_properties[] = {
+	POWER_SUPPLY_PROP_CAPACITY,
+	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
+	POWER_SUPPLY_PROP_CURRENT_NOW,
+	POWER_SUPPLY_PROP_PRESENT,
+	POWER_SUPPLY_PROP_SERIAL_NUMBER,
+	POWER_SUPPLY_PROP_STATUS,
+	POWER_SUPPLY_PROP_TECHNOLOGY,
+	POWER_SUPPLY_PROP_TEMP,
+	POWER_SUPPLY_PROP_VOLTAGE_NOW,
+};
+
+struct a500_battery {
+	struct delayed_work poll_work;
+	struct power_supply *psy;
+	struct a500_ec *ec_chip;
+	unsigned int capacity;
+	char serial[BATTERY_SERIAL_LEN + 1];
+};
+
+static int a500_battery_get_presence(struct a500_battery *bat,
+				     union power_supply_propval *val)
+{
+	s32 ret;
+
+	/*
+	 * DESIGN_CAPACITY register always returns a non-zero value if
+	 * battery is connected and zero if disconnected.
+	 */
+	ret = a500_ec_read(bat->ec_chip, &ec_data[REG_DESIGN_CAPACITY].cmd);
+	if (ret <= 0)
+		val->intval = 0;
+	else
+		val->intval = 1;
+
+	return 0;
+}
+
+static bool a500_battery_update_capacity(struct a500_battery *bat)
+{
+	unsigned int capacity;
+	s32 ret;
+
+	ret = a500_ec_read(bat->ec_chip, &ec_data[REG_CAPACITY].cmd);
+	if (ret < 0)
+		return false;
+
+	/* capacity can be >100% even if max value is 100% */
+	capacity = min(ret, 100);
+
+	if (bat->capacity != capacity) {
+		bat->capacity = capacity;
+		return true;
+	}
+
+	return false;
+}
+
+static void a500_battery_get_status(struct a500_battery *bat,
+				    union power_supply_propval *val)
+{
+	if (bat->capacity < 100) {
+		if (power_supply_am_i_supplied(bat->psy))
+			val->intval = POWER_SUPPLY_STATUS_CHARGING;
+		else
+			val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
+	} else {
+		val->intval = POWER_SUPPLY_STATUS_FULL;
+	}
+}
+
+static void a500_battery_unit_adjustment(struct device *dev,
+					 enum power_supply_property psp,
+					 union power_supply_propval *val)
+{
+	const unsigned int base_unit_conversion = 1000;
+	const unsigned int temp_kelvin_to_celsius = 2731;
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
+	case POWER_SUPPLY_PROP_CURRENT_NOW:
+	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+		val->intval *= base_unit_conversion;
+		break;
+
+	case POWER_SUPPLY_PROP_TEMP:
+		val->intval -= temp_kelvin_to_celsius;
+		break;
+
+	default:
+		dev_dbg(dev,
+			"%s: no need for unit conversion %d\n", __func__, psp);
+	}
+}
+
+static int a500_battery_get_serial_number(struct a500_battery *bat,
+					  union power_supply_propval *val)
+{
+	unsigned int i;
+	s32 ret = 0;
+
+	if (bat->serial[0])
+		goto done;
+
+	a500_ec_lock(bat->ec_chip);
+	for (i = 0; i < BATTERY_SERIAL_LEN / 2; i++) {
+		ret = a500_ec_read_locked(bat->ec_chip,
+					  &ec_data[REG_SERIAL_NUMBER].cmd);
+		if (ret < 0) {
+			bat->serial[0] = '\0';
+			break;
+		}

drivers/power/supply/acer_a500_battery.c new file +133 -0

+static int a500_battery_get_ec_data_index(struct device *dev,
+					  enum power_supply_property psp)
+{
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(ec_data); i++)
+		if (psp == ec_data[i].psp)
+			return i;
+
+	dev_dbg(dev, "%s: invalid property %u\n", __func__, psp);
+
+	return -EINVAL;
+}
+
+static int a500_battery_get_simple_property(struct a500_battery *bat,
+					    union power_supply_propval *val,
+					    unsigned int ec_idx)
+{
+	s32 ret;
+
+	ret = a500_ec_read(bat->ec_chip, &ec_data[ec_idx].cmd);
+	if (ret < 0)
+		return ret;
+
+	val->intval = (u16)ret;
+
+	return 0;
+}
+
+static int a500_battery_get_property(struct power_supply *psy,
+				     enum power_supply_property psp,
+				     union power_supply_propval *val)
+{
+	struct a500_battery *bat = power_supply_get_drvdata(psy);
+	struct device *dev = psy->dev.parent;
+	int ret = 0;
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_SERIAL_NUMBER:
+		ret = a500_battery_get_serial_number(bat, val);
+		break;
+
+	case POWER_SUPPLY_PROP_PRESENT:
+		ret = a500_battery_get_presence(bat, val);
+		break;
+
+	case POWER_SUPPLY_PROP_STATUS:
+		a500_battery_get_status(bat, val);
+		break;
+
+	case POWER_SUPPLY_PROP_TECHNOLOGY:
+		val->intval = POWER_SUPPLY_TECHNOLOGY_LION;
+		break;
+
+	case POWER_SUPPLY_PROP_CAPACITY:
+		a500_battery_update_capacity(bat);
+		val->intval = bat->capacity;
+		break;
+
+	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
+	case POWER_SUPPLY_PROP_CURRENT_NOW:
+	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+	case POWER_SUPPLY_PROP_TEMP:
+		ret = a500_battery_get_ec_data_index(dev, psp);
+		if (ret < 0)
+			break;
+
+		ret = a500_battery_get_simple_property(bat, val, ret);
+		break;
+
+	default:
+		dev_err(dev, "%s: invalid property %u\n", __func__, psp);
+		return -EINVAL;
+	}
+
+	if (!ret) {
+		/* convert units to match requirements for power supply class */
+		a500_battery_unit_adjustment(dev, psp, val);
+	}
+
+	dev_dbg(dev, "%s: property = %d, value = %x\n",
+		__func__, psp, val->intval);
+
+	/* return NODATA for properties if battery not presents */
+	if (ret)
+		return -ENODATA;
+
+	return 0;
+}
+
+static void a500_battery_delayed_work(struct work_struct *work)
+{
+	struct a500_battery *bat;
+	bool capacity_changed;
+
+	bat = container_of(work, struct a500_battery, poll_work.work);
+	capacity_changed = a500_battery_update_capacity(bat);
+
+	if (capacity_changed)
+		power_supply_changed(bat->psy);
+
+	/* continuously send uevent notification */
+	schedule_delayed_work(&bat->poll_work, 30 * HZ);
+}
+
+static const struct power_supply_desc a500_battery_desc = {
+	.name = "ec-battery",
+	.type = POWER_SUPPLY_TYPE_BATTERY,
+	.properties = a500_battery_properties,
+	.get_property = a500_battery_get_property,
+	.num_properties = ARRAY_SIZE(a500_battery_properties),
+	.external_power_changed = power_supply_changed,
+};
+
+static int a500_battery_probe(struct platform_device *pdev)
+{
+	struct power_supply_config psy_cfg = {};
+	struct a500_battery *bat;
+	int err;
+
+	bat = devm_kzalloc(&pdev->dev, sizeof(*bat), GFP_KERNEL);
+	if (!bat)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, bat);
+
+	psy_cfg.of_node = pdev->dev.parent->of_node;
+	psy_cfg.drv_data = bat;
+
+	bat->ec_chip = dev_get_drvdata(pdev->dev.parent);
+
+	bat->psy = devm_power_supply_register_no_ws(&pdev->dev,
+						    &a500_battery_desc,

drivers/power/supply/acer_a500_battery.c new file +41 -0

+
+	INIT_DELAYED_WORK(&bat->poll_work, a500_battery_delayed_work);
+	schedule_delayed_work(&bat->poll_work, HZ);
+
+	return 0;
+}
+
+static int a500_battery_remove(struct platform_device *pdev)
+{
+	struct a500_battery *bat = dev_get_drvdata(&pdev->dev);
+
+	cancel_delayed_work_sync(&bat->poll_work);
+
+	return 0;
+}
+
+static int __maybe_unused a500_battery_suspend(struct device *dev)
+{
+	struct a500_battery *bat = dev_get_drvdata(dev);
+
+	cancel_delayed_work_sync(&bat->poll_work);
+
+	return 0;
+}
+
+static int __maybe_unused a500_battery_resume(struct device *dev)
+{
+	struct a500_battery *bat = dev_get_drvdata(dev);
+
+	schedule_delayed_work(&bat->poll_work, HZ);
+
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(a500_battery_pm_ops,
+			 a500_battery_suspend, a500_battery_resume);
+
+static struct platform_driver a500_battery_driver = {
+	.driver = {
+		.name = "acer-a500-iconia-battery",
+		.pm = &a500_battery_pm_ops,

Hi,

Quoted 21 lines

drivers/power/supply/acer_a500_battery.c new file +21 -0

+static int a500_battery_get_serial_number(struct a500_battery *bat,
+					  union power_supply_propval *val)
+{
+	unsigned int i;
+	s32 ret = 0;
+
+	if (bat->serial[0])
+		goto done;
+
+	a500_ec_lock(bat->ec_chip);
+	for (i = 0; i < BATTERY_SERIAL_LEN / 2; i++) {
+		ret = a500_ec_read_locked(bat->ec_chip,
+					  &ec_data[REG_SERIAL_NUMBER].cmd);
+		if (ret < 0) {
+			bat->serial[0] = '\0';
+			break;
+		}
+
+		bat->serial[i * 2 + 0] = (ret >> 0) & 0xff;
+		bat->serial[i * 2 + 1] = (ret >> 8) & 0xff;
+	}

drivers/power/supply/acer_a500_battery.c line 174

Quoted 6 lines

drivers/power/supply/acer_a500_battery.c new file +6 -0

+	a500_ec_unlock(bat->ec_chip);
+done:
+	val->strval = bat->serial;
+
+	return ret;
+}

If battery is swapped, this will keep the old serial.

drivers/power/supply/acer_a500_battery.c line 318

Quoted 12 lines

drivers/power/supply/acer_a500_battery.c new file +12 -0

+	bat->psy = devm_power_supply_register_no_ws(&pdev->dev,
+						    &a500_battery_desc,
+						    &psy_cfg);
+	err = PTR_ERR_OR_ZERO(bat->psy);
+	if (err) {
+		if (err == -EPROBE_DEFER)
+			dev_dbg(&pdev->dev, "failed to register battery, deferring probe\n");
+		else
+			dev_err(&pdev->dev, "failed to register battery: %d\n",
+				err);
+		return err;
+	}

if (IS_ERR(bat->psy))

    return dev_err_probe(&pdev->dev, PTR_ERR(err), "failed to register battery\n");

drivers/power/supply/acer_a500_battery.c line 369

Quoted 4 lines

drivers/power/supply/acer_a500_battery.c new file +4 -0

+MODULE_DESCRIPTION("Battery gauge driver for Acer Iconia Tab A500");
+MODULE_AUTHOR("Dmitry Osipenko <[email protected]>");
+MODULE_ALIAS("platform:acer-a500-iconia-battery");
+MODULE_LICENSE("GPL v2");

MODULE_LICENSE("GPL");

Otherwise looks good to me.

24.08.2020 17:07, Sebastian Reichel пишет:

Quoted 21 lines

Quoted 21 lines

drivers/power/supply/acer_a500_battery.c new file +21 -0

+static int a500_battery_get_serial_number(struct a500_battery *bat,
+					  union power_supply_propval *val)
+{
+	unsigned int i;
+	s32 ret = 0;
+
+	if (bat->serial[0])
+		goto done;
+
+	a500_ec_lock(bat->ec_chip);
+	for (i = 0; i < BATTERY_SERIAL_LEN / 2; i++) {
+		ret = a500_ec_read_locked(bat->ec_chip,
+					  &ec_data[REG_SERIAL_NUMBER].cmd);
+		if (ret < 0) {
+			bat->serial[0] = '\0';
+			break;
+		}
+
+		bat->serial[i * 2 + 0] = (ret >> 0) & 0xff;
+		bat->serial[i * 2 + 1] = (ret >> 8) & 0xff;
+	}

Quoted 8 lines

drivers/power/supply/acer_a500_battery.c line 174

Quoted 6 lines

drivers/power/supply/acer_a500_battery.c new file +6 -0

+	a500_ec_unlock(bat->ec_chip);
+done:
+	val->strval = bat->serial;
+
+	return ret;
+}

If battery is swapped, this will keep the old serial.

Hello, Sebastian! The battery isn't hot-swappable on A500, but it also should be okay to always re-read the serialno. I'll consider removing the caching in the v2, thanks.

Hi,

Quoted 3 lines

Hello, Sebastian! The battery isn't hot-swappable on A500, but it also should be okay to always re-read the serialno. I'll consider removing the caching in the v2, thanks.

I assumed it would be hot-swappable because of a500_battery_get_presence(). If it's not hot-swappable, the caching is fine.

Hi!

drivers/leds/leds-acer-a500.c line 1

Quoted 7 lines

Acer Iconia Tab A500 is an Android tablet device which has two LEDs embedded into the Power Button. Orange LED indicates "battery charging" status and white LED indicates "wake-up/charge-done" status. The new LED driver provides control over both LEDs to userspace.

drivers/leds/leds-acer-a500.c new file +1 -0

@@ -0,0 +1,121 @@
+// SPDX-License-Identifier: GPL-2.0+

Nice.

drivers/leds/leds-acer-a500.c line 5

Quoted 1 line

drivers/leds/leds-acer-a500.c new file +1 -0

+ * Copyright 2020 GRATE-driver project.

Probably untrue.

drivers/leds/leds-acer-a500.c line 76

Quoted 1 line

drivers/leds/leds-acer-a500.c new file +1 -0

+	white_led->cdev.name = "power-button-white";

"white:power"

drivers/leds/leds-acer-a500.c line 79

Quoted 1 line

drivers/leds/leds-acer-a500.c new file +1 -0

+	white_led->cdev.max_brightness = LED_ON;

= 1. (And you'll need other adjustments over the code).

drivers/leds/leds-acer-a500.c line 86

Quoted 1 line

drivers/leds/leds-acer-a500.c new file +1 -0

+	orange_led->cdev.name = "power-button-orange";

"orange:power" -- or what is this LED usually used for?

drivers/leds/leds-acer-a500.c line 121

Quoted 1 line

drivers/leds/leds-acer-a500.c new file +1 -0

+MODULE_LICENSE("GPL v2");

Should be "GPL"?

drivers/leds/leds-acer-a500.c line 86

Quoted 1 line

drivers/leds/leds-acer-a500.c new file +1 -0

+	orange_led->cdev.name = "power-button-orange";

"orange:power" -- or what is this LED usually used for?

Acer Iconia Tab A500 is an Android tablet device which has two LEDs embedded into the Power Button. Orange LED indicates "battery charging" status and white LED indicates "wake-up/charge-done" status. The new LED driver provides control over both LEDs to userspace.

Quoted 4 lines

Acer Iconia Tab A500 is an Android tablet device which has two LEDs embedded into the Power Button. Orange LED indicates "battery charging" status and white LED indicates "wake-up/charge-done" status. The new LED driver provides control over both LEDs to userspace.

Hmm. If the ENE controller is similar to other devices, should it also share LED driver?

And I guess the cdev names should be different based on info above (I gave you wrong suggestions before)... and they probably should be parsed from the device tree.

The ENE KB930 hardware is compatible with KB3930.

Acer A500 Iconia Tab is Android tablet device, it has KB930 controller that is running firmware specifically customized for the needs of the Acer A500 hardware. This means that firmware interface isn't re-usable by other non-Acer devices. Some akin models of Acer tablets should be able to re-use the FW interface of A500 model, like A200 for example.

This patch adds the new compatibles to the binding.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 Documentation/devicetree/bindings/mfd/ene-kb3930.yaml | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/mfd/ene-kb3930.yaml +4 -1

index 074243c40891..5a1c4a959d9c 100644
@@ -17,8 +17,11 @@ properties:
   compatible:
     items:
       - enum:
+        - acer,a500-iconia-ec # Acer A500 Iconia tablet device
         - dell,wyse-ariel-ec  # Dell Wyse Ariel board (3020)
-      - const: ene,kb3930
+      - enum:
+        - ene,kb3930
+        - ene,kb930
   reg:
     maxItems: 1
 

Signature · 1 line

2.27.0

Quoted 9 lines

The ENE KB930 hardware is compatible with KB3930.

Acer A500 Iconia Tab is Android tablet device, it has KB930 controller that is running firmware specifically customized for the needs of the Acer A500 hardware. This means that firmware interface isn't re-usable by other non-Acer devices. Some akin models of Acer tablets should be able to re-use the FW interface of A500 model, like A200 for example.

This patch adds the new compatibles to the binding.

I've responded to patch 5/6 with what should've been said here [1]. Sorry for the confusion.

In any case please consider adding a new binding file instead of modifying the kb3930 binding doc. It would also remove a dependency on my patch set which should have slipped out of maintainers' radar.

[1] https://lore.kernel.org/lkml/[email protected]/

23.08.2020 21:20, Lubomir Rintel пишет:

Quoted 11 lines · Dmitry Osipenko

Quoted 9 lines

The ENE KB930 hardware is compatible with KB3930.

Acer A500 Iconia Tab is Android tablet device, it has KB930 controller that is running firmware specifically customized for the needs of the Acer A500 hardware. This means that firmware interface isn't re-usable by other non-Acer devices. Some akin models of Acer tablets should be able to re-use the FW interface of A500 model, like A200 for example.

This patch adds the new compatibles to the binding.

Quoted 8 lines

I've responded to patch 5/6 with what should've been said here [1]. Sorry for the confusion.

In any case please consider adding a new binding file instead of modifying the kb3930 binding doc. It would also remove a dependency on my patch set which should have slipped out of maintainers' radar.

[1] https://lore.kernel.org/lkml/[email protected]/

Hello, Lubomir! I was doing some research about the differences of KB3930 and KB930 before created this patch and my understanding is that the controllers are mostly identical. I've seen posts from people who replaced KB3930 with KB930 (and vice versa) on various notebooks and it worked, although not always.

It's a very common practice to re-use binding in a case of a sibling hardware. Do you know what are the exact differences between KB3930 and KB930 which could justify having separate bindings?

The firmware implementation varies a lot from device to device,

and
thus, each device needs to have its own driver in order to talk to the firmware, but hardware description (i.e. DT binding) should be common for all devices.

Hello,

Quoted 27 lines · Dmitry Osipenko

23.08.2020 21:20, Lubomir Rintel пишет:

Quoted 11 lines · Dmitry Osipenko

Quoted 9 lines

The ENE KB930 hardware is compatible with KB3930.

Acer A500 Iconia Tab is Android tablet device, it has KB930 controller that is running firmware specifically customized for the needs of the Acer A500 hardware. This means that firmware interface isn't re-usable by other non-Acer devices. Some akin models of Acer tablets should be able to re-use the FW interface of A500 model, like A200 for example.

This patch adds the new compatibles to the binding.

Quoted 8 lines

I've responded to patch 5/6 with what should've been said here [1]. Sorry for the confusion.

In any case please consider adding a new binding file instead of modifying the kb3930 binding doc. It would also remove a dependency on my patch set which should have slipped out of maintainers' radar.

[1] https://lore.kernel.org/lkml/[email protected]/

Hello, Lubomir! I was doing some research about the differences of KB3930 and KB930 before created this patch and my understanding is that the controllers are mostly identical. I've seen posts from people who replaced KB3930 with KB930 (and vice versa) on various notebooks and it worked, although not always.

Quoted 5 lines

It's a very common practice to re-use binding in a case of a sibling hardware. Do you know what are the exact differences between KB3930 and KB930 which could justify having separate bindings?

The firmware implementation varies a lot from device to device,

It sometimes does. The ENE's downstream driver suggests there are parts that run more-or-less stock firmware that are comatible with each other. That is why I grabbed the generic kb3930 name.

Quoted 4 lines

and
thus, each device needs to have its own driver in order to talk to the firmware, but hardware description (i.e. DT binding) should be common for all devices.

Note the DT is not the hardware description. It's the description of how the hardware presents itself, from the software's perspective. As far as that is concerned, the devices don't seem to have anything in common at all (other than the bus address). The fact that you need an entirely different driver implies this.

This would be the case even if the A500 EC was based directly on a KB3930.

A good reason to keep bindings for different yet somewhat similar devices in a single document is to avoid duplication. Yet here there's very little to share here. If you've done your bindings correctly, you'd need to conditionalize the monitored-battery and power-supplies properties for acer,a500-iconia-ec, complicating the binding too much. It makes more sense to just add a new document.

24.08.2020 00:16, Lubomir Rintel пишет:

Quoted 51 lines

Hello,

Quoted 27 lines · Dmitry Osipenko

23.08.2020 21:20, Lubomir Rintel пишет:

Quoted 11 lines · Dmitry Osipenko

Quoted 9 lines

The ENE KB930 hardware is compatible with KB3930.

Acer A500 Iconia Tab is Android tablet device, it has KB930 controller that is running firmware specifically customized for the needs of the Acer A500 hardware. This means that firmware interface isn't re-usable by other non-Acer devices. Some akin models of Acer tablets should be able to re-use the FW interface of A500 model, like A200 for example.

This patch adds the new compatibles to the binding.

Quoted 8 lines

I've responded to patch 5/6 with what should've been said here [1]. Sorry for the confusion.

In any case please consider adding a new binding file instead of modifying the kb3930 binding doc. It would also remove a dependency on my patch set which should have slipped out of maintainers' radar.

[1] https://lore.kernel.org/lkml/[email protected]/

Hello, Lubomir! I was doing some research about the differences of KB3930 and KB930 before created this patch and my understanding is that the controllers are mostly identical. I've seen posts from people who replaced KB3930 with KB930 (and vice versa) on various notebooks and it worked, although not always.

Quoted 5 lines

It's a very common practice to re-use binding in a case of a sibling hardware. Do you know what are the exact differences between KB3930 and KB930 which could justify having separate bindings?

The firmware implementation varies a lot from device to device,

It sometimes does. The ENE's downstream driver suggests there are parts that run more-or-less stock firmware that are comatible with each other. That is why I grabbed the generic kb3930 name.

Quoted 4 lines

and
thus, each device needs to have its own driver in order to talk to the firmware, but hardware description (i.e. DT binding) should be common for all devices.

Note the DT is not the hardware description. It's the description of how the hardware presents itself, from the software's perspective. As far as that is concerned, the devices don't seem to have anything in common at all (other than the bus address). The fact that you need an entirely different driver implies this.

Quoted 8 lines

This would be the case even if the A500 EC was based directly on a KB3930.

A good reason to keep bindings for different yet somewhat similar devices in a single document is to avoid duplication. Yet here there's very little to share here. If you've done your bindings correctly, you'd need to conditionalize the monitored-battery and power-supplies properties for acer,a500-iconia-ec, complicating the binding too much. It makes more sense to just add a new document.

Alright, I don't mind to separate the bindings. Although, before doing that, I'd want to get opinion from the device-tree experts, i.e. from Rob Herring :)

Rob, will it be fine to have separate bindings for each firmware version of the ENE controller given that firmware is individual for every device and given that FW has no compatibility with other devices?

Quoted 7 lines

Alright, I don't mind to separate the bindings. Although, before doing that, I'd want to get opinion from the device-tree experts, i.e. from Rob Herring :)

Rob, will it be fine to have separate bindings for each firmware version of the ENE controller given that firmware is individual for every device and given that FW has no compatibility with other devices?

Seems like separate bindings makes sense here.

Battery could be connected to the controller and in this case controller will provide a battery-monitor function.

The power-supplies phandle property is needed in order to describe the power supply which is used for charging of the battery, this allows to determine whither battery is charging or discharging, depending on the supply state.

The monitored-battery phandle provides information about the battery cell characteristics.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 .../devicetree/bindings/mfd/ene-kb3930.yaml    | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

Documentation/devicetree/bindings/mfd/ene-kb3930.yaml +18 -0

index 5a1c4a959d9c..435728054f3a 100644
@@ -29,6 +29,8 @@ properties:
     description: GPIO used with the shutdown protocol on Ariel
     maxItems: 2
 
+  monitored-battery: true
+  power-supplies: true
   system-power-controller: true
 
 required:

@@ -41,6 +43,19 @@ examples:
   - |
     #include <dt-bindings/gpio/gpio.h>
 
+    battery: battery-cell {
+            compatible = "simple-battery";
+            charge-full-design-microamp-hours = <3260000>;
+            energy-full-design-microwatt-hours = <24000000>;
+            operating-range-celsius = <0 40>;
+    };
+
+    mains: ac-adapter {
+      compatible = "gpio-charger";
+      charger-type = "mains";
+      gpios = <&gpio 125 GPIO_ACTIVE_LOW>;
+    };
+
     i2c {
       #address-cells = <1>;
       #size-cells = <0>;

@@ -52,6 +67,9 @@ examples:
 
         off-gpios = <&gpio 126 GPIO_ACTIVE_HIGH>,
                     <&gpio 127 GPIO_ACTIVE_HIGH>;
+
+        monitored-battery = <&battery>;
+        power-supplies = <&mains>;
       };
     };
 

Signature · 1 line

2.27.0