[PATCH v1 0/6] Introduce Embedded Controller driver for Acer A500

[PATCH v1 0/6] Introduce Embedded Controller driver for Acer A500

[Dmitry Osipenko]

Hello!

This series adds support for the Embedded Controller which is found on
Acer Iconia Tab A500 (Android tablet device).

The Embedded Controller is ENE KB930 and it's running firmware customized
for the A500. The firmware interface may be reused by some other sibling
Acer tablets, although none of those tablets are supported in upstream yet.
Please review and apply, thanks in advance!

ATTENTION! This series depends on a-yet-unapplied patch from Lubomir Rintel
           which adds the device-tree binding for the ENE controller [1].

           It also depend on the pending patch that adds battery temperature
           properties to the battery binding [2].

[1] https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20200309203818.31266-4-lkundrak@v3.sk/
[2] https://patchwork.ozlabs.org/project/linux-tegra/patch/20200813213409.24222-2-digetx@gmail.com/

Dmitry Osipenko (6):
  mfd: Add driver for Embedded Controller found on Acer Iconia Tab A500
  power: supply: Add battery gauge driver for Acer Iconia Tab A500
  leds: Add driver for Acer Iconia Tab A500
  dt-bindings: mfd: ene-kb3930: Add compatibles for KB930 and Acer A500
  dt-bindings: mfd: ene-kb3930: Document power-supplies and
    monitored-battery properties
  ARM: tegra: acer-a500: Add Embedded Controller

 .../devicetree/bindings/mfd/ene-kb3930.yaml   |  23 +-
 .../boot/dts/tegra20-acer-a500-picasso.dts    |  17 +
 drivers/leds/Kconfig                          |   7 +
 drivers/leds/Makefile                         |   1 +
 drivers/leds/leds-acer-a500.c                 | 121 ++++++
 drivers/mfd/Kconfig                           |  10 +
 drivers/mfd/Makefile                          |   1 +
 drivers/mfd/acer-ec-a500.c                    | 196 ++++++++++
 drivers/power/supply/Kconfig                  |   6 +
 drivers/power/supply/Makefile                 |   1 +
 drivers/power/supply/acer_a500_battery.c      | 369 ++++++++++++++++++
 include/linux/mfd/acer-ec-a500.h              |  80 ++++
 12 files changed, 831 insertions(+), 1 deletion(-)
 create mode 100644 drivers/leds/leds-acer-a500.c
 create mode 100644 drivers/mfd/acer-ec-a500.c
 create mode 100644 drivers/power/supply/acer_a500_battery.c
 create mode 100644 include/linux/mfd/acer-ec-a500.h

-- 
2.27.0

[PATCH v1 1/6] mfd: Add driver for Embedded Controller found on Acer Iconia Tab A500

[Dmitry Osipenko]

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 <digetx@gmail.com>
---
 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

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 33df0837ab41..9e5cf88a52d8 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -2062,6 +2062,16 @@ config MFD_KHADAS_MCU
 	  additional drivers must be enabled in order to use the functionality
 	  of the device.
 
