Re: [PATCH v6 2/7] mfd: Add driver for ASUS Transformer embedded controller

The Linux Kernel Mailing List
 help / color / mirror / Atom feed

* Re: [PATCH v6 2/7] mfd: Add driver for ASUS Transformer embedded controller
       [not found] ` <20260502124055.22475-3-clamor95@gmail.com>
@ 2026-05-14 10:02   ` Lee Jones
  2026-05-14 10:31     ` Svyatoslav Ryhel
  2026-05-14 11:02     ` Svyatoslav Ryhel
  0 siblings, 2 replies; 6+ messages in thread
From: Lee Jones @ 2026-05-14 10:02 UTC (permalink / raw)
  To: Svyatoslav Ryhel
  Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Dmitry Torokhov,
	Pavel Machek, Sebastian Reichel, Ion Agorria,
	Michał Mirosław, devicetree, linux-kernel, linux-input,
	linux-leds, linux-pm

On Sat, 02 May 2026, Svyatoslav Ryhel wrote:

> From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> 
> Support Nuvoton NPCE795-based ECs as used in Asus Transformer TF201,
> TF300T, TF300TG, TF300TL and TF700T pad and dock, as well as TF101 dock
> and TF600T, P1801-T and TF701T pad. This is a glue driver handling
> detection and common operations for EC's functions.
> 
> Co-developed-by: Svyatoslav Ryhel <clamor95@gmail.com>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> ---
>  drivers/mfd/Kconfig                     |  14 +
>  drivers/mfd/Makefile                    |   1 +
>  drivers/mfd/asus-transformer-ec.c       | 762 ++++++++++++++++++++++++
>  include/linux/mfd/asus-transformer-ec.h | 162 +++++
>  4 files changed, 939 insertions(+)
>  create mode 100644 drivers/mfd/asus-transformer-ec.c
>  create mode 100644 include/linux/mfd/asus-transformer-ec.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 7192c9d1d268..5aa4facfd2df 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -137,6 +137,20 @@ config MFD_AAT2870_CORE
>  	  additional drivers must be enabled in order to use the
>  	  functionality of the device.
>  
> +config MFD_ASUS_TRANSFORMER_EC
> +	tristate "ASUS Transformer's embedded controller"
> +	depends on I2C && OF
> +	help
> +	  Support ECs found in ASUS Transformer's Pad and Mobile Dock.
> +
> +	  This provides shared glue for functional part drivers:
> +	    asus-transformer-ec-kbc, asus-transformer-ec-keys,
> +	    leds-asus-transformer-ec, asus-transformer-ec-battery
> +	    and asus-transformer-ec-charger.

A 760 line driver deserves more than just a list of leaf drivers.

> +	  This driver can also be built as a module. If so, the module
> +	  will be called asus-transformer-ec.
> +
>  config MFD_AT91_USART
>  	tristate "AT91 USART Driver"
>  	select MFD_CORE
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index e75e8045c28a..fd80088d8a9a 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -10,6 +10,7 @@ obj-$(CONFIG_MFD_88PM805)	+= 88pm805.o 88pm80x.o
>  obj-$(CONFIG_MFD_88PM886_PMIC)	+= 88pm886.o
>  obj-$(CONFIG_MFD_ACT8945A)	+= act8945a.o
>  obj-$(CONFIG_MFD_SM501)		+= sm501.o
> +obj-$(CONFIG_MFD_ASUS_TRANSFORMER_EC)	+= asus-transformer-ec.o
>  obj-$(CONFIG_ARCH_BCM2835)	+= bcm2835-pm.o
>  obj-$(CONFIG_MFD_BCM590XX)	+= bcm590xx.o
>  obj-$(CONFIG_MFD_BD9571MWV)	+= bd9571mwv.o
> diff --git a/drivers/mfd/asus-transformer-ec.c b/drivers/mfd/asus-transformer-ec.c
> new file mode 100644
> index 000000000000..75aa7ab99387
> --- /dev/null
> +++ b/drivers/mfd/asus-transformer-ec.c
> @@ -0,0 +1,762 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +#include <linux/array_size.h>
> +#include <linux/debugfs.h>
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/device.h>

Should we sort the '#include' directives alphabetically? For instance,
'device.h' should typically appear before 'gpio/consumer.h'.

> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/mfd/asus-transformer-ec.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/property.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +#include <linux/unaligned.h>
> +
> +#define ASUSEC_RSP_BUFFER_SIZE		8
> +
> +struct asus_ec_chip_data {
> +	const char *name;
> +	const struct mfd_cell *mfd_devices;

Use global `static const structs` instead.

Also, please do not pass MFD registration data through another
registration API like DT.  It opens the gates to too much hackery.  I'm
not saying that _this_ driver would do such a thing, but it's easier
just to keep the blanket rule in place.

> +	unsigned int num_devices;
> +	bool clr_fmode; /* clear Factory Mode bit in EC control register */
> +};
> +
> +struct asus_ec_data {
> +	struct asusec_info info;

You have 'data' and 'info' which a) using non-forthcoming nomenclature
and doesn't tell me anything and then you b) put 'info' in the device's
driver_data attribute which is very confusing.  driver_data should be
for what we call ddata which I assume is expressed as 'data' here.

> +	struct mutex ecreq_lock; /* prevent simultaneous access */
> +	struct gpio_desc *ecreq;

If I hadn't seen the declaration, I'd have no idea this was a GPIO
descriptor.  Please improve the nomenclature throughout.

> +	struct i2c_client *self;

Again, please use standard naming conventions:

% git grep "struct i2c_client" | grep "\*self" | wc -l
0

% git grep "struct i2c_client" | grep "\*client" | wc -l
6304

% git grep "struct i2c_client" | grep "\*i2c" | wc -l
903

> +	const struct asus_ec_chip_data *data;

'data', 'priv' and 'info' should be improved.

> +	char ec_data[DOCKRAM_ENTRY_BUFSIZE];

An array of chars called 'data'.  This could be anything.

> +	bool logging_disabled;

This debugging tool is probably never going to be used again.

Keep it local.

