RE: [RFC PATCH 05/06] input/rmi4: F01 - device control

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux