From: James Cameron <quozl@laptop.org>
To: Lubomir Rintel <lkundrak@v3.sk>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>,
Mark Brown <broonie@kernel.org>,
Geert Uytterhoeven <geert+renesas@glider.be>,
Darren Hart <dvhart@infradead.org>,
Andy Shevchenko <andy@infradead.org>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Sebastian Reichel <sre@kernel.org>,
Rob Herring <robh+dt@kernel.org>,
Mark Rutland <mark.rutland@arm.com>,
Eric Miao <eric.y.miao@gmail.com>,
Haojian Zhuang <haojian.zhuang@gmail.com>,
Daniel Mack <daniel@zonque.org>,
Robert Jarzmik <robert.jarzmik@free.fr>,
linux-spi <linux-spi@vger.kernel.org>,
devicetree <devicetree@vger.kernel.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
linux-arm Mailing List <linux-arm-kernel@lists.infradead.org>,
Platform Driver <platform-driver-x86@vger.kernel.org>,
devel@driverdev.osuosl.org, Linux PM <linux-pm@vger.kernel.org>
Subject: Re: [PATCH 06/15] Platform: OLPC: Add XO-1.75 EC driver
Date: Wed, 14 Nov 2018 09:06:31 +1100 [thread overview]
Message-ID: <20181113220631.GB22127@us.netrek.org> (raw)
In-Reply-To: <8881f5e48613c9d9d133e3964031fe2ab57f4801.camel@v3.sk>
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/
next prev parent reply other threads:[~2018-11-13 22:06 UTC|newest]
Thread overview: 62+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-10-10 17:22 [PATCH 0/15] Add support for OLPC XO 1.75 Embedded Controller Lubomir Rintel
2018-10-10 17:22 ` [PATCH 01/15] power: supply: olpc_battery: correct the temperature units Lubomir Rintel
2018-10-19 13:00 ` Andy Shevchenko
2018-10-21 21:27 ` Sebastian Reichel
2018-11-02 22:16 ` Pavel Machek
2018-10-10 17:22 ` [PATCH 02/15] Revert "platform/olpc: Make ec explicitly non-modular" Lubomir Rintel
2018-10-19 13:04 ` Andy Shevchenko
2018-11-02 22:16 ` Pavel Machek
2018-11-15 18:57 ` Lubomir Rintel
2018-10-10 17:22 ` [PATCH 03/15] dt-bindings: olpc,xo1.75-ec: Add OLPC XO-1.75 EC bindings Lubomir Rintel
2018-10-17 19:38 ` Rob Herring
2018-11-04 12:26 ` Pavel Machek
2018-10-10 17:22 ` [PATCH 04/15] Platform: OLPC: Remove an unused include Lubomir Rintel
2018-10-19 13:05 ` Andy Shevchenko
2018-11-13 13:59 ` Lubomir Rintel
2018-11-04 12:26 ` Pavel Machek
2018-10-10 17:22 ` [PATCH 05/15] Platform: OLPC: Move OLPC config symbol out of x86 tree Lubomir Rintel
2018-10-19 13:09 ` Andy Shevchenko
2018-11-04 12:27 ` Pavel Machek
2018-10-10 17:22 ` [PATCH 06/15] Platform: OLPC: Add XO-1.75 EC driver Lubomir Rintel
2018-10-19 16:06 ` Andy Shevchenko
2018-11-13 17:26 ` Lubomir Rintel
2018-11-13 22:06 ` James Cameron [this message]
2018-11-19 10:40 ` Pavel Machek
2018-11-19 13:25 ` Lubomir Rintel
2018-10-10 17:22 ` [PATCH 07/15] Platform: OLPC: Avoid a warning if the EC didn't register yet Lubomir Rintel
2018-10-19 13:11 ` Andy Shevchenko
2018-11-04 12:30 ` Pavel Machek
2018-10-10 17:22 ` [PATCH 08/15] Platform: OLPC: Move EC-specific functionality out from x86 Lubomir Rintel
2018-10-19 13:36 ` Andy Shevchenko
2018-11-14 16:20 ` Lubomir Rintel
2018-11-04 12:31 ` Pavel Machek
2018-10-10 17:22 ` [PATCH 09/15] Platform: OLPC: add a regulator for the DCON Lubomir Rintel
2018-10-19 13:39 ` Andy Shevchenko
2018-10-10 17:22 ` [PATCH 10/15] dt-bindings: olpc_battery: Add XO-1.5 battery Lubomir Rintel
2018-10-17 19:38 ` Rob Herring
2018-11-04 12:32 ` Pavel Machek
2018-10-10 17:22 ` [PATCH 11/15] x86, olpc: Use a correct version when making up a battery node Lubomir Rintel
2018-10-19 13:43 ` Andy Shevchenko
2018-11-14 16:49 ` Lubomir Rintel
2018-11-04 12:34 ` Pavel Machek
2018-10-10 17:22 ` [PATCH 12/15] power: supply: olpc_battery: Use DT to get battery version Lubomir Rintel
2018-10-19 13:45 ` Andy Shevchenko
2018-11-15 18:33 ` Lubomir Rintel
2018-11-04 12:37 ` Pavel Machek
2018-11-15 18:36 ` Lubomir Rintel
2018-10-10 17:22 ` [PATCH 13/15] power: supply: olpc_battery: Move priv data to a struct Lubomir Rintel
2018-10-19 13:48 ` Andy Shevchenko
2018-11-04 14:37 ` Pavel Machek
2018-11-14 17:10 ` Lubomir Rintel
2018-10-10 17:22 ` [PATCH 14/15] power: supply: olpc_battery: Avoid using platform_info Lubomir Rintel
2018-10-19 13:50 ` Andy Shevchenko
2018-11-14 17:19 ` Lubomir Rintel
2018-11-04 14:39 ` Pavel Machek
2018-10-10 17:23 ` [PATCH 15/15] power: supply: olpc_battery: Add OLPC XO 1.75 support Lubomir Rintel
2018-10-19 13:55 ` Andy Shevchenko
2018-11-04 14:39 ` Pavel Machek
2018-10-10 19:26 ` [PATCH 0/15] Add support for OLPC XO 1.75 Embedded Controller Rob Herring
2018-10-11 10:42 ` Lubomir Rintel
2018-10-19 13:57 ` Andy Shevchenko
2018-10-23 17:03 ` Lubomir Rintel
2018-10-21 21:45 ` Sebastian Reichel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20181113220631.GB22127@us.netrek.org \
--to=quozl@laptop.org \
--cc=andy.shevchenko@gmail.com \
--cc=andy@infradead.org \
--cc=broonie@kernel.org \
--cc=daniel@zonque.org \
--cc=devel@driverdev.osuosl.org \
--cc=devicetree@vger.kernel.org \
--cc=dvhart@infradead.org \
--cc=eric.y.miao@gmail.com \
--cc=geert+renesas@glider.be \
--cc=gregkh@linuxfoundation.org \
--cc=haojian.zhuang@gmail.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=linux-spi@vger.kernel.org \
--cc=lkundrak@v3.sk \
--cc=mark.rutland@arm.com \
--cc=platform-driver-x86@vger.kernel.org \
--cc=robert.jarzmik@free.fr \
--cc=robh+dt@kernel.org \
--cc=sre@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).