+config MFD_ACER_A500_EC
+	tristate "Embedded Controller driver for Acer Iconia Tab A500"
+	depends on (I2C_TEGRA && ARCH_TEGRA_2x_SOC) || COMPILE_TEST
+	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
 
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index a60e5f835283..6e3a6162ad94 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -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
diff --git a/drivers/mfd/acer-ec-a500.c b/drivers/mfd/acer-ec-a500.c
new file mode 100644
index 000000000000..f75bb60ab408
--- /dev/null
+++ b/drivers/mfd/acer-ec-a500.c
@@ -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>
+#include <linux/mfd/core.h>
+
+#define A500_EC_I2C_ERR_TIMEOUT		500
+
+/*				cmd	delay ms */
+A500_EC_COMMAND(SHUTDOWN,	0x52,	1000);
+A500_EC_COMMAND(WARM_REBOOT,	0x54,	1000);
+A500_EC_COMMAND(COLD_REBOOT,	0x55,	1000);
+
+static struct a500_ec *a500_ec_scratch;
+
+static void a500_ec_delay(unsigned int delay_ms)
+{
+	/* interrupts could be disabled on shutdown or reboot */
+	if (irqs_disabled())
+		mdelay(delay_ms);
+	else
+		msleep(delay_ms);
+}
+
+int a500_ec_read_locked(struct a500_ec *ec_chip,
+			const struct a500_ec_cmd *cmd_desc)
+{
+	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)
+		a500_ec_write_locked(ec_chip, WARM_REBOOT, 0);
+	else
+		a500_ec_write_locked(ec_chip, COLD_REBOOT, 1);
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block a500_ec_restart_handler = {
+	.notifier_call = a500_ec_restart_notify,
+	.priority = 200,
+};
+
+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",
+		.of_match_table = a500_ec_match,
+	},
+	.probe = a500_ec_probe,
+	.remove = a500_ec_remove,
+};
+module_i2c_driver(a500_ec_driver);
+
+MODULE_DESCRIPTION("Acer Iconia Tab A500 Embedded Controller driver");
+MODULE_AUTHOR("Dmitry Osipenko <digetx@gmail.com>");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mfd/acer-ec-a500.h b/include/linux/mfd/acer-ec-a500.h
new file mode 100644
index 000000000000..08bc681e6525
--- /dev/null
+++ b/include/linux/mfd/acer-ec-a500.h
@@ -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>
+
+struct a500_ec_cmd {
+	unsigned int cmd;
+	unsigned int post_delay;
+};
+
+#define A500_EC_COMMAND(NAME, CMD, DELAY_MS)				\
+static const struct a500_ec_cmd A500_EC_##NAME = {			\
+	.cmd = CMD,							\
+	.post_delay = DELAY_MS,						\
+};									\
+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 */
-- 
2.27.0

[PATCH v1 2/6] power: supply: Add battery gauge driver for Acer Iconia Tab A500

[Dmitry Osipenko]

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 <digetx@gmail.com>
---
 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

diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index faf2830aa152..dff5e5a7383f 100644
--- a/drivers/power/supply/Kconfig
+++ b/drivers/power/supply/Kconfig
@@ -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
diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
index b3c694a65114..08a5b49e2936 100644
--- a/drivers/power/supply/Makefile
+++ b/drivers/power/supply/Makefile
@@ -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
diff --git a/drivers/power/supply/acer_a500_battery.c b/drivers/power/supply/acer_a500_battery.c
new file mode 100644
index 000000000000..933101e1261e
--- /dev/null
+++ b/drivers/power/supply/acer_a500_battery.c
@@ -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;
+		}
+
+		bat->serial[i * 2 + 0] = (ret >> 0) & 0xff;
+		bat->serial[i * 2 + 1] = (ret >> 8) & 0xff;
+	}
+	a500_ec_unlock(bat->ec_chip);
+done:
+	val->strval = bat->serial;
+
+	return ret;
+}
+
+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,
+						    &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;
+	}
+
+	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,
+	},
+	.probe = a500_battery_probe,
+	.remove = a500_battery_remove,
+};
+module_platform_driver(a500_battery_driver);
+
+MODULE_DESCRIPTION("Battery gauge driver for Acer Iconia Tab A500");
+MODULE_AUTHOR("Dmitry Osipenko <digetx@gmail.com>");
+MODULE_ALIAS("platform:acer-a500-iconia-battery");
+MODULE_LICENSE("GPL v2");
-- 
2.27.0

[PATCH v1 3/6] leds: Add driver for Acer Iconia Tab A500

[Dmitry Osipenko]

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.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/leds/Kconfig          |   7 ++
 drivers/leds/Makefile         |   1 +
 drivers/leds/leds-acer-a500.c | 121 ++++++++++++++++++++++++++++++++++
 3 files changed, 129 insertions(+)
 create mode 100644 drivers/leds/leds-acer-a500.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 4f6464a169d5..4c39b53bcf1f 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -921,6 +921,13 @@ config LEDS_SGM3140
 	  This option enables support for the SGM3140 500mA Buck/Boost Charge
 	  Pump LED Driver.
 
