| Message ID | 20181010172300.317643-7-lkundrak@v3.sk (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for OLPC XO 1.75 Embedded Controller | expand |
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. > +};
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
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 >
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
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
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");
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