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@xxxxx/ 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
