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

[Lee Jones <lee@kernel.org>]

On Thu, 11 Jun 2026, Svyatoslav Ryhel wrote:

> чт, 11 черв. 2026 р. о 14:17 Lee Jones <lee@kernel.org> пише:
> >
> > On Thu, 28 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                     |  16 +
> > >  drivers/mfd/Makefile                    |   1 +
> > >  drivers/mfd/asus-transformer-ec.c       | 542 ++++++++++++++++++++++++
> > >  include/linux/mfd/asus-transformer-ec.h |  92 ++++
> > >  4 files changed, 651 insertions(+)
> > >  create mode 100644 drivers/mfd/asus-transformer-ec.c
> > >  create mode 100644 include/linux/mfd/asus-transformer-ec.h

[...]

> > > +static void asus_ec_clear_buffer(struct asus_ec_data *ddata)
> > > +{
> > > +     int ret, retry = ASUSEC_RSP_BUFFER_SIZE;
> > > +
> > > +     /*
> > > +      * Read the buffer till we get valid data by checking ASUSEC_OBF_MASK
> > > +      * of the status byte or till we reach end of the 256 byte buffer.
> > > +      */
> > > +     while (retry--) {
> > > +             ret = i2c_smbus_read_i2c_block_data(ddata->client, ASUSEC_READ_BUF,
> > > +                                                 ASUSEC_ENTRY_SIZE,
> > > +                                                 ddata->ec_buf);
> > > +             if (ret < ASUSEC_ENTRY_SIZE)
> > > +                     continue;
> > > +
> > > +             if (ddata->ec_buf[ASUSEC_IRQ_STATUS] & ASUSEC_OBF_MASK)
> > > +                     continue;
> > > +
> > > +             break;
> > > +     }
> > > +}
> > > +
> > > +static int asus_ec_log_info(struct asus_ec_data *ddata, unsigned int reg,
> > > +                         const char *name, const char **out)

If we can avoid points to pointers, then please do.

We already have ddata, so we can just set the name?

It will remove a lot of the following complexity / ugliness.

