lkml.org 
[lkml]   [2020]   [Aug]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v1 1/6] mfd: Add driver for Embedded Controller found on Acer Iconia Tab A500
    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

    \
     
     \ /
      Last update: 2020-08-23 20:18    [W:7.819 / U:0.088 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site