Re: [PATCH 4/6] platform/apple: Add new Apple Mac SMC driver

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Thu, Sep 1, 2022 at 5:18 PM Russell King <rmk+kernel@xxxxxxxxxxxxxxx> wrote:
>
> From: Hector Martin <marcan@xxxxxxxxx>
>
> This driver implements support for the SMC (System Management
> Controller) in Apple Macs. In contrast to the existing applesmc driver,
> it uses pluggable backends that allow it to support different SMC
> implementations, and uses the MFD subsystem to expose the core SMC
> functionality so that specific features (gpio, hwmon, battery, etc.) can
> be implemented by separate drivers in their respective downstream
> subsystems.
>
> The initial RTKit backend adds support for Apple Silicon Macs (M1 et
> al). We hope a backend for T2 Macs will be written in the future
> (since those are not supported by applesmc), and eventually an x86
> backend would allow us to fully deprecate applesmc in favor of this
> driver.

...

>  drivers/platform/Kconfig           |   2 +
>  drivers/platform/Makefile          |   1 +
>  drivers/platform/apple/Kconfig     |  49 ++++
>  drivers/platform/apple/Makefile    |  11 +

Are you going to collect the code from, e.g., PDx86 which supports
some apple devices here?

...


> +EXPORT_SYMBOL(apple_smc_read);

Can you from day 1 make it a namespaced variant? Ditto for the rest of
the exported stuff.

...

> +#include <asm/unaligned.h>

Usually we put asm/* after linux/*.

Missed bits.h.

> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/ioport.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/soc/apple/rtkit.h>

...

> +       smc->msg_id = (smc->msg_id + 1) & 0xf;

% 16 will tell much cleaner of the purpose, no?

...

> +       while (smc->atomic_pending) {
> +               ret = apple_rtkit_poll(smc->rtk);
> +               if (ret < 0) {
> +                       dev_err(smc->dev, "RTKit poll failed (%llx)", msg);
> +                       return ret;
> +               }
> +               udelay(100);
> +       }

Something from iopoll.h to be utilised?

...

> +       if (FIELD_GET(SMC_ID, smc->cmd_ret) != smc->msg_id) {
> +               dev_err(smc->dev, "Command sequence mismatch (expected %d, got %d)\n",
> +                       smc->msg_id, (unsigned int)FIELD_GET(SMC_ID, smc->cmd_ret));

Why casting?

> +               return -EIO;
> +       }

...

> +       result = FIELD_GET(SMC_RESULT, smc->cmd_ret);
> +       if (result != 0)
> +               return -result;

And this is in Linux error numbering space?!

...

> +       smc->msg_id = (smc->msg_id + 1) & 0xf;

See above. Perhaps you need a macro / inline helper for this to avoid dups.

...

> +       do {
> +               if (wait_for_completion_timeout(&smc->cmd_done,
> +                                               msecs_to_jiffies(SMC_RECV_TIMEOUT)) == 0) {
> +                       dev_err(smc->dev, "Command timed out (%llx)", msg);
> +                       return -ETIMEDOUT;
> +               }
> +               if (FIELD_GET(SMC_ID, smc->cmd_ret) == smc->msg_id)
> +                       break;

> +               dev_err(smc->dev, "Command sequence mismatch (expected %d, got %d)\n",
> +                       smc->msg_id, (unsigned int)FIELD_GET(SMC_ID, smc->cmd_ret));

Guaranteed to flood the logs...

> +       } while(1);

...with such a conditional.

...

> +       result = FIELD_GET(SMC_RESULT, smc->cmd_ret);
> +       if (result != 0)
> +               return -result;

Linux error numbering space?

...

> +       if (size <= 4)
> +               memcpy(buf, &rdata, size);
> +       else
> +               memcpy_fromio(buf, smc->shmem.iomem, size);

This is unclear why plain memcpy() for the small size and what are the
side effects of the memory. Maybe you wanted memremap() instead of
ioremap() to begin with?

...

> +       *key = swab32(*key);

swab32s()

...

> +       if (res.end < res.start || !resource_contains(smc->sram, &res)) {

Is it a reimplementation of something like resource_intersect() and Co?

> +               dev_err(smc->dev,
> +                       "RTKit buffer request outside SRAM region: %pR", &res);
> +               return -EFAULT;
> +       }

...

> +       bfr->iomem = smc->sram_base + (res.start - smc->sram->start);

Isn't it better to write as

  res.start + (base - start)

?

...

> +               if (smc->atomic_pending) {
> +                       smc->atomic_pending = false;
> +               } else {
> +                       complete(&smc->cmd_done);
> +               }

Redundant {} in both cases.

...

> +       smc->sram = platform_get_resource_byname(pdev, IORESOURCE_MEM, "sram");

> +       if (!smc->sram)
> +               return dev_err_probe(dev, EIO,
> +                                    "No SRAM region");

Dup, the below does this message for you.

> +       smc->sram_base = devm_ioremap_resource(dev, smc->sram);
> +       if (IS_ERR(smc->sram_base))
> +               return dev_err_probe(dev, PTR_ERR(smc->sram_base),
> +                                    "Failed to map SRAM region");

Don't we have devm_platform_ioremap_resource_byname() ?

...

> +       ret = apple_rtkit_wake(smc->rtk);
> +       if (ret != 0)

Drop all these ' != 0'

> +               return dev_err_probe(dev, ret,
> +                                    "Failed to wake up SMC");

Why not on one line?

...

> +static const struct of_device_id apple_smc_rtkit_of_match[] = {
> +       { .compatible = "apple,smc" },

> +       {},

No comma for the terminator entry.

> +};

...

> +static struct platform_driver apple_smc_rtkit_driver = {
> +       .driver = {
> +               .name = "macsmc-rtkit",

> +               .owner = THIS_MODULE,

Unneeded dup.

> +               .of_match_table = apple_smc_rtkit_of_match,
> +       },
> +       .probe = apple_smc_rtkit_probe,
> +       .remove = apple_smc_rtkit_remove,
> +};

...

> +typedef u32 smc_key;

Why?!

...

> +#define _SMC_KEY(s) (((s)[0] << 24) | ((s)[1] << 16) | ((s)[2] << 8) | (s)[3])

If s is a byte buffer, the above is NIH get_unaligned_be32(). Or in
case of alignment be32_to_cpu() with respective type (__be32) to be
used.

...

> +static inline int apple_smc_read_flag(struct apple_smc *smc, smc_key key)
> +{
> +       u8 val;
> +       int ret = apple_smc_read_u8(smc, key, &val);

Split assignment and definition.

> +       if (ret < 0)
> +               return ret;
> +       return val ? 1 : 0;
> +}

...

> +#define apple_smc_write_flag apple_smc_write_u8

Why is it needed?

-- 
With Best Regards,
Andy Shevchenko