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

Lubomir Rintel
2020-08-23T18:17:05Z

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