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
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Attachment:
signature.asc
Description: Digital signature
_______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
