Patchwork [06/15] Platform: OLPC: Add XO-1.75 EC driver

login
register
mail settings
Submitter Lubomir Rintel
Date Oct. 10, 2018, 5:22 p.m.
Message ID <20181010172300.317643-7-lkundrak@v3.sk>
Download mbox | patch
Permalink /patch/632585/
State New
Headers show

Comments

Lubomir Rintel - Oct. 10, 2018, 5:22 p.m.
It's based off the driver from the OLPC kernel sources. Somewhat
modernized and cleaned up, for better or worse.

Modified to plug into the olpc-ec driver infrastructure (so that battery
interface and debugfs could be reused) and the SPI slave framework.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 drivers/platform/olpc/Kconfig         |  15 +
 drivers/platform/olpc/Makefile        |   1 +
 drivers/platform/olpc/olpc-xo175-ec.c | 755 ++++++++++++++++++++++++++
 3 files changed, 771 insertions(+)
 create mode 100644 drivers/platform/olpc/olpc-xo175-ec.c
Andy Shevchenko - Oct. 19, 2018, 4:06 p.m.
On Wed, Oct 10, 2018 at 8:24 PM Lubomir Rintel <lkundrak@v3.sk> wrote:
>
> It's based off the driver from the OLPC kernel sources. Somewhat
> modernized and cleaned up, for better or worse.
>
> Modified to plug into the olpc-ec driver infrastructure (so that battery
> interface and debugfs could be reused) and the SPI slave framework.


> +#include <asm/system_misc.h>

asm/* goes after linux/*

> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/spinlock.h>
> +#include <linux/completion.h>
> +#include <linux/slab.h>
> +#include <linux/platform_device.h>
> +#include <linux/ctype.h>
> +#include <linux/olpc-ec.h>
> +#include <linux/spi/spi.h>
> +#include <linux/reboot.h>
> +#include <linux/input.h>
> +#include <linux/kfifo.h>
> +#include <linux/module.h>
> +#include <linux/power_supply.h>

Easy to maintain when it's sorted.

> +       { 0 },

Terminators are better without trailing comma.

> +#define EC_CMD_LEN             8
> +#define EC_MAX_RESP_LEN                16

> +#define LOG_BUF_SIZE           127

127 sounds slightly strange. Is it by specification of protocol? Would
it be better to define it 128 bytes / items?

> +static int olpc_xo175_ec_is_valid_cmd(u8 cmd)
> +{
> +       const struct ec_cmd_t *p;
> +
> +       for (p = olpc_xo175_ec_cmds; p->cmd; p++) {
> +               if (p->cmd == cmd)
> +                       return p->bytes_returned;
> +       }
> +
> +       return -1;

-EINVAL ?

> +}

> +static void olpc_xo175_ec_complete(void *arg);

Hmm... Can we avoid forward declaration?

> +       channel = priv->rx_buf[0];
> +       byte = priv->rx_buf[1];

Maybe specific structures would fit better?

Like

struct olpc_ec_resp_hdr {
 u8 channel;
 u8 byte;
...
}

> +               dev_warn(dev, "kbd/tpad not supported\n");

Please, spell it fully as touchpad and keyboard.

> +                       pm_wakeup_event(priv->pwrbtn->dev.parent, 1000);

Magic number.

> +                       /* For now, we just ignore the unknown events. */

dev_dbg(dev, "Ignored unknown event %.2x\n", byte);

?

> if (isprint(byte)) {
> +                       priv->logbuf[priv->logbuf_len++] = byte;
> +                       if (priv->logbuf_len == LOG_BUF_SIZE)
> +                               olpc_xo175_ec_flush_logbuf(priv);
> +               }

You may consider to take everything and run %pE when printing instead of %s.

> +static int olpc_xo175_ec_cmd(u8 cmd, u8 *inbuf, size_t inlen, u8 *resp,
> +                                       size_t resp_len, void *ec_cb_arg)
> +{
> +       struct olpc_xo175_ec *priv = ec_cb_arg;
> +       struct device *dev = &priv->spi->dev;
> +       unsigned long flags;
> +       int nr_bytes;
> +       int ret = 0;
> +
> +       dev_dbg(dev, "CMD %x, %d bytes expected\n", cmd, resp_len);
> +
> +       if (inlen > 5) {

Magic number.

> +               dev_err(dev, "command len %d too big!\n", resp_len);
> +               return -EOVERFLOW;
> +       }

> +       WARN_ON(priv->suspended);
> +       if (priv->suspended)

if (WARN_ON(...)) ?

> +               return -EBUSY;

> +       if (resp_len > nr_bytes)
> +               resp_len = nr_bytes;

resp_len = min(resp_len, nr_bytes);

> +       priv->cmd[0] = cmd;
> +       priv->cmd[1] = inlen;
> +       priv->cmd[2] = 0;

Perhaps specific struct header for this?

> +       memset(resp, 0, resp_len);

Wouldn't be better to do this in where actual response has been filled?

> +       if (!wait_for_completion_timeout(&priv->cmd_done,
> +                       msecs_to_jiffies(4000))) {

Magic number.

> +       }

> +       /* Deal with the results. */

Somehow feels noisy / unneeded comment.

> +       if (priv->cmd_state == CMD_STATE_ERROR_RECEIVED) {
> +               /* EC-provided error is in the single response byte */
> +               dev_err(dev, "command 0x%x returned error 0x%x\n",
> +                                                       cmd, priv->resp[0]);

Indentation.

> +               ret = -EREMOTEIO;
> +       } else if (priv->resp_len != nr_bytes) {

> +               dev_err(dev, "command 0x%x returned %d bytes, expected %d bytes\n",
> +                                               cmd, priv->resp_len, nr_bytes);
> +               ret = -ETIMEDOUT;

In the message I see nothing about timeout.

> +       } else {

> +       }

> +}


> +static int olpc_xo175_ec_set_event_mask(unsigned int mask)
> +{
> +       unsigned char args[2];

u8

> +
> +       args[0] = mask & 0xff;
> +       args[1] = (mask >> 8) & 0xff;

...mask >> 0;
...mask >> 8;

> +       return olpc_ec_cmd(CMD_WRITE_EXT_SCI_MASK, args, 2, NULL, 0);
> +}

> +
> +static void olpc_xo175_ec_restart(enum reboot_mode mode, const char *cmd)
> +{
> +       while (1) {
> +               olpc_ec_cmd(CMD_POWER_CYCLE, NULL, 0, NULL, 0);
> +               mdelay(1000);
> +       }
> +}
> +

> +static void olpc_xo175_ec_power_off(void)
> +{
> +       while (1) {
> +               olpc_ec_cmd(CMD_POWER_OFF, NULL, 0, NULL, 0);
> +               mdelay(1000);
> +       }
> +}
> +

> +#ifdef CONFIG_PM
> +static int olpc_xo175_ec_suspend(struct device *dev)

__maybe_unused  instead of ugly #ifdef?

> +{

> +       struct platform_device *pdev = to_platform_device(dev);
> +       struct olpc_xo175_ec *priv = platform_get_drvdata(pdev);

dev_get_drvdata() or how is it called?

> +       unsigned char hintargs[5];

struct olpc_ec_hint_cmd {
u8 ...
u32 ...
};

?

> +       static unsigned int suspend_count;

u32 I suppose.

> +
> +       suspend_count++;
> +       dev_dbg(dev, "%s: suspend sync %08x\n", __func__, suspend_count);

__func__ can be issued if user asked for via Dynamic Debug interface.

> +       /*
> +        * First byte is 1 to indicate suspend, the rest is an integer
> +        * counter.
> +        */
> +       hintargs[0] = 1;
> +       memcpy(&hintargs[1], &suspend_count, 4);
> +       olpc_ec_cmd(CMD_SUSPEND_HINT, hintargs, 5, NULL, 0);

What do you need this counter for?

> +       priv->suspended = true;

Hmm... Who is the user of it?

> +       return 0;
> +}
> +
> +static int olpc_xo175_ec_resume_noirq(struct device *dev)
> +{
> +       struct platform_device *pdev = to_platform_device(dev);
> +       struct olpc_xo175_ec *priv = platform_get_drvdata(pdev);
> +
> +       priv->suspended = false;
> +
> +       return 0;
> +}
> +
> +static int olpc_xo175_ec_resume(struct device *dev)
> +{
> +       struct platform_device *pdev = to_platform_device(dev);
> +       struct olpc_xo175_ec *priv = platform_get_drvdata(pdev);

> +       unsigned char x = 0;

u8

> +       priv->suspended = false;

Isn't it redundant since noirq callback above?

> +       /*
> +        * The resume hint is only needed if no other commands are
> +        * being sent during resume. all it does is tell the EC
> +        * the SoC is definitely awake.
> +        */
> +       olpc_ec_cmd(CMD_SUSPEND_HINT, &x, 1, NULL, 0);
> +

> +       /* Enable all EC events while we're awake */
> +       olpc_xo175_ec_set_event_mask(0xffff);

#define EC_ALL_EVENTS GENMASK(15, 0)

> +}
> +#endif

> +static struct platform_device *olpc_ec;

I would rather see this at the top of file.

