| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20201113093747.GJ2787115@dell>
Date: Fri, 13 Nov 2020 09:37:47 +0000
From: Lee Jones <lee.jones@...aro.org>
To: Dmitry Osipenko <digetx@...il.com>
Cc: Rob Herring <robh+dt@...nel.org>,
Thierry Reding <thierry.reding@...il.com>,
Jonathan Hunter <jonathanh@...dia.com>,
Dan Murphy <dmurphy@...com>,
Sebastian Reichel <sre@...nel.org>, devicetree@...r.kernel.org,
linux-tegra@...r.kernel.org, linux-pm@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 2/4] mfd: Add driver for Embedded Controller found on
Acer Iconia Tab A500
On Wed, 04 Nov 2020, 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@...il.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
>
> 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"
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
> + 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.
An "MFD" isn't a thing. Please describe the device.
> + * Copyright 2020 GRATE-driver project.
> + *
> + * Based on downstream driver from Acer Inc.
Most drivers are. Not sure this is required.
> + */
> +
> +#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>
Alphabetical please. ;)
> +#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)
No magic numbers please.
What does this *mean*?
> + return -EOPNOTSUPP;
Why 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);
Ooo, new toy!
> + return 0;
> +}
I'm surprised there isn't a generic function which does this kind of
read. Seems like pretty common/boilerplate stuff.
> +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)
Define or comment needed.
> + 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;
> +}
Again, seems pretty boilerplate. Are you sure there isn't a helper?
> +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);
What's with the magic '0' and '1's at the end?
> + 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;
'rmap' barely makes the top 10:
$ git grep -ho "struct regmap \*[a-z]*" | sort | uniq -c | sort -rn | head
1814 struct regmap *regmap
581 struct regmap *map
97 struct regmap *
50 struct regmap *syscon
50 struct regmap *r
35 struct regmap *reg
34 struct regmap *cfgchip
32 struct regmap *grf
30 struct regmap *rmap
27 struct regmap *base
Please use regmap here.
> + 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 */
This comment is superfluous and is just the type of documentation that
becomes out-of-date quickly.
> + 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 */
We know what "power" and "pm" mean. You can safely remove this
comment.
> + 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@...il.com>");
> +MODULE_LICENSE("GPL");
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
Powered by blists - more mailing lists