Introduce Embedded Controller driver for Acer A500

Add binding document for the ENE KB930 Embedded Controller.

Reviewed-by Rob Herring <[email protected]>

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

---
 .../devicetree/bindings/mfd/ene-kb930.yaml    | 66 +++++++++++++++++++
 1 file changed, 66 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/ene-kb930.yaml

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        |  12 +++
 drivers/mfd/Makefile       |   1 +
 drivers/mfd/acer-ec-a500.c | 203 +++++++++++++++++++++++++++++++++++++
 3 files changed, 216 insertions(+)
 create mode 100644 drivers/mfd/acer-ec-a500.c

drivers/mfd/Kconfig +0 -0

index 8b99a13669bf..527ba5054d80 100644

drivers/mfd/Kconfig +9 -0

+	depends on ARCH_TEGRA_2x_SOC || COMPILE_TEST
+	select MFD_CORE
+	select REGMAP
+	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 1780019d2474..7bfc57c8b363 100644
@@ -263,6 +263,7 @@ 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
 obj-$(CONFIG_MFD_SIMPLE_MFD_I2C)	+= simple-mfd-i2c.o

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

new file mode 100644
index 000000000000..2785a6d9bcc4

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

+#define A500_EC_I2C_ERR_TIMEOUT		500
+#define A500_EC_POWER_CMD_TIMEOUT	1000
+
+enum {
+	REG_CURRENT_NOW = 0x03,
+	REG_SHUTDOWN = 0x52,
+	REG_WARM_REBOOT = 0x54,
+	REG_COLD_REBOOT = 0x55,
+};
+
+static struct i2c_client *a500_ec_client_pm_off;

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

+	reg = *(u8 *)reg_buf;
+
+	while (retries-- > 0) {
+		ret = i2c_smbus_read_word_data(client, reg);
+		if (ret >= 0)
+			break;
+
+		msleep(A500_EC_I2C_ERR_TIMEOUT);
+	}

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