> +};
> +
> +struct dockram_ec_data {
> +	struct mutex ctl_lock; /* prevent simultaneous access */
> +	char ctl_data[DOCKRAM_ENTRY_BUFSIZE];
> +};
> +
> +#define to_ec_data(ec) \
> +	container_of(ec, struct asus_ec_data, info)
> +
> +/**
> + * asus_dockram_read - Read a register from the DockRAM device.
> + * @client: Handle to the DockRAM device.
> + * @reg: Register to read.
> + * @buf: Byte array into which data will be read; must be large enough to
> + *	 hold the data returned by the DockRAM.
> + *
> + * This executes the DockRAM read based on the SMBus "block read" protocol
> + * or its emulation. It extracts DOCKRAM_ENTRY_SIZE bytes from the set
> + * register address.
> + *
> + * Returns a negative errno code else zero on success.
> + */
> +int asus_dockram_read(struct i2c_client *client, int reg, char *buf)
> +{

Have you considered using Regmap for register access instead of
implementing custom functions?  Remaps already deals with caching and
locking mechanisms that you'd get for free.

This looks like it would be replaced with devm_regmap_init_i2c().

> +	struct device *dev = &client->dev;
> +	int ret;
> +
> +	memset(buf, 0, DOCKRAM_ENTRY_BUFSIZE);
> +	ret = i2c_smbus_read_i2c_block_data(client, reg,
> +					    DOCKRAM_ENTRY_BUFSIZE, buf);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (buf[0] > DOCKRAM_ENTRY_SIZE) {
> +		dev_err(dev, "bad data len; buffer: %*ph; ret: %d\n",
> +			DOCKRAM_ENTRY_BUFSIZE, buf, ret);
> +		return -EPROTO;
> +	}
> +
> +	dev_dbg(dev, "got data; buffer: %*ph; ret: %d\n",
> +		DOCKRAM_ENTRY_BUFSIZE, buf, ret);

Please remove all of these debug messages.

> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(asus_dockram_read);
> +
> +/**
> + * asus_dockram_write - Write a byte array to a register of the DockRAM device.
> + * @client: Handle to the DockRAM device.
> + * @reg: Register to write to.
> + * @buf: Byte array to be written (up to DOCKRAM_ENTRY_SIZE bytes).
> + *
> + * This executes the DockRAM write based on the SMBus "block write"
> + * protocol or its emulation. It writes DOCKRAM_ENTRY_SIZE bytes to the
> + * specified register address.
> + *
> + * Returns a negative errno code else zero on success.
> + */
> +int asus_dockram_write(struct i2c_client *client, int reg, const char *buf)
> +{
> +	if (buf[0] > DOCKRAM_ENTRY_SIZE)
> +		return -EINVAL;
> +
> +	dev_dbg(&client->dev, "sending data; buffer: %*ph\n", buf[0] + 1, buf);
> +
> +	return i2c_smbus_write_i2c_block_data(client, reg, buf[0] + 1, buf);
> +}
> +EXPORT_SYMBOL_GPL(asus_dockram_write);
> +
> +/**
> + * asus_dockram_access_ctl - Read from or write to the DockRAM control register.
> + * @client: Handle to the DockRAM device.
> + * @out: Pointer to a variable where the register value will be stored.
> + * @mask: Bitmask of bits to be cleared.
> + * @xor: Bitmask of bits to be set (via XOR).
> + *
> + * This performs a control register read if @out is provided and both @mask
> + * and @xor are zero. Otherwise, it performs a control register update if
> + * @mask and @xor are provided.
> + *
> + * Returns a negative errno code else zero on success.
> + */
> +int asus_dockram_access_ctl(struct i2c_client *client, u64 *out, u64 mask,
> +			    u64 xor)
> +{
> +	struct dockram_ec_data *priv = i2c_get_clientdata(client);
> +	char *buf = priv->ctl_data;
> +	u64 val;
> +	int ret = 0;
> +
> +	guard(mutex)(&priv->ctl_lock);
> +
> +	ret = asus_dockram_read(client, ASUSEC_DOCKRAM_CONTROL, buf);
> +	if (ret < 0)

Could we check for errors using 'if (ret)' instead of 'if (ret < 0)' here,
unless a positive return value has a specific meaning we need to handle?

> +		goto exit;
> +
> +	if (buf[0] != ASUSEC_CTL_SIZE) {
> +		ret = -EPROTO;
> +		goto exit;
> +	}
> +
> +	val = get_unaligned_le64(buf + 1);
> +
> +	if (out)
> +		*out = val;
> +
> +	if (mask || xor) {
> +		put_unaligned_le64((val & ~mask) ^ xor, buf + 1);
> +		ret = asus_dockram_write(client, ASUSEC_DOCKRAM_CONTROL, buf);
> +	}
> +
> +exit:
> +	if (ret < 0)
> +		dev_err(&client->dev, "Failed to access control flags: %d\n",
> +			ret);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(asus_dockram_access_ctl);
> +
> +static void asus_ec_remove_notifier(struct device *dev, void *res)
> +{
> +	struct asusec_info *ec = dev_get_drvdata(dev->parent);
> +	struct notifier_block **nb = res;
> +
> +	blocking_notifier_chain_unregister(&ec->notify_list, *nb);
> +}
> +
> +/**
> + * devm_asus_ec_register_notifier - Managed registration of notifier to an
> + *				    ASUS EC blocking notifier chain.
> + * @pdev: Device requesting the notifier (used for resource management).
> + * @nb: Notifier block to be registered.
> + *
> + * Register a notifier to the ASUS EC blocking notifier chain. The notifier
> + * will be automatically unregistered when the requesting device is detached.
> + *
> + * Return: 0 on success or a negative error code on failure.
> + */
> +int devm_asus_ec_register_notifier(struct platform_device *pdev,
> +				   struct notifier_block *nb)
> +{

Hand-rolling devres managed resources is usually reserved for subsystem
level API calls.  Why do the usual device driver .remove() handling work
for you?

> +	struct asusec_info *ec = dev_get_drvdata(pdev->dev.parent);
> +	struct notifier_block **res;
> +	int ret;
> +
> +	res = devres_alloc(asus_ec_remove_notifier, sizeof(*res), GFP_KERNEL);
> +	if (!res)
> +		return -ENOMEM;
> +
> +	*res = nb;
> +	ret = blocking_notifier_chain_register(&ec->notify_list, nb);
> +	if (ret) {
> +		devres_free(res);
> +		return ret;
> +	}
> +
> +	devres_add(&pdev->dev, res);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(devm_asus_ec_register_notifier);
> +
> +static int asus_ec_signal_request(const struct asusec_info *ec)
> +{
> +	struct asus_ec_data *priv = to_ec_data(ec);
> +
> +	guard(mutex)(&priv->ecreq_lock);
> +
> +	dev_dbg(&priv->self->dev, "EC request\n");
> +
> +	gpiod_set_value_cansleep(priv->ecreq, 1);
> +	msleep(50);
> +
> +	gpiod_set_value_cansleep(priv->ecreq, 0);
> +	msleep(200);
> +
> +	return 0;
> +}
> +
> +static int asus_ec_write(struct asus_ec_data *priv, u16 data)
> +{
> +	int ret = i2c_smbus_write_word_data(priv->self, ASUSEC_WRITE_BUF, data);
> +
> +	dev_dbg(&priv->self->dev, "EC write: %04x, ret = %d\n", data, ret);
> +
> +	return ret;
> +}
> +
> +static int asus_ec_read(struct asus_ec_data *priv, bool in_irq)
> +{
> +	int ret = i2c_smbus_read_i2c_block_data(priv->self, ASUSEC_READ_BUF,
> +						sizeof(priv->ec_data),
> +						priv->ec_data);
> +
> +	dev_dbg(&priv->self->dev, "EC read: %*ph, ret = %d%s\n",
> +		sizeof(priv->ec_data), priv->ec_data,
> +		ret, in_irq ? "; in irq" : "");
> +
> +	return ret;
> +}

No abstractions for the sake of it.  All this goes away with Regmap anyway.
> +
> +/**
> + * asus_ec_i2c_command - Send a 16-bit command to the ASUS EC.
> + * @ec: Pointer to the shared ASUS EC structure.
> + * @data: The 16-bit command (word) to be sent.
> + *
> + * Return: 0 on success or a negative error code on failure.
> + */
> +int asus_ec_i2c_command(const struct asusec_info *ec, u16 data)
> +{
> +	return asus_ec_write(to_ec_data(ec), data);
> +}

Is this wrapper function strictly necessary? We should try to avoid
superfluous abstractions that do nothing but call another function.

> +EXPORT_SYMBOL_GPL(asus_ec_i2c_command);
> +
> +static void asus_ec_clear_buffer(struct asus_ec_data *priv)
> +{
> +	int retry = ASUSEC_RSP_BUFFER_SIZE;
> +
> +	while (retry--) {
> +		if (asus_ec_read(priv, false) < 0)
> +			continue;
> +
> +		if (priv->ec_data[1] & ASUSEC_OBF_MASK)
> +			continue;
> +
> +		break;
> +	}
> +}
> +
> +static int asus_ec_log_info(struct asus_ec_data *priv, unsigned int reg,
> +			    const char *name, char **out)
> +{
> +	char buf[DOCKRAM_ENTRY_BUFSIZE];
> +	int ret;
> +
> +	ret = asus_dockram_read(priv->info.dockram, reg, buf);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (!priv->logging_disabled)
> +		dev_info(&priv->self->dev, "%-14s: %.*s\n", name,
> +			 buf[0], buf + 1);
> +
> +	if (out)
> +		*out = kstrndup(buf + 1, buf[0], GFP_KERNEL);
> +
> +	return 0;
> +}
> +
> +static int asus_ec_reset(struct asus_ec_data *priv)
> +{
> +	int retry, ret;
> +
> +	for (retry = 0; retry < 3; retry++) {
> +		ret = asus_ec_write(priv, 0);
> +		if (!ret)
> +			return 0;
> +
> +		msleep(300);
> +	}
> +
> +	return ret;
> +}
> +
> +static int asus_ec_magic_debug(struct asus_ec_data *priv)
> +{
> +	u64 flag;
> +	int ret;
> +
> +	ret = asus_ec_get_ctl(&priv->info, &flag);
> +	if (ret < 0)
> +		return ret;
> +
> +	flag &= ASUSEC_CTL_SUSB_MODE;
> +	dev_info(&priv->self->dev, "EC FW behaviour: %s\n",
> +		 flag ? "susb on when receive ec_req" :
> +		 "susb on when system wakeup");
> +
> +	return 0;
> +}
> +
> +static int asus_ec_set_factory_mode(struct asus_ec_data *priv, bool on)
> +{
> +	dev_info(&priv->self->dev, "Entering %s mode.\n", on ? "factory" :
> +		 "normal");
> +
> +	return asus_ec_update_ctl(&priv->info, ASUSEC_CTL_FACTORY_MODE,
> +				  on ? ASUSEC_CTL_FACTORY_MODE : 0);
> +}
> +
> +static int asus_ec_detect(struct asus_ec_data *priv)
> +{
> +	char *model = NULL;
> +	int ret;
> +
> +	ret = asus_ec_reset(priv);
> +	if (ret)
> +		goto err_exit;
> +
> +	asus_ec_clear_buffer(priv);
> +
> +	ret = asus_ec_log_info(priv, ASUSEC_DOCKRAM_INFO_MODEL, "model", &model);
> +	if (ret)
> +		goto err_exit;
> +
> +	ret = asus_ec_log_info(priv, ASUSEC_DOCKRAM_INFO_FW, "FW version", NULL);
> +	if (ret)
> +		goto err_exit;
> +
> +	ret = asus_ec_log_info(priv, ASUSEC_DOCKRAM_INFO_CFGFMT, "Config format", NULL);
> +	if (ret)
> +		goto err_exit;
> +
> +	ret = asus_ec_log_info(priv, ASUSEC_DOCKRAM_INFO_HW, "HW version", NULL);
> +	if (ret)
> +		goto err_exit;
> +
> +	priv->logging_disabled = true;
> +
> +	ret = asus_ec_magic_debug(priv);
> +	if (ret)
> +		goto err_exit;
> +
> +	priv->info.model = model;
> +	priv->info.name = priv->data->name;
> +
> +	if (priv->data->clr_fmode)
> +		asus_ec_set_factory_mode(priv, false);
> +
> +err_exit:
> +	if (ret)
> +		dev_err(&priv->self->dev, "failed to access EC: %d\n", ret);
> +
> +	return ret;
> +}
> +
> +static void asus_ec_handle_smi(struct asus_ec_data *priv, unsigned int code)
> +{
> +	dev_dbg(&priv->self->dev, "SMI interrupt: 0x%02x\n", code);
> +
> +	switch (code) {
> +	case ASUSEC_SMI_HANDSHAKE:
> +	case ASUSEC_SMI_RESET:
> +		asus_ec_detect(priv);
> +		break;
> +	}
> +}
> +
> +static irqreturn_t asus_ec_interrupt(int irq, void *dev_id)
> +{
> +	struct asus_ec_data *priv = dev_id;
> +	unsigned long notify_action;
> +	int ret;
> +
> +	ret = asus_ec_read(priv, true);
> +	if (ret <= 0 || !(priv->ec_data[1] & ASUSEC_OBF_MASK))
> +		return IRQ_NONE;
> +
> +	notify_action = priv->ec_data[1];
> +	if (notify_action & ASUSEC_SMI_MASK) {
> +		unsigned int code = priv->ec_data[2];
> +
> +		asus_ec_handle_smi(priv, code);
> +
> +		notify_action |= code << 8;
> +		dev_dbg(&priv->self->dev, "SMI code: 0x%02x\n", code);
> +	}
> +
> +	blocking_notifier_call_chain(&priv->info.notify_list,
> +				     notify_action, priv->ec_data);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static ssize_t dockram_read(struct file *filp, char __user *buf,
> +			    size_t count, loff_t *ppos)
> +{
> +	struct i2c_client *client = filp->private_data;
> +	unsigned int reg, rsize;
> +	ssize_t n_read = 0, val;
> +	loff_t off = *ppos;
> +	char *data;
> +	int ret;
> +
> +	reg = off / DOCKRAM_ENTRY_SIZE;
> +	off %= DOCKRAM_ENTRY_SIZE;
> +	rsize = DOCKRAM_ENTRIES * DOCKRAM_ENTRY_SIZE;
> +
> +	if (!count)
> +		return 0;
> +
> +	data = kmalloc(DOCKRAM_ENTRY_BUFSIZE, GFP_KERNEL);
> +
> +	while (reg < DOCKRAM_ENTRIES) {
> +		unsigned int len = DOCKRAM_ENTRY_SIZE - off;
> +
> +		if (len > rsize)
> +			len = rsize;
> +
> +		ret = asus_dockram_read(client, reg, data);
> +		if (ret < 0) {
> +			if (!n_read)
> +				n_read = ret;
> +			break;
> +		}
> +
> +		val = copy_to_user(buf, data + 1 + off, len);
> +		if (val == len)
> +			return -EFAULT;
> +
> +		*ppos += len;
> +		n_read += len;
> +
> +		if (len == rsize)
> +			break;
> +
> +		rsize -= len;
> +		buf += len;
> +		off = 0;
> +		++reg;
> +	}
> +
> +	kfree(data);
> +
> +	return n_read;
> +}
> +
> +static int dockram_write_one(struct i2c_client *client, int reg,
> +			     const char __user *buf, size_t count)
> +{
> +	struct dockram_ec_data *priv = i2c_get_clientdata(client);
> +	int ret;
> +
> +	if (!count || count > DOCKRAM_ENTRY_SIZE)
> +		return -EINVAL;
> +	if (buf[0] != count - 1)
> +		return -EINVAL;
> +
> +	guard(mutex)(&priv->ctl_lock);
> +
> +	priv->ctl_data[0] = (u8)count;
> +	memcpy(priv->ctl_data + 1, buf, count);
> +	ret = asus_dockram_write(client, reg, priv->ctl_data);
> +
> +	return ret;
> +}
> +
> +static ssize_t dockram_write(struct file *filp, const char __user *buf,
> +			     size_t count, loff_t *ppos)
> +{
> +	struct i2c_client *client = filp->private_data;
> +	unsigned int reg;
> +	loff_t off = *ppos;
> +	int ret;
> +
> +	if (off % DOCKRAM_ENTRY_SIZE != 0)
> +		return -EINVAL;
> +
> +	reg = off / DOCKRAM_ENTRY_SIZE;
> +	if (reg >= DOCKRAM_ENTRIES)
> +		return -EINVAL;
> +
> +	ret = dockram_write_one(client, reg, buf, count);
> +
> +	return ret < 0 ? ret : count;
> +}
> +
> +static const struct debugfs_short_fops dockram_fops = {
> +	.read	= dockram_read,
> +	.write	= dockram_write,
> +	.llseek	= default_llseek,
> +};

You should not be giving userspace free reign over device memory.

If you switch to Regmap, you can use regmap-debugfs.c for this.

> +static int control_reg_get(void *ec, u64 *val)
> +{
> +	return asus_ec_get_ctl(ec, val);
> +}
> +
> +static int control_reg_set(void *ec, u64 val)
> +{
> +	return asus_ec_update_ctl(ec, ~0ull, val);
> +}
> +
> +DEFINE_DEBUGFS_ATTRIBUTE(control_reg_fops, control_reg_get,
> +			 control_reg_set, "%016llx\n");
> +
> +static int ec_request_set(void *ec, u64 val)
> +{
> +	if (val)
> +		asus_ec_signal_request(ec);
> +
> +	return 0;
> +}
> +
> +DEFINE_DEBUGFS_ATTRIBUTE(ec_request_fops, NULL, ec_request_set, "%llu\n");
> +
> +static int ec_irq_set(void *ec, u64 val)
> +{
> +	struct asus_ec_data *priv = to_ec_data(ec);
> +
> +	if (val)
> +		irq_wake_thread(priv->self->irq, priv);
> +
> +	return 0;
> +}
> +
> +DEFINE_DEBUGFS_ATTRIBUTE(ec_irq_fops, NULL, ec_irq_set, "%llu\n");
> +
> +static void asus_ec_debugfs_remove(void *debugfs_root)
> +{
> +	debugfs_remove_recursive(debugfs_root);
> +}
> +
> +static void devm_asus_ec_debugfs_init(struct device *dev)
> +{
> +	struct asusec_info *ec = dev_get_drvdata(dev);
> +	struct asus_ec_data *priv = to_ec_data(ec);
> +	struct dentry *debugfs_root, *dockram_dir;
> +	char *name = devm_kasprintf(dev, GFP_KERNEL, "asus-ec-%s",
> +				    priv->data->name);
> +
> +	debugfs_root = debugfs_create_dir(name, NULL);
> +	dockram_dir = debugfs_create_dir("dockram", debugfs_root);
> +
> +	debugfs_create_file("ec_irq", 0200, debugfs_root, ec,
> +			    &ec_irq_fops);
> +	debugfs_create_file("ec_request", 0200, debugfs_root, ec,
> +			    &ec_request_fops);
> +	debugfs_create_file("control_reg", 0644, dockram_dir, ec,
> +			    &control_reg_fops);
> +	debugfs_create_file("dockram", 0644, dockram_dir,
> +			    priv->info.dockram, &dockram_fops);
> +
> +	devm_add_action_or_reset(dev, asus_ec_debugfs_remove, debugfs_root);
> +}

Why is this being controlled via debugfs?

Use the correct kernel APIs instead.

If this is just for development, keep it and all of the extra verbose
printing in the BSP tree.  It does not belong in the upstream kernel.

> +static void asus_ec_release_dockram_dev(void *client)
> +{
> +	i2c_unregister_device(client);
> +}
> +
> +static struct i2c_client *devm_asus_dockram_get(struct device *dev)
> +{
> +	struct i2c_client *parent = to_i2c_client(dev);
> +	struct i2c_client *dockram;
> +	struct dockram_ec_data *priv;
> +	int ret;
> +
> +	dockram = i2c_new_ancillary_device(parent, "dockram",
> +					   parent->addr + 2);
> +	if (IS_ERR(dockram))
> +		return dockram;
> +
> +	ret = devm_add_action_or_reset(dev, asus_ec_release_dockram_dev,
> +				       dockram);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	priv = devm_kzalloc(&dockram->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return ERR_PTR(-ENOMEM);
> +
> +	i2c_set_clientdata(dockram, priv);
> +	mutex_init(&priv->ctl_lock);
> +
> +	return dockram;
> +}
> +
> +static int asus_ec_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct asus_ec_data *priv;

Could we use a clearer, more specific name for the private data variable
instead of the generic term 'priv'? Using something like 'ddata' is usually
preferred.

> +	int ret;
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_I2C_BLOCK))
> +		return dev_err_probe(dev, -ENXIO,
> +			"I2C bus is missing required SMBus block mode support\n");
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->data = device_get_match_data(dev);
> +	if (!priv->data)
> +		return -ENODEV;
> +
> +	i2c_set_clientdata(client, priv);
> +	priv->self = client;
> +
> +	priv->info.dockram = devm_asus_dockram_get(dev);
> +	if (IS_ERR(priv->info.dockram))
> +		return dev_err_probe(dev, PTR_ERR(priv->info.dockram),
> +				     "failed to get dockram\n");
> +
> +	priv->ecreq = devm_gpiod_get(dev, "request", GPIOD_OUT_LOW);
> +	if (IS_ERR(priv->ecreq))
> +		return dev_err_probe(dev, PTR_ERR(priv->ecreq),
> +				     "failed to get request GPIO\n");

"get" or "request"

> +	BLOCKING_INIT_NOTIFIER_HEAD(&priv->info.notify_list);
> +	mutex_init(&priv->ecreq_lock);
> +
> +	asus_ec_signal_request(&priv->info);
> +
> +	ret = asus_ec_detect(priv);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to detect EC version\n");
> +
> +	ret = devm_request_threaded_irq(dev, client->irq, NULL,
> +					&asus_ec_interrupt,
> +					IRQF_ONESHOT | IRQF_SHARED,
> +					client->name, priv);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to register IRQ\n");
> +
> +	/* Parent I2C controller uses DMA, ASUS EC and child devices do not */
> +	client->dev.coherent_dma_mask = 0;
> +	client->dev.dma_mask = &client->dev.coherent_dma_mask;
> +
> +	if (IS_ENABLED(CONFIG_DEBUG_FS))
> +		devm_asus_ec_debugfs_init(dev);
> +
> +	return devm_mfd_add_devices(dev, 0, priv->data->mfd_devices,
> +				    priv->data->num_devices, NULL, 0, NULL);
> +}
> +
> +static const struct mfd_cell asus_ec_sl101_dock_mfd_devices[] = {
> +	{
> +		.name = "asus-transformer-ec-kbc",
> +	},
> +};
> +
> +static const struct asus_ec_chip_data asus_ec_sl101_dock_data = {
> +	.name = "dock",
> +	.mfd_devices = asus_ec_sl101_dock_mfd_devices,
> +	.num_devices = ARRAY_SIZE(asus_ec_sl101_dock_mfd_devices),
> +	.clr_fmode = false,
> +};
> +
> +static const struct mfd_cell asus_ec_tf101_dock_mfd_devices[] = {
> +	{
> +		.name = "asus-transformer-ec-battery",
> +		.id = 1,

Why doesn't PLATFORM_DEVID_AUTO already do the right thing?

> +	}, {
> +		.name = "asus-transformer-ec-charger",
> +		.id = 1,
> +	}, {
> +		.name = "asus-transformer-ec-led",
> +		.id = 1,
> +	}, {
> +		.name = "asus-transformer-ec-keys",
> +	}, {
> +		.name = "asus-transformer-ec-kbc",
> +	},
> +};
> +
> +static const struct asus_ec_chip_data asus_ec_tf101_dock_data = {
> +	.name = "dock",
> +	.mfd_devices = asus_ec_tf101_dock_mfd_devices,
> +	.num_devices = ARRAY_SIZE(asus_ec_tf101_dock_mfd_devices),
> +	.clr_fmode = false,
> +};
> +
> +static const struct mfd_cell asus_ec_tf201_pad_mfd_devices[] = {
> +	{
> +		.name = "asus-transformer-ec-battery",
> +		.id = 0,
> +	}, {
> +		.name = "asus-transformer-ec-led",
> +		.id = 0,
> +	},
> +};
> +
> +static const struct asus_ec_chip_data asus_ec_tf201_pad_data = {
> +	.name = "pad",
> +	.mfd_devices = asus_ec_tf201_pad_mfd_devices,
> +	.num_devices = ARRAY_SIZE(asus_ec_tf201_pad_mfd_devices),
> +	.clr_fmode = true,
> +};
> +
> +static const struct mfd_cell asus_ec_tf600t_pad_mfd_devices[] = {
> +	{
> +		.name = "asus-transformer-ec-battery",
> +		.id = 0,
> +	}, {
> +		.name = "asus-transformer-ec-charger",
> +		.id = 0,
> +	}, {
> +		.name = "asus-transformer-ec-led",
> +		.id = 0,
> +	},
> +};
> +
> +static const struct asus_ec_chip_data asus_ec_tf600t_pad_data = {
> +	.name = "pad",
> +	.mfd_devices = asus_ec_tf600t_pad_mfd_devices,
> +	.num_devices = ARRAY_SIZE(asus_ec_tf600t_pad_mfd_devices),
> +	.clr_fmode = true,
> +};
> +
> +static const struct of_device_id asus_ec_match[] = {
> +	{ .compatible = "asus,sl101-ec-dock", .data = &asus_ec_sl101_dock_data },
> +	{ .compatible = "asus,tf101-ec-dock", .data = &asus_ec_tf101_dock_data },

Could we use the match table's data field to store an 'enum' or integer ID
instead of passing platform data via the '.data' field? We could then use a
'switch' statement in the C code to select the correct 'mfd_cell' array.

> +	{ .compatible = "asus,tf201-ec-pad", .data = &asus_ec_tf201_pad_data },
> +	{ .compatible = "asus,tf600t-ec-pad", .data = &asus_ec_tf600t_pad_data },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, asus_ec_match);
> +
> +static struct i2c_driver asus_ec_driver = {
> +	.driver	= {
> +		.name = "asus-transformer-ec",
> +		.of_match_table = asus_ec_match,
> +	},
> +	.probe = asus_ec_probe,
> +};
> +module_i2c_driver(asus_ec_driver);
> +
> +MODULE_AUTHOR("Michał Mirosław <mirq-linux@rere.qmqm.pl>");
> +MODULE_AUTHOR("Svyatoslav Ryhel <clamor95@gmail.com>");
> +MODULE_DESCRIPTION("ASUS Transformer's EC driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/asus-transformer-ec.h b/include/linux/mfd/asus-transformer-ec.h
> new file mode 100644
> index 000000000000..0a72de40352e
> --- /dev/null
> +++ b/include/linux/mfd/asus-transformer-ec.h
> @@ -0,0 +1,162 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef __MFD_ASUS_TRANSFORMER_EC_H
> +#define __MFD_ASUS_TRANSFORMER_EC_H
> +
> +#include <linux/notifier.h>
> +#include <linux/platform_device.h>
> +#include <linux/workqueue.h>
> +
> +struct i2c_client;
> +
> +struct asusec_info {
> +	const char *model;
> +	const char *name;
> +	struct i2c_client *dockram;
> +	struct workqueue_struct *wq;
> +	struct blocking_notifier_head notify_list;
> +};
> +
> +#define DOCKRAM_ENTRIES			0x100
> +#define DOCKRAM_ENTRY_SIZE		32
> +#define DOCKRAM_ENTRY_BUFSIZE		(DOCKRAM_ENTRY_SIZE + 1)
> +
> +/* interrupt sources */
> +#define ASUSEC_OBF_MASK			BIT(0)
> +#define ASUSEC_KEY_MASK			BIT(2)
> +#define ASUSEC_KBC_MASK			BIT(3)
> +#define ASUSEC_AUX_MASK			BIT(5)
> +#define ASUSEC_SCI_MASK			BIT(6)
> +#define ASUSEC_SMI_MASK			BIT(7)
> +
> +/* SMI notification codes */
> +#define ASUSEC_SMI_POWER_NOTIFY		0x31	/* [un]plugging USB cable */
> +#define ASUSEC_SMI_HANDSHAKE		0x50	/* response to ec_req edge */
> +#define ASUSEC_SMI_WAKE			0x53
> +#define ASUSEC_SMI_RESET		0x5f
> +#define ASUSEC_SMI_ADAPTER_EVENT	0x60	/* [un]plugging charger to dock */
> +#define ASUSEC_SMI_BACKLIGHT_ON		0x63
> +#define ASUSEC_SMI_AUDIO_DOCK_IN	0x70
> +
> +#define ASUSEC_SMI_ACTION(code)		(ASUSEC_SMI_MASK | ASUSEC_OBF_MASK | \
> +					(ASUSEC_SMI_##code << 8))
> +
> +/* control register [0x0a] layout */
> +#define ASUSEC_CTL_SIZE			8
> +
> +/*
> + * EC reports power from 40-pin connector in the LSB of the control
> + * register.  The following values have been observed (xor 0x02):
> + *
> + * PAD-ec no-plug  0x40 / PAD-ec DOCK     0x20 / DOCK-ec no-plug 0x40
> + * PAD-ec AC       0x25 / PAD-ec DOCK+AC  0x24 / DOCK-ec AC      0x25
> + * PAD-ec USB      0x45 / PAD-ec DOCK+USB 0x24 / DOCK-ec USB     0x41
> + */
> +
> +#define ASUSEC_CTL_DIRECT_POWER_SOURCE	BIT_ULL(0)
> +#define ASUSEC_STAT_CHARGING		BIT_ULL(2)
> +#define ASUSEC_CTL_FULL_POWER_SOURCE	BIT_ULL(5)
> +#define ASUSEC_CTL_SUSB_MODE		BIT_ULL(9)
> +#define ASUSEC_CMD_SUSPEND_S3		BIT_ULL(33)
> +#define ASUSEC_CTL_TEST_DISCHARGE	BIT_ULL(35)
> +#define ASUSEC_CMD_SUSPEND_INHIBIT	BIT_ULL(37)
> +#define ASUSEC_CTL_FACTORY_MODE		BIT_ULL(38)
> +#define ASUSEC_CTL_KEEP_AWAKE		BIT_ULL(39)
> +#define ASUSEC_CTL_USB_CHARGE		BIT_ULL(40)
> +#define ASUSEC_CTL_LED_BLINK		BIT_ULL(40)
> +#define ASUSEC_CTL_LED_AMBER		BIT_ULL(41)
> +#define ASUSEC_CTL_LED_GREEN		BIT_ULL(42)
> +#define ASUSEC_CMD_SWITCH_HDMI		BIT_ULL(56)
> +#define ASUSEC_CMD_WIN_SHUTDOWN		BIT_ULL(62)
> +
> +#define ASUSEC_DOCKRAM_INFO_MODEL	0x01
> +#define ASUSEC_DOCKRAM_INFO_FW		0x02
> +#define ASUSEC_DOCKRAM_INFO_CFGFMT	0x03
> +#define ASUSEC_DOCKRAM_INFO_HW		0x04
> +#define ASUSEC_DOCKRAM_CONTROL		0x0a
> +#define ASUSEC_DOCKRAM_BATT_CTL		0x14
> +
> +#define ASUSEC_WRITE_BUF		0x64
> +#define ASUSEC_READ_BUF			0x6a
> +
> +/* dockram comm */
> +int asus_dockram_read(struct i2c_client *client, int reg, char *buf);
> +int asus_dockram_write(struct i2c_client *client, int reg, const char *buf);
> +int asus_dockram_access_ctl(struct i2c_client *client,
> +			    u64 *out, u64 mask, u64 xor);
> +
> +/* EC public API */
> +
> +/**
> + * cell_to_ec - Request the shared ASUS EC structure via a subdevice's pdev.
> + * @pdev: EC subdevice pdev requesting access to the shared ASUS EC structure.
> + *
> + * Returns a pointer to the asusec_info structure.
> + */
> +static inline struct asusec_info *cell_to_ec(struct platform_device *pdev)
> +{
> +	return dev_get_drvdata(pdev->dev.parent);
> +}

This is both misleading and already exists.

> +
> +/**
> + * asus_ec_get_ctl - Read from the DockRAM control register.
> + * @ec:  Pointer to the shared ASUS EC structure.
> + * @out: Pointer to the variable where the register value will be stored.
> + *
> + * Performs a control register read and stores the value in @out.
> + *
> + * Return: 0 on success, or a negative errno code on failure.
> + */
> +static inline int asus_ec_get_ctl(const struct asusec_info *ec, u64 *out)
> +{
> +	return asus_dockram_access_ctl(ec->dockram, out, 0, 0);
> +}
> +
> +/**
> + * asus_ec_update_ctl - Update the DockRAM control register.
> + * @ec:   Pointer to the shared ASUS EC structure.
> + * @mask: Bitmask of bits to be cleared.
> + * @xor:  Bitmask of bits to be toggled or set (via XOR).
> + *
> + * Performs a read-modify-write update on the control register using
> + * the provided @mask and @xor values.
> + *
> + * Return: 0 on success, or a negative errno code on failure.
> + */
> +static inline int asus_ec_update_ctl(const struct asusec_info *ec,
> +				     u64 mask, u64 xor)
> +{
> +	return asus_dockram_access_ctl(ec->dockram, NULL, mask, xor);
> +}
> +
> +/**
> + * asus_ec_set_ctl_bits - Sets bits of the DockRAM control register.
> + * @ec:   Pointer to the shared ASUS EC structure.
> + * @mask: Bitmask of bits to be set.
> + *
> + * Sets bits of the control register using the provided @mask value.
> + *
> + * Return: 0 on success, or a negative errno code on failure.
> + */
> +static inline int asus_ec_set_ctl_bits(const struct asusec_info *ec, u64 mask)
> +{
> +	return asus_dockram_access_ctl(ec->dockram, NULL, mask, mask);
> +}
> +
> +/**
> + * asus_ec_clear_ctl_bits - Clears bits of the DockRAM control register.
> + * @ec:   Pointer to the shared ASUS EC structure.
> + * @mask: Bitmask of bits to be cleared.
> + *
> + * Clears bits of the control register using the provided @mask value.
> + *
> + * Return: 0 on success, or a negative errno code on failure.
> + */
> +static inline int asus_ec_clear_ctl_bits(const struct asusec_info *ec, u64 mask)
> +{
> +	return asus_dockram_access_ctl(ec->dockram, NULL, mask, 0);
> +}

We don't typically allow abstraction for the sake of abstraction.

> +int asus_ec_i2c_command(const struct asusec_info *ec, u16 data);
> +int devm_asus_ec_register_notifier(struct platform_device *dev,
> +				   struct notifier_block *nb);
> +#endif /* __MFD_ASUS_TRANSFORMER_EC_H */
> -- 
> 2.51.0
> 
> 

-- 
Lee Jones

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v6 5/7] leds: Add driver for ASUS Transformer LEDs
       [not found] ` <20260502124055.22475-6-clamor95@gmail.com>
@ 2026-05-14 10:09   ` Lee Jones
  0 siblings, 0 replies; 6+ messages in thread
From: Lee Jones @ 2026-05-14 10:09 UTC (permalink / raw)
  To: Svyatoslav Ryhel
  Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Dmitry Torokhov,
	Pavel Machek, Sebastian Reichel, Ion Agorria,
	Michał Mirosław, devicetree, linux-kernel, linux-input,
	linux-leds, linux-pm

On Sat, 02 May 2026, Svyatoslav Ryhel wrote:

> From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> 
> ASUS Transformer tablets have a green and an amber LED on both the Pad
> and the Dock. If both LEDs are enabled simultaneously, the emitted light
> will be yellow.
> 
> Co-developed-by: Svyatoslav Ryhel <clamor95@gmail.com>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> ---
>  drivers/leds/Kconfig                    | 11 ++++
>  drivers/leds/Makefile                   |  1 +
>  drivers/leds/leds-asus-transformer-ec.c | 79 +++++++++++++++++++++++++
>  3 files changed, 91 insertions(+)
>  create mode 100644 drivers/leds/leds-asus-transformer-ec.c
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index f4a0a3c8c870..f637d23400a8 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -120,6 +120,17 @@ config LEDS_OSRAM_AMS_AS3668
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called leds-as3668.
>  
> +config LEDS_ASUS_TRANSFORMER_EC
> +	tristate "LED Support for Asus Transformer charging LED"
> +	depends on LEDS_CLASS
> +	depends on MFD_ASUS_TRANSFORMER_EC
> +	help
> +	  This option enables support for charging indicator on
> +	  Asus Transformer's Pad and it's Dock.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called leds-asus-transformer-ec.
> +
>  config LEDS_AW200XX
>  	tristate "LED support for Awinic AW20036/AW20054/AW20072/AW20108"
>  	depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 8fdb45d5b439..d5395c3f1124 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -16,6 +16,7 @@ obj-$(CONFIG_LEDS_AN30259A)		+= leds-an30259a.o
>  obj-$(CONFIG_LEDS_APU)			+= leds-apu.o
>  obj-$(CONFIG_LEDS_ARIEL)		+= leds-ariel.o
>  obj-$(CONFIG_LEDS_AS3668)		+= leds-as3668.o
> +obj-$(CONFIG_LEDS_ASUS_TRANSFORMER_EC)	+= leds-asus-transformer-ec.o
>  obj-$(CONFIG_LEDS_AW200XX)		+= leds-aw200xx.o
>  obj-$(CONFIG_LEDS_AW2013)		+= leds-aw2013.o
>  obj-$(CONFIG_LEDS_BCM6328)		+= leds-bcm6328.o
> diff --git a/drivers/leds/leds-asus-transformer-ec.c b/drivers/leds/leds-asus-transformer-ec.c
> new file mode 100644
> index 000000000000..3186038e3be7
> --- /dev/null
> +++ b/drivers/leds/leds-asus-transformer-ec.c
> @@ -0,0 +1,79 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/err.h>
> +#include <linux/leds.h>
> +#include <linux/mfd/asus-transformer-ec.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +static void asus_ec_led_set_brightness_amber(struct led_classdev *led,
> +					     enum led_brightness brightness)
> +{
> +	const struct asusec_info *ec = dev_get_drvdata(led->dev->parent);
> +
> +	if (brightness)
> +		asus_ec_set_ctl_bits(ec, ASUSEC_CTL_LED_AMBER);

Why not Regmap?

> +	else
> +		asus_ec_clear_ctl_bits(ec, ASUSEC_CTL_LED_AMBER);
> +}
> +
> +static void asus_ec_led_set_brightness_green(struct led_classdev *led,
> +					     enum led_brightness brightness)
> +{
> +	const struct asusec_info *ec = dev_get_drvdata(led->dev->parent);
> +
> +	if (brightness)
> +		asus_ec_set_ctl_bits(ec, ASUSEC_CTL_LED_GREEN);
> +	else
> +		asus_ec_clear_ctl_bits(ec, ASUSEC_CTL_LED_GREEN);
> +}

These are identical bar one variable.


> +static int asus_ec_led_probe(struct platform_device *pdev)
> +{
> +	struct asusec_info *ec = cell_to_ec(pdev);
> +	struct device *dev = &pdev->dev;
> +	struct led_classdev *amber_led, *green_led;
> +	int ret;
> +
> +	platform_set_drvdata(pdev, ec);

This appears to be unused.

> +	amber_led = devm_kzalloc(dev, sizeof(*amber_led), GFP_KERNEL);
> +	if (!amber_led)
> +		return -ENOMEM;
> +
> +	amber_led->name = devm_kasprintf(dev, GFP_KERNEL, "%s::amber", ec->name);
> +	amber_led->max_brightness = 1;
> +	amber_led->flags = LED_CORE_SUSPENDRESUME | LED_RETAIN_AT_SHUTDOWN;
> +	amber_led->brightness_set = asus_ec_led_set_brightness_amber;
> +
> +	ret = devm_led_classdev_register(dev, amber_led);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to register amber LED\n");
> +
> +	green_led = devm_kzalloc(dev, sizeof(*green_led), GFP_KERNEL);
> +	if (!green_led)
> +		return -ENOMEM;
> +
> +	green_led->name = devm_kasprintf(dev, GFP_KERNEL, "%s::green", ec->name);
> +	green_led->max_brightness = 1;
> +	green_led->flags = LED_CORE_SUSPENDRESUME | LED_RETAIN_AT_SHUTDOWN;
> +	green_led->brightness_set = asus_ec_led_set_brightness_green;
> +
> +	ret = devm_led_classdev_register(dev, green_led);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to register green LED\n");

Imagine instead of 2 LEDs, you had 20.  Would you copy and paste this 20
times?  Please re-author this as though there were many more so it
remains efficient and scaleable.

> +	return 0;
> +}
> +
> +static struct platform_driver asus_ec_led_driver = {
> +	.driver.name = "asus-transformer-ec-led",
> +	.probe = asus_ec_led_probe,
> +};
> +module_platform_driver(asus_ec_led_driver);
> +
> +MODULE_AUTHOR("Michał Mirosław <mirq-linux@rere.qmqm.pl>");
> +MODULE_AUTHOR("Svyatoslav Ryhel <clamor95@gmail.com>");
> +MODULE_DESCRIPTION("ASUS Transformer's charging LED driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.51.0
> 
> 

-- 
Lee Jones

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v6 2/7] mfd: Add driver for ASUS Transformer embedded controller
  2026-05-14 10:02   ` [PATCH v6 2/7] mfd: Add driver for ASUS Transformer embedded controller Lee Jones
@ 2026-05-14 10:31     ` Svyatoslav Ryhel
  2026-05-14 15:50       ` Lee Jones
  2026-05-14 11:02     ` Svyatoslav Ryhel
  1 sibling, 1 reply; 6+ messages in thread
From: Svyatoslav Ryhel @ 2026-05-14 10:31 UTC (permalink / raw)
  To: Lee Jones
  Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Dmitry Torokhov,
	Pavel Machek, Sebastian Reichel, Ion Agorria,
	Michał Mirosław, devicetree, linux-kernel, linux-input,
	linux-leds, linux-pm

чт, 14 трав. 2026 р. о 13:02 Lee Jones <lee@kernel.org> пише:
>
> On Sat, 02 May 2026, Svyatoslav Ryhel wrote:
>
> > From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> >
> > Support Nuvoton NPCE795-based ECs as used in Asus Transformer TF201,
> > TF300T, TF300TG, TF300TL and TF700T pad and dock, as well as TF101 dock
> > and TF600T, P1801-T and TF701T pad. This is a glue driver handling
> > detection and common operations for EC's functions.
> >
> > Co-developed-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > ---
> >  drivers/mfd/Kconfig                     |  14 +
> >  drivers/mfd/Makefile                    |   1 +
> >  drivers/mfd/asus-transformer-ec.c       | 762 ++++++++++++++++++++++++
> >  include/linux/mfd/asus-transformer-ec.h | 162 +++++
> >  4 files changed, 939 insertions(+)
> >  create mode 100644 drivers/mfd/asus-transformer-ec.c
> >  create mode 100644 include/linux/mfd/asus-transformer-ec.h
> >
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index 7192c9d1d268..5aa4facfd2df 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -137,6 +137,20 @@ config MFD_AAT2870_CORE
> >         additional drivers must be enabled in order to use the
> >         functionality of the device.
> >
> > +config MFD_ASUS_TRANSFORMER_EC
> > +     tristate "ASUS Transformer's embedded controller"
> > +     depends on I2C && OF
> > +     help
> > +       Support ECs found in ASUS Transformer's Pad and Mobile Dock.
> > +
> > +       This provides shared glue for functional part drivers:
> > +         asus-transformer-ec-kbc, asus-transformer-ec-keys,
> > +         leds-asus-transformer-ec, asus-transformer-ec-battery
> > +         and asus-transformer-ec-charger.
>
> A 760 line driver deserves more than just a list of leaf drivers.
>

Ok, I will make some meaningful description.

> > +       This driver can also be built as a module. If so, the module
> > +       will be called asus-transformer-ec.
> > +
> >  config MFD_AT91_USART
> >       tristate "AT91 USART Driver"
> >       select MFD_CORE
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index e75e8045c28a..fd80088d8a9a 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -10,6 +10,7 @@ obj-$(CONFIG_MFD_88PM805)   += 88pm805.o 88pm80x.o
> >  obj-$(CONFIG_MFD_88PM886_PMIC)       += 88pm886.o
> >  obj-$(CONFIG_MFD_ACT8945A)   += act8945a.o
> >  obj-$(CONFIG_MFD_SM501)              += sm501.o
> > +obj-$(CONFIG_MFD_ASUS_TRANSFORMER_EC)        += asus-transformer-ec.o
> >  obj-$(CONFIG_ARCH_BCM2835)   += bcm2835-pm.o
> >  obj-$(CONFIG_MFD_BCM590XX)   += bcm590xx.o
> >  obj-$(CONFIG_MFD_BD9571MWV)  += bd9571mwv.o
> > diff --git a/drivers/mfd/asus-transformer-ec.c b/drivers/mfd/asus-transformer-ec.c
> > new file mode 100644
> > index 000000000000..75aa7ab99387
> > --- /dev/null
> > +++ b/drivers/mfd/asus-transformer-ec.c
> > @@ -0,0 +1,762 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +
> > +#include <linux/array_size.h>
> > +#include <linux/debugfs.h>
> > +#include <linux/delay.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/device.h>
>
> Should we sort the '#include' directives alphabetically? For instance,
> 'device.h' should typically appear before 'gpio/consumer.h'.
>

Yes, it is my bad, I will fix it.

> > +#include <linux/err.h>
> > +#include <linux/i2c.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/mfd/asus-transformer-ec.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/property.h>
> > +#include <linux/slab.h>
> > +#include <linux/string.h>
> > +#include <linux/types.h>
> > +#include <linux/unaligned.h>
> > +
> > +#define ASUSEC_RSP_BUFFER_SIZE               8
> > +
> > +struct asus_ec_chip_data {
> > +     const char *name;
> > +     const struct mfd_cell *mfd_devices;
>
> Use global `static const structs` instead.
>
> Also, please do not pass MFD registration data through another
> registration API like DT.  It opens the gates to too much hackery.  I'm
> not saying that _this_ driver would do such a thing, but it's easier
> just to keep the blanket rule in place.
>

Yes, you have already made it clear in my CPCAP patchset. I have it
applied for the v7 here.

> > +     unsigned int num_devices;
> > +     bool clr_fmode; /* clear Factory Mode bit in EC control register */
> > +};
> > +
> > +struct asus_ec_data {
> > +     struct asusec_info info;
>
> You have 'data' and 'info' which a) using non-forthcoming nomenclature
> and doesn't tell me anything and then you b) put 'info' in the device's
> driver_data attribute which is very confusing.  driver_data should be
> for what we call ddata which I assume is expressed as 'data' here.
>

asusec_info is shared among all child devices and is exposed while
remaining elements of this struct are for internal use only.

> > +     struct mutex ecreq_lock; /* prevent simultaneous access */
> > +     struct gpio_desc *ecreq;
>
> If I hadn't seen the declaration, I'd have no idea this was a GPIO
> descriptor.  Please improve the nomenclature throughout.
>
> > +     struct i2c_client *self;
>
> Again, please use standard naming conventions:
>
> % git grep "struct i2c_client" | grep "\*self" | wc -l
> 0
>
> % git grep "struct i2c_client" | grep "\*client" | wc -l
> 6304
>
> % git grep "struct i2c_client" | grep "\*i2c" | wc -l
> 903
>

ok, noted.

> > +     const struct asus_ec_chip_data *data;
>
> 'data', 'priv' and 'info' should be improved.
>
> > +     char ec_data[DOCKRAM_ENTRY_BUFSIZE];
>
> An array of chars called 'data'.  This could be anything.
>

Do you have a comprehensive list of name conventions you find suitable?

> > +     bool logging_disabled;
>
> This debugging tool is probably never going to be used again.
>
> Keep it local.
>
> > +};
> > +
> > +struct dockram_ec_data {
> > +     struct mutex ctl_lock; /* prevent simultaneous access */
> > +     char ctl_data[DOCKRAM_ENTRY_BUFSIZE];
> > +};
> > +
> > +#define to_ec_data(ec) \
> > +     container_of(ec, struct asus_ec_data, info)
> > +
> > +/**
> > + * asus_dockram_read - Read a register from the DockRAM device.
> > + * @client: Handle to the DockRAM device.
> > + * @reg: Register to read.
> > + * @buf: Byte array into which data will be read; must be large enough to
> > + *    hold the data returned by the DockRAM.
> > + *
> > + * This executes the DockRAM read based on the SMBus "block read" protocol
> > + * or its emulation. It extracts DOCKRAM_ENTRY_SIZE bytes from the set
> > + * register address.
> > + *
> > + * Returns a negative errno code else zero on success.
> > + */
> > +int asus_dockram_read(struct i2c_client *client, int reg, char *buf)
> > +{
>
> Have you considered using Regmap for register access instead of
> implementing custom functions?  Remaps already deals with caching and
> locking mechanisms that you'd get for free.
>
> This looks like it would be replaced with devm_regmap_init_i2c().
>

I will consider this, thank you.

> > +     struct device *dev = &client->dev;
> > +     int ret;
> > +
> > +     memset(buf, 0, DOCKRAM_ENTRY_BUFSIZE);
> > +     ret = i2c_smbus_read_i2c_block_data(client, reg,
> > +                                         DOCKRAM_ENTRY_BUFSIZE, buf);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     if (buf[0] > DOCKRAM_ENTRY_SIZE) {
> > +             dev_err(dev, "bad data len; buffer: %*ph; ret: %d\n",
> > +                     DOCKRAM_ENTRY_BUFSIZE, buf, ret);
> > +             return -EPROTO;
> > +     }
> > +
> > +     dev_dbg(dev, "got data; buffer: %*ph; ret: %d\n",
> > +             DOCKRAM_ENTRY_BUFSIZE, buf, ret);
>
> Please remove all of these debug messages.
>

Why debug messages cannot be preserved? They are specifically marked as dev_dbg

> > +
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(asus_dockram_read);
> > +
> > +/**
> > + * asus_dockram_write - Write a byte array to a register of the DockRAM device.
> > + * @client: Handle to the DockRAM device.
> > + * @reg: Register to write to.
> > + * @buf: Byte array to be written (up to DOCKRAM_ENTRY_SIZE bytes).
> > + *
> > + * This executes the DockRAM write based on the SMBus "block write"
> > + * protocol or its emulation. It writes DOCKRAM_ENTRY_SIZE bytes to the
> > + * specified register address.
> > + *
> > + * Returns a negative errno code else zero on success.
> > + */
> > +int asus_dockram_write(struct i2c_client *client, int reg, const char *buf)
> > +{
> > +     if (buf[0] > DOCKRAM_ENTRY_SIZE)
> > +             return -EINVAL;
> > +
> > +     dev_dbg(&client->dev, "sending data; buffer: %*ph\n", buf[0] + 1, buf);
> > +
> > +     return i2c_smbus_write_i2c_block_data(client, reg, buf[0] + 1, buf);
> > +}
> > +EXPORT_SYMBOL_GPL(asus_dockram_write);
> > +
> > +/**
> > + * asus_dockram_access_ctl - Read from or write to the DockRAM control register.
> > + * @client: Handle to the DockRAM device.
> > + * @out: Pointer to a variable where the register value will be stored.
> > + * @mask: Bitmask of bits to be cleared.
> > + * @xor: Bitmask of bits to be set (via XOR).
> > + *
> > + * This performs a control register read if @out is provided and both @mask
> > + * and @xor are zero. Otherwise, it performs a control register update if
> > + * @mask and @xor are provided.
> > + *
> > + * Returns a negative errno code else zero on success.
> > + */
> > +int asus_dockram_access_ctl(struct i2c_client *client, u64 *out, u64 mask,
> > +                         u64 xor)
> > +{
> > +     struct dockram_ec_data *priv = i2c_get_clientdata(client);
> > +     char *buf = priv->ctl_data;
> > +     u64 val;
> > +     int ret = 0;
> > +
> > +     guard(mutex)(&priv->ctl_lock);
> > +
> > +     ret = asus_dockram_read(client, ASUSEC_DOCKRAM_CONTROL, buf);
> > +     if (ret < 0)
>
> Could we check for errors using 'if (ret)' instead of 'if (ret < 0)' here,
> unless a positive return value has a specific meaning we need to handle?
>

Sure, I will switch to  'if (ret)'

> > +             goto exit;
> > +
> > +     if (buf[0] != ASUSEC_CTL_SIZE) {
> > +             ret = -EPROTO;
> > +             goto exit;
> > +     }
> > +
> > +     val = get_unaligned_le64(buf + 1);
> > +
> > +     if (out)
> > +             *out = val;
> > +
> > +     if (mask || xor) {
> > +             put_unaligned_le64((val & ~mask) ^ xor, buf + 1);
> > +             ret = asus_dockram_write(client, ASUSEC_DOCKRAM_CONTROL, buf);
> > +     }
> > +
> > +exit:
> > +     if (ret < 0)
> > +             dev_err(&client->dev, "Failed to access control flags: %d\n",
> > +                     ret);
> > +
> > +     return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(asus_dockram_access_ctl);
> > +
> > +static void asus_ec_remove_notifier(struct device *dev, void *res)
> > +{
> > +     struct asusec_info *ec = dev_get_drvdata(dev->parent);
> > +     struct notifier_block **nb = res;
> > +
> > +     blocking_notifier_chain_unregister(&ec->notify_list, *nb);
> > +}
> > +
> > +/**
> > + * devm_asus_ec_register_notifier - Managed registration of notifier to an
> > + *                               ASUS EC blocking notifier chain.
> > + * @pdev: Device requesting the notifier (used for resource management).
> > + * @nb: Notifier block to be registered.
> > + *
> > + * Register a notifier to the ASUS EC blocking notifier chain. The notifier
> > + * will be automatically unregistered when the requesting device is detached.
> > + *
> > + * Return: 0 on success or a negative error code on failure.
> > + */
> > +int devm_asus_ec_register_notifier(struct platform_device *pdev,
> > +                                struct notifier_block *nb)
> > +{
>
> Hand-rolling devres managed resources is usually reserved for subsystem
> level API calls.  Why do the usual device driver .remove() handling work
> for you?
>

This is used by 3 subdevices: serio, keys and charger, so this just
seems cleaner way to register and deregister notifier.

> > +     struct asusec_info *ec = dev_get_drvdata(pdev->dev.parent);
> > +     struct notifier_block **res;
> > +     int ret;
> > +
> > +     res = devres_alloc(asus_ec_remove_notifier, sizeof(*res), GFP_KERNEL);
> > +     if (!res)
> > +             return -ENOMEM;
> > +
> > +     *res = nb;
> > +     ret = blocking_notifier_chain_register(&ec->notify_list, nb);
> > +     if (ret) {
> > +             devres_free(res);
> > +             return ret;
> > +     }
> > +
> > +     devres_add(&pdev->dev, res);
> > +
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(devm_asus_ec_register_notifier);
> > +
> > +static int asus_ec_signal_request(const struct asusec_info *ec)
> > +{
> > +     struct asus_ec_data *priv = to_ec_data(ec);
> > +
> > +     guard(mutex)(&priv->ecreq_lock);
> > +
> > +     dev_dbg(&priv->self->dev, "EC request\n");
> > +
> > +     gpiod_set_value_cansleep(priv->ecreq, 1);
> > +     msleep(50);
> > +
> > +     gpiod_set_value_cansleep(priv->ecreq, 0);
> > +     msleep(200);
> > +
> > +     return 0;
> > +}
> > +
> > +static int asus_ec_write(struct asus_ec_data *priv, u16 data)
> > +{
> > +     int ret = i2c_smbus_write_word_data(priv->self, ASUSEC_WRITE_BUF, data);
> > +
> > +     dev_dbg(&priv->self->dev, "EC write: %04x, ret = %d\n", data, ret);
> > +
> > +     return ret;
> > +}
> > +
> > +static int asus_ec_read(struct asus_ec_data *priv, bool in_irq)
> > +{
> > +     int ret = i2c_smbus_read_i2c_block_data(priv->self, ASUSEC_READ_BUF,
> > +                                             sizeof(priv->ec_data),
> > +                                             priv->ec_data);
> > +
> > +     dev_dbg(&priv->self->dev, "EC read: %*ph, ret = %d%s\n",
> > +             sizeof(priv->ec_data), priv->ec_data,
> > +             ret, in_irq ? "; in irq" : "");
> > +
> > +     return ret;
> > +}
>
> No abstractions for the sake of it.  All this goes away with Regmap anyway.
> > +
> > +/**
> > + * asus_ec_i2c_command - Send a 16-bit command to the ASUS EC.
> > + * @ec: Pointer to the shared ASUS EC structure.
> > + * @data: The 16-bit command (word) to be sent.
> > + *
> > + * Return: 0 on success or a negative error code on failure.
> > + */
> > +int asus_ec_i2c_command(const struct asusec_info *ec, u16 data)
> > +{
> > +     return asus_ec_write(to_ec_data(ec), data);
> > +}
>
> Is this wrapper function strictly necessary? We should try to avoid
> superfluous abstractions that do nothing but call another function.
>

This one is used by serio and keys too. I will see what I can do with regmaps.

> > +EXPORT_SYMBOL_GPL(asus_ec_i2c_command);
> > +
> > +static void asus_ec_clear_buffer(struct asus_ec_data *priv)
> > +{
> > +     int retry = ASUSEC_RSP_BUFFER_SIZE;
> > +
> > +     while (retry--) {
> > +             if (asus_ec_read(priv, false) < 0)
> > +                     continue;
> > +
> > +             if (priv->ec_data[1] & ASUSEC_OBF_MASK)
> > +                     continue;
> > +
> > +             break;
> > +     }
> > +}
> > +
> > +static int asus_ec_log_info(struct asus_ec_data *priv, unsigned int reg,
> > +                         const char *name, char **out)
> > +{
> > +     char buf[DOCKRAM_ENTRY_BUFSIZE];
> > +     int ret;
> > +
> > +     ret = asus_dockram_read(priv->info.dockram, reg, buf);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     if (!priv->logging_disabled)
> > +             dev_info(&priv->self->dev, "%-14s: %.*s\n", name,
> > +                      buf[0], buf + 1);
> > +
> > +     if (out)
> > +             *out = kstrndup(buf + 1, buf[0], GFP_KERNEL);
> > +
> > +     return 0;
> > +}
> > +
> > +static int asus_ec_reset(struct asus_ec_data *priv)
> > +{
> > +     int retry, ret;
> > +
> > +     for (retry = 0; retry < 3; retry++) {
> > +             ret = asus_ec_write(priv, 0);
> > +             if (!ret)
> > +                     return 0;
> > +
> > +             msleep(300);
> > +     }
> > +
> > +     return ret;
> > +}
> > +
> > +static int asus_ec_magic_debug(struct asus_ec_data *priv)
> > +{
> > +     u64 flag;
> > +     int ret;
> > +
> > +     ret = asus_ec_get_ctl(&priv->info, &flag);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     flag &= ASUSEC_CTL_SUSB_MODE;
> > +     dev_info(&priv->self->dev, "EC FW behaviour: %s\n",
> > +              flag ? "susb on when receive ec_req" :
> > +              "susb on when system wakeup");
> > +
> > +     return 0;
> > +}
> > +
> > +static int asus_ec_set_factory_mode(struct asus_ec_data *priv, bool on)
> > +{
> > +     dev_info(&priv->self->dev, "Entering %s mode.\n", on ? "factory" :
> > +              "normal");
> > +
> > +     return asus_ec_update_ctl(&priv->info, ASUSEC_CTL_FACTORY_MODE,
> > +                               on ? ASUSEC_CTL_FACTORY_MODE : 0);
> > +}
> > +
> > +static int asus_ec_detect(struct asus_ec_data *priv)
> > +{
> > +     char *model = NULL;
> > +     int ret;
> > +
> > +     ret = asus_ec_reset(priv);
> > +     if (ret)
> > +             goto err_exit;
> > +
> > +     asus_ec_clear_buffer(priv);
> > +
> > +     ret = asus_ec_log_info(priv, ASUSEC_DOCKRAM_INFO_MODEL, "model", &model);
> > +     if (ret)
> > +             goto err_exit;
> > +
> > +     ret = asus_ec_log_info(priv, ASUSEC_DOCKRAM_INFO_FW, "FW version", NULL);
> > +     if (ret)
> > +             goto err_exit;
> > +
> > +     ret = asus_ec_log_info(priv, ASUSEC_DOCKRAM_INFO_CFGFMT, "Config format", NULL);
> > +     if (ret)
> > +             goto err_exit;
> > +
> > +     ret = asus_ec_log_info(priv, ASUSEC_DOCKRAM_INFO_HW, "HW version", NULL);
> > +     if (ret)
> > +             goto err_exit;
> > +
> > +     priv->logging_disabled = true;
> > +
> > +     ret = asus_ec_magic_debug(priv);
> > +     if (ret)
> > +             goto err_exit;
> > +
> > +     priv->info.model = model;
> > +     priv->info.name = priv->data->name;
> > +
> > +     if (priv->data->clr_fmode)
> > +             asus_ec_set_factory_mode(priv, false);
> > +
> > +err_exit:
> > +     if (ret)
> > +             dev_err(&priv->self->dev, "failed to access EC: %d\n", ret);
> > +
> > +     return ret;
> > +}
> > +
> > +static void asus_ec_handle_smi(struct asus_ec_data *priv, unsigned int code)
> > +{
> > +     dev_dbg(&priv->self->dev, "SMI interrupt: 0x%02x\n", code);
> > +
> > +     switch (code) {
> > +     case ASUSEC_SMI_HANDSHAKE:
> > +     case ASUSEC_SMI_RESET:
> > +             asus_ec_detect(priv);
> > +             break;
> > +     }
> > +}
> > +
> > +static irqreturn_t asus_ec_interrupt(int irq, void *dev_id)
> > +{
> > +     struct asus_ec_data *priv = dev_id;
> > +     unsigned long notify_action;
> > +     int ret;
> > +
> > +     ret = asus_ec_read(priv, true);
> > +     if (ret <= 0 || !(priv->ec_data[1] & ASUSEC_OBF_MASK))
> > +             return IRQ_NONE;
> > +
> > +     notify_action = priv->ec_data[1];
> > +     if (notify_action & ASUSEC_SMI_MASK) {
> > +             unsigned int code = priv->ec_data[2];
> > +
> > +             asus_ec_handle_smi(priv, code);
> > +
> > +             notify_action |= code << 8;
> > +             dev_dbg(&priv->self->dev, "SMI code: 0x%02x\n", code);
> > +     }
> > +
> > +     blocking_notifier_call_chain(&priv->info.notify_list,
> > +                                  notify_action, priv->ec_data);
> > +
> > +     return IRQ_HANDLED;
> > +}
> > +
> > +static ssize_t dockram_read(struct file *filp, char __user *buf,
> > +                         size_t count, loff_t *ppos)
> > +{
> > +     struct i2c_client *client = filp->private_data;
> > +     unsigned int reg, rsize;
> > +     ssize_t n_read = 0, val;
> > +     loff_t off = *ppos;
> > +     char *data;
> > +     int ret;
> > +
> > +     reg = off / DOCKRAM_ENTRY_SIZE;
> > +     off %= DOCKRAM_ENTRY_SIZE;
> > +     rsize = DOCKRAM_ENTRIES * DOCKRAM_ENTRY_SIZE;
> > +
> > +     if (!count)
> > +             return 0;
> > +
> > +     data = kmalloc(DOCKRAM_ENTRY_BUFSIZE, GFP_KERNEL);
> > +
> > +     while (reg < DOCKRAM_ENTRIES) {
> > +             unsigned int len = DOCKRAM_ENTRY_SIZE - off;
> > +
> > +             if (len > rsize)
> > +                     len = rsize;
> > +
> > +             ret = asus_dockram_read(client, reg, data);
> > +             if (ret < 0) {
> > +                     if (!n_read)
> > +                             n_read = ret;
> > +                     break;
> > +             }
> > +
> > +             val = copy_to_user(buf, data + 1 + off, len);
> > +             if (val == len)
> > +                     return -EFAULT;
> > +
> > +             *ppos += len;
> > +             n_read += len;
> > +
> > +             if (len == rsize)
> > +                     break;
> > +
> > +             rsize -= len;
> > +             buf += len;
> > +             off = 0;
> > +             ++reg;
> > +     }
> > +
> > +     kfree(data);
> > +
> > +     return n_read;
> > +}
> > +
> > +static int dockram_write_one(struct i2c_client *client, int reg,
> > +                          const char __user *buf, size_t count)
> > +{
> > +     struct dockram_ec_data *priv = i2c_get_clientdata(client);
> > +     int ret;
> > +
> > +     if (!count || count > DOCKRAM_ENTRY_SIZE)
> > +             return -EINVAL;
> > +     if (buf[0] != count - 1)
> > +             return -EINVAL;
> > +
> > +     guard(mutex)(&priv->ctl_lock);
> > +
> > +     priv->ctl_data[0] = (u8)count;
> > +     memcpy(priv->ctl_data + 1, buf, count);
> > +     ret = asus_dockram_write(client, reg, priv->ctl_data);
> > +
> > +     return ret;
> > +}
> > +
> > +static ssize_t dockram_write(struct file *filp, const char __user *buf,
> > +                          size_t count, loff_t *ppos)
> > +{
> > +     struct i2c_client *client = filp->private_data;
> > +     unsigned int reg;
> > +     loff_t off = *ppos;
> > +     int ret;
> > +
> > +     if (off % DOCKRAM_ENTRY_SIZE != 0)
> > +             return -EINVAL;
> > +
> > +     reg = off / DOCKRAM_ENTRY_SIZE;
> > +     if (reg >= DOCKRAM_ENTRIES)
> > +             return -EINVAL;
> > +
> > +     ret = dockram_write_one(client, reg, buf, count);
> > +
> > +     return ret < 0 ? ret : count;
> > +}
> > +
> > +static const struct debugfs_short_fops dockram_fops = {
> > +     .read   = dockram_read,
> > +     .write  = dockram_write,
> > +     .llseek = default_llseek,
> > +};
>
> You should not be giving userspace free reign over device memory.
>
> If you switch to Regmap, you can use regmap-debugfs.c for this.
>

Noted

> > +static int control_reg_get(void *ec, u64 *val)
> > +{
> > +     return asus_ec_get_ctl(ec, val);
> > +}
> > +
> > +static int control_reg_set(void *ec, u64 val)
> > +{
> > +     return asus_ec_update_ctl(ec, ~0ull, val);
> > +}
> > +
> > +DEFINE_DEBUGFS_ATTRIBUTE(control_reg_fops, control_reg_get,
> > +                      control_reg_set, "%016llx\n");
> > +
> > +static int ec_request_set(void *ec, u64 val)
> > +{
> > +     if (val)
> > +             asus_ec_signal_request(ec);
> > +
> > +     return 0;
> > +}
> > +
> > +DEFINE_DEBUGFS_ATTRIBUTE(ec_request_fops, NULL, ec_request_set, "%llu\n");
> > +
> > +static int ec_irq_set(void *ec, u64 val)
> > +{
> > +     struct asus_ec_data *priv = to_ec_data(ec);
> > +
> > +     if (val)
> > +             irq_wake_thread(priv->self->irq, priv);
> > +
> > +     return 0;
> > +}
> > +
> > +DEFINE_DEBUGFS_ATTRIBUTE(ec_irq_fops, NULL, ec_irq_set, "%llu\n");
> > +
> > +static void asus_ec_debugfs_remove(void *debugfs_root)
> > +{
> > +     debugfs_remove_recursive(debugfs_root);
> > +}
> > +
> > +static void devm_asus_ec_debugfs_init(struct device *dev)
> > +{
> > +     struct asusec_info *ec = dev_get_drvdata(dev);
> > +     struct asus_ec_data *priv = to_ec_data(ec);
> > +     struct dentry *debugfs_root, *dockram_dir;
> > +     char *name = devm_kasprintf(dev, GFP_KERNEL, "asus-ec-%s",
> > +                                 priv->data->name);
> > +
> > +     debugfs_root = debugfs_create_dir(name, NULL);
> > +     dockram_dir = debugfs_create_dir("dockram", debugfs_root);
> > +
> > +     debugfs_create_file("ec_irq", 0200, debugfs_root, ec,
> > +                         &ec_irq_fops);
> > +     debugfs_create_file("ec_request", 0200, debugfs_root, ec,
> > +                         &ec_request_fops);
> > +     debugfs_create_file("control_reg", 0644, dockram_dir, ec,
> > +                         &control_reg_fops);
> > +     debugfs_create_file("dockram", 0644, dockram_dir,
> > +                         priv->info.dockram, &dockram_fops);
> > +
> > +     devm_add_action_or_reset(dev, asus_ec_debugfs_remove, debugfs_root);
> > +}
>
> Why is this being controlled via debugfs?
>
> Use the correct kernel APIs instead.
>
> If this is just for development, keep it and all of the extra verbose
> printing in the BSP tree.  It does not belong in the upstream kernel.
>

Prints are required to show Firmware type, version and behavior.

> > +static void asus_ec_release_dockram_dev(void *client)
> > +{
> > +     i2c_unregister_device(client);
> > +}
> > +
> > +static struct i2c_client *devm_asus_dockram_get(struct device *dev)
> > +{
> > +     struct i2c_client *parent = to_i2c_client(dev);
> > +     struct i2c_client *dockram;
> > +     struct dockram_ec_data *priv;
> > +     int ret;
> > +
> > +     dockram = i2c_new_ancillary_device(parent, "dockram",
> > +                                        parent->addr + 2);
> > +     if (IS_ERR(dockram))
> > +             return dockram;
> > +
> > +     ret = devm_add_action_or_reset(dev, asus_ec_release_dockram_dev,
> > +                                    dockram);
> > +     if (ret)
> > +             return ERR_PTR(ret);
> > +
> > +     priv = devm_kzalloc(&dockram->dev, sizeof(*priv), GFP_KERNEL);
> > +     if (!priv)
> > +             return ERR_PTR(-ENOMEM);
> > +
> > +     i2c_set_clientdata(dockram, priv);
> > +     mutex_init(&priv->ctl_lock);
> > +
> > +     return dockram;
> > +}
> > +
> > +static int asus_ec_probe(struct i2c_client *client)
> > +{
> > +     struct device *dev = &client->dev;
> > +     struct asus_ec_data *priv;
>
> Could we use a clearer, more specific name for the private data variable
> instead of the generic term 'priv'? Using something like 'ddata' is usually
> preferred.
>

fine

> > +     int ret;
> > +
> > +     if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_I2C_BLOCK))
> > +             return dev_err_probe(dev, -ENXIO,
> > +                     "I2C bus is missing required SMBus block mode support\n");
> > +
> > +     priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +     if (!priv)
> > +             return -ENOMEM;
> > +
> > +     priv->data = device_get_match_data(dev);
> > +     if (!priv->data)
> > +             return -ENODEV;
> > +
> > +     i2c_set_clientdata(client, priv);
> > +     priv->self = client;
> > +
> > +     priv->info.dockram = devm_asus_dockram_get(dev);
> > +     if (IS_ERR(priv->info.dockram))
> > +             return dev_err_probe(dev, PTR_ERR(priv->info.dockram),
> > +                                  "failed to get dockram\n");
> > +
> > +     priv->ecreq = devm_gpiod_get(dev, "request", GPIOD_OUT_LOW);
> > +     if (IS_ERR(priv->ecreq))
> > +             return dev_err_probe(dev, PTR_ERR(priv->ecreq),
> > +                                  "failed to get request GPIO\n");
>
> "get" or "request"
>

request is gpio's name, request gpio

> > +     BLOCKING_INIT_NOTIFIER_HEAD(&priv->info.notify_list);
> > +     mutex_init(&priv->ecreq_lock);
> > +
> > +     asus_ec_signal_request(&priv->info);
> > +
> > +     ret = asus_ec_detect(priv);
> > +     if (ret)
> > +             return dev_err_probe(dev, ret, "failed to detect EC version\n");
> > +
> > +     ret = devm_request_threaded_irq(dev, client->irq, NULL,
> > +                                     &asus_ec_interrupt,
> > +                                     IRQF_ONESHOT | IRQF_SHARED,
> > +                                     client->name, priv);
> > +     if (ret)
> > +             return dev_err_probe(dev, ret, "failed to register IRQ\n");
> > +
> > +     /* Parent I2C controller uses DMA, ASUS EC and child devices do not */
> > +     client->dev.coherent_dma_mask = 0;
> > +     client->dev.dma_mask = &client->dev.coherent_dma_mask;
> > +
> > +     if (IS_ENABLED(CONFIG_DEBUG_FS))
> > +             devm_asus_ec_debugfs_init(dev);
> > +
> > +     return devm_mfd_add_devices(dev, 0, priv->data->mfd_devices,
> > +                                 priv->data->num_devices, NULL, 0, NULL);
> > +}
> > +
> > +static const struct mfd_cell asus_ec_sl101_dock_mfd_devices[] = {
> > +     {
> > +             .name = "asus-transformer-ec-kbc",
> > +     },
> > +};
> > +
> > +static const struct asus_ec_chip_data asus_ec_sl101_dock_data = {
> > +     .name = "dock",
> > +     .mfd_devices = asus_ec_sl101_dock_mfd_devices,
> > +     .num_devices = ARRAY_SIZE(asus_ec_sl101_dock_mfd_devices),
> > +     .clr_fmode = false,
> > +};
> > +
> > +static const struct mfd_cell asus_ec_tf101_dock_mfd_devices[] = {
> > +     {
> > +             .name = "asus-transformer-ec-battery",
> > +             .id = 1,
>
> Why doesn't PLATFORM_DEVID_AUTO already do the right thing?
>

There may be 2 of this EC in the device with similar set of cells,
last time I have tested EC that was registered second caused clash
with first EC cause of these cells. I will check if this behavior
changed now.

> > +     }, {
> > +             .name = "asus-transformer-ec-charger",
> > +             .id = 1,
> > +     }, {
> > +             .name = "asus-transformer-ec-led",
> > +             .id = 1,
> > +     }, {
> > +             .name = "asus-transformer-ec-keys",
> > +     }, {
> > +             .name = "asus-transformer-ec-kbc",
> > +     },
> > +};
> > +
> > +static const struct asus_ec_chip_data asus_ec_tf101_dock_data = {
> > +     .name = "dock",
> > +     .mfd_devices = asus_ec_tf101_dock_mfd_devices,
> > +     .num_devices = ARRAY_SIZE(asus_ec_tf101_dock_mfd_devices),
> > +     .clr_fmode = false,
> > +};
> > +
> > +static const struct mfd_cell asus_ec_tf201_pad_mfd_devices[] = {
> > +     {
> > +             .name = "asus-transformer-ec-battery",
> > +             .id = 0,
> > +     }, {
> > +             .name = "asus-transformer-ec-led",
> > +             .id = 0,
> > +     },
> > +};
> > +
> > +static const struct asus_ec_chip_data asus_ec_tf201_pad_data = {
> > +     .name = "pad",
> > +     .mfd_devices = asus_ec_tf201_pad_mfd_devices,
> > +     .num_devices = ARRAY_SIZE(asus_ec_tf201_pad_mfd_devices),
> > +     .clr_fmode = true,
> > +};
> > +
> > +static const struct mfd_cell asus_ec_tf600t_pad_mfd_devices[] = {
> > +     {
> > +             .name = "asus-transformer-ec-battery",
> > +             .id = 0,
> > +     }, {
> > +             .name = "asus-transformer-ec-charger",
> > +             .id = 0,
> > +     }, {
> > +             .name = "asus-transformer-ec-led",
> > +             .id = 0,
> > +     },
> > +};
> > +
> > +static const struct asus_ec_chip_data asus_ec_tf600t_pad_data = {
> > +     .name = "pad",
> > +     .mfd_devices = asus_ec_tf600t_pad_mfd_devices,
> > +     .num_devices = ARRAY_SIZE(asus_ec_tf600t_pad_mfd_devices),
> > +     .clr_fmode = true,
> > +};
> > +
> > +static const struct of_device_id asus_ec_match[] = {
> > +     { .compatible = "asus,sl101-ec-dock", .data = &asus_ec_sl101_dock_data },
> > +     { .compatible = "asus,tf101-ec-dock", .data = &asus_ec_tf101_dock_data },
>
> Could we use the match table's data field to store an 'enum' or integer ID
> instead of passing platform data via the '.data' field? We could then use a
> 'switch' statement in the C code to select the correct 'mfd_cell' array.

Yes

> > +     { .compatible = "asus,tf201-ec-pad", .data = &asus_ec_tf201_pad_data },
> > +     { .compatible = "asus,tf600t-ec-pad", .data = &asus_ec_tf600t_pad_data },
> > +     { }
> > +};
> > +MODULE_DEVICE_TABLE(of, asus_ec_match);
> > +
> > +static struct i2c_driver asus_ec_driver = {
> > +     .driver = {
> > +             .name = "asus-transformer-ec",
> > +             .of_match_table = asus_ec_match,
> > +     },
> > +     .probe = asus_ec_probe,
> > +};
> > +module_i2c_driver(asus_ec_driver);
> > +
> > +MODULE_AUTHOR("Michał Mirosław <mirq-linux@rere.qmqm.pl>");
> > +MODULE_AUTHOR("Svyatoslav Ryhel <clamor95@gmail.com>");
> > +MODULE_DESCRIPTION("ASUS Transformer's EC driver");
> > +MODULE_LICENSE("GPL");
> > diff --git a/include/linux/mfd/asus-transformer-ec.h b/include/linux/mfd/asus-transformer-ec.h
> > new file mode 100644
> > index 000000000000..0a72de40352e
> > --- /dev/null
> > +++ b/include/linux/mfd/asus-transformer-ec.h
> > @@ -0,0 +1,162 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +#ifndef __MFD_ASUS_TRANSFORMER_EC_H
> > +#define __MFD_ASUS_TRANSFORMER_EC_H
> > +
> > +#include <linux/notifier.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/workqueue.h>
> > +
> > +struct i2c_client;
> > +
> > +struct asusec_info {
> > +     const char *model;
> > +     const char *name;
> > +     struct i2c_client *dockram;
> > +     struct workqueue_struct *wq;
> > +     struct blocking_notifier_head notify_list;
> > +};
> > +
> > +#define DOCKRAM_ENTRIES                      0x100
> > +#define DOCKRAM_ENTRY_SIZE           32
> > +#define DOCKRAM_ENTRY_BUFSIZE                (DOCKRAM_ENTRY_SIZE + 1)
> > +
> > +/* interrupt sources */
> > +#define ASUSEC_OBF_MASK                      BIT(0)
> > +#define ASUSEC_KEY_MASK                      BIT(2)
> > +#define ASUSEC_KBC_MASK                      BIT(3)
> > +#define ASUSEC_AUX_MASK                      BIT(5)
> > +#define ASUSEC_SCI_MASK                      BIT(6)
> > +#define ASUSEC_SMI_MASK                      BIT(7)
> > +
> > +/* SMI notification codes */
> > +#define ASUSEC_SMI_POWER_NOTIFY              0x31    /* [un]plugging USB cable */
> > +#define ASUSEC_SMI_HANDSHAKE         0x50    /* response to ec_req edge */
> > +#define ASUSEC_SMI_WAKE                      0x53
> > +#define ASUSEC_SMI_RESET             0x5f
> > +#define ASUSEC_SMI_ADAPTER_EVENT     0x60    /* [un]plugging charger to dock */
> > +#define ASUSEC_SMI_BACKLIGHT_ON              0x63
> > +#define ASUSEC_SMI_AUDIO_DOCK_IN     0x70
> > +
> > +#define ASUSEC_SMI_ACTION(code)              (ASUSEC_SMI_MASK | ASUSEC_OBF_MASK | \
> > +                                     (ASUSEC_SMI_##code << 8))
> > +
> > +/* control register [0x0a] layout */
> > +#define ASUSEC_CTL_SIZE                      8
> > +
> > +/*
> > + * EC reports power from 40-pin connector in the LSB of the control
> > + * register.  The following values have been observed (xor 0x02):
> > + *
> > + * PAD-ec no-plug  0x40 / PAD-ec DOCK     0x20 / DOCK-ec no-plug 0x40
> > + * PAD-ec AC       0x25 / PAD-ec DOCK+AC  0x24 / DOCK-ec AC      0x25
> > + * PAD-ec USB      0x45 / PAD-ec DOCK+USB 0x24 / DOCK-ec USB     0x41
> > + */
> > +
> > +#define ASUSEC_CTL_DIRECT_POWER_SOURCE       BIT_ULL(0)
> > +#define ASUSEC_STAT_CHARGING         BIT_ULL(2)
> > +#define ASUSEC_CTL_FULL_POWER_SOURCE BIT_ULL(5)
> > +#define ASUSEC_CTL_SUSB_MODE         BIT_ULL(9)
> > +#define ASUSEC_CMD_SUSPEND_S3                BIT_ULL(33)
> > +#define ASUSEC_CTL_TEST_DISCHARGE    BIT_ULL(35)
> > +#define ASUSEC_CMD_SUSPEND_INHIBIT   BIT_ULL(37)
> > +#define ASUSEC_CTL_FACTORY_MODE              BIT_ULL(38)
> > +#define ASUSEC_CTL_KEEP_AWAKE                BIT_ULL(39)
> > +#define ASUSEC_CTL_USB_CHARGE                BIT_ULL(40)
> > +#define ASUSEC_CTL_LED_BLINK         BIT_ULL(40)
> > +#define ASUSEC_CTL_LED_AMBER         BIT_ULL(41)
> > +#define ASUSEC_CTL_LED_GREEN         BIT_ULL(42)
> > +#define ASUSEC_CMD_SWITCH_HDMI               BIT_ULL(56)
> > +#define ASUSEC_CMD_WIN_SHUTDOWN              BIT_ULL(62)
> > +
> > +#define ASUSEC_DOCKRAM_INFO_MODEL    0x01
> > +#define ASUSEC_DOCKRAM_INFO_FW               0x02
> > +#define ASUSEC_DOCKRAM_INFO_CFGFMT   0x03
> > +#define ASUSEC_DOCKRAM_INFO_HW               0x04
> > +#define ASUSEC_DOCKRAM_CONTROL               0x0a
> > +#define ASUSEC_DOCKRAM_BATT_CTL              0x14
> > +
> > +#define ASUSEC_WRITE_BUF             0x64
> > +#define ASUSEC_READ_BUF                      0x6a
> > +
> > +/* dockram comm */
> > +int asus_dockram_read(struct i2c_client *client, int reg, char *buf);
> > +int asus_dockram_write(struct i2c_client *client, int reg, const char *buf);
> > +int asus_dockram_access_ctl(struct i2c_client *client,
> > +                         u64 *out, u64 mask, u64 xor);
> > +
> > +/* EC public API */
> > +
> > +/**
> > + * cell_to_ec - Request the shared ASUS EC structure via a subdevice's pdev.
> > + * @pdev: EC subdevice pdev requesting access to the shared ASUS EC structure.
> > + *
> > + * Returns a pointer to the asusec_info structure.
> > + */
> > +static inline struct asusec_info *cell_to_ec(struct platform_device *pdev)
> > +{
> > +     return dev_get_drvdata(pdev->dev.parent);
> > +}
>
> This is both misleading and already exists.
>
> > +
> > +/**
> > + * asus_ec_get_ctl - Read from the DockRAM control register.
> > + * @ec:  Pointer to the shared ASUS EC structure.
> > + * @out: Pointer to the variable where the register value will be stored.
> > + *
> > + * Performs a control register read and stores the value in @out.
> > + *
> > + * Return: 0 on success, or a negative errno code on failure.
> > + */
> > +static inline int asus_ec_get_ctl(const struct asusec_info *ec, u64 *out)
> > +{
> > +     return asus_dockram_access_ctl(ec->dockram, out, 0, 0);
> > +}
> > +
> > +/**
> > + * asus_ec_update_ctl - Update the DockRAM control register.
> > + * @ec:   Pointer to the shared ASUS EC structure.
> > + * @mask: Bitmask of bits to be cleared.
> > + * @xor:  Bitmask of bits to be toggled or set (via XOR).
> > + *
> > + * Performs a read-modify-write update on the control register using
> > + * the provided @mask and @xor values.
> > + *
> > + * Return: 0 on success, or a negative errno code on failure.
> > + */
> > +static inline int asus_ec_update_ctl(const struct asusec_info *ec,
> > +                                  u64 mask, u64 xor)
> > +{
> > +     return asus_dockram_access_ctl(ec->dockram, NULL, mask, xor);
> > +}
> > +
> > +/**
> > + * asus_ec_set_ctl_bits - Sets bits of the DockRAM control register.
> > + * @ec:   Pointer to the shared ASUS EC structure.
> > + * @mask: Bitmask of bits to be set.
> > + *
> > + * Sets bits of the control register using the provided @mask value.
> > + *
> > + * Return: 0 on success, or a negative errno code on failure.
> > + */
> > +static inline int asus_ec_set_ctl_bits(const struct asusec_info *ec, u64 mask)
> > +{
> > +     return asus_dockram_access_ctl(ec->dockram, NULL, mask, mask);
> > +}
> > +
> > +/**
> > + * asus_ec_clear_ctl_bits - Clears bits of the DockRAM control register.
> > + * @ec:   Pointer to the shared ASUS EC structure.
> > + * @mask: Bitmask of bits to be cleared.
> > + *
> > + * Clears bits of the control register using the provided @mask value.
> > + *
> > + * Return: 0 on success, or a negative errno code on failure.
> > + */
> > +static inline int asus_ec_clear_ctl_bits(const struct asusec_info *ec, u64 mask)
> > +{
> > +     return asus_dockram_access_ctl(ec->dockram, NULL, mask, 0);
> > +}
>
> We don't typically allow abstraction for the sake of abstraction.
>
> > +int asus_ec_i2c_command(const struct asusec_info *ec, u16 data);
> > +int devm_asus_ec_register_notifier(struct platform_device *dev,
> > +                                struct notifier_block *nb);
> > +#endif /* __MFD_ASUS_TRANSFORMER_EC_H */
> > --
> > 2.51.0
> >
> >
>
> --
> Lee Jones

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v6 2/7] mfd: Add driver for ASUS Transformer embedded controller
  2026-05-14 10:02   ` [PATCH v6 2/7] mfd: Add driver for ASUS Transformer embedded controller Lee Jones
  2026-05-14 10:31     ` Svyatoslav Ryhel
@ 2026-05-14 11:02     ` Svyatoslav Ryhel
  1 sibling, 0 replies; 6+ messages in thread
From: Svyatoslav Ryhel @ 2026-05-14 11:02 UTC (permalink / raw)
  To: Lee Jones
  Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Dmitry Torokhov,
	Pavel Machek, Sebastian Reichel, Ion Agorria,
	Michał Mirosław, devicetree, linux-kernel, linux-input,
	linux-leds, linux-pm

чт, 14 трав. 2026 р. о 13:02 Lee Jones <lee@kernel.org> пише:
>
> On Sat, 02 May 2026, Svyatoslav Ryhel wrote:
>
> > From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> >
> > Support Nuvoton NPCE795-based ECs as used in Asus Transformer TF201,
> > TF300T, TF300TG, TF300TL and TF700T pad and dock, as well as TF101 dock
> > and TF600T, P1801-T and TF701T pad. This is a glue driver handling
> > detection and common operations for EC's functions.
> >
> > Co-developed-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > ---
> >  drivers/mfd/Kconfig                     |  14 +
> >  drivers/mfd/Makefile                    |   1 +
> >  drivers/mfd/asus-transformer-ec.c       | 762 ++++++++++++++++++++++++
> >  include/linux/mfd/asus-transformer-ec.h | 162 +++++
> >  4 files changed, 939 insertions(+)
> >  create mode 100644 drivers/mfd/asus-transformer-ec.c
> >  create mode 100644 include/linux/mfd/asus-transformer-ec.h
> >
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index 7192c9d1d268..5aa4facfd2df 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -137,6 +137,20 @@ config MFD_AAT2870_CORE
> >         additional drivers must be enabled in order to use the
> >         functionality of the device.
> >
> > +config MFD_ASUS_TRANSFORMER_EC
> > +     tristate "ASUS Transformer's embedded controller"
> > +     depends on I2C && OF
> > +     help
> > +       Support ECs found in ASUS Transformer's Pad and Mobile Dock.
> > +
> > +       This provides shared glue for functional part drivers:
> > +         asus-transformer-ec-kbc, asus-transformer-ec-keys,
> > +         leds-asus-transformer-ec, asus-transformer-ec-battery
> > +         and asus-transformer-ec-charger.
>
> A 760 line driver deserves more than just a list of leaf drivers.
>
> > +       This driver can also be built as a module. If so, the module
> > +       will be called asus-transformer-ec.
> > +
> >  config MFD_AT91_USART
> >       tristate "AT91 USART Driver"
> >       select MFD_CORE
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index e75e8045c28a..fd80088d8a9a 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -10,6 +10,7 @@ obj-$(CONFIG_MFD_88PM805)   += 88pm805.o 88pm80x.o
> >  obj-$(CONFIG_MFD_88PM886_PMIC)       += 88pm886.o
> >  obj-$(CONFIG_MFD_ACT8945A)   += act8945a.o
> >  obj-$(CONFIG_MFD_SM501)              += sm501.o
> > +obj-$(CONFIG_MFD_ASUS_TRANSFORMER_EC)        += asus-transformer-ec.o
> >  obj-$(CONFIG_ARCH_BCM2835)   += bcm2835-pm.o
> >  obj-$(CONFIG_MFD_BCM590XX)   += bcm590xx.o
> >  obj-$(CONFIG_MFD_BD9571MWV)  += bd9571mwv.o
> > diff --git a/drivers/mfd/asus-transformer-ec.c b/drivers/mfd/asus-transformer-ec.c
> > new file mode 100644
> > index 000000000000..75aa7ab99387
> > --- /dev/null
> > +++ b/drivers/mfd/asus-transformer-ec.c
> > @@ -0,0 +1,762 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +
> > +#include <linux/array_size.h>
> > +#include <linux/debugfs.h>
> > +#include <linux/delay.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/device.h>
>
> Should we sort the '#include' directives alphabetically? For instance,
> 'device.h' should typically appear before 'gpio/consumer.h'.
>
> > +#include <linux/err.h>
> > +#include <linux/i2c.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/mfd/asus-transformer-ec.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/property.h>
> > +#include <linux/slab.h>
> > +#include <linux/string.h>
> > +#include <linux/types.h>
> > +#include <linux/unaligned.h>
> > +
> > +#define ASUSEC_RSP_BUFFER_SIZE               8
> > +
> > +struct asus_ec_chip_data {
> > +     const char *name;
> > +     const struct mfd_cell *mfd_devices;
>
> Use global `static const structs` instead.
>
> Also, please do not pass MFD registration data through another
> registration API like DT.  It opens the gates to too much hackery.  I'm
> not saying that _this_ driver would do such a thing, but it's easier
> just to keep the blanket rule in place.
>
> > +     unsigned int num_devices;
> > +     bool clr_fmode; /* clear Factory Mode bit in EC control register */
> > +};
> > +
> > +struct asus_ec_data {
> > +     struct asusec_info info;
>
> You have 'data' and 'info' which a) using non-forthcoming nomenclature
> and doesn't tell me anything and then you b) put 'info' in the device's
> driver_data attribute which is very confusing.  driver_data should be
> for what we call ddata which I assume is expressed as 'data' here.
>
> > +     struct mutex ecreq_lock; /* prevent simultaneous access */
> > +     struct gpio_desc *ecreq;
>
> If I hadn't seen the declaration, I'd have no idea this was a GPIO
> descriptor.  Please improve the nomenclature throughout.
>
> > +     struct i2c_client *self;
>
> Again, please use standard naming conventions:
>
> % git grep "struct i2c_client" | grep "\*self" | wc -l
> 0
>
> % git grep "struct i2c_client" | grep "\*client" | wc -l
> 6304
>
> % git grep "struct i2c_client" | grep "\*i2c" | wc -l
> 903
>
> > +     const struct asus_ec_chip_data *data;
>
> 'data', 'priv' and 'info' should be improved.
>
> > +     char ec_data[DOCKRAM_ENTRY_BUFSIZE];
>
> An array of chars called 'data'.  This could be anything.
>
> > +     bool logging_disabled;
>
> This debugging tool is probably never going to be used again.
>
> Keep it local.
>
> > +};
> > +
> > +struct dockram_ec_data {
> > +     struct mutex ctl_lock; /* prevent simultaneous access */
> > +     char ctl_data[DOCKRAM_ENTRY_BUFSIZE];
> > +};
> > +
> > +#define to_ec_data(ec) \
> > +     container_of(ec, struct asus_ec_data, info)
> > +
> > +/**
> > + * asus_dockram_read - Read a register from the DockRAM device.
> > + * @client: Handle to the DockRAM device.
> > + * @reg: Register to read.
> > + * @buf: Byte array into which data will be read; must be large enough to
> > + *    hold the data returned by the DockRAM.
> > + *
> > + * This executes the DockRAM read based on the SMBus "block read" protocol
> > + * or its emulation. It extracts DOCKRAM_ENTRY_SIZE bytes from the set
> > + * register address.
> > + *
> > + * Returns a negative errno code else zero on success.
> > + */
> > +int asus_dockram_read(struct i2c_client *client, int reg, char *buf)
> > +{
>
> Have you considered using Regmap for register access instead of
> implementing custom functions?  Remaps already deals with caching and
> locking mechanisms that you'd get for free.
>
> This looks like it would be replaced with devm_regmap_init_i2c().
>

I don't see any instances regmap handling 256 bit registers.

> > +     struct device *dev = &client->dev;
> > +     int ret;
> > +
> > +     memset(buf, 0, DOCKRAM_ENTRY_BUFSIZE);
> > +     ret = i2c_smbus_read_i2c_block_data(client, reg,
> > +                                         DOCKRAM_ENTRY_BUFSIZE, buf);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     if (buf[0] > DOCKRAM_ENTRY_SIZE) {
> > +             dev_err(dev, "bad data len; buffer: %*ph; ret: %d\n",
> > +                     DOCKRAM_ENTRY_BUFSIZE, buf, ret);
> > +             return -EPROTO;
> > +     }
> > +
> > +     dev_dbg(dev, "got data; buffer: %*ph; ret: %d\n",
> > +             DOCKRAM_ENTRY_BUFSIZE, buf, ret);
>
> Please remove all of these debug messages.
>
> > +
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(asus_dockram_read);
> > +
> > +/**
> > + * asus_dockram_write - Write a byte array to a register of the DockRAM device.
> > + * @client: Handle to the DockRAM device.
> > + * @reg: Register to write to.
> > + * @buf: Byte array to be written (up to DOCKRAM_ENTRY_SIZE bytes).
> > + *
> > + * This executes the DockRAM write based on the SMBus "block write"
> > + * protocol or its emulation. It writes DOCKRAM_ENTRY_SIZE bytes to the
> > + * specified register address.
> > + *
> > + * Returns a negative errno code else zero on success.
> > + */
> > +int asus_dockram_write(struct i2c_client *client, int reg, const char *buf)
> > +{
> > +     if (buf[0] > DOCKRAM_ENTRY_SIZE)
> > +             return -EINVAL;
> > +
> > +     dev_dbg(&client->dev, "sending data; buffer: %*ph\n", buf[0] + 1, buf);
> > +
> > +     return i2c_smbus_write_i2c_block_data(client, reg, buf[0] + 1, buf);
> > +}
> > +EXPORT_SYMBOL_GPL(asus_dockram_write);
> > +
> > +/**
> > + * asus_dockram_access_ctl - Read from or write to the DockRAM control register.
> > + * @client: Handle to the DockRAM device.
> > + * @out: Pointer to a variable where the register value will be stored.
> > + * @mask: Bitmask of bits to be cleared.
> > + * @xor: Bitmask of bits to be set (via XOR).
> > + *
> > + * This performs a control register read if @out is provided and both @mask
> > + * and @xor are zero. Otherwise, it performs a control register update if
> > + * @mask and @xor are provided.
> > + *
> > + * Returns a negative errno code else zero on success.
> > + */
> > +int asus_dockram_access_ctl(struct i2c_client *client, u64 *out, u64 mask,
> > +                         u64 xor)
> > +{
> > +     struct dockram_ec_data *priv = i2c_get_clientdata(client);
> > +     char *buf = priv->ctl_data;
> > +     u64 val;
> > +     int ret = 0;
> > +
> > +     guard(mutex)(&priv->ctl_lock);
> > +
> > +     ret = asus_dockram_read(client, ASUSEC_DOCKRAM_CONTROL, buf);
> > +     if (ret < 0)
>
> Could we check for errors using 'if (ret)' instead of 'if (ret < 0)' here,
> unless a positive return value has a specific meaning we need to handle?
>
> > +             goto exit;
> > +
> > +     if (buf[0] != ASUSEC_CTL_SIZE) {
> > +             ret = -EPROTO;
> > +             goto exit;
> > +     }
> > +
> > +     val = get_unaligned_le64(buf + 1);
> > +
> > +     if (out)
> > +             *out = val;
> > +
> > +     if (mask || xor) {
> > +             put_unaligned_le64((val & ~mask) ^ xor, buf + 1);
> > +             ret = asus_dockram_write(client, ASUSEC_DOCKRAM_CONTROL, buf);
> > +     }
> > +
> > +exit:
> > +     if (ret < 0)
> > +             dev_err(&client->dev, "Failed to access control flags: %d\n",
> > +                     ret);
> > +
> > +     return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(asus_dockram_access_ctl);
> > +
> > +static void asus_ec_remove_notifier(struct device *dev, void *res)
> > +{
> > +     struct asusec_info *ec = dev_get_drvdata(dev->parent);
> > +     struct notifier_block **nb = res;
> > +
> > +     blocking_notifier_chain_unregister(&ec->notify_list, *nb);
> > +}
> > +
> > +/**
> > + * devm_asus_ec_register_notifier - Managed registration of notifier to an
> > + *                               ASUS EC blocking notifier chain.
> > + * @pdev: Device requesting the notifier (used for resource management).
> > + * @nb: Notifier block to be registered.
> > + *
> > + * Register a notifier to the ASUS EC blocking notifier chain. The notifier
> > + * will be automatically unregistered when the requesting device is detached.
> > + *
> > + * Return: 0 on success or a negative error code on failure.
> > + */
> > +int devm_asus_ec_register_notifier(struct platform_device *pdev,
> > +                                struct notifier_block *nb)
> > +{
>
> Hand-rolling devres managed resources is usually reserved for subsystem
> level API calls.  Why do the usual device driver .remove() handling work
> for you?
>
> > +     struct asusec_info *ec = dev_get_drvdata(pdev->dev.parent);
> > +     struct notifier_block **res;
> > +     int ret;
> > +
> > +     res = devres_alloc(asus_ec_remove_notifier, sizeof(*res), GFP_KERNEL);
> > +     if (!res)
> > +             return -ENOMEM;
> > +
> > +     *res = nb;
> > +     ret = blocking_notifier_chain_register(&ec->notify_list, nb);
> > +     if (ret) {
> > +             devres_free(res);
> > +             return ret;
> > +     }
> > +
> > +     devres_add(&pdev->dev, res);
> > +
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(devm_asus_ec_register_notifier);
> > +
> > +static int asus_ec_signal_request(const struct asusec_info *ec)
> > +{
> > +     struct asus_ec_data *priv = to_ec_data(ec);
> > +
> > +     guard(mutex)(&priv->ecreq_lock);
> > +
> > +     dev_dbg(&priv->self->dev, "EC request\n");
> > +
> > +     gpiod_set_value_cansleep(priv->ecreq, 1);
> > +     msleep(50);
> > +
> > +     gpiod_set_value_cansleep(priv->ecreq, 0);
> > +     msleep(200);
> > +
> > +     return 0;
> > +}
> > +
> > +static int asus_ec_write(struct asus_ec_data *priv, u16 data)
> > +{
> > +     int ret = i2c_smbus_write_word_data(priv->self, ASUSEC_WRITE_BUF, data);
> > +
> > +     dev_dbg(&priv->self->dev, "EC write: %04x, ret = %d\n", data, ret);
> > +
> > +     return ret;
> > +}
> > +
> > +static int asus_ec_read(struct asus_ec_data *priv, bool in_irq)
> > +{
> > +     int ret = i2c_smbus_read_i2c_block_data(priv->self, ASUSEC_READ_BUF,
> > +                                             sizeof(priv->ec_data),
> > +                                             priv->ec_data);
> > +
> > +     dev_dbg(&priv->self->dev, "EC read: %*ph, ret = %d%s\n",
> > +             sizeof(priv->ec_data), priv->ec_data,
> > +             ret, in_irq ? "; in irq" : "");
> > +
> > +     return ret;
> > +}
>
> No abstractions for the sake of it.  All this goes away with Regmap anyway.
> > +
> > +/**
> > + * asus_ec_i2c_command - Send a 16-bit command to the ASUS EC.
> > + * @ec: Pointer to the shared ASUS EC structure.
> > + * @data: The 16-bit command (word) to be sent.
> > + *
> > + * Return: 0 on success or a negative error code on failure.
> > + */
> > +int asus_ec_i2c_command(const struct asusec_info *ec, u16 data)
> > +{
> > +     return asus_ec_write(to_ec_data(ec), data);
> > +}
>
> Is this wrapper function strictly necessary? We should try to avoid
> superfluous abstractions that do nothing but call another function.
>
> > +EXPORT_SYMBOL_GPL(asus_ec_i2c_command);
> > +
> > +static void asus_ec_clear_buffer(struct asus_ec_data *priv)
> > +{
> > +     int retry = ASUSEC_RSP_BUFFER_SIZE;
> > +
> > +     while (retry--) {
> > +             if (asus_ec_read(priv, false) < 0)
> > +                     continue;
> > +
> > +             if (priv->ec_data[1] & ASUSEC_OBF_MASK)
> > +                     continue;
> > +
> > +             break;
> > +     }
> > +}
> > +
> > +static int asus_ec_log_info(struct asus_ec_data *priv, unsigned int reg,
> > +                         const char *name, char **out)
> > +{
> > +     char buf[DOCKRAM_ENTRY_BUFSIZE];
> > +     int ret;
> > +
> > +     ret = asus_dockram_read(priv->info.dockram, reg, buf);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     if (!priv->logging_disabled)
> > +             dev_info(&priv->self->dev, "%-14s: %.*s\n", name,
> > +                      buf[0], buf + 1);
> > +
> > +     if (out)
> > +             *out = kstrndup(buf + 1, buf[0], GFP_KERNEL);
> > +
> > +     return 0;
> > +}
> > +
> > +static int asus_ec_reset(struct asus_ec_data *priv)
> > +{
> > +     int retry, ret;
> > +
> > +     for (retry = 0; retry < 3; retry++) {
> > +             ret = asus_ec_write(priv, 0);
> > +             if (!ret)
> > +                     return 0;
> > +
> > +             msleep(300);
> > +     }
> > +
> > +     return ret;
> > +}
> > +
> > +static int asus_ec_magic_debug(struct asus_ec_data *priv)
> > +{
> > +     u64 flag;
> > +     int ret;
> > +
> > +     ret = asus_ec_get_ctl(&priv->info, &flag);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     flag &= ASUSEC_CTL_SUSB_MODE;
> > +     dev_info(&priv->self->dev, "EC FW behaviour: %s\n",
> > +              flag ? "susb on when receive ec_req" :
> > +              "susb on when system wakeup");
> > +
> > +     return 0;
> > +}
> > +
> > +static int asus_ec_set_factory_mode(struct asus_ec_data *priv, bool on)
> > +{
> > +     dev_info(&priv->self->dev, "Entering %s mode.\n", on ? "factory" :
> > +              "normal");
> > +
> > +     return asus_ec_update_ctl(&priv->info, ASUSEC_CTL_FACTORY_MODE,
> > +                               on ? ASUSEC_CTL_FACTORY_MODE : 0);
> > +}
> > +
> > +static int asus_ec_detect(struct asus_ec_data *priv)
> > +{
> > +     char *model = NULL;
> > +     int ret;
> > +
> > +     ret = asus_ec_reset(priv);
> > +     if (ret)
> > +             goto err_exit;
> > +
> > +     asus_ec_clear_buffer(priv);
> > +
> > +     ret = asus_ec_log_info(priv, ASUSEC_DOCKRAM_INFO_MODEL, "model", &model);
> > +     if (ret)
> > +             goto err_exit;
> > +
> > +     ret = asus_ec_log_info(priv, ASUSEC_DOCKRAM_INFO_FW, "FW version", NULL);
> > +     if (ret)
> > +             goto err_exit;
> > +
> > +     ret = asus_ec_log_info(priv, ASUSEC_DOCKRAM_INFO_CFGFMT, "Config format", NULL);
> > +     if (ret)
> > +             goto err_exit;
> > +
> > +     ret = asus_ec_log_info(priv, ASUSEC_DOCKRAM_INFO_HW, "HW version", NULL);
> > +     if (ret)
> > +             goto err_exit;
> > +
> > +     priv->logging_disabled = true;
> > +
> > +     ret = asus_ec_magic_debug(priv);
> > +     if (ret)
> > +             goto err_exit;
> > +
> > +     priv->info.model = model;
> > +     priv->info.name = priv->data->name;
> > +
> > +     if (priv->data->clr_fmode)
> > +             asus_ec_set_factory_mode(priv, false);
> > +
> > +err_exit:
> > +     if (ret)
> > +             dev_err(&priv->self->dev, "failed to access EC: %d\n", ret);
> > +
> > +     return ret;
> > +}
> > +
> > +static void asus_ec_handle_smi(struct asus_ec_data *priv, unsigned int code)
> > +{
> > +     dev_dbg(&priv->self->dev, "SMI interrupt: 0x%02x\n", code);
> > +
> > +     switch (code) {
> > +     case ASUSEC_SMI_HANDSHAKE:
> > +     case ASUSEC_SMI_RESET:
> > +             asus_ec_detect(priv);
> > +             break;
> > +     }
> > +}
> > +
> > +static irqreturn_t asus_ec_interrupt(int irq, void *dev_id)
> > +{
> > +     struct asus_ec_data *priv = dev_id;
> > +     unsigned long notify_action;
> > +     int ret;
> > +
> > +     ret = asus_ec_read(priv, true);
> > +     if (ret <= 0 || !(priv->ec_data[1] & ASUSEC_OBF_MASK))
> > +             return IRQ_NONE;
> > +
> > +     notify_action = priv->ec_data[1];
> > +     if (notify_action & ASUSEC_SMI_MASK) {
> > +             unsigned int code = priv->ec_data[2];
> > +
> > +             asus_ec_handle_smi(priv, code);
> > +
> > +             notify_action |= code << 8;
> > +             dev_dbg(&priv->self->dev, "SMI code: 0x%02x\n", code);
> > +     }
> > +
> > +     blocking_notifier_call_chain(&priv->info.notify_list,
> > +                                  notify_action, priv->ec_data);
> > +
> > +     return IRQ_HANDLED;
> > +}
> > +
> > +static ssize_t dockram_read(struct file *filp, char __user *buf,
> > +                         size_t count, loff_t *ppos)
> > +{
> > +     struct i2c_client *client = filp->private_data;
> > +     unsigned int reg, rsize;
> > +     ssize_t n_read = 0, val;
> > +     loff_t off = *ppos;
> > +     char *data;
> > +     int ret;
> > +
> > +     reg = off / DOCKRAM_ENTRY_SIZE;
> > +     off %= DOCKRAM_ENTRY_SIZE;
> > +     rsize = DOCKRAM_ENTRIES * DOCKRAM_ENTRY_SIZE;
> > +
> > +     if (!count)
> > +             return 0;
> > +
> > +     data = kmalloc(DOCKRAM_ENTRY_BUFSIZE, GFP_KERNEL);
> > +
> > +     while (reg < DOCKRAM_ENTRIES) {
> > +             unsigned int len = DOCKRAM_ENTRY_SIZE - off;
> > +
> > +             if (len > rsize)
> > +                     len = rsize;
> > +
> > +             ret = asus_dockram_read(client, reg, data);
> > +             if (ret < 0) {
> > +                     if (!n_read)
> > +                             n_read = ret;
> > +                     break;
> > +             }
> > +
> > +             val = copy_to_user(buf, data + 1 + off, len);
> > +             if (val == len)
> > +                     return -EFAULT;
> > +
> > +             *ppos += len;
> > +             n_read += len;
> > +
> > +             if (len == rsize)
> > +                     break;
> > +
> > +             rsize -= len;
> > +             buf += len;
> > +             off = 0;
> > +             ++reg;
> > +     }
> > +
> > +     kfree(data);
> > +
> > +     return n_read;
> > +}
> > +
> > +static int dockram_write_one(struct i2c_client *client, int reg,
> > +                          const char __user *buf, size_t count)
> > +{
> > +     struct dockram_ec_data *priv = i2c_get_clientdata(client);
> > +     int ret;
> > +
> > +     if (!count || count > DOCKRAM_ENTRY_SIZE)
> > +             return -EINVAL;
> > +     if (buf[0] != count - 1)
> > +             return -EINVAL;
> > +
> > +     guard(mutex)(&priv->ctl_lock);
> > +
> > +     priv->ctl_data[0] = (u8)count;
> > +     memcpy(priv->ctl_data + 1, buf, count);
> > +     ret = asus_dockram_write(client, reg, priv->ctl_data);
> > +
> > +     return ret;
> > +}
> > +
> > +static ssize_t dockram_write(struct file *filp, const char __user *buf,
> > +                          size_t count, loff_t *ppos)
> > +{
> > +     struct i2c_client *client = filp->private_data;
> > +     unsigned int reg;
> > +     loff_t off = *ppos;
> > +     int ret;
> > +
> > +     if (off % DOCKRAM_ENTRY_SIZE != 0)
> > +             return -EINVAL;
> > +
> > +     reg = off / DOCKRAM_ENTRY_SIZE;
> > +     if (reg >= DOCKRAM_ENTRIES)
> > +             return -EINVAL;
> > +
> > +     ret = dockram_write_one(client, reg, buf, count);
> > +
> > +     return ret < 0 ? ret : count;
> > +}
> > +
> > +static const struct debugfs_short_fops dockram_fops = {
> > +     .read   = dockram_read,
> > +     .write  = dockram_write,
> > +     .llseek = default_llseek,
> > +};
>
> You should not be giving userspace free reign over device memory.
>
> If you switch to Regmap, you can use regmap-debugfs.c for this.
>
> > +static int control_reg_get(void *ec, u64 *val)
> > +{
> > +     return asus_ec_get_ctl(ec, val);
> > +}
> > +
> > +static int control_reg_set(void *ec, u64 val)
> > +{
> > +     return asus_ec_update_ctl(ec, ~0ull, val);
> > +}
> > +
> > +DEFINE_DEBUGFS_ATTRIBUTE(control_reg_fops, control_reg_get,
> > +                      control_reg_set, "%016llx\n");
> > +
> > +static int ec_request_set(void *ec, u64 val)
> > +{
> > +     if (val)
> > +             asus_ec_signal_request(ec);
> > +
> > +     return 0;
> > +}
> > +
> > +DEFINE_DEBUGFS_ATTRIBUTE(ec_request_fops, NULL, ec_request_set, "%llu\n");
> > +
> > +static int ec_irq_set(void *ec, u64 val)
> > +{
> > +     struct asus_ec_data *priv = to_ec_data(ec);
> > +
> > +     if (val)
> > +             irq_wake_thread(priv->self->irq, priv);
> > +
> > +     return 0;
> > +}
> > +
> > +DEFINE_DEBUGFS_ATTRIBUTE(ec_irq_fops, NULL, ec_irq_set, "%llu\n");
> > +
> > +static void asus_ec_debugfs_remove(void *debugfs_root)
> > +{
> > +     debugfs_remove_recursive(debugfs_root);
> > +}
> > +
> > +static void devm_asus_ec_debugfs_init(struct device *dev)
> > +{
> > +     struct asusec_info *ec = dev_get_drvdata(dev);
> > +     struct asus_ec_data *priv = to_ec_data(ec);
> > +     struct dentry *debugfs_root, *dockram_dir;
> > +     char *name = devm_kasprintf(dev, GFP_KERNEL, "asus-ec-%s",
> > +                                 priv->data->name);
> > +
> > +     debugfs_root = debugfs_create_dir(name, NULL);
> > +     dockram_dir = debugfs_create_dir("dockram", debugfs_root);
> > +
> > +     debugfs_create_file("ec_irq", 0200, debugfs_root, ec,
> > +                         &ec_irq_fops);
> > +     debugfs_create_file("ec_request", 0200, debugfs_root, ec,
> > +                         &ec_request_fops);
> > +     debugfs_create_file("control_reg", 0644, dockram_dir, ec,
> > +                         &control_reg_fops);
> > +     debugfs_create_file("dockram", 0644, dockram_dir,
> > +                         priv->info.dockram, &dockram_fops);
> > +
> > +     devm_add_action_or_reset(dev, asus_ec_debugfs_remove, debugfs_root);
> > +}
>
> Why is this being controlled via debugfs?
>
> Use the correct kernel APIs instead.
>
> If this is just for development, keep it and all of the extra verbose
> printing in the BSP tree.  It does not belong in the upstream kernel.
>
> > +static void asus_ec_release_dockram_dev(void *client)
> > +{
> > +     i2c_unregister_device(client);
> > +}
> > +
> > +static struct i2c_client *devm_asus_dockram_get(struct device *dev)
> > +{
> > +     struct i2c_client *parent = to_i2c_client(dev);
> > +     struct i2c_client *dockram;
> > +     struct dockram_ec_data *priv;
> > +     int ret;
> > +
> > +     dockram = i2c_new_ancillary_device(parent, "dockram",
> > +                                        parent->addr + 2);
> > +     if (IS_ERR(dockram))
> > +             return dockram;
> > +
> > +     ret = devm_add_action_or_reset(dev, asus_ec_release_dockram_dev,
> > +                                    dockram);
> > +     if (ret)
> > +             return ERR_PTR(ret);
> > +
> > +     priv = devm_kzalloc(&dockram->dev, sizeof(*priv), GFP_KERNEL);
> > +     if (!priv)
> > +             return ERR_PTR(-ENOMEM);
> > +
> > +     i2c_set_clientdata(dockram, priv);
> > +     mutex_init(&priv->ctl_lock);
> > +
> > +     return dockram;
> > +}
> > +
> > +static int asus_ec_probe(struct i2c_client *client)
> > +{
> > +     struct device *dev = &client->dev;
> > +     struct asus_ec_data *priv;
>
> Could we use a clearer, more specific name for the private data variable
> instead of the generic term 'priv'? Using something like 'ddata' is usually
> preferred.
>
> > +     int ret;
> > +
> > +     if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_I2C_BLOCK))
> > +             return dev_err_probe(dev, -ENXIO,
> > +                     "I2C bus is missing required SMBus block mode support\n");
> > +
> > +     priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +     if (!priv)
> > +             return -ENOMEM;
> > +
> > +     priv->data = device_get_match_data(dev);
> > +     if (!priv->data)
> > +             return -ENODEV;
> > +
> > +     i2c_set_clientdata(client, priv);
> > +     priv->self = client;
> > +
> > +     priv->info.dockram = devm_asus_dockram_get(dev);
> > +     if (IS_ERR(priv->info.dockram))
> > +             return dev_err_probe(dev, PTR_ERR(priv->info.dockram),
> > +                                  "failed to get dockram\n");
> > +
> > +     priv->ecreq = devm_gpiod_get(dev, "request", GPIOD_OUT_LOW);
> > +     if (IS_ERR(priv->ecreq))
> > +             return dev_err_probe(dev, PTR_ERR(priv->ecreq),
> > +                                  "failed to get request GPIO\n");
>
> "get" or "request"
>
> > +     BLOCKING_INIT_NOTIFIER_HEAD(&priv->info.notify_list);
> > +     mutex_init(&priv->ecreq_lock);
> > +
> > +     asus_ec_signal_request(&priv->info);
> > +
> > +     ret = asus_ec_detect(priv);
> > +     if (ret)
> > +             return dev_err_probe(dev, ret, "failed to detect EC version\n");
> > +
> > +     ret = devm_request_threaded_irq(dev, client->irq, NULL,
> > +                                     &asus_ec_interrupt,
> > +                                     IRQF_ONESHOT | IRQF_SHARED,
> > +                                     client->name, priv);
> > +     if (ret)
> > +             return dev_err_probe(dev, ret, "failed to register IRQ\n");
> > +
> > +     /* Parent I2C controller uses DMA, ASUS EC and child devices do not */
> > +     client->dev.coherent_dma_mask = 0;
> > +     client->dev.dma_mask = &client->dev.coherent_dma_mask;
> > +
> > +     if (IS_ENABLED(CONFIG_DEBUG_FS))
> > +             devm_asus_ec_debugfs_init(dev);
> > +
> > +     return devm_mfd_add_devices(dev, 0, priv->data->mfd_devices,
> > +                                 priv->data->num_devices, NULL, 0, NULL);
> > +}
> > +
> > +static const struct mfd_cell asus_ec_sl101_dock_mfd_devices[] = {
> > +     {
> > +             .name = "asus-transformer-ec-kbc",
> > +     },
> > +};
> > +
> > +static const struct asus_ec_chip_data asus_ec_sl101_dock_data = {
> > +     .name = "dock",
> > +     .mfd_devices = asus_ec_sl101_dock_mfd_devices,
> > +     .num_devices = ARRAY_SIZE(asus_ec_sl101_dock_mfd_devices),
> > +     .clr_fmode = false,
> > +};
> > +
> > +static const struct mfd_cell asus_ec_tf101_dock_mfd_devices[] = {
> > +     {
> > +             .name = "asus-transformer-ec-battery",
> > +             .id = 1,
>
> Why doesn't PLATFORM_DEVID_AUTO already do the right thing?
>
> > +     }, {
> > +             .name = "asus-transformer-ec-charger",
> > +             .id = 1,
> > +     }, {
> > +             .name = "asus-transformer-ec-led",
> > +             .id = 1,
> > +     }, {
> > +             .name = "asus-transformer-ec-keys",
> > +     }, {
> > +             .name = "asus-transformer-ec-kbc",
> > +     },
> > +};
> > +
> > +static const struct asus_ec_chip_data asus_ec_tf101_dock_data = {
> > +     .name = "dock",
> > +     .mfd_devices = asus_ec_tf101_dock_mfd_devices,
> > +     .num_devices = ARRAY_SIZE(asus_ec_tf101_dock_mfd_devices),
> > +     .clr_fmode = false,
> > +};
> > +
> > +static const struct mfd_cell asus_ec_tf201_pad_mfd_devices[] = {
> > +     {
> > +             .name = "asus-transformer-ec-battery",
> > +             .id = 0,
> > +     }, {
> > +             .name = "asus-transformer-ec-led",
> > +             .id = 0,
> > +     },
> > +};
> > +
> > +static const struct asus_ec_chip_data asus_ec_tf201_pad_data = {
> > +     .name = "pad",
> > +     .mfd_devices = asus_ec_tf201_pad_mfd_devices,
> > +     .num_devices = ARRAY_SIZE(asus_ec_tf201_pad_mfd_devices),
> > +     .clr_fmode = true,
> > +};
> > +
> > +static const struct mfd_cell asus_ec_tf600t_pad_mfd_devices[] = {
> > +     {
> > +             .name = "asus-transformer-ec-battery",
> > +             .id = 0,
> > +     }, {
> > +             .name = "asus-transformer-ec-charger",
> > +             .id = 0,
> > +     }, {
> > +             .name = "asus-transformer-ec-led",
> > +             .id = 0,
> > +     },
> > +};
> > +
> > +static const struct asus_ec_chip_data asus_ec_tf600t_pad_data = {
> > +     .name = "pad",
> > +     .mfd_devices = asus_ec_tf600t_pad_mfd_devices,
> > +     .num_devices = ARRAY_SIZE(asus_ec_tf600t_pad_mfd_devices),
> > +     .clr_fmode = true,
> > +};
> > +
> > +static const struct of_device_id asus_ec_match[] = {
> > +     { .compatible = "asus,sl101-ec-dock", .data = &asus_ec_sl101_dock_data },
> > +     { .compatible = "asus,tf101-ec-dock", .data = &asus_ec_tf101_dock_data },
>
> Could we use the match table's data field to store an 'enum' or integer ID
> instead of passing platform data via the '.data' field? We could then use a
> 'switch' statement in the C code to select the correct 'mfd_cell' array.
>
> > +     { .compatible = "asus,tf201-ec-pad", .data = &asus_ec_tf201_pad_data },
> > +     { .compatible = "asus,tf600t-ec-pad", .data = &asus_ec_tf600t_pad_data },
> > +     { }
> > +};
> > +MODULE_DEVICE_TABLE(of, asus_ec_match);
> > +
> > +static struct i2c_driver asus_ec_driver = {
> > +     .driver = {
> > +             .name = "asus-transformer-ec",
> > +             .of_match_table = asus_ec_match,
> > +     },
> > +     .probe = asus_ec_probe,
> > +};
> > +module_i2c_driver(asus_ec_driver);
> > +
> > +MODULE_AUTHOR("Michał Mirosław <mirq-linux@rere.qmqm.pl>");
> > +MODULE_AUTHOR("Svyatoslav Ryhel <clamor95@gmail.com>");
> > +MODULE_DESCRIPTION("ASUS Transformer's EC driver");
> > +MODULE_LICENSE("GPL");
> > diff --git a/include/linux/mfd/asus-transformer-ec.h b/include/linux/mfd/asus-transformer-ec.h
> > new file mode 100644
> > index 000000000000..0a72de40352e
> > --- /dev/null
> > +++ b/include/linux/mfd/asus-transformer-ec.h
> > @@ -0,0 +1,162 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +#ifndef __MFD_ASUS_TRANSFORMER_EC_H
> > +#define __MFD_ASUS_TRANSFORMER_EC_H
> > +
> > +#include <linux/notifier.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/workqueue.h>
> > +
> > +struct i2c_client;
> > +
> > +struct asusec_info {
> > +     const char *model;
> > +     const char *name;
> > +     struct i2c_client *dockram;
> > +     struct workqueue_struct *wq;
> > +     struct blocking_notifier_head notify_list;
> > +};
> > +
> > +#define DOCKRAM_ENTRIES                      0x100
> > +#define DOCKRAM_ENTRY_SIZE           32
> > +#define DOCKRAM_ENTRY_BUFSIZE                (DOCKRAM_ENTRY_SIZE + 1)
> > +
> > +/* interrupt sources */
> > +#define ASUSEC_OBF_MASK                      BIT(0)
> > +#define ASUSEC_KEY_MASK                      BIT(2)
> > +#define ASUSEC_KBC_MASK                      BIT(3)
> > +#define ASUSEC_AUX_MASK                      BIT(5)
> > +#define ASUSEC_SCI_MASK                      BIT(6)
> > +#define ASUSEC_SMI_MASK                      BIT(7)
> > +
> > +/* SMI notification codes */
> > +#define ASUSEC_SMI_POWER_NOTIFY              0x31    /* [un]plugging USB cable */
> > +#define ASUSEC_SMI_HANDSHAKE         0x50    /* response to ec_req edge */
> > +#define ASUSEC_SMI_WAKE                      0x53
> > +#define ASUSEC_SMI_RESET             0x5f
> > +#define ASUSEC_SMI_ADAPTER_EVENT     0x60    /* [un]plugging charger to dock */
> > +#define ASUSEC_SMI_BACKLIGHT_ON              0x63
> > +#define ASUSEC_SMI_AUDIO_DOCK_IN     0x70
> > +
> > +#define ASUSEC_SMI_ACTION(code)              (ASUSEC_SMI_MASK | ASUSEC_OBF_MASK | \
> > +                                     (ASUSEC_SMI_##code << 8))
> > +
> > +/* control register [0x0a] layout */
> > +#define ASUSEC_CTL_SIZE                      8
> > +
> > +/*
> > + * EC reports power from 40-pin connector in the LSB of the control
> > + * register.  The following values have been observed (xor 0x02):
> > + *
> > + * PAD-ec no-plug  0x40 / PAD-ec DOCK     0x20 / DOCK-ec no-plug 0x40
> > + * PAD-ec AC       0x25 / PAD-ec DOCK+AC  0x24 / DOCK-ec AC      0x25
> > + * PAD-ec USB      0x45 / PAD-ec DOCK+USB 0x24 / DOCK-ec USB     0x41
> > + */
> > +
> > +#define ASUSEC_CTL_DIRECT_POWER_SOURCE       BIT_ULL(0)
> > +#define ASUSEC_STAT_CHARGING         BIT_ULL(2)
> > +#define ASUSEC_CTL_FULL_POWER_SOURCE BIT_ULL(5)
> > +#define ASUSEC_CTL_SUSB_MODE         BIT_ULL(9)
> > +#define ASUSEC_CMD_SUSPEND_S3                BIT_ULL(33)
> > +#define ASUSEC_CTL_TEST_DISCHARGE    BIT_ULL(35)
> > +#define ASUSEC_CMD_SUSPEND_INHIBIT   BIT_ULL(37)
> > +#define ASUSEC_CTL_FACTORY_MODE              BIT_ULL(38)
> > +#define ASUSEC_CTL_KEEP_AWAKE                BIT_ULL(39)
> > +#define ASUSEC_CTL_USB_CHARGE                BIT_ULL(40)
> > +#define ASUSEC_CTL_LED_BLINK         BIT_ULL(40)
> > +#define ASUSEC_CTL_LED_AMBER         BIT_ULL(41)
> > +#define ASUSEC_CTL_LED_GREEN         BIT_ULL(42)
> > +#define ASUSEC_CMD_SWITCH_HDMI               BIT_ULL(56)
> > +#define ASUSEC_CMD_WIN_SHUTDOWN              BIT_ULL(62)
> > +
> > +#define ASUSEC_DOCKRAM_INFO_MODEL    0x01
> > +#define ASUSEC_DOCKRAM_INFO_FW               0x02
> > +#define ASUSEC_DOCKRAM_INFO_CFGFMT   0x03
> > +#define ASUSEC_DOCKRAM_INFO_HW               0x04
> > +#define ASUSEC_DOCKRAM_CONTROL               0x0a
> > +#define ASUSEC_DOCKRAM_BATT_CTL              0x14
> > +
> > +#define ASUSEC_WRITE_BUF             0x64
> > +#define ASUSEC_READ_BUF                      0x6a
> > +
> > +/* dockram comm */
> > +int asus_dockram_read(struct i2c_client *client, int reg, char *buf);
> > +int asus_dockram_write(struct i2c_client *client, int reg, const char *buf);
> > +int asus_dockram_access_ctl(struct i2c_client *client,
> > +                         u64 *out, u64 mask, u64 xor);
> > +
> > +/* EC public API */
> > +
> > +/**
> > + * cell_to_ec - Request the shared ASUS EC structure via a subdevice's pdev.
> > + * @pdev: EC subdevice pdev requesting access to the shared ASUS EC structure.
> > + *
> > + * Returns a pointer to the asusec_info structure.
> > + */
> > +static inline struct asusec_info *cell_to_ec(struct platform_device *pdev)
> > +{
> > +     return dev_get_drvdata(pdev->dev.parent);
> > +}
>
> This is both misleading and already exists.
>
> > +
> > +/**
> > + * asus_ec_get_ctl - Read from the DockRAM control register.
> > + * @ec:  Pointer to the shared ASUS EC structure.
> > + * @out: Pointer to the variable where the register value will be stored.
> > + *
> > + * Performs a control register read and stores the value in @out.
> > + *
> > + * Return: 0 on success, or a negative errno code on failure.
> > + */
> > +static inline int asus_ec_get_ctl(const struct asusec_info *ec, u64 *out)
> > +{
> > +     return asus_dockram_access_ctl(ec->dockram, out, 0, 0);
> > +}
> > +
> > +/**
> > + * asus_ec_update_ctl - Update the DockRAM control register.
> > + * @ec:   Pointer to the shared ASUS EC structure.
> > + * @mask: Bitmask of bits to be cleared.
> > + * @xor:  Bitmask of bits to be toggled or set (via XOR).
> > + *
> > + * Performs a read-modify-write update on the control register using
> > + * the provided @mask and @xor values.
> > + *
> > + * Return: 0 on success, or a negative errno code on failure.
> > + */
> > +static inline int asus_ec_update_ctl(const struct asusec_info *ec,
> > +                                  u64 mask, u64 xor)
> > +{
> > +     return asus_dockram_access_ctl(ec->dockram, NULL, mask, xor);
> > +}
> > +
> > +/**
> > + * asus_ec_set_ctl_bits - Sets bits of the DockRAM control register.
> > + * @ec:   Pointer to the shared ASUS EC structure.
> > + * @mask: Bitmask of bits to be set.
> > + *
> > + * Sets bits of the control register using the provided @mask value.
> > + *
> > + * Return: 0 on success, or a negative errno code on failure.
> > + */
> > +static inline int asus_ec_set_ctl_bits(const struct asusec_info *ec, u64 mask)
> > +{
> > +     return asus_dockram_access_ctl(ec->dockram, NULL, mask, mask);
> > +}
> > +
> > +/**
> > + * asus_ec_clear_ctl_bits - Clears bits of the DockRAM control register.
> > + * @ec:   Pointer to the shared ASUS EC structure.
> > + * @mask: Bitmask of bits to be cleared.
> > + *
> > + * Clears bits of the control register using the provided @mask value.
> > + *
> > + * Return: 0 on success, or a negative errno code on failure.
> > + */
> > +static inline int asus_ec_clear_ctl_bits(const struct asusec_info *ec, u64 mask)
> > +{
> > +     return asus_dockram_access_ctl(ec->dockram, NULL, mask, 0);
> > +}
>
> We don't typically allow abstraction for the sake of abstraction.
>
> > +int asus_ec_i2c_command(const struct asusec_info *ec, u16 data);
> > +int devm_asus_ec_register_notifier(struct platform_device *dev,
> > +                                struct notifier_block *nb);
> > +#endif /* __MFD_ASUS_TRANSFORMER_EC_H */
> > --
> > 2.51.0
> >
> >
>
> --
> Lee Jones

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v6 2/7] mfd: Add driver for ASUS Transformer embedded controller
  2026-05-14 10:31     ` Svyatoslav Ryhel
@ 2026-05-14 15:50       ` Lee Jones
  2026-05-14 16:06         ` Svyatoslav Ryhel
  0 siblings, 1 reply; 6+ messages in thread
From: Lee Jones @ 2026-05-14 15:50 UTC (permalink / raw)
  To: Svyatoslav Ryhel
  Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Dmitry Torokhov,
	Pavel Machek, Sebastian Reichel, Ion Agorria,
	Michał Mirosław, devicetree, linux-kernel, linux-input,
	linux-leds, linux-pm

On Thu, 14 May 2026, Svyatoslav Ryhel wrote:

> чт, 14 трав. 2026 р. о 13:02 Lee Jones <lee@kernel.org> пише:
> >
> > On Sat, 02 May 2026, Svyatoslav Ryhel wrote:
> >
> > > From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > >
> > > Support Nuvoton NPCE795-based ECs as used in Asus Transformer TF201,
> > > TF300T, TF300TG, TF300TL and TF700T pad and dock, as well as TF101 dock
> > > and TF600T, P1801-T and TF701T pad. This is a glue driver handling
> > > detection and common operations for EC's functions.
> > >
> > > Co-developed-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > > ---
> > >  drivers/mfd/Kconfig                     |  14 +
> > >  drivers/mfd/Makefile                    |   1 +
> > >  drivers/mfd/asus-transformer-ec.c       | 762 ++++++++++++++++++++++++
> > >  include/linux/mfd/asus-transformer-ec.h | 162 +++++
> > >  4 files changed, 939 insertions(+)
> > >  create mode 100644 drivers/mfd/asus-transformer-ec.c
> > >  create mode 100644 include/linux/mfd/asus-transformer-ec.h

[...]

> > > +     unsigned int num_devices;
> > > +     bool clr_fmode; /* clear Factory Mode bit in EC control register */
> > > +};
> > > +
> > > +struct asus_ec_data {
> > > +     struct asusec_info info;
> >
> > You have 'data' and 'info' which a) using non-forthcoming nomenclature
> > and doesn't tell me anything and then you b) put 'info' in the device's
> > driver_data attribute which is very confusing.  driver_data should be
> > for what we call ddata which I assume is expressed as 'data' here.
> >
> 
> asusec_info is shared among all child devices and is exposed while
> remaining elements of this struct are for internal use only.

Our terminology for that is usually ddata, that gets stored in
'struct devices' device_data attribute.

> > > +     struct mutex ecreq_lock; /* prevent simultaneous access */
> > > +     struct gpio_desc *ecreq;
> >
> > If I hadn't seen the declaration, I'd have no idea this was a GPIO
> > descriptor.  Please improve the nomenclature throughout.
> >
> > > +     struct i2c_client *self;
> >
> > Again, please use standard naming conventions:
> >
> > % git grep "struct i2c_client" | grep "\*self" | wc -l
> > 0
> >
> > % git grep "struct i2c_client" | grep "\*client" | wc -l
> > 6304
> >
> > % git grep "struct i2c_client" | grep "\*i2c" | wc -l
> > 903
> >
> 
> ok, noted.
> 
> > > +     const struct asus_ec_chip_data *data;
> >
> > 'data', 'priv' and 'info' should be improved.
> >
> > > +     char ec_data[DOCKRAM_ENTRY_BUFSIZE];
> >
> > An array of chars called 'data'.  This could be anything.
> >
> 
> Do you have a comprehensive list of name conventions you find suitable?

Anything descriptive that alludes to the type of data being held there.

There are 100's of good examples, but a handful of generic / bad ones.

> > > +     bool logging_disabled;
> >
> > This debugging tool is probably never going to be used again.
> >
> > Keep it local.
> >
> > > +};
> > > +
> > > +struct dockram_ec_data {
> > > +     struct mutex ctl_lock; /* prevent simultaneous access */
> > > +     char ctl_data[DOCKRAM_ENTRY_BUFSIZE];
> > > +};
> > > +
> > > +#define to_ec_data(ec) \
> > > +     container_of(ec, struct asus_ec_data, info)
> > > +
> > > +/**
> > > + * asus_dockram_read - Read a register from the DockRAM device.
> > > + * @client: Handle to the DockRAM device.
> > > + * @reg: Register to read.
> > > + * @buf: Byte array into which data will be read; must be large enough to
> > > + *    hold the data returned by the DockRAM.
> > > + *
> > > + * This executes the DockRAM read based on the SMBus "block read" protocol
> > > + * or its emulation. It extracts DOCKRAM_ENTRY_SIZE bytes from the set
> > > + * register address.
> > > + *
> > > + * Returns a negative errno code else zero on success.
> > > + */
> > > +int asus_dockram_read(struct i2c_client *client, int reg, char *buf)
> > > +{
> >
> > Have you considered using Regmap for register access instead of
> > implementing custom functions?  Remaps already deals with caching and
> > locking mechanisms that you'd get for free.
> >
> > This looks like it would be replaced with devm_regmap_init_i2c().
> >
> 
> I will consider this, thank you.
> 
> > > +     struct device *dev = &client->dev;
> > > +     int ret;
> > > +
> > > +     memset(buf, 0, DOCKRAM_ENTRY_BUFSIZE);
> > > +     ret = i2c_smbus_read_i2c_block_data(client, reg,
> > > +                                         DOCKRAM_ENTRY_BUFSIZE, buf);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     if (buf[0] > DOCKRAM_ENTRY_SIZE) {
> > > +             dev_err(dev, "bad data len; buffer: %*ph; ret: %d\n",
> > > +                     DOCKRAM_ENTRY_BUFSIZE, buf, ret);
> > > +             return -EPROTO;
> > > +     }
> > > +
> > > +     dev_dbg(dev, "got data; buffer: %*ph; ret: %d\n",
> > > +             DOCKRAM_ENTRY_BUFSIZE, buf, ret);
> >
> > Please remove all of these debug messages.
> >
> 
> Why debug messages cannot be preserved? They are specifically marked as dev_dbg

It's a general convention.

After initial development, they tend to just litter the code-base.

Debug prints can be useful higher up the stack though.

[...] 

> > > +/**
> > > + * devm_asus_ec_register_notifier - Managed registration of notifier to an
> > > + *                               ASUS EC blocking notifier chain.
> > > + * @pdev: Device requesting the notifier (used for resource management).
> > > + * @nb: Notifier block to be registered.
> > > + *
> > > + * Register a notifier to the ASUS EC blocking notifier chain. The notifier
> > > + * will be automatically unregistered when the requesting device is detached.
> > > + *
> > > + * Return: 0 on success or a negative error code on failure.
> > > + */
> > > +int devm_asus_ec_register_notifier(struct platform_device *pdev,
> > > +                                struct notifier_block *nb)
> > > +{
> >
> > Hand-rolling devres managed resources is usually reserved for subsystem
> > level API calls.  Why do the usual device driver .remove() handling work
> > for you?
> >
> 
> This is used by 3 subdevices: serio, keys and charger, so this just
> seems cleaner way to register and deregister notifier.

Clean to me would be to use the infrastructure that's put in place
already.  Unless I am missing the point of all of this.

[...]

> > > +     int ret;
> > > +
> > > +     if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_I2C_BLOCK))
> > > +             return dev_err_probe(dev, -ENXIO,
> > > +                     "I2C bus is missing required SMBus block mode support\n");
> > > +
> > > +     priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > > +     if (!priv)
> > > +             return -ENOMEM;
> > > +
> > > +     priv->data = device_get_match_data(dev);
> > > +     if (!priv->data)
> > > +             return -ENODEV;
> > > +
> > > +     i2c_set_clientdata(client, priv);
> > > +     priv->self = client;
> > > +
> > > +     priv->info.dockram = devm_asus_dockram_get(dev);
> > > +     if (IS_ERR(priv->info.dockram))
> > > +             return dev_err_probe(dev, PTR_ERR(priv->info.dockram),
> > > +                                  "failed to get dockram\n");
> > > +
> > > +     priv->ecreq = devm_gpiod_get(dev, "request", GPIOD_OUT_LOW);
> > > +     if (IS_ERR(priv->ecreq))
> > > +             return dev_err_probe(dev, PTR_ERR(priv->ecreq),
> > > +                                  "failed to get request GPIO\n");
> >
> > "get" or "request"
> >
> 
> request is gpio's name, request gpio

