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

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