Message-ID: <CACRpkdZht7RfL48FjxnCKD9R7tHaKdQjM=hO47coMkoctPH_tw@mail.gmail.com>
Date:	Tue, 9 Oct 2012 11:31:23 +0200
From:	Linus Walleij <linus.walleij@...aro.org>
To:	Christopher Heiny <cheiny@...aptics.com>
Cc:	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Jean Delvare <khali@...ux-fr.org>,
	Linux Kernel <linux-kernel@...r.kernel.org>,
	Linux Input <linux-input@...r.kernel.org>,
	Allie Xiong <axiong@...aptics.com>,
	Vivian Ly <vly@...aptics.com>,
	Daniel Rosenberg <daniel.rosenberg@...aptics.com>,
	Alexandra Chen <alexandra.chen@...synaptics.com>,
	Joerie de Gram <j.de.gram@...il.com>,
	Wolfram Sang <w.sang@...gutronix.de>,
	Mathieu Poirier <mathieu.poirier@...aro.org>,
	Linus Walleij <linus.walleij@...ricsson.com>,
	Naveen Kumar Gaddipati <naveen.gaddipati@...ricsson.com>
Subject: Re: [RFC PATCH 05/06] input/rmi4: F01 - device control

On Sat, Oct 6, 2012 at 6:10 AM, Christopher Heiny <cheiny@...aptics.com> 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@...aptics.com>
>
> Cc: Dmitry Torokhov <dmitry.torokhov@...il.com>
> Cc: Linus Walleij <linus.walleij@...ricsson.com>
> Cc: Naveen Kumar Gaddipati <naveen.gaddipati@...ricsson.com>
> Cc: Joeri de Gram <j.de.gram@...il.com>
>
> ---

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.

> +#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. :-(

> +       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;

(...)
> +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?

(...)
> +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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists