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

[Svyatoslav Ryhel]

чт, 18 черв. 2026 р. о 15:26 Lee Jones <lee@xxxxxxxxxx> пише:
>
> On Thu, 11 Jun 2026, Svyatoslav Ryhel wrote:
>
> > чт, 11 черв. 2026 р. о 14:17 Lee Jones <lee@xxxxxxxxxx> пише:
> > >
> > > On Thu, 28 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                     |  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?
>

Model is passed to cells to form names, particularly input device names.

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

I totally forgot about dev_info_once(), thank you.

> > > > +     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().
>

I thought about this, but that would require combining it with (!)
which will not help with readabilty.

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

AHAHAH, ok, this makes things clearer. Lemmy have a note about this quirk.

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

Well, I would need to pass OF data regardless, so why not bundle it
with all required info and leave probes switch for strictly mfd cell
assignment. I suppose this is more a personal preference issue. I hope
you will not mind if I leave this as it is?

All other comments you have left and I did not answer directly were
read and acknowledged. Thank you!

> --
> Lee Jones