> +static int olpc_xo175_ec_probe(struct spi_device *spi)
> +{

> +       if (olpc_ec) {
> +               dev_err(&spi->dev, "OLPC EC already registered.\n");
> +               return -EBUSY;
> +       }

It's racy against parallel probe called. I don't think it would be a
real issue, just let you know.


> +       /* Set up power button input device */
> +       priv->pwrbtn = devm_input_allocate_device(&spi->dev);
> +       if (!priv->pwrbtn)
> +               return -ENOMEM;
> +       priv->pwrbtn->name = "Power Button";
> +       priv->pwrbtn->dev.parent = &spi->dev;
> +       input_set_capability(priv->pwrbtn, EV_KEY, KEY_POWER);
> +       ret = input_register_device(priv->pwrbtn);
> +       if (ret) {
> +               dev_err(&spi->dev, "error registering input device: %d\n", ret);
> +               return ret;
> +       }

I would split out power button driver, but it's up to you.


> +       /* Enable all EC events while we're awake */
> +       olpc_xo175_ec_set_event_mask(0xffff);

See above about this magic.

> +}

> +#ifdef CONFIG_PM
> +       .suspend        = olpc_xo175_ec_suspend,
> +       .resume_noirq   = olpc_xo175_ec_resume_noirq,
> +       .resume         = olpc_xo175_ec_resume,
> +#endif

SET_SYSTEM_SLEEP_PM_OPS() ?
SET_NOIRQ_SYSTEM_SLEEP_PM_OPS() ?

> +static const struct of_device_id olpc_xo175_ec_of_match[] = {
> +       { .compatible = "olpc,xo1.75-ec" },

> +       { },

No comma for terminators.

> +};
Lubomir Rintel - Nov. 13, 2018, 5:26 p.m.
Hi,

first of all -- thanks for such a careful review. It is very helpful.

Wherever I don't respond to you, I'm just following what you wrote. It
would perhaps be tiresome to respond to "Yes, will fix in next version"
to every single point.

I'll be following up with a new version in a few days; I'm mostly done
with this one but I've not finished addressing the followup ones.

