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

[Svyatoslav Ryhel]

чт, 14 трав. 2026 р. о 18:50 Lee Jones <lee@xxxxxxxxxx> пише:
>
> On Thu, 14 May 2026, Svyatoslav Ryhel wrote:
>
> > чт, 14 трав. 2026 р. о 13:02 Lee Jones <lee@xxxxxxxxxx> пише:
> > >
> > > On Sat, 02 May 2026, Svyatoslav Ryhel wrote:
> > >
> > > > From: Michał Mirosław <mirq-linux@xxxxxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxx>
> > > > Signed-off-by: Svyatoslav Ryhel <clamor95@xxxxxxxxx>
> > > > Signed-off-by: Michał Mirosław <mirq-linux@xxxxxxxxxxxx>
> > > > ---
> > > >  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