+config LEDS_ACER_A500
+	tristate "Power button LED support for Acer Iconia Tab A500"
+	depends on LEDS_CLASS && MFD_ACER_A500_EC
+	help
+	  This option enables support for the Power Button LED of
+	  Acer Iconia Tab A500.
+
 comment "LED Triggers"
 source "drivers/leds/trigger/Kconfig"
 
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 778cb4bb8c52..73e603e1727e 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_LEDS_TRIGGERS)		+= led-triggers.o
 # LED Platform Drivers (keep this sorted, M-| sort)
 obj-$(CONFIG_LEDS_88PM860X)		+= leds-88pm860x.o
 obj-$(CONFIG_LEDS_AAT1290)		+= leds-aat1290.o
+obj-$(CONFIG_LEDS_ACER_A500)		+= leds-acer-a500.o
 obj-$(CONFIG_LEDS_ADP5520)		+= leds-adp5520.o
 obj-$(CONFIG_LEDS_AN30259A)		+= leds-an30259a.o
 obj-$(CONFIG_LEDS_APU)			+= leds-apu.o
diff --git a/drivers/leds/leds-acer-a500.c b/drivers/leds/leds-acer-a500.c
new file mode 100644
index 000000000000..65e69a40a91a
--- /dev/null
+++ b/drivers/leds/leds-acer-a500.c
@@ -0,0 +1,121 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Power button LED driver for Acer Iconia Tab A500.
+ *
+ * Copyright 2020 GRATE-driver project.
+ */
+
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+#include <linux/mfd/acer-ec-a500.h>
+
+struct a500_ec_led {
+	struct led_classdev cdev;
+	struct a500_ec_led *other_led;
+	const struct a500_ec_cmd *cmd;
+};
+
+/*					cmd	delay ms */
+A500_EC_COMMAND(RESET_LEDS,		0x40,	100);
+A500_EC_COMMAND(POWER_LED_ON,		0x42,	100);
+A500_EC_COMMAND(CHARGE_LED_ON,		0x43,	100);
+A500_EC_COMMAND(ANDROID_LEDS_OFF,	0x5A,	100);
+
+static int a500_ec_led_brightness_set(struct led_classdev *led_cdev,
+				      enum led_brightness value)
+{
+	struct device *a500_ec_leds_dev = led_cdev->dev->parent;
+	struct a500_ec *ec_chip = dev_get_drvdata(a500_ec_leds_dev->parent);
+	struct a500_ec_led *led = container_of(led_cdev, struct a500_ec_led,
+					       cdev);
+	int ret;
+
+	a500_ec_lock(ec_chip);
+
+	if (value) {
+		ret = a500_ec_write_locked(ec_chip, led->cmd, 0);
+	} else {
+		/*
+		 * There is no separate controls which can disable LEDs
+		 * individually, there is only RESET_LEDS command that turns
+		 * off both LEDs.
+		 */
+		ret = a500_ec_write_locked(ec_chip, RESET_LEDS, 0);
+		if (ret)
+			goto unlock;
+
+		led = led->other_led;
+
+		/* RESET_LEDS turns off both LEDs, thus restore other LED */
+		if (led->cdev.brightness == LED_ON)
+			ret = a500_ec_write_locked(ec_chip, led->cmd, 0);
+	}
+
+unlock:
+	a500_ec_unlock(ec_chip);
+
+	return ret;
+}
+
+static int a500_ec_leds_probe(struct platform_device *pdev)
+{
+	struct a500_ec *ec_chip = dev_get_drvdata(pdev->dev.parent);
+	struct a500_ec_led *white_led, *orange_led;
+	int err;
+
+	/* reset and turn off all LEDs */
+	a500_ec_write(ec_chip, RESET_LEDS, 0);
+	a500_ec_write(ec_chip, ANDROID_LEDS_OFF, 0);
+
+	white_led = devm_kzalloc(&pdev->dev, sizeof(*white_led), GFP_KERNEL);
+	if (!white_led)
+		return -ENOMEM;
+
+	white_led->cdev.name = "power-button-white";
+	white_led->cdev.brightness_set_blocking = a500_ec_led_brightness_set;
+	white_led->cdev.flags = LED_CORE_SUSPENDRESUME;
+	white_led->cdev.max_brightness = LED_ON;
+	white_led->cmd = &A500_EC_POWER_LED_ON;
+
+	orange_led = devm_kzalloc(&pdev->dev, sizeof(*orange_led), GFP_KERNEL);
+	if (!orange_led)
+		return -ENOMEM;
+
+	orange_led->cdev.name = "power-button-orange";
+	orange_led->cdev.brightness_set_blocking = a500_ec_led_brightness_set;
+	orange_led->cdev.flags = LED_CORE_SUSPENDRESUME;
+	orange_led->cdev.max_brightness = LED_ON;
+	orange_led->cmd = &A500_EC_CHARGE_LED_ON;
+
+	white_led->other_led = orange_led;
+	orange_led->other_led = white_led;
+
+	err = devm_led_classdev_register(&pdev->dev, &white_led->cdev);
+	if (err) {
+		dev_err(&pdev->dev, "failed to register white LED\n");
+		return err;
+	}
+
+	err = devm_led_classdev_register(&pdev->dev, &orange_led->cdev);
+	if (err) {
+		dev_err(&pdev->dev, "failed to register orange LED\n");
+		return err;
+	}
+
+	return 0;
+}
+
+static struct platform_driver a500_ec_leds_driver = {
+	.driver = {
+		.name = "acer-a500-iconia-leds",
+	},
+	.probe = a500_ec_leds_probe,
+};
+module_platform_driver(a500_ec_leds_driver);
+
+MODULE_DESCRIPTION("LED driver for Acer Iconia Tab A500 Power Button");
+MODULE_AUTHOR("Dmitry Osipenko <digetx@gmail.com>");
+MODULE_ALIAS("platform:acer-a500-iconia-leds");
+MODULE_LICENSE("GPL v2");
-- 
2.27.0

