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

[Svyatoslav Ryhel]

пн, 18 трав. 2026 р. о 13:13 Lee Jones <lee@xxxxxxxxxx> пише:
>
> On Thu, 14 May 2026, Svyatoslav Ryhel wrote:
>
> > чт, 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
>
> Can you explain why Regmap doesn't work for you?
>
> Regmap is just a wrapper about i2c_smbus_read_i2c_block_data() and friends.
>

Honestly, it seems that this will cause more hassle. Regmap has max
bit size of reg 64 which will require use of regmap_read/write_raw and
obviously no regmap bit update operations will be possible. If this
chip was a plain 8 bit register i2c device, obviously I would use
regmap.

I have removed most of custom functions in favor for i2c smbus block
helpers and left only asus_dockram_access_ctl since it is used in many
places.

> > > > > > +     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.
>
> Yes, you can keep this sort of thing as INFO.
>

Noted. I have send v7 with your suggestions applied. Change log is in
the cover commit.

> --
> Lee Jones