lkml.org 
[lkml]   [2018]   [Nov]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 06/15] Platform: OLPC: Add XO-1.75 EC driver
    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
    >

    --
    James Cameron
    http://quozl.netrek.org/

    \
     
     \ /
      Last update: 2018-11-13 23:07    [W:7.697 / U:0.344 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site