[PATCH v1 4/6] dt-bindings: mfd: ene-kb3930: Add compatibles for KB930 and Acer A500

[Dmitry Osipenko]

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 <digetx@gmail.com>
---
 Documentation/devicetree/bindings/mfd/ene-kb3930.yaml | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/mfd/ene-kb3930.yaml b/Documentation/devicetree/bindings/mfd/ene-kb3930.yaml
index 074243c40891..5a1c4a959d9c 100644
--- a/Documentation/devicetree/bindings/mfd/ene-kb3930.yaml
+++ b/Documentation/devicetree/bindings/mfd/ene-kb3930.yaml
@@ -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
 
-- 
2.27.0

[PATCH v1 5/6] dt-bindings: mfd: ene-kb3930: Document power-supplies and monitored-battery properties

[Dmitry Osipenko]

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 <digetx@gmail.com>
---
 .../devicetree/bindings/mfd/ene-kb3930.yaml    | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/ene-kb3930.yaml b/Documentation/devicetree/bindings/mfd/ene-kb3930.yaml
index 5a1c4a959d9c..435728054f3a 100644
--- a/Documentation/devicetree/bindings/mfd/ene-kb3930.yaml
+++ b/Documentation/devicetree/bindings/mfd/ene-kb3930.yaml
@@ -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>;
       };
     };
 
-- 
2.27.0

[PATCH v1 6/6] ARM: tegra: acer-a500: Add Embedded Controller

[Dmitry Osipenko]

This patch adds device-tree node for the Embedded Controller which is
found on the Picasso board. The Embedded Controller itself is ENE KB930,
it provides functions like battery-gauge/LED/GPIO/etc and it uses firmware
that is specifically customized for the Acer A500 device.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 arch/arm/boot/dts/tegra20-acer-a500-picasso.dts | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/arch/arm/boot/dts/tegra20-acer-a500-picasso.dts b/arch/arm/boot/dts/tegra20-acer-a500-picasso.dts
index 2d683c9a1a5d..f92712e4bd34 100644
--- a/arch/arm/boot/dts/tegra20-acer-a500-picasso.dts
+++ b/arch/arm/boot/dts/tegra20-acer-a500-picasso.dts
@@ -502,6 +502,16 @@ panel_ddc: i2c@1 {
 			reg = <1>;
 			#address-cells = <1>;
 			#size-cells = <0>;
+
+			embedded-controller@58 {
+				compatible = "acer,a500-iconia-ec", "ene,kb930";
+				reg = <0x58>;
+
+				system-power-controller;
+
+				monitored-battery = <&bat1010>;
+				power-supplies = <&mains>;
+			};
 		};
 	};
 
@@ -780,6 +790,13 @@ backlight: backlight {
 		default-brightness-level = <20>;
 	};
 
+	bat1010: battery-2s1p {
+		compatible = "simple-battery";
+		charge-full-design-microamp-hours = <3260000>;
+		energy-full-design-microwatt-hours = <24000000>;
+		operating-range-celsius = <0 40>;
+	};
+
 	/* PMIC has a built-in 32KHz oscillator which is used by PMC */
 	clk32k_in: clock@0 {
 		compatible = "fixed-clock";
-- 
2.27.0

Re: [PATCH v1 5/6] dt-bindings: mfd: ene-kb3930: Document power-supplies and monitored-battery properties

[Lubomir Rintel]

Hi,

On Sun, Aug 23, 2020 at 05:08:45PM +0300, Dmitry Osipenko wrote:
> 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.

I believe it would be better if you created a new binding document
instead of reusing this one -- the hardware part iseems to be a
different one and the firmware it runs seems to be behaving totally
differently than the usual ENE firmware [1].

[1] This eneec.c seems to be coming from ENE, so I'm assuming it's a
    good enough description of how their firmware behaves:
    https://git.kernel.org/pub/scm/linux/kernel/git/lkundrak/linux-mmp3-dell-ariel.git/tree/drivers/input/serio/eneec.c

Cheers
Lubo

> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  .../devicetree/bindings/mfd/ene-kb3930.yaml    | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/ene-kb3930.yaml b/Documentation/devicetree/bindings/mfd/ene-kb3930.yaml
> index 5a1c4a959d9c..435728054f3a 100644
> --- a/Documentation/devicetree/bindings/mfd/ene-kb3930.yaml
> +++ b/Documentation/devicetree/bindings/mfd/ene-kb3930.yaml
> @@ -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>;
>        };
>      };
>  
> -- 
> 2.27.0
> 

Re: [PATCH v1 1/6] mfd: Add driver for Embedded Controller found on Acer Iconia Tab A500

[Lubomir Rintel]

Hello,

On Sun, Aug 23, 2020 at 05:08:41PM +0300, Dmitry Osipenko wrote:
> 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 <digetx@gmail.com>
> ---
>  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
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 33df0837ab41..9e5cf88a52d8 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -2062,6 +2062,16 @@ config MFD_KHADAS_MCU
>  	  additional drivers must be enabled in order to use the functionality
>  	  of the device.
>  
> +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. 

> +	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
>  
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index a60e5f835283..6e3a6162ad94 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -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
> diff --git a/drivers/mfd/acer-ec-a500.c b/drivers/mfd/acer-ec-a500.c
> new file mode 100644
> index 000000000000..f75bb60ab408
> --- /dev/null
> +++ b/drivers/mfd/acer-ec-a500.c
> @@ -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>
> +#include <linux/mfd/core.h>
> +
> +#define A500_EC_I2C_ERR_TIMEOUT		500
> +
> +/*				cmd	delay ms */
> +A500_EC_COMMAND(SHUTDOWN,	0x52,	1000);
> +A500_EC_COMMAND(WARM_REBOOT,	0x54,	1000);
> +A500_EC_COMMAND(COLD_REBOOT,	0x55,	1000);
> +
> +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/

> +
> +static void a500_ec_delay(unsigned int delay_ms)
> +{
> +	/* interrupts could be disabled on shutdown or reboot */
> +	if (irqs_disabled())
> +		mdelay(delay_ms);
> +	else
> +		msleep(delay_ms);
> +}
> +
> +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.

> +{
> +	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)
> +		a500_ec_write_locked(ec_chip, WARM_REBOOT, 0);
> +	else
> +		a500_ec_write_locked(ec_chip, COLD_REBOOT, 1);
> +
> +	return NOTIFY_DONE;
> +}
> +
> +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?

> +};
> +
> +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",
> +		.of_match_table = a500_ec_match,
> +	},
> +	.probe = a500_ec_probe,
> +	.remove = a500_ec_remove,
> +};
> +module_i2c_driver(a500_ec_driver);
> +
> +MODULE_DESCRIPTION("Acer Iconia Tab A500 Embedded Controller driver");
> +MODULE_AUTHOR("Dmitry Osipenko <digetx@gmail.com>");
> +MODULE_LICENSE("GPL v2");