On Fri, 2018-10-19 at 19:06 +0300, Andy Shevchenko wrote:
> On Wed, Oct 10, 2018 at 8:24 PM Lubomir Rintel <lkundrak@v3.sk>
> wrote:
> > It's based off the driver from the OLPC kernel sources. Somewhat
> > modernized and cleaned up, for better or worse.
> > 
> > Modified to plug into the olpc-ec driver infrastructure (so that
> > battery
> > interface and debugfs could be reused) and the SPI slave framework.
> > +#include <asm/system_misc.h>
> 
> asm/* goes after linux/*
> 
> > +#include <linux/delay.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/completion.h>
> > +#include <linux/slab.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/ctype.h>
> > +#include <linux/olpc-ec.h>
> > +#include <linux/spi/spi.h>
> > +#include <linux/reboot.h>
> > +#include <linux/input.h>
> > +#include <linux/kfifo.h>
> > +#include <linux/module.h>
> > +#include <linux/power_supply.h>
> 
> Easy to maintain when it's sorted.
> 
> > +       { 0 },
> 
> Terminators are better without trailing comma.
> 
> > +#define EC_CMD_LEN             8
> > +#define EC_MAX_RESP_LEN                16
> > +#define LOG_BUF_SIZE           127
> 
> 127 sounds slightly strange. Is it by specification of protocol?
> Would
> it be better to define it 128 bytes / items?
> 
> > +static int olpc_xo175_ec_is_valid_cmd(u8 cmd)
> > +{
> > +       const struct ec_cmd_t *p;
> > +
> > +       for (p = olpc_xo175_ec_cmds; p->cmd; p++) {
> > +               if (p->cmd == cmd)
> > +                       return p->bytes_returned;
> > +       }
> > +
> > +       return -1;
> 
> -EINVAL ?
> 
> > +}
> > +static void olpc_xo175_ec_complete(void *arg);
> 
> Hmm... Can we avoid forward declaration?

I don't think we can.

> > +       channel = priv->rx_buf[0];
> > +       byte = priv->rx_buf[1];
> 
> Maybe specific structures would fit better?
> 
> Like
> 
> struct olpc_ec_resp_hdr {
>  u8 channel;
>  u8 byte;
> ...
> }
> 
> > +               dev_warn(dev, "kbd/tpad not supported\n");
> 
> Please, spell it fully as touchpad and keyboard.
> 
> > +                       pm_wakeup_event(priv->pwrbtn->dev.parent,
> > 1000);
> 
> Magic number.
> 
> > +                       /* For now, we just ignore the unknown
> > events. */
> 
> dev_dbg(dev, "Ignored unknown event %.2x\n", byte);
> 
> ?
> 
> > if (isprint(byte)) {
> > +                       priv->logbuf[priv->logbuf_len++] = byte;
> > +                       if (priv->logbuf_len == LOG_BUF_SIZE)
> > +                               olpc_xo175_ec_flush_logbuf(priv);
> > +               }
> 
> You may consider to take everything and run %pE when printing instead
> of %s.
> 
> > +static int olpc_xo175_ec_cmd(u8 cmd, u8 *inbuf, size_t inlen, u8
> > *resp,
> > +                                       size_t resp_len, void
> > *ec_cb_arg)
> > +{
> > +       struct olpc_xo175_ec *priv = ec_cb_arg;
> > +       struct device *dev = &priv->spi->dev;
> > +       unsigned long flags;
> > +       int nr_bytes;
> > +       int ret = 0;
> > +
> > +       dev_dbg(dev, "CMD %x, %d bytes expected\n", cmd, resp_len);
> > +
> > +       if (inlen > 5) {
> 
> Magic number.
> 
> > +               dev_err(dev, "command len %d too big!\n",
> > resp_len);
> > +               return -EOVERFLOW;
> > +       }
> > +       WARN_ON(priv->suspended);
> > +       if (priv->suspended)
> 
> if (WARN_ON(...)) ?
> 
> > +               return -EBUSY;
> > +       if (resp_len > nr_bytes)
> > +               resp_len = nr_bytes;
> 
> resp_len = min(resp_len, nr_bytes);
> 
> > +       priv->cmd[0] = cmd;
> > +       priv->cmd[1] = inlen;
> > +       priv->cmd[2] = 0;
> 
> Perhaps specific struct header for this?
> 
> > +       memset(resp, 0, resp_len);
> 
> Wouldn't be better to do this in where actual response has been
> filled?
> 
> > +       if (!wait_for_completion_timeout(&priv->cmd_done,
> > +                       msecs_to_jiffies(4000))) {
> 
> Magic number.
> 
> > +       }
> > +       /* Deal with the results. */
> 
> Somehow feels noisy / unneeded comment.
> 
> > +       if (priv->cmd_state == CMD_STATE_ERROR_RECEIVED) {
> > +               /* EC-provided error is in the single response byte
> > */
> > +               dev_err(dev, "command 0x%x returned error 0x%x\n",
> > +                                                       cmd, priv-
> > >resp[0]);
> 
> Indentation.
> 
> > +               ret = -EREMOTEIO;
> > +       } else if (priv->resp_len != nr_bytes) {
> > +               dev_err(dev, "command 0x%x returned %d bytes,
> > expected %d bytes\n",
> > +                                               cmd, priv-
> > >resp_len, nr_bytes);
> > +               ret = -ETIMEDOUT;
> 
> In the message I see nothing about timeout.
> 
> > +       } else {
> > +       }
> > +}
> > +static int olpc_xo175_ec_set_event_mask(unsigned int mask)
> > +{
> > +       unsigned char args[2];
> 
> u8
> 
> > +
> > +       args[0] = mask & 0xff;
> > +       args[1] = (mask >> 8) & 0xff;
> 
> ...mask >> 0;
> ...mask >> 8;
> 
> > +       return olpc_ec_cmd(CMD_WRITE_EXT_SCI_MASK, args, 2, NULL,
> > 0);
> > +}
> > +
> > +static void olpc_xo175_ec_restart(enum reboot_mode mode, const
> > char *cmd)
> > +{
> > +       while (1) {
> > +               olpc_ec_cmd(CMD_POWER_CYCLE, NULL, 0, NULL, 0);
> > +               mdelay(1000);
> > +       }
> > +}
> > +
> > +static void olpc_xo175_ec_power_off(void)
> > +{
> > +       while (1) {
> > +               olpc_ec_cmd(CMD_POWER_OFF, NULL, 0, NULL, 0);
> > +               mdelay(1000);
> > +       }
> > +}
> > +
> > +#ifdef CONFIG_PM
> > +static int olpc_xo175_ec_suspend(struct device *dev)
> 
> __maybe_unused  instead of ugly #ifdef?
> 
> > +{
> > +       struct platform_device *pdev = to_platform_device(dev);
> > +       struct olpc_xo175_ec *priv = platform_get_drvdata(pdev);
> 
> dev_get_drvdata() or how is it called?
> 
> > +       unsigned char hintargs[5];
> 
> struct olpc_ec_hint_cmd {
> u8 ...
> u32 ...
> };
> 
> ?
> 
> > +       static unsigned int suspend_count;
> 
> u32 I suppose.
> 
> > +
> > +       suspend_count++;
> > +       dev_dbg(dev, "%s: suspend sync %08x\n", __func__,
> > suspend_count);
> 
> __func__ can be issued if user asked for via Dynamic Debug interface.
> 
> > +       /*
> > +        * First byte is 1 to indicate suspend, the rest is an
> > integer
> > +        * counter.
> > +        */
> > +       hintargs[0] = 1;
> > +       memcpy(&hintargs[1], &suspend_count, 4);
> > +       olpc_ec_cmd(CMD_SUSPEND_HINT, hintargs, 5, NULL, 0);
> 
> What do you need this counter for?

It doesn't seem to be actually used in the EC; the firmware just
includes it in its debug log. I'm not sure if all firmware versions
behave this way and I'd prefer to keep it.

I'm adding a comment.

> 
> > +       priv->suspended = true;
> 
> Hmm... Who is the user of it?
> 
> > +       return 0;
> > +}
> > +
> > +static int olpc_xo175_ec_resume_noirq(struct device *dev)
> > +{
> > +       struct platform_device *pdev = to_platform_device(dev);
> > +       struct olpc_xo175_ec *priv = platform_get_drvdata(pdev);
> > +
> > +       priv->suspended = false;
> > +
> > +       return 0;
> > +}
> > +
> > +static int olpc_xo175_ec_resume(struct device *dev)
> > +{
> > +       struct platform_device *pdev = to_platform_device(dev);
> > +       struct olpc_xo175_ec *priv = platform_get_drvdata(pdev);
> > +       unsigned char x = 0;
> 
> u8
> 
> > +       priv->suspended = false;
> 
> Isn't it redundant since noirq callback above?
> 
> > +       /*
> > +        * The resume hint is only needed if no other commands are
> > +        * being sent during resume. all it does is tell the EC
> > +        * the SoC is definitely awake.
> > +        */
> > +       olpc_ec_cmd(CMD_SUSPEND_HINT, &x, 1, NULL, 0);
> > +
> > +       /* Enable all EC events while we're awake */
> > +       olpc_xo175_ec_set_event_mask(0xffff);
> 
> #define EC_ALL_EVENTS GENMASK(15, 0)
> 
> > +}
> > +#endif
> > +static struct platform_device *olpc_ec;
> 
> I would rather see this at the top of file.
> 
> > +static int olpc_xo175_ec_probe(struct spi_device *spi)
> > +{
> > +       if (olpc_ec) {
> > +               dev_err(&spi->dev, "OLPC EC already
> > registered.\n");
> > +               return -EBUSY;
> > +       }
> 
> It's racy against parallel probe called. I don't think it would be a
> real issue, just let you know.
> 
> 
> > +       /* Set up power button input device */
> > +       priv->pwrbtn = devm_input_allocate_device(&spi->dev);
> > +       if (!priv->pwrbtn)
> > +               return -ENOMEM;
> > +       priv->pwrbtn->name = "Power Button";
> > +       priv->pwrbtn->dev.parent = &spi->dev;
> > +       input_set_capability(priv->pwrbtn, EV_KEY, KEY_POWER);
> > +       ret = input_register_device(priv->pwrbtn);
> > +       if (ret) {
> > +               dev_err(&spi->dev, "error registering input device:
> > %d\n", ret);
> > +               return ret;
> > +       }
> 
> I would split out power button driver, but it's up to you.
> 
> 
> > +       /* Enable all EC events while we're awake */
> > +       olpc_xo175_ec_set_event_mask(0xffff);
> 
> See above about this magic.
> 
> > +}
> > +#ifdef CONFIG_PM
> > +       .suspend        = olpc_xo175_ec_suspend,
> > +       .resume_noirq   = olpc_xo175_ec_resume_noirq,
> > +       .resume         = olpc_xo175_ec_resume,
> > +#endif
> 
> SET_SYSTEM_SLEEP_PM_OPS() ?
> SET_NOIRQ_SYSTEM_SLEEP_PM_OPS() ?
> 
> > +static const struct of_device_id olpc_xo175_ec_of_match[] = {
> > +       { .compatible = "olpc,xo1.75-ec" },
> > +       { },
> 
> No comma for terminators.
> 
> > +};

Thanks,
Lubo
James Cameron - Nov. 13, 2018, 10:06 p.m.
On Tue, Nov 13, 2018 at 06:26:09PM +0100, Lubomir Rintel wrote:
> Hi,
> 
> first of all -- thanks for such a careful review. It is very helpful.
> 
> Wherever I don't respond to you, I'm just following what you wrote. It
> would perhaps be tiresome to respond to "Yes, will fix in next version"
> to every single point.
> 
> I'll be following up with a new version in a few days; I'm mostly done
> with this one but I've not finished addressing the followup ones.
> 
> On Fri, 2018-10-19 at 19:06 +0300, Andy Shevchenko wrote:
> > On Wed, Oct 10, 2018 at 8:24 PM Lubomir Rintel <lkundrak@v3.sk>
> > wrote:
> > > It's based off the driver from the OLPC kernel sources. Somewhat
> > > modernized and cleaned up, for better or worse.
> > > 
> > > Modified to plug into the olpc-ec driver infrastructure (so that
> > > battery
> > > interface and debugfs could be reused) and the SPI slave framework.
> > > +#include <asm/system_misc.h>
> > 
> > asm/* goes after linux/*
> > 
> > > +#include <linux/delay.h>
> > > +#include <linux/gpio/consumer.h>
> > > +#include <linux/spinlock.h>
> > > +#include <linux/completion.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/ctype.h>
> > > +#include <linux/olpc-ec.h>
> > > +#include <linux/spi/spi.h>
> > > +#include <linux/reboot.h>
> > > +#include <linux/input.h>
> > > +#include <linux/kfifo.h>
> > > +#include <linux/module.h>
> > > +#include <linux/power_supply.h>
> > 
> > Easy to maintain when it's sorted.
> > 
> > > +       { 0 },
> > 
> > Terminators are better without trailing comma.
> > 
> > > +#define EC_CMD_LEN             8
> > > +#define EC_MAX_RESP_LEN                16
> > > +#define LOG_BUF_SIZE           127
> > 
> > 127 sounds slightly strange. Is it by specification of protocol?
> > Would
> > it be better to define it 128 bytes / items?
> > 
> > > +static int olpc_xo175_ec_is_valid_cmd(u8 cmd)
> > > +{
> > > +       const struct ec_cmd_t *p;
> > > +
> > > +       for (p = olpc_xo175_ec_cmds; p->cmd; p++) {
> > > +               if (p->cmd == cmd)
> > > +                       return p->bytes_returned;
> > > +       }
> > > +
> > > +       return -1;
> > 
> > -EINVAL ?
> > 
> > > +}
> > > +static void olpc_xo175_ec_complete(void *arg);
> > 
> > Hmm... Can we avoid forward declaration?
> 
> I don't think we can.
> 
> > > +       channel = priv->rx_buf[0];
> > > +       byte = priv->rx_buf[1];
> > 
> > Maybe specific structures would fit better?
> > 
> > Like
> > 
> > struct olpc_ec_resp_hdr {
> >  u8 channel;
> >  u8 byte;
> > ...
> > }
> > 
> > > +               dev_warn(dev, "kbd/tpad not supported\n");
> > 
> > Please, spell it fully as touchpad and keyboard.
> > 
> > > +                       pm_wakeup_event(priv->pwrbtn->dev.parent,
> > > 1000);
> > 
> > Magic number.
> > 
> > > +                       /* For now, we just ignore the unknown
> > > events. */
> > 
> > dev_dbg(dev, "Ignored unknown event %.2x\n", byte);
> > 
> > ?
> > 
> > > if (isprint(byte)) {
> > > +                       priv->logbuf[priv->logbuf_len++] = byte;
> > > +                       if (priv->logbuf_len == LOG_BUF_SIZE)
> > > +                               olpc_xo175_ec_flush_logbuf(priv);
> > > +               }
> > 
> > You may consider to take everything and run %pE when printing instead
> > of %s.
> > 
> > > +static int olpc_xo175_ec_cmd(u8 cmd, u8 *inbuf, size_t inlen, u8
> > > *resp,
> > > +                                       size_t resp_len, void
> > > *ec_cb_arg)
> > > +{
> > > +       struct olpc_xo175_ec *priv = ec_cb_arg;
> > > +       struct device *dev = &priv->spi->dev;
> > > +       unsigned long flags;
> > > +       int nr_bytes;
> > > +       int ret = 0;
> > > +
> > > +       dev_dbg(dev, "CMD %x, %d bytes expected\n", cmd, resp_len);
> > > +
> > > +       if (inlen > 5) {
> > 
> > Magic number.
> > 
> > > +               dev_err(dev, "command len %d too big!\n",
> > > resp_len);
> > > +               return -EOVERFLOW;
> > > +       }
> > > +       WARN_ON(priv->suspended);
> > > +       if (priv->suspended)
> > 
> > if (WARN_ON(...)) ?
> > 
> > > +               return -EBUSY;
> > > +       if (resp_len > nr_bytes)
> > > +               resp_len = nr_bytes;
> > 
> > resp_len = min(resp_len, nr_bytes);
> > 
> > > +       priv->cmd[0] = cmd;
> > > +       priv->cmd[1] = inlen;
> > > +       priv->cmd[2] = 0;
> > 
> > Perhaps specific struct header for this?
> > 
> > > +       memset(resp, 0, resp_len);
> > 
> > Wouldn't be better to do this in where actual response has been
> > filled?
> > 
> > > +       if (!wait_for_completion_timeout(&priv->cmd_done,
> > > +                       msecs_to_jiffies(4000))) {
> > 
> > Magic number.
> > 
> > > +       }
> > > +       /* Deal with the results. */
> > 
> > Somehow feels noisy / unneeded comment.
> > 
> > > +       if (priv->cmd_state == CMD_STATE_ERROR_RECEIVED) {
> > > +               /* EC-provided error is in the single response byte
> > > */
> > > +               dev_err(dev, "command 0x%x returned error 0x%x\n",
> > > +                                                       cmd, priv-
> > > >resp[0]);
> > 
> > Indentation.
> > 
> > > +               ret = -EREMOTEIO;
> > > +       } else if (priv->resp_len != nr_bytes) {
> > > +               dev_err(dev, "command 0x%x returned %d bytes,
> > > expected %d bytes\n",
> > > +                                               cmd, priv-
> > > >resp_len, nr_bytes);
> > > +               ret = -ETIMEDOUT;
> > 
> > In the message I see nothing about timeout.
> > 
> > > +       } else {
> > > +       }
> > > +}
> > > +static int olpc_xo175_ec_set_event_mask(unsigned int mask)
> > > +{
> > > +       unsigned char args[2];
> > 
> > u8
> > 
> > > +
> > > +       args[0] = mask & 0xff;
> > > +       args[1] = (mask >> 8) & 0xff;
> > 
> > ...mask >> 0;
> > ...mask >> 8;
> > 
> > > +       return olpc_ec_cmd(CMD_WRITE_EXT_SCI_MASK, args, 2, NULL,
> > > 0);
> > > +}
> > > +
> > > +static void olpc_xo175_ec_restart(enum reboot_mode mode, const
> > > char *cmd)
> > > +{
> > > +       while (1) {
> > > +               olpc_ec_cmd(CMD_POWER_CYCLE, NULL, 0, NULL, 0);
> > > +               mdelay(1000);
> > > +       }
> > > +}
> > > +
> > > +static void olpc_xo175_ec_power_off(void)
> > > +{
> > > +       while (1) {
> > > +               olpc_ec_cmd(CMD_POWER_OFF, NULL, 0, NULL, 0);
> > > +               mdelay(1000);
> > > +       }
> > > +}
> > > +
> > > +#ifdef CONFIG_PM
> > > +static int olpc_xo175_ec_suspend(struct device *dev)
> > 
> > __maybe_unused  instead of ugly #ifdef?
> > 
> > > +{
> > > +       struct platform_device *pdev = to_platform_device(dev);
> > > +       struct olpc_xo175_ec *priv = platform_get_drvdata(pdev);
> > 
> > dev_get_drvdata() or how is it called?
> > 
> > > +       unsigned char hintargs[5];
> > 
> > struct olpc_ec_hint_cmd {
> > u8 ...
> > u32 ...
> > };
> > 
> > ?
> > 
> > > +       static unsigned int suspend_count;
> > 
> > u32 I suppose.
> > 
> > > +
> > > +       suspend_count++;
> > > +       dev_dbg(dev, "%s: suspend sync %08x\n", __func__,
> > > suspend_count);
> > 
> > __func__ can be issued if user asked for via Dynamic Debug interface.
> > 
> > > +       /*
> > > +        * First byte is 1 to indicate suspend, the rest is an
> > > integer
> > > +        * counter.
> > > +        */
> > > +       hintargs[0] = 1;
> > > +       memcpy(&hintargs[1], &suspend_count, 4);
> > > +       olpc_ec_cmd(CMD_SUSPEND_HINT, hintargs, 5, NULL, 0);
> > 
> > What do you need this counter for?
> 
> It doesn't seem to be actually used in the EC; the firmware just
> includes it in its debug log. I'm not sure if all firmware versions
> behave this way and I'd prefer to keep it.

Some firmware versions rely on it, as the SOC_SLEEP line was
unreliable where the board revision is B3 or earlier.

(internal reference: Paul Fox Wed, 10 Oct 2012 09:39:44 -0400)

Although population of B3 and earlier was low, prototypes were given
out to many rather than destroyed.

> 
> I'm adding a comment.
> 
> > 
> > > +       priv->suspended = true;
> > 
> > Hmm... Who is the user of it?
> > 
> > > +       return 0;
> > > +}
> > > +
> > > +static int olpc_xo175_ec_resume_noirq(struct device *dev)
> > > +{
> > > +       struct platform_device *pdev = to_platform_device(dev);
> > > +       struct olpc_xo175_ec *priv = platform_get_drvdata(pdev);
> > > +
> > > +       priv->suspended = false;
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static int olpc_xo175_ec_resume(struct device *dev)
> > > +{
> > > +       struct platform_device *pdev = to_platform_device(dev);
> > > +       struct olpc_xo175_ec *priv = platform_get_drvdata(pdev);
> > > +       unsigned char x = 0;
> > 
> > u8
> > 
> > > +       priv->suspended = false;
> > 
> > Isn't it redundant since noirq callback above?
> > 
> > > +       /*
> > > +        * The resume hint is only needed if no other commands are
> > > +        * being sent during resume. all it does is tell the EC
> > > +        * the SoC is definitely awake.
> > > +        */
> > > +       olpc_ec_cmd(CMD_SUSPEND_HINT, &x, 1, NULL, 0);
> > > +
> > > +       /* Enable all EC events while we're awake */
> > > +       olpc_xo175_ec_set_event_mask(0xffff);
> > 
> > #define EC_ALL_EVENTS GENMASK(15, 0)
> > 
> > > +}
> > > +#endif
> > > +static struct platform_device *olpc_ec;
> > 
> > I would rather see this at the top of file.
> > 
> > > +static int olpc_xo175_ec_probe(struct spi_device *spi)
> > > +{
> > > +       if (olpc_ec) {
> > > +               dev_err(&spi->dev, "OLPC EC already
> > > registered.\n");
> > > +               return -EBUSY;
> > > +       }
> > 
> > It's racy against parallel probe called. I don't think it would be a
> > real issue, just let you know.
> > 
> > 
> > > +       /* Set up power button input device */
> > > +       priv->pwrbtn = devm_input_allocate_device(&spi->dev);
> > > +       if (!priv->pwrbtn)
> > > +               return -ENOMEM;
> > > +       priv->pwrbtn->name = "Power Button";
> > > +       priv->pwrbtn->dev.parent = &spi->dev;
> > > +       input_set_capability(priv->pwrbtn, EV_KEY, KEY_POWER);
> > > +       ret = input_register_device(priv->pwrbtn);
> > > +       if (ret) {
> > > +               dev_err(&spi->dev, "error registering input device:
> > > %d\n", ret);
> > > +               return ret;
> > > +       }
> > 
> > I would split out power button driver, but it's up to you.
> > 
> > 
> > > +       /* Enable all EC events while we're awake */
> > > +       olpc_xo175_ec_set_event_mask(0xffff);
> > 
> > See above about this magic.
> > 
> > > +}
> > > +#ifdef CONFIG_PM
> > > +       .suspend        = olpc_xo175_ec_suspend,
> > > +       .resume_noirq   = olpc_xo175_ec_resume_noirq,
> > > +       .resume         = olpc_xo175_ec_resume,
> > > +#endif
> > 
> > SET_SYSTEM_SLEEP_PM_OPS() ?
> > SET_NOIRQ_SYSTEM_SLEEP_PM_OPS() ?
> > 
> > > +static const struct of_device_id olpc_xo175_ec_of_match[] = {
> > > +       { .compatible = "olpc,xo1.75-ec" },
> > > +       { },
> > 
> > No comma for terminators.
> > 
> > > +};
> 
> Thanks,
> Lubo
>
Pavel Machek - Nov. 19, 2018, 10:40 a.m.
Hi!

> > > +#include <linux/delay.h>
> > > +#include <linux/gpio/consumer.h>
> > > +#include <linux/spinlock.h>
> > > +#include <linux/completion.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/ctype.h>
> > > +#include <linux/olpc-ec.h>
> > > +#include <linux/spi/spi.h>
> > > +#include <linux/reboot.h>
> > > +#include <linux/input.h>
> > > +#include <linux/kfifo.h>
> > > +#include <linux/module.h>
> > > +#include <linux/power_supply.h>
> > 
> > Easy to maintain when it's sorted.

No / it depends.

> > > +       channel = priv->rx_buf[0];
> > > +       byte = priv->rx_buf[1];
> > 
> > Maybe specific structures would fit better?
> > 
> > Like
> > 
> > struct olpc_ec_resp_hdr {
> >  u8 channel;
> >  u8 byte;
> > ...
> > }

Structures have padding and other nastyness...

> > > +                       pm_wakeup_event(priv->pwrbtn->dev.parent,
> > > 1000);
> > 
> > Magic number.

Nothing wrong with magic numbers.

> > > +       args[0] = mask & 0xff;
> > > +       args[1] = (mask >> 8) & 0xff;
> > 
> > ...mask >> 0;
> > ...mask >> 8;

No, please.

> > __maybe_unused  instead of ugly #ifdef?
> > 
> > > +{
> > > +       struct platform_device *pdev = to_platform_device(dev);
> > > +       struct olpc_xo175_ec *priv = platform_get_drvdata(pdev);
> > 
> > dev_get_drvdata() or how is it called?
> > 
> > > +       unsigned char hintargs[5];
> > 
> > struct olpc_ec_hint_cmd {
> > u8 ...
> > u32 ...
> > };
> > 
> > ?

No, unless you want to break the code. Or add __attribute__ packed and
deal with endianness.

Just no.

> > > +       static unsigned int suspend_count;
> > 
> > u32 I suppose.

You know, there's semantic difference between unsigned int and
u32. And this sounds like candidate for unsigned int.

> > > +       /* Enable all EC events while we're awake */
> > > +       olpc_xo175_ec_set_event_mask(0xffff);
> > 
> > #define EC_ALL_EVENTS GENMASK(15, 0)

Actually that's less readable. Just don't.

> > > +static const struct of_device_id olpc_xo175_ec_of_match[] = {
> > > +       { .compatible = "olpc,xo1.75-ec" },
> > > +       { },
> > 
> > No comma for terminators.

Comma is fine.
									Pavel
Lubomir Rintel - Nov. 19, 2018, 1:25 p.m.
Hi Pavel,

I've addressed some of Andy's concerns you're replying to below in a
follow-up version of the patch. To many points I'm generally
indifferent and prefer to lean towards making things easy for whoever
may be likely to deal with the code in past. It's not clear to me who
that might be, or how important either of you consider your remarks to
be. I'll be thankful if anyone makes this clearer to me.

(I hoped that you're in the Cc list; thought git send-mail will see
your address in the Acked-by tags and add you. I failed to notice that
it's not the case. I apologize. Here's the v2 posting: [1].)

[1] https://lore.kernel.org/lkml/20181116162403.49854-1-lkundrak@v3.sk/


On Mon, 2018-11-19 at 11:40 +0100, Pavel Machek wrote:
> Hi!
> 
> > > > +#include <linux/delay.h>
> > > > +#include <linux/gpio/consumer.h>
> > > > +#include <linux/spinlock.h>
> > > > +#include <linux/completion.h>
> > > > +#include <linux/slab.h>
> > > > +#include <linux/platform_device.h>
> > > > +#include <linux/ctype.h>
> > > > +#include <linux/olpc-ec.h>
> > > > +#include <linux/spi/spi.h>
> > > > +#include <linux/reboot.h>
> > > > +#include <linux/input.h>
> > > > +#include <linux/kfifo.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/power_supply.h>
> > > 
> > > Easy to maintain when it's sorted.
> 
> No / it depends.

I've sorted it in the new files; following a rule of keeping things
sorted at the very least has an advantage of being unambiguous.

I'm not sorting things where they currently are not in order, since it
would obscure real changes.

> > > > +       channel = priv->rx_buf[0];
> > > > +       byte = priv->rx_buf[1];
> > > 
> > > Maybe specific structures would fit better?
> > > 
> > > Like
> > > 
> > > struct olpc_ec_resp_hdr {
> > >  u8 channel;
> > >  u8 byte;
> > > ...
> > > }
> 
> Structures have padding and other nastyness...

I've turned them into structs. It perhaps looks a bit better -- the
structure members serving as a documentation for the protocol.

> > > > +                       pm_wakeup_event(priv->pwrbtn-
> > > > >dev.parent,
> > > > 1000);
> > > 
> > > Magic number.
> 
> Nothing wrong with magic numbers.

Turned it into a define, but it's not like it's any clearer why that
particular value works...

I actually have little idea. I've copied this from the vendor's
original driver.

> > > > +       args[0] = mask & 0xff;
> > > > +       args[1] = (mask >> 8) & 0xff;
> > > 
> > > ...mask >> 0;
> > > ...mask >> 8;
> 
> No, please.

I've done this too, but I don't see much point either. Perhaps those
within the OCD spectrum may appreciate :)

> > > __maybe_unused  instead of ugly #ifdef?
> > > 
> > > > +{
> > > > +       struct platform_device *pdev = to_platform_device(dev);
> > > > +       struct olpc_xo175_ec *priv =
> > > > platform_get_drvdata(pdev);
> > > 
> > > dev_get_drvdata() or how is it called?
> > > 
> > > > +       unsigned char hintargs[5];
> > > 
> > > struct olpc_ec_hint_cmd {
> > > u8 ...
> > > u32 ...
> > > };
> > > 
> > > ?
> 
> No, unless you want to break the code. Or add __attribute__ packed
> and
> deal with endianness.
> 
> Just no.

Turned it into a packed struct too for the same reasons as above.

The endianness of the hints argument doesn't actually matter, since the
EC firmware actually uses this in a debugging printf. It's not even
cleat what endianness did EC expect initially.

> > > > +       static unsigned int suspend_count;
> > > 
> > > u32 I suppose.
> 
> You know, there's semantic difference between unsigned int and
> u32. And this sounds like candidate for unsigned int.
> 
> > > > +       /* Enable all EC events while we're awake */
> > > > +       olpc_xo175_ec_set_event_mask(0xffff);
> > > 
> > > #define EC_ALL_EVENTS GENMASK(15, 0)
> 
> Actually that's less readable. Just don't.

Done this too, and, like the points above, I am indifferent too. Both
seem equal to me.

> > > > +static const struct of_device_id olpc_xo175_ec_of_match[] = {
> > > > +       { .compatible = "olpc,xo1.75-ec" },
> > > > +       { },
> > > 
> > > No comma for terminators.
> 
> Comma is fine.

Removed it. Perhaps the lack of a comma could be understood as an
indication that no further initializers shall follow.

> 	Pavel

Just in case I didn't stress that enough: I'm much more interested in
things working well than style issues that too often are just matter of
tast. I happily accept advice on those though.

Take care,
Lubo

Patch

diff --git a/drivers/platform/olpc/Kconfig b/drivers/platform/olpc/Kconfig
index 7b736c9e67ac..7c643d24ad0f 100644
--- a/drivers/platform/olpc/Kconfig
+++ b/drivers/platform/olpc/Kconfig
@@ -9,3 +9,18 @@  config OLPC
 	help
 	  Add support for detecting the unique features of the OLPC
 	  XO hardware.
+
+if OLPC
+
+config OLPC_XO175_EC
+	tristate "OLPC XO 1.75 Embedded Controller"
+	depends on ARCH_MMP || COMPILE_TEST
+	select SPI_SLAVE
+	help
+	  Include support for the OLPC XO Embedded Controller (EC). The EC
+	  provides various platform services, including support for the power,
+	  button, restart, shutdown and battery charging status.
+
+	  Unless you have an OLPC XO laptop, you will want to say N.
+
+endif
diff --git a/drivers/platform/olpc/Makefile b/drivers/platform/olpc/Makefile
index dc8b26bc7209..5b43f383289e 100644
--- a/drivers/platform/olpc/Makefile
+++ b/drivers/platform/olpc/Makefile
@@ -2,3 +2,4 @@ 
 # OLPC XO platform-specific drivers
 #
 obj-$(CONFIG_OLPC)		+= olpc-ec.o
+obj-$(CONFIG_OLPC_XO175_EC)	+= olpc-xo175-ec.o
diff --git a/drivers/platform/olpc/olpc-xo175-ec.c b/drivers/platform/olpc/olpc-xo175-ec.c
new file mode 100644
index 000000000000..6333a1366730
--- /dev/null
+++ b/drivers/platform/olpc/olpc-xo175-ec.c
@@ -0,0 +1,755 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Driver for the OLPC XO-1.75 Embedded Controller.
+ *
+ * The EC protocol is documented at:
+ * http://wiki.laptop.org/go/XO_1.75_HOST_to_EC_Protocol
+ *
+ * Copyright (C) 2010 One Laptop per Child Foundation.
+ * Copyright (C) 2018 Lubomir Rintel <lkundrak@v3.sk>
+ */
+
+#include <asm/system_misc.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/spinlock.h>
+#include <linux/completion.h>
+#include <linux/slab.h>
+#include <linux/platform_device.h>
+#include <linux/ctype.h>
+#include <linux/olpc-ec.h>
+#include <linux/spi/spi.h>
+#include <linux/reboot.h>
+#include <linux/input.h>
+#include <linux/kfifo.h>
+#include <linux/module.h>
+#include <linux/power_supply.h>
+
+struct ec_cmd_t {
+	u8 cmd;
+	u8 bytes_returned;
+};
+
+enum ec_chan_t {
+	CHAN_NONE = 0,
+	CHAN_SWITCH,
+	CHAN_CMD_RESP,
+	CHAN_KEYBOARD,
+	CHAN_TOUCHPAD,
+	CHAN_EVENT,
+	CHAN_DEBUG,
+	CHAN_CMD_ERROR,
+};
+
+/*
+ * EC events
+ */
+#define EVENT_AC_CHANGE			1  /* AC plugged/unplugged */
+#define EVENT_BATTERY_STATUS		2  /* Battery low/full/error/gone */
+#define EVENT_BATTERY_CRITICAL		3  /* Battery critical voltage */
+#define EVENT_BATTERY_SOC_CHANGE	4  /* 1% SOC Change */
+#define EVENT_BATTERY_ERROR		5  /* Abnormal error, query for cause */
+#define EVENT_POWER_PRESSED		6  /* Power button was pressed */
+#define EVENT_POWER_PRESS_WAKE		7  /* Woken up with a power button */
+#define EVENT_TIMED_HOST_WAKE		8  /* Host wake timer */
+#define EVENT_OLS_HIGH_LIMIT		9  /* OLS crossed dark threshold */
+#define EVENT_OLS_LOW_LIMIT		10 /* OLS crossed light threshold */
+
+/*
+ * EC commands
+ * (from http://dev.laptop.org/git/users/rsmith/ec-1.75/tree/ec_cmd.h)
+ */
+#define CMD_GET_API_VERSION		0x08 /* out: u8 */
+#define CMD_READ_VOLTAGE		0x10 /* out: u16, *9.76/32, mV */
+#define CMD_READ_CURRENT		0x11 /* out: s16, *15.625/120, mA */
+#define CMD_READ_ACR			0x12 /* out: s16, *6250/15, uAh */
+#define CMD_READ_BATT_TEMPERATURE	0x13 /* out: u16, *100/256, deg C */
+#define CMD_READ_AMBIENT_TEMPERATURE	0x14 /* unimplemented, no hardware */
+#define CMD_READ_BATTERY_STATUS		0x15 /* out: u8, bitmask */
+#define CMD_READ_SOC			0x16 /* out: u8, percentage */
+#define CMD_READ_GAUGE_ID		0x17 /* out: u8 * 8 */
+#define CMD_READ_GAUGE_DATA		0x18 /* in: u8 addr, out: u8 data */
+#define CMD_READ_BOARD_ID		0x19 /* out: u16 (platform id) */
+#define CMD_READ_BATT_ERR_CODE		0x1f /* out: u8, error bitmask */
+#define CMD_SET_DCON_POWER		0x26 /* in: u8 */
+#define CMD_RESET_EC			0x28 /* none */
+#define CMD_READ_BATTERY_TYPE		0x2c /* out: u8 */
+#define CMD_SET_AUTOWAK			0x33 /* out: u8 */
+#define CMD_SET_EC_WAKEUP_TIMER		0x36 /* in: u32, out: ? */
+#define CMD_READ_EXT_SCI_MASK		0x37 /* ? */
+#define CMD_WRITE_EXT_SCI_MASK		0x38 /* ? */
+#define CMD_CLEAR_EC_WAKEUP_TIMER	0x39 /* none */
+#define CMD_ENABLE_RUNIN_DISCHARGE	0x3B /* none */
+#define CMD_DISABLE_RUNIN_DISCHARGE	0x3C /* none */
+#define CMD_READ_MPPT_ACTIVE		0x3d /* out: u8 */
+#define CMD_READ_MPPT_LIMIT		0x3e /* out: u8 */
+#define CMD_SET_MPPT_LIMIT		0x3f /* in: u8 */
+#define CMD_DISABLE_MPPT		0x40 /* none */
+#define CMD_ENABLE_MPPT			0x41 /* none */
+#define CMD_READ_VIN			0x42 /* out: u16 */
+#define CMD_EXT_SCI_QUERY		0x43 /* ? */
+#define RSP_KEYBOARD_DATA		0x48 /* ? */
+#define RSP_TOUCHPAD_DATA		0x49 /* ? */
+#define CMD_GET_FW_VERSION		0x4a /* out: u8 * 16 */
+#define CMD_POWER_CYCLE			0x4b /* none */
+#define CMD_POWER_OFF			0x4c /* none */
+#define CMD_RESET_EC_SOFT		0x4d /* none */
+#define CMD_READ_GAUGE_U16		0x4e /* ? */
+#define CMD_ENABLE_MOUSE		0x4f /* ? */
+#define CMD_ECHO			0x52 /* in: u8 * 5, out: u8 * 5 */
+#define CMD_GET_FW_DATE			0x53 /* out: u8 * 16 */
+#define CMD_GET_FW_USER			0x54 /* out: u8 * 16 */
+#define CMD_TURN_OFF_POWER		0x55 /* none (same as 0x4c) */
+#define CMD_READ_OLS			0x56 /* out: u16 */
+#define CMD_OLS_SMT_LEDON		0x57 /* none */
+#define CMD_OLS_SMT_LEDOFF		0x58 /* none */
+#define CMD_START_OLS_ASSY		0x59 /* none */
+#define CMD_STOP_OLS_ASSY		0x5a /* none */
+#define CMD_OLS_SMTTEST_STOP		0x5b /* none */
+#define CMD_READ_VIN_SCALED		0x5c /* out: u16 */
+#define CMD_READ_BAT_MIN_W		0x5d /* out: u16 */
+#define CMD_READ_BAR_MAX_W		0x5e /* out: u16 */
+#define CMD_RESET_BAT_MINMAX_W		0x5f /* none */
+#define CMD_READ_LOCATION		0x60 /* in: u16 addr, out: u8 data */
+#define CMD_WRITE_LOCATION		0x61 /* in: u16 addr, u8 data */
+#define CMD_KEYBOARD_CMD		0x62 /* in: u8, out: ? */
+#define CMD_TOUCHPAD_CMD		0x63 /* in: u8, out: ? */
+#define CMD_GET_FW_HASH			0x64 /* out: u8 * 16 */
+#define CMD_SUSPEND_HINT		0x65 /* in: u8 */
+#define CMD_ENABLE_WAKE_TIMER		0x66 /* in: u8 */
+#define CMD_SET_WAKE_TIMER		0x67 /* in: 32 */
+#define CMD_ENABLE_WAKE_AUTORESET	0x68 /* in: u8 */
+#define CMD_OLS_SET_LIMITS		0x69 /* in: u16, u16 */
+#define CMD_OLS_GET_LIMITS		0x6a /* out: u16, u16 */
+#define CMD_OLS_SET_CEILING		0x6b /* in: u16 */
+#define CMD_OLS_GET_CEILING		0x6c /* out: u16 */
+
+/*
+ * Accepted EC commands, and how many bytes they return. There are plenty
+ * of EC commands that are no longer implemented, or are implemented only on
+ * certain older boards.
+ */
+static const struct ec_cmd_t olpc_xo175_ec_cmds[] = {
+	{ CMD_GET_API_VERSION, 1 },
+	{ CMD_READ_VOLTAGE, 2 },
+	{ CMD_READ_CURRENT, 2 },
+	{ CMD_READ_ACR, 2 },
+	{ CMD_READ_BATT_TEMPERATURE, 2 },
+	{ CMD_READ_BATTERY_STATUS, 1 },
+	{ CMD_READ_SOC, 1 },
+	{ CMD_READ_GAUGE_ID, 8 },
+	{ CMD_READ_GAUGE_DATA, 1 },
+	{ CMD_READ_BOARD_ID, 2 },
+	{ CMD_READ_BATT_ERR_CODE, 1 },
+	{ CMD_SET_DCON_POWER, 0 },
+	{ CMD_RESET_EC, 0 },
+	{ CMD_READ_BATTERY_TYPE, 1 },
+	{ CMD_ENABLE_RUNIN_DISCHARGE, 0 },
+	{ CMD_DISABLE_RUNIN_DISCHARGE, 0 },
+	{ CMD_READ_MPPT_ACTIVE, 1 },
+	{ CMD_READ_MPPT_LIMIT, 1 },
+	{ CMD_SET_MPPT_LIMIT, 0 },
+	{ CMD_DISABLE_MPPT, 0 },
+	{ CMD_ENABLE_MPPT, 0 },
+	{ CMD_READ_VIN, 2 },
+	{ CMD_GET_FW_VERSION, 16 },
+	{ CMD_POWER_CYCLE, 0 },
+	{ CMD_POWER_OFF, 0 },
+	{ CMD_RESET_EC_SOFT, 0 },
+	{ CMD_ECHO, 5 },
+	{ CMD_GET_FW_DATE, 16 },
+	{ CMD_GET_FW_USER, 16 },
+	{ CMD_TURN_OFF_POWER, 0 },
+	{ CMD_READ_OLS, 2 },
+	{ CMD_OLS_SMT_LEDON, 0 },
+	{ CMD_OLS_SMT_LEDOFF, 0 },
+	{ CMD_START_OLS_ASSY, 0 },
+	{ CMD_STOP_OLS_ASSY, 0 },
+	{ CMD_OLS_SMTTEST_STOP, 0 },
+	{ CMD_READ_VIN_SCALED, 2 },
+	{ CMD_READ_BAT_MIN_W, 2 },
+	{ CMD_READ_BAR_MAX_W, 2 },
+	{ CMD_RESET_BAT_MINMAX_W, 0 },
+	{ CMD_READ_LOCATION, 1 },
+	{ CMD_WRITE_LOCATION, 0 },
+	{ CMD_GET_FW_HASH, 16 },
+	{ CMD_SUSPEND_HINT, 0 },
+	{ CMD_ENABLE_WAKE_TIMER, 0 },
+	{ CMD_SET_WAKE_TIMER, 0 },
+	{ CMD_ENABLE_WAKE_AUTORESET, 0 },
+	{ CMD_OLS_SET_LIMITS, 0 },
+	{ CMD_OLS_GET_LIMITS, 4 },
+	{ CMD_OLS_SET_CEILING, 0 },
+	{ CMD_OLS_GET_CEILING, 2 },
+	{ CMD_READ_EXT_SCI_MASK, 2 },
+	{ CMD_WRITE_EXT_SCI_MASK, 0 },
+
+	{ 0 },
+};
+
+#define EC_CMD_LEN		8
+#define EC_MAX_RESP_LEN		16
+#define LOG_BUF_SIZE		127
+
+enum ec_state_t {
+	CMD_STATE_IDLE = 0,
+	CMD_STATE_WAITING_FOR_SWITCH,
+	CMD_STATE_CMD_IN_TX_FIFO,
+	CMD_STATE_CMD_SENT,
+	CMD_STATE_RESP_RECEIVED,
+	CMD_STATE_ERROR_RECEIVED,
+};
+
+struct olpc_xo175_ec {
+	struct spi_device *spi;
+
+	struct spi_transfer xfer;
+	struct spi_message msg;
+
+	u8 tx_buf[EC_CMD_LEN];
+	u8 rx_buf[EC_MAX_RESP_LEN];
+
+	bool suspended;
+
+	/* GPIOs for ACK and CMD signals. */
+	struct gpio_desc *gpio_cmd;
+
+	/* Command handling related state. */
+	spinlock_t cmd_state_lock;
+	int cmd_state;
+	bool cmd_running;
+	struct completion cmd_done;
+	u8 cmd[EC_CMD_LEN];
+	int expected_resp_len;
+	u8 resp[EC_MAX_RESP_LEN];
+	int resp_len;
+
+	/* Power button. */
+	struct input_dev *pwrbtn;
+
+	/* Debug handling. */
+	char logbuf[LOG_BUF_SIZE + 1];
+	int logbuf_len;
+};
+
+static int olpc_xo175_ec_is_valid_cmd(u8 cmd)
+{
+	const struct ec_cmd_t *p;
+
+	for (p = olpc_xo175_ec_cmds; p->cmd; p++) {
+		if (p->cmd == cmd)
+			return p->bytes_returned;
+	}
+
+	return -1;
+}
+
+static void olpc_xo175_ec_flush_logbuf(struct olpc_xo175_ec *priv)
+{
+	priv->logbuf[priv->logbuf_len] = 0;
+	priv->logbuf_len = 0;
+
+	dev_dbg(&priv->spi->dev, "got debug string [%s]\n", priv->logbuf);
+}
+
+static void olpc_xo175_ec_complete(void *arg);
+
+static void olpc_xo175_ec_send_command(struct olpc_xo175_ec *priv, u8 *cmd,
+								size_t cmdlen)
+{
+	int ret;
+
+	memcpy(priv->tx_buf, cmd, cmdlen);
+	priv->xfer.len = cmdlen;
+
+	spi_message_init_with_transfers(&priv->msg, &priv->xfer, 1);
+
+	priv->msg.complete = olpc_xo175_ec_complete;
+	priv->msg.context = priv;
+
+	ret = spi_async(priv->spi, &priv->msg);
+	if (ret)
+		dev_err(&priv->spi->dev, "spi_async() failed %d\n", ret);
+}
+
+static void olpc_xo175_ec_read_packet(struct olpc_xo175_ec *priv)
+{
+	u8 nonce[] = {0xA5, 0x5A};
+
+	olpc_xo175_ec_send_command(priv, nonce, sizeof(nonce));
+}
+
+static void olpc_xo175_ec_complete(void *arg)
+{
+	struct olpc_xo175_ec *priv = arg;
+	struct device *dev = &priv->spi->dev;
+	struct power_supply *psy;
+	unsigned long flags;
+	u8 channel;
+	u8 byte;
+	int ret;
+
+	ret = priv->msg.status;
+	if (ret) {
+		dev_err(dev, "SPI transfer failed: %d\n", ret);
+
+		spin_lock_irqsave(&priv->cmd_state_lock, flags);
+		if (priv->cmd_running) {
+			priv->resp_len = 0;
+			priv->cmd_state = CMD_STATE_ERROR_RECEIVED;
+			complete(&priv->cmd_done);
+		}
+		spin_unlock_irqrestore(&priv->cmd_state_lock, flags);
+
+		if (ret != -EINTR)
+			olpc_xo175_ec_read_packet(priv);
+
+		return;
+	}
+
+	channel = priv->rx_buf[0];
+	byte = priv->rx_buf[1];
+
+	switch (channel) {
+	case CHAN_NONE:
+		spin_lock_irqsave(&priv->cmd_state_lock, flags);
+
+		if (!priv->cmd_running) {
+			/* We can safely ignore these */
+			dev_err(dev, "spurious FIFO read packet\n");
+			spin_unlock_irqrestore(&priv->cmd_state_lock, flags);
+			return;
+		}
+
+		priv->cmd_state = CMD_STATE_CMD_SENT;
+		if (!priv->expected_resp_len)
+			complete(&priv->cmd_done);
+		olpc_xo175_ec_read_packet(priv);
+
+		spin_unlock_irqrestore(&priv->cmd_state_lock, flags);
+		return;
+
+	case CHAN_SWITCH:
+		spin_lock_irqsave(&priv->cmd_state_lock, flags);
+
+		if (!priv->cmd_running) {
+			/* Just go with the flow */
+			dev_err(dev, "spurious SWITCH packet\n");
+			memset(priv->cmd, 0, sizeof(priv->cmd));
+			priv->cmd[0] = CMD_ECHO;
+		}
+
+		priv->cmd_state = CMD_STATE_CMD_IN_TX_FIFO;
+
+		/* Throw command into TxFIFO */
+		gpiod_set_value_cansleep(priv->gpio_cmd, 0);
+		olpc_xo175_ec_send_command(priv, priv->cmd, sizeof(priv->cmd));
+
+		spin_unlock_irqrestore(&priv->cmd_state_lock, flags);
+		return;
+
+	case CHAN_CMD_RESP:
+		spin_lock_irqsave(&priv->cmd_state_lock, flags);
+
+		if (!priv->cmd_running) {
+			dev_err(dev, "spurious response packet\n");
+		} else if (priv->resp_len >= priv->expected_resp_len) {
+			dev_err(dev, "too many response packets\n");
+		} else {
+			priv->resp[priv->resp_len++] = byte;
+
+			if (priv->resp_len == priv->expected_resp_len) {
+				priv->cmd_state = CMD_STATE_RESP_RECEIVED;
+				complete(&priv->cmd_done);
+			}
+		}
+
+		spin_unlock_irqrestore(&priv->cmd_state_lock, flags);
+		break;
+
+	case CHAN_CMD_ERROR:
+		spin_lock_irqsave(&priv->cmd_state_lock, flags);
+
+		if (!priv->cmd_running) {
+			dev_err(dev, "spurious cmd error packet\n");
+		} else {
+			priv->resp[0] = byte;
+			priv->resp_len = 1;
+			priv->cmd_state = CMD_STATE_ERROR_RECEIVED;
+			complete(&priv->cmd_done);
+		}
+		spin_unlock_irqrestore(&priv->cmd_state_lock, flags);
+		break;
+
+	case CHAN_KEYBOARD:
+	case CHAN_TOUCHPAD:
+		dev_warn(dev, "kbd/tpad not supported\n");
+		break;
+
+	case CHAN_EVENT:
+		dev_dbg(dev, "got event %.2x\n", byte);
+		switch (byte) {
+		case EVENT_AC_CHANGE:
+			psy = power_supply_get_by_name("olpc-ac");
+			if (psy) {
+				power_supply_changed(psy);
+				power_supply_put(psy);
+			}
+			break;
+		case EVENT_BATTERY_STATUS:
+		case EVENT_BATTERY_CRITICAL:
+		case EVENT_BATTERY_SOC_CHANGE:
+		case EVENT_BATTERY_ERROR:
+			psy = power_supply_get_by_name("olpc-battery");
+			if (psy) {
+				power_supply_changed(psy);
+				power_supply_put(psy);
+			}
+			break;
+		case EVENT_POWER_PRESSED:
+			input_report_key(priv->pwrbtn, KEY_POWER, 1);
+			input_sync(priv->pwrbtn);
+			input_report_key(priv->pwrbtn, KEY_POWER, 0);
+			input_sync(priv->pwrbtn);
+			/* fall through */
+		case EVENT_POWER_PRESS_WAKE:
+		case EVENT_TIMED_HOST_WAKE:
+			pm_wakeup_event(priv->pwrbtn->dev.parent, 1000);
+			break;
+		default:
+			/* For now, we just ignore the unknown events. */
+			break;
+		}
+		break;
+
+	case CHAN_DEBUG:
+		if (byte == '\n') {
+			olpc_xo175_ec_flush_logbuf(priv);
+		} else if (isprint(byte)) {
+			priv->logbuf[priv->logbuf_len++] = byte;
+			if (priv->logbuf_len == LOG_BUF_SIZE)
+				olpc_xo175_ec_flush_logbuf(priv);
+		}
+		break;
+
+	default:
+		dev_warn(dev, "unknown channel: %d, %.2x\n", channel, byte);
+		break;
+	}
+
+	/* Most non-command packets get the TxFIFO refilled and an ACK. */
+	olpc_xo175_ec_read_packet(priv);
+}
+
+/*
+ * This function is protected with a mutex. We can safely assume that
+ * there will be only one instance of this function running at a time.
+ * One of the ways in which we enforce this is by waiting until we get
+ * all response bytes back from the EC, rather than just the number that
+ * the caller requests (otherwise, we might start a new command while an
+ * old command's response bytes are still incoming).
+ */
+static int olpc_xo175_ec_cmd(u8 cmd, u8 *inbuf, size_t inlen, u8 *resp,
+					size_t resp_len, void *ec_cb_arg)
+{
+	struct olpc_xo175_ec *priv = ec_cb_arg;
+	struct device *dev = &priv->spi->dev;
+	unsigned long flags;
+	int nr_bytes;
+	int ret = 0;
+
+	dev_dbg(dev, "CMD %x, %d bytes expected\n", cmd, resp_len);
+
+	if (inlen > 5) {
+		dev_err(dev, "command len %d too big!\n", resp_len);
+		return -EOVERFLOW;
+	}
+
+	/* Suspending in the middle of an EC command hoses things badly! */
+	WARN_ON(priv->suspended);
+	if (priv->suspended)
+		return -EBUSY;
+
+	/* Ensure a valid command and return bytes */
+	nr_bytes = olpc_xo175_ec_is_valid_cmd(cmd);
+	if (nr_bytes < 0) {
+		dev_err_ratelimited(dev, "unknown command 0x%x\n", cmd);
+
+		/*
+		 * Assume the best in our callers, and allow unknown commands
+		 * through. I'm not the charitable type, but it was beaten
+		 * into me. Just maintain a minimum standard of sanity.
+		 */
+		if (resp_len > sizeof(priv->resp)) {
+			dev_err(dev, "response too big: %d!\n", resp_len);
+			return -EOVERFLOW;
+		}
+		nr_bytes = resp_len;
+	}
+	if (resp_len > nr_bytes)
+		resp_len = nr_bytes;
+
+	spin_lock_irqsave(&priv->cmd_state_lock, flags);
+
+	/* Initialize the state machine */
+	init_completion(&priv->cmd_done);
+	priv->cmd_running = true;
+	priv->cmd_state = CMD_STATE_WAITING_FOR_SWITCH;
+	memset(priv->cmd, 0, sizeof(priv->cmd));
+	priv->cmd[0] = cmd;
+	priv->cmd[1] = inlen;
+	priv->cmd[2] = 0;
+	memcpy(&priv->cmd[3], inbuf, inlen);
+	priv->expected_resp_len = nr_bytes;
+	priv->resp_len = 0;
+	memset(resp, 0, resp_len);
+
+	/* Tickle the cmd gpio to get things started */
+	gpiod_set_value_cansleep(priv->gpio_cmd, 1);
+
+	spin_unlock_irqrestore(&priv->cmd_state_lock, flags);
+
+	/* The irq handler should do the rest */
+	if (!wait_for_completion_timeout(&priv->cmd_done,
+			msecs_to_jiffies(4000))) {
+		dev_err(dev, "EC cmd error: timeout in STATE %d\n",
+				priv->cmd_state);
+		gpiod_set_value_cansleep(priv->gpio_cmd, 0);
+		spi_slave_abort(priv->spi);
+		olpc_xo175_ec_read_packet(priv);
+		return -ETIMEDOUT;
+	}
+
+	spin_lock_irqsave(&priv->cmd_state_lock, flags);
+
+	/* Deal with the results. */
+	if (priv->cmd_state == CMD_STATE_ERROR_RECEIVED) {
+		/* EC-provided error is in the single response byte */
+		dev_err(dev, "command 0x%x returned error 0x%x\n",
+							cmd, priv->resp[0]);
+		ret = -EREMOTEIO;
+	} else if (priv->resp_len != nr_bytes) {
+		dev_err(dev, "command 0x%x returned %d bytes, expected %d bytes\n",
+						cmd, priv->resp_len, nr_bytes);
+		ret = -ETIMEDOUT;
+	} else {
+		/*
+		 * We may have 8 bytes in priv->resp, but we only care about
+		 * what we've been asked for. If the caller asked for only 2
+		 * bytes, give them that. We've guaranteed that
+		 * resp_len <= priv->resp_len and priv->resp_len == nr_bytes.
+		 */
+		memcpy(resp, priv->resp, resp_len);
+	}
+
+	/* This should already be low, but just in case. */
+	gpiod_set_value_cansleep(priv->gpio_cmd, 0);
+	priv->cmd_running = false;
+
+	spin_unlock_irqrestore(&priv->cmd_state_lock, flags);
+
+	return ret;
+}
+
+static int olpc_xo175_ec_set_event_mask(unsigned int mask)
+{
+	unsigned char args[2];
+
+	args[0] = mask & 0xff;
+	args[1] = (mask >> 8) & 0xff;
+	return olpc_ec_cmd(CMD_WRITE_EXT_SCI_MASK, args, 2, NULL, 0);
+}
+
+static void olpc_xo175_ec_restart(enum reboot_mode mode, const char *cmd)
+{
+	while (1) {
+		olpc_ec_cmd(CMD_POWER_CYCLE, NULL, 0, NULL, 0);
+		mdelay(1000);
+	}
+}
+
+static void olpc_xo175_ec_power_off(void)
+{
+	while (1) {
+		olpc_ec_cmd(CMD_POWER_OFF, NULL, 0, NULL, 0);
+		mdelay(1000);
+	}
+}
+
+#ifdef CONFIG_PM
+static int olpc_xo175_ec_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct olpc_xo175_ec *priv = platform_get_drvdata(pdev);
+	unsigned char hintargs[5];
+	static unsigned int suspend_count;
+
+	suspend_count++;
+	dev_dbg(dev, "%s: suspend sync %08x\n", __func__, suspend_count);
+
+	/*
+	 * First byte is 1 to indicate suspend, the rest is an integer
+	 * counter.
+	 */
+	hintargs[0] = 1;
+	memcpy(&hintargs[1], &suspend_count, 4);
+	olpc_ec_cmd(CMD_SUSPEND_HINT, hintargs, 5, NULL, 0);
+
+	/*
+	 * After we've sent the suspend hint, don't allow further EC commands
+	 * to be run until we've resumed. Userspace tasks should be frozen,
+	 * but kernel threads and interrupts could still schedule EC commands.
+	 */
+	priv->suspended = true;
+
+	return 0;
+}
+
+static int olpc_xo175_ec_resume_noirq(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct olpc_xo175_ec *priv = platform_get_drvdata(pdev);
+
+	priv->suspended = false;
+
+	return 0;
+}
+
+static int olpc_xo175_ec_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct olpc_xo175_ec *priv = platform_get_drvdata(pdev);
+	unsigned char x = 0;
+
+	priv->suspended = false;
+
+	/*
+	 * The resume hint is only needed if no other commands are
+	 * being sent during resume. all it does is tell the EC
+	 * the SoC is definitely awake.
+	 */
+	olpc_ec_cmd(CMD_SUSPEND_HINT, &x, 1, NULL, 0);
+
+	/* Enable all EC events while we're awake */
+	olpc_xo175_ec_set_event_mask(0xffff);
+
+	return 0;
+}
+#endif
+
+static struct platform_device *olpc_ec;
+
+static struct olpc_ec_driver olpc_xo175_ec_driver = {
+	.ec_cmd = olpc_xo175_ec_cmd,
+};
+
+static int olpc_xo175_ec_remove(struct spi_device *spi)
+{
+	if (pm_power_off == olpc_xo175_ec_power_off)
+		pm_power_off = NULL;
+	if (arm_pm_restart == olpc_xo175_ec_restart)
+		arm_pm_restart = NULL;
+
+	spi_slave_abort(spi);
+
+	platform_device_unregister(olpc_ec);
+	olpc_ec = NULL;
+
+	return 0;
+}
+
+static int olpc_xo175_ec_probe(struct spi_device *spi)
+{
+	struct olpc_xo175_ec *priv;
+	int ret;
+
+	if (olpc_ec) {
+		dev_err(&spi->dev, "OLPC EC already registered.\n");
+		return -EBUSY;
+	}
+
+	priv = devm_kzalloc(&spi->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->gpio_cmd = devm_gpiod_get(&spi->dev, "cmd", GPIOD_OUT_LOW);
+	if (IS_ERR(priv->gpio_cmd)) {
+		dev_err(&spi->dev, "failed to get cmd gpio: %ld\n",
+					PTR_ERR(priv->gpio_cmd));
+		return PTR_ERR(priv->gpio_cmd);
+	}
+
+	priv->spi = spi;
+
+	spin_lock_init(&priv->cmd_state_lock);
+	priv->cmd_state = CMD_STATE_IDLE;
+	init_completion(&priv->cmd_done);
+
+	priv->logbuf_len = 0;
+
+	/* Set up power button input device */
+	priv->pwrbtn = devm_input_allocate_device(&spi->dev);
+	if (!priv->pwrbtn)
+		return -ENOMEM;
+	priv->pwrbtn->name = "Power Button";
+	priv->pwrbtn->dev.parent = &spi->dev;
+	input_set_capability(priv->pwrbtn, EV_KEY, KEY_POWER);
+	ret = input_register_device(priv->pwrbtn);
+	if (ret) {
+		dev_err(&spi->dev, "error registering input device: %d\n", ret);
+		return ret;
+	}
+
+	spi_set_drvdata(spi, priv);
+
+	priv->xfer.rx_buf = priv->rx_buf;
+	priv->xfer.tx_buf = priv->tx_buf;
+
+	olpc_xo175_ec_read_packet(priv);
+
+	olpc_ec_driver_register(&olpc_xo175_ec_driver, priv);
+	olpc_ec = platform_device_register_resndata(&spi->dev, "olpc-ec", -1,
+							NULL, 0, NULL, 0);
+
+	/* Enable all EC events while we're awake */
+	olpc_xo175_ec_set_event_mask(0xffff);
+
+	if (pm_power_off == NULL)
+		pm_power_off = olpc_xo175_ec_power_off;
+	if (arm_pm_restart == NULL)
+		arm_pm_restart = olpc_xo175_ec_restart;
+
+	dev_info(&spi->dev, "OLPC XO-1.75 Embedded Controller driver\n");
+
+	return 0;
+}
+
+static const struct dev_pm_ops olpc_xo175_ec_pm_ops = {
+#ifdef CONFIG_PM
+	.suspend	= olpc_xo175_ec_suspend,
+	.resume_noirq	= olpc_xo175_ec_resume_noirq,
+	.resume		= olpc_xo175_ec_resume,
+#endif
+};
+
+static const struct of_device_id olpc_xo175_ec_of_match[] = {
+	{ .compatible = "olpc,xo1.75-ec" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, olpc_xo175_ec_of_match);
+
+static struct spi_driver olpc_xo175_ec_spi_driver = {
+	.driver = {
+		.name	= "olpc-xo175-ec",
+		.of_match_table = olpc_xo175_ec_of_match,
+		.pm = &olpc_xo175_ec_pm_ops,
+	},
+	.probe		= olpc_xo175_ec_probe,
+	.remove		= olpc_xo175_ec_remove,
+};
+module_spi_driver(olpc_xo175_ec_spi_driver);
+
+MODULE_DESCRIPTION("OLPC XO-1.75 Embedded Controller driver");
+MODULE_AUTHOR("Lennert Buytenhek <buytenh@wantstofly.org>"); /* Functionality */
+MODULE_AUTHOR("Lubomir Rintel <lkundrak@v3.sk>"); /* Bugs */
+MODULE_LICENSE("GPL");