Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754054Ab2JIIkj (ORCPT ); Tue, 9 Oct 2012 04:40:39 -0400 Received: from mail-wi0-f172.google.com ([209.85.212.172]:32828 "EHLO mail-wi0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752321Ab2JIIkh (ORCPT ); Tue, 9 Oct 2012 04:40:37 -0400 MIME-Version: 1.0 In-Reply-To: <1349496603-20775-3-git-send-email-cheiny@synaptics.com> References: <1349496603-20775-1-git-send-email-cheiny@synaptics.com> <1349496603-20775-3-git-send-email-cheiny@synaptics.com> Date: Tue, 9 Oct 2012 10:40:35 +0200 Message-ID: Subject: Re: [RFC PATCH 02/06] input/rmi4: Core files From: Linus Walleij To: Christopher Heiny , Greg KH Cc: Dmitry Torokhov , Jean Delvare , Linux Kernel , Linux Input , Allie Xiong , Vivian Ly , Daniel Rosenberg , Alexandra Chen , Joerie de Gram , Wolfram Sang , Mathieu Poirier , Linus Walleij , Naveen Kumar Gaddipati Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6483 Lines: 203 On Sat, Oct 6, 2012 at 6:09 AM, Christopher Heiny 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 > +#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 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 ? (...) > +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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/