+		return -EOPNOTSUPP;
+
+	reg = *(u8  *)(data + 0);
+	val = *(u16 *)(data + 1);
+
+	while (retries-- > 0) {
+		ret = i2c_smbus_write_word_data(client, reg, val);
+		if (ret >= 0)
+			break;
+

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

+static const struct regmap_config a500_ec_regmap_config = {
+	.name = "KB930",
+	.reg_bits = 8,
+	.val_bits = 16,
+	.max_register = 0xff,
+};
+
+static const struct regmap_bus a500_ec_regmap_bus = {
+	.reg_format_endian_default = REGMAP_ENDIAN_NATIVE,
+	.val_format_endian_default = REGMAP_ENDIAN_LITTLE,
+	.write = a500_ec_write,
+	.read = a500_ec_read,
+	.max_raw_read = 2,
+};
+
+static void a500_ec_poweroff(void)
+{
+	i2c_smbus_write_word_data(a500_ec_client_pm_off, REG_SHUTDOWN, 0);
+
+	mdelay(A500_EC_POWER_CMD_TIMEOUT);
+}

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

+	mdelay(A500_EC_POWER_CMD_TIMEOUT);
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block a500_ec_restart_handler = {
+	.notifier_call = a500_ec_restart_notify,
+	.priority = 200,

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

+	if (of_device_is_system_power_controller(client->dev.of_node)) {
+		a500_ec_client_pm_off = client;
+
+		err = register_restart_handler(&a500_ec_restart_handler);
+		if (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_new = 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 <[email protected]>");
+MODULE_LICENSE("GPL");

drivers/mfd/Kconfig config MFD_KHADAS_MCU · line 2101

Quoted 2 lines

drivers/mfd/Kconfig +2 -0

+config MFD_ACER_A500_EC
+	tristate "Embedded Controller driver for Acer Iconia Tab A500"

Drop "driver". This is meant to be describing the device.

drivers/mfd/Kconfig config MFD_KHADAS_MCU · line 2102

Quoted 1 line

drivers/mfd/Kconfig +1 -0

+	depends on I2C

depends on OF ?

drivers/mfd/Kconfig config MFD_KHADAS_MCU · line 2106

Quoted 4 lines

drivers/mfd/Kconfig +4 -0

+	depends on ARCH_TEGRA_2x_SOC || COMPILE_TEST
+	select MFD_CORE
+	select REGMAP
+	help

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

Quoted 9 lines

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

+static int a500_ec_read(void *context, const void *reg_buf, size_t reg_size,
+			void *val_buf, size_t val_sizel)
+{
+	struct i2c_client *client = context;
+	unsigned int reg, retries = 5;
+	u16 *ret_val = val_buf;
+	s32 ret = 0;
+
+	if (reg_size != 1 || val_sizel != 2)

No magic numbers please.

What does this *mean*?

Review

Quoted 1 line

(quoted diff) +1 -0

+		return -EOPNOTSUPP;

Why EOPNOTSUPP?

Quoted 9 lines

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

+	reg = *(u8 *)reg_buf;
+
+	while (retries-- > 0) {
+		ret = i2c_smbus_read_word_data(client, reg);
+		if (ret >= 0)
+			break;
+
+		msleep(A500_EC_I2C_ERR_TIMEOUT);
+	}

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

Quoted 10 lines

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

+
+	if (ret < 0) {
+		dev_err(&client->dev, "read 0x%x failed: %d\n", reg, ret);
+		return ret;
+	}
+
+	*ret_val = ret;
+
+	if (reg == REG_CURRENT_NOW)
+		fsleep(10000);

Ooo, new toy!

Review

Quoted 2 lines

(quoted diff) +2 -0

+	return 0;
+}

I'm surprised there isn't a generic function which does this kind of read. Seems like pretty common/boilerplate stuff.

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

Quoted 9 lines

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

+static int a500_ec_restart_notify(struct notifier_block *this,
+				  unsigned long reboot_mode, void *data)
+{
+	if (reboot_mode == REBOOT_WARM)
+		i2c_smbus_write_word_data(a500_ec_client_pm_off,
+					  REG_WARM_REBOOT, 0);
+	else
+		i2c_smbus_write_word_data(a500_ec_client_pm_off,
+					  REG_COLD_REBOOT, 1);

What's with the magic '0' and '1's at the end?

13.11.2020 12:37, Lee Jones пишет:
...

Quoted 8 lines

drivers/mfd/Kconfig config MFD_KHADAS_MCU · line 2101

Quoted 2 lines

drivers/mfd/Kconfig +2 -0

+config MFD_ACER_A500_EC
+	tristate "Embedded Controller driver for Acer Iconia Tab A500"

Drop "driver". This is meant to be describing the device.

drivers/mfd/Kconfig config MFD_KHADAS_MCU · line 2102

Quoted 1 line

drivers/mfd/Kconfig +1 -0

+	depends on I2C

depends on OF ?

...

Quoted 4 lines

drivers/mfd/Kconfig config MFD_KHADAS_MCU · line 2106

Quoted 4 lines

drivers/mfd/Kconfig +4 -0

+	depends on ARCH_TEGRA_2x_SOC || COMPILE_TEST
+	select MFD_CORE
+	select REGMAP
+	help

ARCH_TEGRA_2x_SOC selects OF.

For COMPILE_TEST it doesn't matter since OF framework provides stubs for !OF.

...

Quoted 12 lines

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

Quoted 9 lines

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

+static int a500_ec_read(void *context, const void *reg_buf, size_t reg_size,
+			void *val_buf, size_t val_sizel)
+{
+	struct i2c_client *client = context;
+	unsigned int reg, retries = 5;
+	u16 *ret_val = val_buf;
+	s32 ret = 0;
+
+	if (reg_size != 1 || val_sizel != 2)

No magic numbers please.

Quoted 1 line

What does this *mean*?

These are the size of address register and size of a read value, both in bytes.

Quoted 3 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+		return -EOPNOTSUPP;

Why EOPNOTSUPP?

Other sizes aren't supported by embedded controller.

Although, perhaps this check isn't really needed at all since the sizes are already expressed in the a500_ec_regmap_config.

I'll need to take a closer look at why this size-checking was added because don't quite remember already. If it's not needed, then I'll remove it in the next revision, otherwise will add a clarifying comment.

Quoted 22 lines

Quoted 9 lines

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

+	reg = *(u8 *)reg_buf;
+
+	while (retries-- > 0) {
+		ret = i2c_smbus_read_word_data(client, reg);
+		if (ret >= 0)
+			break;
+
+		msleep(A500_EC_I2C_ERR_TIMEOUT);
+	}

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

Quoted 10 lines

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

+
+	if (ret < 0) {
+		dev_err(&client->dev, "read 0x%x failed: %d\n", reg, ret);
+		return ret;
+	}
+
+	*ret_val = ret;
+
+	if (reg == REG_CURRENT_NOW)
+		fsleep(10000);

Ooo, new toy!

Quoted 5 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+	return 0;
+}

I'm surprised there isn't a generic function which does this kind of read. Seems like pretty common/boilerplate stuff.

I'm not quite sure that this is a really very common pattern which deserves a generic helper.

...

Quoted 11 lines

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

Quoted 9 lines

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

+static int a500_ec_restart_notify(struct notifier_block *this,
+				  unsigned long reboot_mode, void *data)
+{
+	if (reboot_mode == REBOOT_WARM)
+		i2c_smbus_write_word_data(a500_ec_client_pm_off,
+					  REG_WARM_REBOOT, 0);
+	else
+		i2c_smbus_write_word_data(a500_ec_client_pm_off,
+					  REG_COLD_REBOOT, 1);

What's with the magic '0' and '1's at the end?

These are the values which controller's firmware wants to see, otherwise it will reject command as invalid. I'll add a clarifying comment to the code.

Quoted 16 lines

Quoted 8 lines

drivers/mfd/Kconfig config MFD_KHADAS_MCU · line 2101

Quoted 2 lines

drivers/mfd/Kconfig +2 -0

+config MFD_ACER_A500_EC
+	tristate "Embedded Controller driver for Acer Iconia Tab A500"

Drop "driver". This is meant to be describing the device.

drivers/mfd/Kconfig config MFD_KHADAS_MCU · line 2102

Quoted 1 line

drivers/mfd/Kconfig +1 -0

+	depends on I2C

depends on OF ?

...

Quoted 4 lines

drivers/mfd/Kconfig config MFD_KHADAS_MCU · line 2106

Quoted 4 lines

drivers/mfd/Kconfig +4 -0

+	depends on ARCH_TEGRA_2x_SOC || COMPILE_TEST
+	select MFD_CORE
+	select REGMAP
+	help

ARCH_TEGRA_2x_SOC selects OF.

Quoted 2 lines

For COMPILE_TEST it doesn't matter since OF framework provides stubs for !OF.

I always thought it was best to be explicit.

Quoted 8 lines

...

Quoted 1 line

Why EOPNOTSUPP?

Other sizes aren't supported by embedded controller.

Although, perhaps this check isn't really needed at all since the sizes are already expressed in the a500_ec_regmap_config.

Quoted 3 lines

I'll need to take a closer look at why this size-checking was added because don't quite remember already. If it's not needed, then I'll remove it in the next revision, otherwise will add a clarifying comment.

"Operation not supported on transport endpoint" doesn't seem like a good fit here. If the check says, please consider changing it to something like -EINVAL.

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

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

+	while (retries-- > 0) {
+		ret = i2c_smbus_read_word_data(client, reg);
+		if (ret >= 0)
+			break;
+
+		msleep(A500_EC_I2C_ERR_TIMEOUT);
+	}

Quoted 5 lines

Quoted 2 lines

I'm surprised there isn't a generic function which does this kind of read. Seems like pretty common/boilerplate stuff.

I'm not quite sure that this is a really very common pattern which deserves a generic helper.

What? Read from I2C a few times, then sleep sounds like a pretty common pattern to me.

Quoted 13 lines

...

Quoted 11 lines

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

Quoted 9 lines

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

+static int a500_ec_restart_notify(struct notifier_block *this,
+				  unsigned long reboot_mode, void *data)
+{
+	if (reboot_mode == REBOOT_WARM)
+		i2c_smbus_write_word_data(a500_ec_client_pm_off,
+					  REG_WARM_REBOOT, 0);
+	else
+		i2c_smbus_write_word_data(a500_ec_client_pm_off,
+					  REG_COLD_REBOOT, 1);

What's with the magic '0' and '1's at the end?

Quoted 3 lines

These are the values which controller's firmware wants to see, otherwise it will reject command as invalid. I'll add a clarifying comment to the code.

Thanks. Hopefully with a bit more information as to why the firmware expects to see them. Hopefully they're not random.

...

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

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

+	while (retries-- > 0) {
+		ret = i2c_smbus_read_word_data(client, reg);
+		if (ret >= 0)
+			break;
+
+		msleep(A500_EC_I2C_ERR_TIMEOUT);
+	}

...

Quoted 3 lines

Quoted 3 lines

Quoted 2 lines

I'm surprised there isn't a generic function which does this kind of read. Seems like pretty common/boilerplate stuff.

Quoted 5 lines

Quoted 2 lines

I'm not quite sure that this is a really very common pattern which deserves a generic helper.

What? Read from I2C a few times, then sleep sounds like a pretty common pattern to me.

Maybe the read_poll_timeout() helper could be used for that, but I think the open-coded variant is much easier to perceive, don't you agree?

Quoted 6 lines

Quoted 3 lines

These are the values which controller's firmware wants to see, otherwise it will reject command as invalid. I'll add a clarifying comment to the code.

Thanks. Hopefully with a bit more information as to why the firmware expects to see them. Hopefully they're not random.

These are the firmware-defined specific command opcodes, I'll add defines for them to make it more clear, thanks.