MODULE_LICENSE("GPL");

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

> diff --git a/include/linux/mfd/acer-ec-a500.h b/include/linux/mfd/acer-ec-a500.h
> new file mode 100644
> index 000000000000..08bc681e6525
> --- /dev/null
> +++ b/include/linux/mfd/acer-ec-a500.h
> @@ -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>
> +
> +struct a500_ec_cmd {
> +	unsigned int cmd;
> +	unsigned int post_delay;
> +};
> +
> +#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.

> +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 */
> -- 
> 2.27.0
> 

Cheers
Lubo

Re: [PATCH v1 4/6] dt-bindings: mfd: ene-kb3930: Add compatibles for KB930 and Acer A500

[Lubomir Rintel]

On Sun, Aug 23, 2020 at 05:08:44PM +0300, Dmitry Osipenko wrote:
> 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/20200823180041.GB209852@demiurge.local/

Take care
Lubo

> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  Documentation/devicetree/bindings/mfd/ene-kb3930.yaml | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/ene-kb3930.yaml b/Documentation/devicetree/bindings/mfd/ene-kb3930.yaml
> index 074243c40891..5a1c4a959d9c 100644
> --- a/Documentation/devicetree/bindings/mfd/ene-kb3930.yaml
> +++ b/Documentation/devicetree/bindings/mfd/ene-kb3930.yaml
> @@ -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
>  
> -- 
> 2.27.0
> 

Re: [PATCH v1 1/6] mfd: Add driver for Embedded Controller found on Acer Iconia Tab A500

[Dmitry Osipenko]

23.08.2020 21:16, Lubomir Rintel пишет:
> Hello,
...
>> +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.

...
>> +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.

...
>> +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.

Yes, perhaps you're right. Right now I can't recall what stopped me from
using regmap. I'll give it a shot for v2, thank you for the suggestion!

...
>> +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?

Then the Tegra's default CPU soft-reset handler will be used for
rebooting [1].

[1]
https://elixir.bootlin.com/linux/v5.9-rc1/source/drivers/soc/tegra/pmc.c#L977

...
>> +MODULE_LICENSE("GPL v2");
> 
> MODULE_LICENSE("GPL");
> 
> Your SPDX tag suggests newer versions of GPL than v2 are okay.

Okay! I'll change this in v2, thanks.

...
>> +#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.

This should be a good idea, especially combined with the regmap, thanks!

Re: [PATCH v1 4/6] dt-bindings: mfd: ene-kb3930: Add compatibles for KB930 and Acer A500

[Dmitry Osipenko]

23.08.2020 21:20, Lubomir Rintel пишет:
> On Sun, Aug 23, 2020 at 05:08:44PM +0300, Dmitry Osipenko wrote:
>> 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/20200823180041.GB209852@demiurge.local/

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.

Re: [PATCH v1 4/6] dt-bindings: mfd: ene-kb3930: Add compatibles for KB930 and Acer A500

[Lubomir Rintel]

Hello,

On Sun, Aug 23, 2020 at 10:31:36PM +0300, Dmitry Osipenko wrote:
> 23.08.2020 21:20, Lubomir Rintel пишет:
> > On Sun, Aug 23, 2020 at 05:08:44PM +0300, Dmitry Osipenko wrote:
> >> 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/20200823180041.GB209852@demiurge.local/
> 
> 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,

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.

> 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.

Thanks,
Lubo

Re: [PATCH v1 3/6] leds: Add driver for Acer Iconia Tab A500

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 922 bytes --]

Hi!

> 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.

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

Nice.

> + * Copyright 2020 GRATE-driver project.

Probably untrue.


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

"white:power"

> +	white_led->cdev.max_brightness = LED_ON;

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

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

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

> +MODULE_LICENSE("GPL v2");

Should be "GPL"?
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH v1 3/6] leds: Add driver for Acer Iconia Tab A500

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 767 bytes --]

On Sun 2020-08-23 17:08:43, Dmitry Osipenko wrote:
> 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.

Best regards,
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH v1 1/6] mfd: Add driver for Embedded Controller found on Acer Iconia Tab A500

[Lee Jones]

On Sun, 23 Aug 2020, Dmitry Osipenko wrote:

> 23.08.2020 21:16, Lubomir Rintel пишет:
> > Hello,
> ...
> >> +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.
> 
> ...
> >> +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.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v1 4/6] dt-bindings: mfd: ene-kb3930: Add compatibles for KB930 and Acer A500

[Dmitry Osipenko]

24.08.2020 00:16, Lubomir Rintel пишет:
> Hello,
> 
> On Sun, Aug 23, 2020 at 10:31:36PM +0300, Dmitry Osipenko wrote:
>> 23.08.2020 21:20, Lubomir Rintel пишет:
>>> On Sun, Aug 23, 2020 at 05:08:44PM +0300, Dmitry Osipenko wrote:
>>>> 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/20200823180041.GB209852@demiurge.local/
>>
>> 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,
> 
> 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.
> 
>> 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.

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?

Re: [PATCH v1 1/6] mfd: Add driver for Embedded Controller found on Acer Iconia Tab A500

[Dmitry Osipenko]

24.08.2020 10:33, Lee Jones пишет:
>> ...
>>>> +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.
> 

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.

Re: [PATCH v1 3/6] leds: Add driver for Acer Iconia Tab A500

[Dmitry Osipenko]

24.08.2020 01:30, Pavel Machek пишет:
> Hi!

Hello, Pavel! Thank you very much for the review! I'll take into account
yours comments in the v2!

>> 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.
> 
>> @@ -0,0 +1,121 @@
>> +// SPDX-License-Identifier: GPL-2.0+
> 
> Nice.
> 
>> + * Copyright 2020 GRATE-driver project.
> 
> Probably untrue.
> 
> 
>> +	white_led->cdev.name = "power-button-white";
> 
> "white:power"
> 
>> +	white_led->cdev.max_brightness = LED_ON;
> 
> = 1. (And you'll need other adjustments over the code).
> 
>> +	orange_led->cdev.name = "power-button-orange";
> 
> "orange:power" -- or what is this LED usually used for?
> 
>> +MODULE_LICENSE("GPL v2");
> 
> Should be "GPL"?

Yes, apparently it should be "GPL". I'll try to double-check it for the v2.

Thanks!

Re: [PATCH v1 3/6] leds: Add driver for Acer Iconia Tab A500

[Dmitry Osipenko]

24.08.2020 01:34, Pavel Machek пишет:
> On Sun 2020-08-23 17:08:43, Dmitry Osipenko wrote:
>> 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 controller hardware is the same on all devices that use it, but
firmware isn't the same and apparently every vendor invents its own
thing in regards to the firmware because firmware features and interface
varies vastly from device to device. Hence, unfortunately, usually there
is very little compatibility even if devices come form the same vendor

AFAIK, the ENE controller provides some compatibility on x86 machines
via ACPI EC standard, but this doesn't apply to the ARM devices.

I know that Acer A200 should be able to re-use the A500 EC driver as-is,
but A200 is pretty much the same device as A500, so it's not surprising.
IIRC, A200 model only misses back camera in comparison to A500. Hence
there shouldn't be a need to parse the names from a device-tree, but
I'll try to double-check it to be sure.

Thanks!

Re: [PATCH v1 1/6] mfd: Add driver for Embedded Controller found on Acer Iconia Tab A500

[Lee Jones]

On Mon, 24 Aug 2020, Dmitry Osipenko wrote:

> 24.08.2020 10:33, Lee Jones пишет:
> >> ...
> >>>> +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.
> > 
> 
> 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.

That would be really good. :)

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v1 3/6] leds: Add driver for Acer Iconia Tab A500

[Dmitry Osipenko]

24.08.2020 01:30, Pavel Machek пишет:
>> +	orange_led->cdev.name = "power-button-orange";
> 
> "orange:power" -- or what is this LED usually used for?

The orange LED is supposed to indicate that battery is charging up.

Re: [PATCH v1 2/6] power: supply: Add battery gauge driver for Acer Iconia Tab A500

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 12308 bytes --]

Hi,

On Sun, Aug 23, 2020 at 05:08:42PM +0300, Dmitry Osipenko wrote:
> 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 <digetx@gmail.com>
> ---
>  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
> 
> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> index faf2830aa152..dff5e5a7383f 100644
> --- a/drivers/power/supply/Kconfig
> +++ b/drivers/power/supply/Kconfig
> @@ -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
> diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
> index b3c694a65114..08a5b49e2936 100644
> --- a/drivers/power/supply/Makefile
> +++ b/drivers/power/supply/Makefile
> @@ -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
> diff --git a/drivers/power/supply/acer_a500_battery.c b/drivers/power/supply/acer_a500_battery.c
> new file mode 100644
> index 000000000000..933101e1261e
> --- /dev/null
> +++ b/drivers/power/supply/acer_a500_battery.c
> @@ -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;
> +		}
> +
> +		bat->serial[i * 2 + 0] = (ret >> 0) & 0xff;
> +		bat->serial[i * 2 + 1] = (ret >> 8) & 0xff;
> +	}
> +	a500_ec_unlock(bat->ec_chip);
> +done:
> +	val->strval = bat->serial;
> +
> +	return ret;
> +}

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

> +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,
> +						    &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");

> +
> +	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,
> +	},
> +	.probe = a500_battery_probe,
> +	.remove = a500_battery_remove,
> +};
> +module_platform_driver(a500_battery_driver);
> +
> +MODULE_DESCRIPTION("Battery gauge driver for Acer Iconia Tab A500");
> +MODULE_AUTHOR("Dmitry Osipenko <digetx@gmail.com>");
> +MODULE_ALIAS("platform:acer-a500-iconia-battery");
> +MODULE_LICENSE("GPL v2");

MODULE_LICENSE("GPL");

Otherwise looks good to me.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 2/6] power: supply: Add battery gauge driver for Acer Iconia Tab A500

[Dmitry Osipenko]

24.08.2020 17:07, Sebastian Reichel пишет:
> Hi,
...
>> +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;
>> +	}
>> +	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.

...
>> +	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");

I didn't know that dev_err_probe() is available now, very nice! I'll use
it in the v2, thanks.

...
>> +MODULE_DESCRIPTION("Battery gauge driver for Acer Iconia Tab A500");
>> +MODULE_AUTHOR("Dmitry Osipenko <digetx@gmail.com>");
>> +MODULE_ALIAS("platform:acer-a500-iconia-battery");
>> +MODULE_LICENSE("GPL v2");
> 
> MODULE_LICENSE("GPL");
> 
> Otherwise looks good to me.

Okay, thank you!

Re: [PATCH v1 2/6] power: supply: Add battery gauge driver for Acer Iconia Tab A500

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1278 bytes --]

Hi,

On Mon, Aug 24, 2020 at 09:55:14PM +0300, Dmitry Osipenko wrote:
> 24.08.2020 17:07, Sebastian Reichel пишет:
> >> +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;
> >> +	}
> >> +	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.

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

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 2/6] power: supply: Add battery gauge driver for Acer Iconia Tab A500

[Dmitry Osipenko]

25.08.2020 00:38, Sebastian Reichel пишет:
> Hi,
...
>> 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.

The battery could be disconnected from the motherboard, but this
requires to have device disassembled.

Okay, I'll keep the caching.

Thanks!

Re: [PATCH v1 4/6] dt-bindings: mfd: ene-kb3930: Add compatibles for KB930 and Acer A500

[Rob Herring]

On Mon, Aug 24, 2020 at 01:09:22PM +0300, Dmitry Osipenko wrote:
> 24.08.2020 00:16, Lubomir Rintel пишет:
> > Hello,
> > 
> > On Sun, Aug 23, 2020 at 10:31:36PM +0300, Dmitry Osipenko wrote:
> >> 23.08.2020 21:20, Lubomir Rintel пишет:
> >>> On Sun, Aug 23, 2020 at 05:08:44PM +0300, Dmitry Osipenko wrote:
> >>>> 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/20200823180041.GB209852@demiurge.local/
> >>
> >> 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,
> > 
> > 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.
> > 
> >> 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.
> 
> 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.

Rob

Re: [PATCH v1 4/6] dt-bindings: mfd: ene-kb3930: Add compatibles for KB930 and Acer A500

[Dmitry Osipenko]

09.09.2020 00:53, Rob Herring пишет:
...
>> 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.

Okay, thank you for the answers and for the review of the v3!