Re: [RFC PATCH 02/06] input/rmi4: Core files

[Linus Walleij]

On Sat, Oct 6, 2012 at 6:09 AM, Christopher Heiny <cheiny@xxxxxxxxxxxxx> wrote:

> rmi_bus.c implements the basic functionality of the RMI bus.  This file is
> greatly simplified compared to the previous patch - we've switched from
> "do it yourself" device/driver binding to using device_type to distinguish
> between the two kinds of devices on the bus (sensor devices and function
> specific devices) and using the standard bus implementation to manage devices
> and drivers.

So I think you really want Greg KH to look at this bus implementation
now. Please include Greg on future mailings...

It looks much improved from previous versions, and sorry if I am now
adding even more comments, but it's because you cleared out some
noise that was disturbing my perception so I can cleanly review
the architecture of this thing now. (I'm impressed by your work and
new high-speed turnaround time!)

> +#ifdef CONFIG_RMI4_DEBUG
> +#include <linux/debugfs.h>
> +#endif

As noted previously, drop the #ifdef:s

> +#ifdef CONFIG_RMI4_DEBUG
> +static struct dentry *rmi_debugfs_root;
> +#endif

I'd use #ifdef CONFIG_DEBUG_FS and try to
move this declaration closer to the actual debugfs
code block.

Apart from this the core bus looks good to me, but it's not my
area of expertise...

(...)
> diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c

> +#ifdef CONFIG_RMI4_DEBUG

I'd just use CONFIG_DEBUG_FS

> +struct driver_debugfs_data {
> +       bool done;
> +       struct rmi_device *rmi_dev;
> +};

(...)
> +#define DELAY_NAME "delay"

This is only used in one place, why not just use the string
"delay" there?

(...)
> +       if (IS_ENABLED(CONFIG_RMI4_SPI) && !strncmp("spi", info->proto, 3)) {
> +               data->debugfs_delay = debugfs_create_file(DELAY_NAME,
> +                               RMI_RW_ATTR, rmi_dev->debugfs_root, rmi_dev,
> +                               &delay_fops);

i.e. there.

(...)
> +/* Useful helper functions for u8* */
> +
> +static bool u8_is_any_set(u8 *target, int size)
> +{
> +       int i;
> +       /* We'd like to use find_first_bit, but it ALWAYS returns 1,
> +       *  no matter what we pass it.  So we have to do this the hard way.
> +       *  return find_first_bit((long unsigned int *)  target, size) != 0;
> +       */
> +       for (i = 0; i < size; i++) {
> +               if (target[i])
> +                       return true;
> +       }
> +       return false;
> +}

Instead of:

if (u8_is_any_set(foo, 128) {}

Why can't you use:

if (!bitmap_empty(foo, 128*8) {}

?

If you look at the implementation in the <linux/bitmap.h> header
and __bitmap_empty() in lib/bitmap.c you will realize that this
function is already optimized like this (and I actually don't think
the RMI4 code is performance-critical for these functions anyway,
but prove me wrong!)

> +
> +/** This is here because all those casts made for some ugly code.
> + */
> +static void u8_and(u8 *dest, u8 *target1, u8 *target2, int nbits)
> +{
> +       bitmap_and((long unsigned int *) dest,
> +                  (long unsigned int *) target1,
> +                  (long unsigned int *) target2,
> +                  nbits);
> +}

Hm, getting rid of unreadable casts is a valid case.

I'll be OK with this but maybe the real solution is to introduce such
helpers into <linux/bitmap.h>?

(...)
> +static int process_interrupt_requests(struct rmi_device *rmi_dev)
> +{
> +       struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev);
> +       struct device *dev = &rmi_dev->dev;
> +       struct rmi_function_container *entry;
> +       u8 irq_status[data->num_of_irq_regs];

Looking at this...

What does the data->num_of_irq_regs actually contain?

I just fear that it is something constant like always 2 or always 4,
so there is actually, in reality, a 16 or 32 bit register hiding in there.

In that case what you should do is to represent it as a u16 or u32 here,
just or the bits into a status word, and then walk over that status
word with something like ffs(bitword); ...

(...)
> +static int standard_resume(struct rmi_device *rmi_dev)

Standard eh? :-)

Atleast prefix with rmi4_*...

> +static int rmi_driver_suspend(struct device *dev)
> +{
> +       struct rmi_device *rmi_dev = to_rmi_device(dev);
> +       return standard_suspend(rmi_dev);
> +}
> +
> +static int rmi_driver_resume(struct device *dev)
> +{
> +       struct rmi_device *rmi_dev = to_rmi_device(dev);
> +       return standard_resume(rmi_dev);
> +}

If all these two are doing are to call another function with almost
the same name, what is the point? Just sink the code from
"standard_suspend()" and "standard_resume()" into these
functions and remove a pointless layer of abstraction.

Apart from this the core driver looks good to me.

(...)
> diff --git a/drivers/input/rmi4/rmi_driver.h b/drivers/input/rmi4/rmi_driver.h

(...)
> +#define simple_show_union_struct(regtype, propname, fmt)\
> +static ssize_t tricat(rmi_fn_, FNUM, _##propname##_show)(struct device *dev,\
> +                               struct device_attribute *attr, char *buf) {\
> +       struct rmi_function_container *fc;\
> +       struct FUNCTION_DATA *data;\
> +\
> +       fc = to_rmi_function_container(dev);\
> +       data = fc->data;\
> +\
> +       return snprintf(buf, PAGE_SIZE, fmt,\
> +                       data->regtype.propname);\
> +}

OK I see the point, but is there really no other way to do this than
to #define huge static inlines like these? Is it really not possible to
create just generic functions instead of going this far?

(same comment for all)

> +union pdt_properties {
> +       struct {
> +               u8 reserved_1:6;
> +               u8 has_bsr:1;
> +               u8 reserved_2:1;
> +       } __attribute__((__packed__));
> +       u8 regs[1];

I don't understand what this union is trying to achieve.

regs[1] does not look right considering what you're trying to
achieve. Since the above fields require a regs[2] (9 bits!)
to be stored. Maybe write out what you're trying to do here
so I can understand it? (If everyone else in the world gets
it immediately, it's maybe me that need fixing instead...)

Apart from these remarks it's looking real nice now!

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/