> > > +{
> > > +     struct device *dev = &ddata->client->dev;
> > > +     u8 buf[ASUSEC_ENTRY_BUFSIZE];
> > > +     int ret;
> > > +
> > > +     memset(buf, 0, ASUSEC_ENTRY_BUFSIZE);
> > > +     ret = i2c_smbus_read_i2c_block_data(ddata->ec.dockram, reg,
> > > +                                         ASUSEC_ENTRY_SIZE, buf);
> > > +     if (ret < ASUSEC_ENTRY_SIZE)
> > > +             return ret;
> >
> > Same here.  These should be negative.
> >
> 
> return ret < 0 ? ret : -EIO same as above
> 
> > > +
> > > +     if (buf[0] > ASUSEC_ENTRY_SIZE) {
> > > +             dev_err(dev, "bad data len; buffer: %*ph; ret: %d\n",
> > > +                     ASUSEC_ENTRY_BUFSIZE, buf, ret);
> > > +             return -EPROTO;
> > > +     }
> > > +
> > > +     if (!ddata->logging_disabled) {
> > > +             dev_info(dev, "%-14s: %.*s\n", name, buf[0], buf + 1);
> > > +
> > > +             if (out) {
> > > +                     *out = devm_kasprintf(dev, GFP_KERNEL, "%.*s",
> > > +                                           buf[0], buf + 1);
> > > +                     if (!*out)
> > > +                             return -ENOMEM;
> > > +             }
> > > +     }
> >
> > FWIW, I hate this!  What does it give you now that development is done?
> >
> 
> We have already discussed this, and you agreed that EC and firmware
> prints may stay! This prints EC model and firmware info as well as EC
> firmware behavior. It allows identify possible new revisions of EC -
> Firmware combo and address possible regressions (check if it is chip
> malfunction or firmware needs a new programming model) without
> rebuilding kernel and digging downstream kernel for needed bits of
> code.

Right, so just print it out and remove all of the 'logging_disabled' and
'out' nonsense.

> > > +     return 0;
> > > +}
> > > +
> > > +static int asus_ec_reset(struct asus_ec_data *ddata)
> > > +{
> > > +     int retry, ret;
> > > +
> > > +     guard(mutex)(&ddata->ecreq_lock);
> > > +
> > > +     for (retry = 0; retry < ASUSEC_RETRY_MAX; retry++) {
> >
> > for (int retry = ... is generally preferred for throwaway variables.
> >
> 
> Not that I care too much, but I am defining ret anyway, why not add
> retry too there?

This is the new and preferred way to use throw-away variables.

ret is not a throw-away variable.

[...]

> > > +static int asus_ec_set_factory_mode(struct asus_ec_data *ddata,
> > > +                                 enum asusec_mode fmode)
> > > +{
> > > +     dev_info(&ddata->client->dev, "Entering %s mode.\n",
> > > +              fmode == ASUSEC_MODE_FACTORY ? "factory" : "normal");
> > > +
> > > +     return asus_dockram_access_ctl(ddata->ec.dockram, NULL,
> > > +                                    ASUSEC_CTL_FACTORY_MODE,
> > > +                                    fmode == ASUSEC_MODE_FACTORY ?
> > > +                                    ASUSEC_CTL_FACTORY_MODE : 0);
> >
> > Why not create make:
> >
> > ASUSEC_MODE_FACTORY == ASUSEC_CTL_FACTORY_MODE
> >
> > What happens to NORMAL?
> >
> 
> ASUSEC_CTL_FACTORY_MODE is a bit in the ctl register. For NORMAL mode

I get that, but if the values can be shared, it make the code simpler.

> bit is cleared,
> for FACTORY bit it set, for NONE bit is ignored.
> 
> > > +}
> > > +
> > > +static int asus_ec_detect(struct asus_ec_data *ddata)
> > > +{
> > > +     int ret;
> > > +
> > > +     ret = asus_ec_reset(ddata);
> > > +     if (ret)
> > > +             goto err_exit;
> > > +
> > > +     asus_ec_clear_buffer(ddata);
> > > +
> > > +     ret = asus_ec_log_info(ddata, ASUSEC_DOCKRAM_INFO_MODEL, "Model",
> > > +                            &ddata->ec.model);

Where is this model used?

> > You can use 100-chars and make the code look beautiful! :)
> >
> 
> Not every subsystem permits 100 chars, some stick to 80 as a strict
> rule, so it is better be safe.

Right, but we are forward thinking here.

You can and should use 100-chars in this subsystem.

> > > +     if (ret)
> > > +             goto err_exit;
> > > +
> > > +     ret = asus_ec_log_info(ddata, ASUSEC_DOCKRAM_INFO_FW, "FW version",
> > > +                            NULL);
> > > +     if (ret)
> > > +             goto err_exit;
> > > +
> > > +     ret = asus_ec_log_info(ddata, ASUSEC_DOCKRAM_INFO_CFGFMT, "Config format",
> > > +                            NULL);
> > > +     if (ret)
> > > +             goto err_exit;
> > > +
> > > +     ret = asus_ec_log_info(ddata, ASUSEC_DOCKRAM_INFO_HW, "HW version",
> > > +                            NULL);
> > > +     if (ret)
> > > +             goto err_exit;
> > > +
> > > +     /* Disable logging on next EC request */
> >
> > Why, but why?
> >
> 
> Cause EC requests are frequent (handshake/reset) and constant logging
> same data is not acceptable.

Then rid the prints entirely or do them at a more appropriate point like
during probe?

Or maybe consider dev_info_once() and friends.

> > > +     ddata->logging_disabled = true;
> > > +
> > > +     /* Check and inform about EC firmware behavior */
> > > +     ret = asus_ec_susb_on_status(ddata);
> > > +     if (ret)
> > > +             goto err_exit;
> > > +
> > > +     ddata->ec.name = ddata->info->name;
> > > +
> > > +     /* Some EC require factory mode to be set normal on each request */
> > > +     if (ddata->info->fmode)
> > > +             ret = asus_ec_set_factory_mode(ddata, ddata->info->fmode);
> > > +
> > > +err_exit:
> > > +     if (ret)
> > > +             dev_err(&ddata->client->dev, "failed to access EC: %d\n", ret);
> > > +
> > > +     return ret;
> > > +}
> > > +
> > > +static void asus_ec_handle_smi(struct asus_ec_data *ddata, unsigned int code)
> > > +{
> > > +     switch (code) {
> > > +     case ASUSEC_SMI_HANDSHAKE:
> > > +     case ASUSEC_SMI_RESET:
> > > +             asus_ec_detect(ddata);
> > > +             break;
> > > +     }
> > > +}
> > > +
> > > +static irqreturn_t asus_ec_interrupt(int irq, void *dev_id)
> > > +{
> > > +     struct asus_ec_data *ddata = dev_id;
> > > +     unsigned long notify_action;
> > > +     int ret;
> > > +
> > > +     ret = i2c_smbus_read_i2c_block_data(ddata->client, ASUSEC_READ_BUF,
> > > +                                         ASUSEC_ENTRY_SIZE, ddata->ec_buf);
> > > +     if (ret < ASUSEC_ENTRY_SIZE ||
> > > +         !(ddata->ec_buf[ASUSEC_IRQ_STATUS] & ASUSEC_OBF_MASK))
> >
> > Unwrap for readability.
> >
> > Also, I think a comment would be helpful.
> >
> 
> if (ret < ASUSEC_ENTRY_SIZE)
>     return IRQ_NONE;
> 
> ret = ddata->ec_buf[ASUSEC_IRQ_STATUS] & ASUSEC_OBF_MASK;
> if (!ret)
>     return IRQ_NONE;
> 
> This would be acceptable? (I will add comments later on)

Yes, better.

If you're not using ret again, you could just put 'ddata.." inside the if().

> > > +             return IRQ_NONE;
> > > +
> > > +     notify_action = ddata->ec_buf[ASUSEC_IRQ_STATUS];
> > > +     if (notify_action & ASUSEC_SMI_MASK) {
> > > +             unsigned int code = ddata->ec_buf[ASUSEC_SMI_CODE];
> > > +
> > > +             asus_ec_handle_smi(ddata, code);
> > > +
> > > +             notify_action |= code << 8;
> > > +     }
> > > +
> > > +     blocking_notifier_call_chain(&ddata->ec.notify_list,
> > > +                                  notify_action, ddata->ec_buf);
> > > +
> > > +     return IRQ_HANDLED;
> > > +}
> > > +
> > > +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 *ddata;
> > > +     int ret;
> > > +
> > > +     dockram = i2c_new_ancillary_device(parent, "dockram",
> > > +                                        parent->addr + 2);
> >
> > Could we define a macro for the address offset '2' here to avoid using a magic
> > number?
> >
> 
> It seems that you are excessively concerned with "magic numbers".

Bingo!  I HATE magic numbers.

https://lore.kernel.org/all/?q=%22Lee+Jones%22+magic

~900 messages!  =:-D

[...]

> > > +static const struct asus_ec_chip_info asus_ec_tf600t_pad_data = {
> > > +     .name = "pad",
> > > +     .variant = ASUSEC_TF600T_PAD,
> > > +     .fmode = ASUSEC_MODE_NORMAL,
> > > +};
> >
> > Any reason not to just pass the identifier (variant) and add the name
> > and fmode attribues to the switch() above?
> 
> Why not set it here, I am not passing any mfd or any other API via of data.

I get that, and you're not breaking any of my golden rules.

However, I just think doing everything in one place, usually a which
based off of the 'variant' which you pass as a single value, is a nicer,
more consolidated way of doing things.

-- 
Lee Jones