Re: [RFC PATCH 01/06] input/rmi4: Public header and documentation
From: Linus Walleij
Date: Tue Oct 09 2012 - 03:43:08 EST
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-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/