Linus Walleij wrote:
> On Sat, Oct 6, 2012 at 6:10 AM, Christopher Heiny <cheiny@xxxxxxxxxxxxx> wrote:
> > RMI Function 01 implements basic device control and power management
> > behaviors for the RMI4 sensor. Since the last patch, we've decoupled
> > rmi_f01.c implementation from rmi_driver.c, so rmi_f01.c acts as a
> > standard driver module to handle F01 devices on the RMI bus.
> >
> > Like other modules, a number of attributes have been moved from sysfs to
> > debugfs, depending on their expected use.
> >
> >
> > rmi_f01.h exports definitions that we expect to be used by other
> > functionality in the future (such as firmware reflash).
> >
> >
> > Signed-off-by: Christopher Heiny <cheiny@xxxxxxxxxxxxx>
> >
> > Cc: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
> > Cc: Linus Walleij <linus.walleij@xxxxxxxxxxxxxx>
> > Cc: Naveen Kumar Gaddipati <naveen.gaddipati@xxxxxxxxxxxxxx>
> > Cc: Joeri de Gram <j.de.gram@xxxxxxxxx>
> >
> > ---
>
> There is liberal whitespacing above. (No big deal, but anyway.)
>
> (...)
>
> > +/**
> > + * @reset - set this bit to force a firmware reset of the sensor.
> > + */
> > +union f01_device_commands {
> > + struct {
> > + bool reset:1;
> > + u8 reserved:7;
> > + } __attribute__((__packed__));
> > + u8 reg;
> > +};
>
> I'm still scared by these unions. I see what you're doing but my
> preferred style of driver writing is to use a simple u8 if you just treat
> it the right way with some |= and &= ...
>
> #include <linux/bitops.h>
>
> #define F01_RESET BIT(0)
>
> u8 my_command = F01_RESET;
>
> send(&my_command);
>
> I will not insist on this because it's a bit about programming style.
> For memory-mapped devices we usually do it my way, but this
> is more like some protocol and I know protocols like to do things
> with structs and stuff so no big deal.
That's a good summary of what we're trying to do. Our original version did more of the traditional mask+shift approach to manipulating the fields in the various registers, but in the case of complicated functions such as F11 this rapidly became unreadable. We found the unions worked a lot better - the code was more readable and less error prone. For consistency we decided to apply them throughout the code.
>
> > +#ifdef CONFIG_RMI4_DEBUG
> > +struct f01_debugfs_data {
> > + bool done;
> > + struct rmi_function_container *fc;
> > +};
> > +
> > +static int f01_debug_open(struct inode *inodep, struct file *filp)
> > +{
> > + struct f01_debugfs_data *data;
> > + struct rmi_function_container *fc = inodep->i_private;
> > +
> > + data = devm_kzalloc(&fc->dev, sizeof(struct f01_debugfs_data),
> > + GFP_KERNEL);
>
> Wait, you probably did this because I requested it, but I was maybe
> wrong?
>
> Will this not re-allocate a chunk every time you look at a debugfs
> file? So it leaks memory?
>
> In that case common kzalloc() and kfree() is the way to go, as it
> is for dynamic buffers. Sorry for screwing things up for you.
No problem - we'll fix it. Or unfix it. Or something like that. :-)
>
> > + for (i = 0; i < f01->irq_count && *local_buf != 0;
> > + i++, local_buf += 2) {
> > + int irq_shift;
> > + int interrupt_enable;
> > + int result;
> > +
> > + irq_reg = i / 8;
> > + irq_shift = i % 8;
>
> Please stop doing these arithmetics-turned-maths things.
>
> irq_reg = i >> 8;
> irq_shift = i & 0xFF;
See note on this in a previous email.
>
> (...)
>
> > +static ssize_t rmi_fn_01_interrupt_enable_show(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct rmi_function_container *fc;
> > + struct f01_data *data;
> > + int i, len, total_len = 0;
> > + char *current_buf = buf;
> > +
> > + fc = to_rmi_function_container(dev);
> > + data = fc->data;
> > + /* loop through each irq value and copy its
> > + * string representation into buf */
> > + for (i = 0; i < data->irq_count; i++) {
> > + int irq_reg;
> > + int irq_shift;
> > + int interrupt_enable;
> > +
> > + irq_reg = i / 8;
> > + irq_shift = i % 8;
>
> Dito.
>
> (...)
>
> > +static int f01_probe(struct device *dev);
>
> Do you really need to forward-declare this?
It's a leftover from the process of eliminating roll-your-own bus implementation, and move the other code around as well. (same applies for similar code in rmi_f11.c).
>
> (...)
>
> > +static struct rmi_function_handler function_handler = {
> > + .driver = {
> > + .owner = THIS_MODULE,
> > + .name = "rmi_f01",
> > + .bus = &rmi_bus_type,
> > + .probe = f01_probe,
> > + .remove = f01_remove_device,
> > + },
> > + .func = 0x01,
> > + .config = rmi_f01_config,
> > + .attention = rmi_f01_attention,
> > +
> > +#ifdef CONFIG_PM
> > + .suspend = rmi_f01_suspend,
> > + .resume = rmi_f01_resume,
> > +#endif /* CONFIG_PM */
> > +};
>
> Just move this block of struct below the probe function...
>
> > +static __devinit int f01_probe(struct device *dev)
>
> Header file looks OK (if these unions is the way to go...)
>
> Yours,
> Linus Walleij--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