Ah yes.  Maybe use 's to help with that.  Right now is just reads strangely.

[...]

-- 
Lee Jones

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v6 2/7] mfd: Add driver for ASUS Transformer embedded controller
  2026-05-14 15:50       ` Lee Jones
@ 2026-05-14 16:06         ` Svyatoslav Ryhel
  0 siblings, 0 replies; 6+ messages in thread
From: Svyatoslav Ryhel @ 2026-05-14 16:06 UTC (permalink / raw)
  To: Lee Jones
  Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Dmitry Torokhov,
	Pavel Machek, Sebastian Reichel, Ion Agorria,
	Michał Mirosław, devicetree, linux-kernel, linux-input,
	linux-leds, linux-pm

чт, 14 трав. 2026 р. о 18:50 Lee Jones <lee@kernel.org> пише:
>
> On Thu, 14 May 2026, Svyatoslav Ryhel wrote:
>
> > чт, 14 трав. 2026 р. о 13:02 Lee Jones <lee@kernel.org> пише:
> > >
> > > On Sat, 02 May 2026, Svyatoslav Ryhel wrote:
> > >
> > > > From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > > >
> > > > Support Nuvoton NPCE795-based ECs as used in Asus Transformer TF201,
> > > > TF300T, TF300TG, TF300TL and TF700T pad and dock, as well as TF101 dock
> > > > and TF600T, P1801-T and TF701T pad. This is a glue driver handling
> > > > detection and common operations for EC's functions.
> > > >
> > > > Co-developed-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > > > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > > > ---
> > > >  drivers/mfd/Kconfig                     |  14 +
> > > >  drivers/mfd/Makefile                    |   1 +
> > > >  drivers/mfd/asus-transformer-ec.c       | 762 ++++++++++++++++++++++++
> > > >  include/linux/mfd/asus-transformer-ec.h | 162 +++++
> > > >  4 files changed, 939 insertions(+)
> > > >  create mode 100644 drivers/mfd/asus-transformer-ec.c
> > > >  create mode 100644 include/linux/mfd/asus-transformer-ec.h
>
> [...]
>
> > > > +     unsigned int num_devices;
> > > > +     bool clr_fmode; /* clear Factory Mode bit in EC control register */
> > > > +};
> > > > +
> > > > +struct asus_ec_data {
> > > > +     struct asusec_info info;
> > >
> > > You have 'data' and 'info' which a) using non-forthcoming nomenclature
> > > and doesn't tell me anything and then you b) put 'info' in the device's
> > > driver_data attribute which is very confusing.  driver_data should be
> > > for what we call ddata which I assume is expressed as 'data' here.
> > >
> >
> > asusec_info is shared among all child devices and is exposed while
> > remaining elements of this struct are for internal use only.
>
> Our terminology for that is usually ddata, that gets stored in
> 'struct devices' device_data attribute.
>
> > > > +     struct mutex ecreq_lock; /* prevent simultaneous access */
> > > > +     struct gpio_desc *ecreq;
> > >
> > > If I hadn't seen the declaration, I'd have no idea this was a GPIO
> > > descriptor.  Please improve the nomenclature throughout.
> > >
> > > > +     struct i2c_client *self;
> > >
> > > Again, please use standard naming conventions:
> > >
> > > % git grep "struct i2c_client" | grep "\*self" | wc -l
> > > 0
> > >
> > > % git grep "struct i2c_client" | grep "\*client" | wc -l
> > > 6304
> > >
> > > % git grep "struct i2c_client" | grep "\*i2c" | wc -l
> > > 903
> > >
> >
> > ok, noted.
> >
> > > > +     const struct asus_ec_chip_data *data;
> > >
> > > 'data', 'priv' and 'info' should be improved.
> > >
> > > > +     char ec_data[DOCKRAM_ENTRY_BUFSIZE];
> > >
> > > An array of chars called 'data'.  This could be anything.
> > >
> >
> > Do you have a comprehensive list of name conventions you find suitable?
>
> Anything descriptive that alludes to the type of data being held there.
>
> There are 100's of good examples, but a handful of generic / bad ones.
>
> > > > +     bool logging_disabled;
> > >
> > > This debugging tool is probably never going to be used again.
> > >
> > > Keep it local.
> > >
> > > > +};
> > > > +
> > > > +struct dockram_ec_data {
> > > > +     struct mutex ctl_lock; /* prevent simultaneous access */
> > > > +     char ctl_data[DOCKRAM_ENTRY_BUFSIZE];
> > > > +};
> > > > +
> > > > +#define to_ec_data(ec) \
> > > > +     container_of(ec, struct asus_ec_data, info)
> > > > +
> > > > +/**
> > > > + * asus_dockram_read - Read a register from the DockRAM device.
> > > > + * @client: Handle to the DockRAM device.
> > > > + * @reg: Register to read.
> > > > + * @buf: Byte array into which data will be read; must be large enough to
> > > > + *    hold the data returned by the DockRAM.
> > > > + *
> > > > + * This executes the DockRAM read based on the SMBus "block read" protocol
> > > > + * or its emulation. It extracts DOCKRAM_ENTRY_SIZE bytes from the set
> > > > + * register address.
> > > > + *
> > > > + * Returns a negative errno code else zero on success.
> > > > + */
> > > > +int asus_dockram_read(struct i2c_client *client, int reg, char *buf)
> > > > +{
> > >
> > > Have you considered using Regmap for register access instead of
> > > implementing custom functions?  Remaps already deals with caching and
> > > locking mechanisms that you'd get for free.
> > >
> > > This looks like it would be replaced with devm_regmap_init_i2c().
> > >
> >
> > I will consider this, thank you.
> >

It seems that regmap does not fit for this purpose, but I might switch
to plain i2c_smbus_read_i2c_block_data

> > > > +     struct device *dev = &client->dev;
> > > > +     int ret;
> > > > +
> > > > +     memset(buf, 0, DOCKRAM_ENTRY_BUFSIZE);
> > > > +     ret = i2c_smbus_read_i2c_block_data(client, reg,
> > > > +                                         DOCKRAM_ENTRY_BUFSIZE, buf);
> > > > +     if (ret < 0)
> > > > +             return ret;
> > > > +
> > > > +     if (buf[0] > DOCKRAM_ENTRY_SIZE) {
> > > > +             dev_err(dev, "bad data len; buffer: %*ph; ret: %d\n",
> > > > +                     DOCKRAM_ENTRY_BUFSIZE, buf, ret);
> > > > +             return -EPROTO;
> > > > +     }
> > > > +
> > > > +     dev_dbg(dev, "got data; buffer: %*ph; ret: %d\n",
> > > > +             DOCKRAM_ENTRY_BUFSIZE, buf, ret);
> > >
> > > Please remove all of these debug messages.
> > >
> >
> > Why debug messages cannot be preserved? They are specifically marked as dev_dbg
>
> It's a general convention.
>
> After initial development, they tend to just litter the code-base.
>
> Debug prints can be useful higher up the stack though.
>

I am fine with removing all debugs and logging but I strongly would
like to keep EC model and firmware version along with susb and factory
status. That may be quite useful in identifying EC used and its
behavior without need in rebuilding the kernel and digging huge piles
of downstream code in order to find how to dump these values.

> [...]
>
> > > > +/**
> > > > + * devm_asus_ec_register_notifier - Managed registration of notifier to an
> > > > + *                               ASUS EC blocking notifier chain.
> > > > + * @pdev: Device requesting the notifier (used for resource management).
> > > > + * @nb: Notifier block to be registered.
> > > > + *
> > > > + * Register a notifier to the ASUS EC blocking notifier chain. The notifier
> > > > + * will be automatically unregistered when the requesting device is detached.
> > > > + *
> > > > + * Return: 0 on success or a negative error code on failure.
> > > > + */
> > > > +int devm_asus_ec_register_notifier(struct platform_device *pdev,
> > > > +                                struct notifier_block *nb)
> > > > +{
> > >
> > > Hand-rolling devres managed resources is usually reserved for subsystem
> > > level API calls.  Why do the usual device driver .remove() handling work
> > > for you?
> > >
> >
> > This is used by 3 subdevices: serio, keys and charger, so this just
> > seems cleaner way to register and deregister notifier.
>
> Clean to me would be to use the infrastructure that's put in place
> already.  Unless I am missing the point of all of this.
>
> [...]
>
> > > > +     int ret;
> > > > +
> > > > +     if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_I2C_BLOCK))
> > > > +             return dev_err_probe(dev, -ENXIO,
> > > > +                     "I2C bus is missing required SMBus block mode support\n");
> > > > +
> > > > +     priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > > > +     if (!priv)
> > > > +             return -ENOMEM;
> > > > +
> > > > +     priv->data = device_get_match_data(dev);
> > > > +     if (!priv->data)
> > > > +             return -ENODEV;
> > > > +
> > > > +     i2c_set_clientdata(client, priv);
> > > > +     priv->self = client;
> > > > +
> > > > +     priv->info.dockram = devm_asus_dockram_get(dev);
> > > > +     if (IS_ERR(priv->info.dockram))
> > > > +             return dev_err_probe(dev, PTR_ERR(priv->info.dockram),
> > > > +                                  "failed to get dockram\n");
> > > > +
> > > > +     priv->ecreq = devm_gpiod_get(dev, "request", GPIOD_OUT_LOW);
> > > > +     if (IS_ERR(priv->ecreq))
> > > > +             return dev_err_probe(dev, PTR_ERR(priv->ecreq),
> > > > +                                  "failed to get request GPIO\n");
> > >
> > > "get" or "request"
> > >
> >
> > request is gpio's name, request gpio
>
> Ah yes.  Maybe use 's to help with that.  Right now is just reads strangely.
>
> [...]
>
> --
> Lee Jones

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2026-05-14 16:06 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <20260502124055.22475-1-clamor95@gmail.com>
     [not found] ` <20260502124055.22475-3-clamor95@gmail.com>
2026-05-14 10:02   ` [PATCH v6 2/7] mfd: Add driver for ASUS Transformer embedded controller Lee Jones
2026-05-14 10:31     ` Svyatoslav Ryhel
2026-05-14 15:50       ` Lee Jones
2026-05-14 16:06         ` Svyatoslav Ryhel
2026-05-14 11:02     ` Svyatoslav Ryhel
     [not found] ` <20260502124055.22475-6-clamor95@gmail.com>
2026-05-14 10:09   ` [PATCH v6 5/7] leds: Add driver for ASUS Transformer LEDs Lee Jones

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox