lkml.org 
[lkml]   [2012]   [Oct]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [RFC PATCH 01/06] input/rmi4: Public header and documentation
From
On Sat, Oct 6, 2012 at 6:09 AM, Christopher Heiny <cheiny@synaptics.com> 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


\
 
 \ /
  Last update: 2012-10-09 10:21    [W:0.210 / U:0.200 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site