On Sat, Oct 6, 2012 at 6:09 AM, Christopher Heiny <cheiny@xxxxxxxxxxxxx> wrote: > As requested in the feedback from the previous patch, we've documented the > debugfs and sysfs attributes in files in Documentation/ABI/testing. There's > two files, one for debugfs and one for sysfs. This is a massive improvement! Atleast as far as I've read... If you fix the below remarks I think I'm ready to accept this file, but that's just me and doesn't say anything about what Dmitry et al will comment on... (...) > + The RMI4 driver implementation exposes a set of informational and control > + parameters via debugs. These parameters are those that typically are only s/debugs/debugfs (...) > + comms_debug - (rw) Write 1 to this dump information about register > + reads and writes to the console. Write 0 to this to turn > + this feature off. WARNING: Imposes a major performance > + penalty when turned on. > + irq_debug - (rw) Write 1 to this dump information about interrupts > + to the console. Write 0 to this to turn this feature off. > + WARNIG: Imposes a major performance penalty when turned on. Hm. Usually we control dynamic debug prints by standard kernel frameworks, can you tell what is wrong with this and why you need a custom mechanism? See the following: Documentation/dynamic-debug-howto.txt http://lwn.net/Articles/434833/ (...) > +++ b/Documentation/ABI/testing/sysfs-rmi4 (...) > + chargerinput ... (rw) User space programs can use this to tell the > + sensor that the system is plugged into an external power > + source (as opposed to running on batteries). This allows > + the sensor firmware to make necessary adjustments for the > + current capacitence regime. Write 1 to this when the > + system is using external power, write 0 to this when the > + system is running on batteries. See spec for full details. I remember discussing in-kernel notifiers for this. I don't really see the point in tunnelling a notification from the drivers/power subsystem to the drivers/input subsystem through userspace for no good. It's no blocker though, I don't expect you to fix this as part of this driver submission. Maybe Anton can comment? (...) > + interrupt_enable ... (ro) This represents the current RMI4 interrupt > + mask (F01_RMI_Ctrl1 registers). See spec for full details. What does the userspace have to do with this stuff? Seems way too low-level, but whatever. (...) > + sleepmode ... (rw) Controls power management on the device. Writing > + 0 to this parameter puts the device into its normal operating > + mode. Writing 1 to this parameter fully disables touch > + sensors and similar inputs - no touch data will be reported > + from the device in this mode. Writing 2 or 3 to this device > + may or may not have an effect, depending on the particular > + device - see the product specification for your sensor for > + details. Usually power management is controlled from kernelspace, but no big deal, maybe userspace knows even more details in some cases. > + unconfigured ... (ro) This is the opposite of the configured bit, > + described above. So why is it needed? Isn't it implicit from the "configured" property if this is 0 then it's unconfigured? Seems superfluous. (...) > +++ b/include/linux/rmi.h (...) > +#ifdef CONFIG_RMI4_DEBUG > +#include <linux/debugfs.h> > +#endif Don't include it conditionally, always just include it whether you use it or not. It doesn't hurt, and doesn't cause compile problems. (...) > +/** > + * struct rmi_device_platform_data_spi - provides paramters used in SPI s/paramters/parameters/ > + * communitcations. All Synaptics SPI products support a standard SPI s/communitcations/communications > + * @cs_assert - For systems where the SPI subsystem does not control the CS/SSB > + * line, or where such control is broken, you can provide a custom routine to > + * handle a GPIO as CS/SSB. This routine will be called at the beginning and > + * end of each SPI transaction. The RMI SPI implementation will wait > + * pre_delay_us after this routine returns before starting the SPI transfer; > + * and post_delay_us after completion of the SPI transfer(s) before calling it > + * with assert==FALSE. Hm hm, can you describe the case where this happens? Usually we don't avoid fixes for broken drivers by duct-taping solutions into other drivers, instead we fix the SPI driver. I can think of systems where CS is asserted not by using GPIO but by poking some special register for example, which is a valid reason for including this, but working around broken SPI drivers is not a valid reason to include this. (Paging Mark about it.) (...) > +/** > + * struct rmi_device_platform_data - system specific configuration info. > + * > + * @driver_name (...) > +struct rmi_device_platform_data { > + char *driver_name; I don't understand what the driver name is doing in the platform data. The driver name should be part of the device driver struct, and match on dev_name(struct device *), not be passed in here. Please explain... (...) > +#ifdef CONFIG_RMI4_DEBUG > + struct dentry *debugfs_root; > +#endif Maybe just use CONFIG_DEBUG_FS instead, it's more to the point? (occurs twice) (...) > +/** > + * rmi_register_phys_device - register a physical device connection on the RMI > + * bus. Physical drivers provide communication from the devices on the bus to > + * the RMI4 sensor on a bus such as SPI, I2C, and so on. > + * > + * @phys: the physical device to register > + */ Don't put the kerneldoc here in the header, put it in the code. Only documentation for structs, enums and inline functions go into the header files. (Same comment for all functions.) > +int rmi_register_phys_device(struct rmi_phys_device *phys); (...) > +/** > + * Helper function to convert a 16 bit value (in host processor endianess) to > + * a byte array in the RMI endianess for u16s. See above comment for > + * why we dont us htons or something like that. > + */ So in this case it's correct to document it here, because this is an inline function. > +static inline void hstoba(u8 *dest, u16 src) > +{ > + dest[0] = src % 0x100; > + dest[1] = src / 0x100; But please use arithmetic operators (I think I said this on the last review): dest[0] = src & 0xFF; dest[1] = src >> 8; Doing it the above way makes artithmetic look like maths, and it isn't. Besides it's done this way in most parts of the kernel and we're familiar with it. (...) > +#ifdef CONFIG_RMI4_DEBUG > +/** > + * Utility routine to handle writes to read-only attributes. Hopefully > + * this will never happen, but if the user does something stupid, we don't > + * want to accept it quietly (which is what can happen if you just put NULL > + * for the attribute's store function). > + */ > +static inline ssize_t rmi_store_error(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + dev_warn(dev, > + "WARNING: Attempt to write %d characters to read-only attribute %s.", > + count, attr->attr.name); > + return -EPERM; > +} Here it looks like you're hiding a lot of stuff that should be dev_warn()? Consider my earlier point about dynamic debug. > +static inline ssize_t rmi_show_error(struct device *dev, > + struct device_attribute *attr, char *buf) Dito. 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
