diff mbox series

[v5,2/4] mfd: Add driver for Embedded Controller found on Acer Iconia Tab A500

Message ID 20201104203403.24937-3-digetx@gmail.com
State New
Headers show
Series None | expand

Commit Message

Dmitry Osipenko Nov. 4, 2020, 8:34 p.m. UTC 
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        |  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

Comments

Dmitry Osipenko Nov. 16, 2020, 9:53 a.m. UTC | #1 
16.11.2020 11:48, Lee Jones пишет:
>>>> +config MFD_ACER_A500_EC

>>>> +	tristate "Embedded Controller driver for Acer Iconia Tab A500"

>>>

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

>>>

>>>> +	depends on I2C

>>>

>>> depends on OF ?

>> ...

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

> 

> I always thought it was best to be explicit.


Alright, I see that the OF selection is all over the MFD Kconfig, hence
let's keep it consistent.

I also prefer the explicit variant more, but some other kernel
maintainers may disagree.

>> ...

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

> 

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


The regmap core code performs all the necessary checks before driver's
code is reached, perhaps I just overlooked something before. Hence it
will be removed in the next revision.

...
>>>> +	while (retries-- > 0) {

>>>> +		ret = i2c_smbus_read_word_data(client, reg);

>>>> +		if (ret >= 0)

>>>> +			break;

>>>> +

>>>> +		msleep(A500_EC_I2C_ERR_TIMEOUT);

>>>> +	}

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


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?

>> ...

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

> 

> 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.
Lee Jones Nov. 16, 2020, 10:29 a.m. UTC | #2 
On Mon, 16 Nov 2020, Dmitry Osipenko wrote:
> ...
> >>>> +	while (retries-- > 0) {
> >>>> +		ret = i2c_smbus_read_word_data(client, reg);
> >>>> +		if (ret >= 0)
> >>>> +			break;
> >>>> +
> >>>> +		msleep(A500_EC_I2C_ERR_TIMEOUT);
> >>>> +	}
> ...
> >>> 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.
> 
> 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?

I haven't looked into it.  My comment was more general in nature.

As a general rule, helpers are better than open coding, but there are
always exceptions.  There may not even be a suitable helper for this
use-case.  As I say, the comment was more of a passing remark.

[...]

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

That'll do.
diff mbox series

Patch

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 8b99a13669bf..527ba5054d80 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -2097,6 +2097,18 @@  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
+	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
 
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 1780019d2474..7bfc57c8b363 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -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
diff --git a/drivers/mfd/acer-ec-a500.c b/drivers/mfd/acer-ec-a500.c
new file mode 100644
index 000000000000..2785a6d9bcc4
--- /dev/null
+++ b/drivers/mfd/acer-ec-a500.c
@@ -0,0 +1,203 @@ 
+// 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/mfd/core.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/regmap.h>
+#include <linux/reboot.h>
+
+#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;
+
+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)
+		return -EOPNOTSUPP;
+
+	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);
+	}
+
+	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);
+
+	return 0;
+}
+
+static int a500_ec_write(void *context, const void *data, size_t count)
+{
+	struct i2c_client *client = context;
+	unsigned int reg, val, retries = 5;
+	s32 ret = 0;
+
+	if (count != 3)
+		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;
+
+		msleep(A500_EC_I2C_ERR_TIMEOUT);
+	}
+
+	if (ret < 0) {
+		dev_err(&client->dev, "write 0x%x failed: %d\n", reg, ret);
+		return ret;
+	}
+
+	return 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);
+}
+
+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);
+
+	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,
+};
+
+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)
+{
+	struct regmap *rmap;
+	int err;
+
+	rmap = devm_regmap_init(&client->dev, &a500_ec_regmap_bus,
+				client, &a500_ec_regmap_config);
+	if (IS_ERR(rmap))
+		return PTR_ERR(rmap);
+
+	/* 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_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 <digetx@gmail.com>");
+MODULE_LICENSE("GPL");