Re: [RFC PATCH 1/17] input: RMI4 public header file and documentation.

[Linus Walleij]

On Sat, Aug 18, 2012 at 12:17 AM, Christopher Heiny
<cheiny@xxxxxxxxxxxxx> wrote:

(...)
> +#ifdef CONFIG_HAS_EARLYSUSPEND
> +#include <linux/earlysuspend.h>
> +#endif

This does not exist in the mainline kernel, I can see you're having fun with
Android ... check with Rafael on how to really handle this in a contemporary
kernel.

> +/**
> + * struct rmi_f11_axis_alignment - target axis alignment
> + * @swap_axes: set to TRUE if desired to swap x- and y-axis
> + * @flip_x: set to TRUE if desired to flip direction on x-axis
> + * @flip_y: set to TRUE if desired to flip direction on y-axis

This kerneldoc is quite incomplete. There is a *lot* of members below
that are not documented.

> + */
> +struct rmi_f11_2d_axis_alignment {
> +       bool swap_axes;
> +       bool flip_x;
> +       bool flip_y;
> +       int clip_X_low;
> +       int clip_Y_low;
> +       int clip_X_high;
> +       int clip_Y_high;
> +       int offset_X;
> +       int offset_Y;
> +       int rel_report_enabled;
> +
> +#ifdef CONFIG_RMI4_DEBUG
> +       struct dentry *debugfs_flip;
> +       struct dentry *debugfs_clip;
> +       struct dentry *debugfs_offset;
> +       struct dentry *debugfs_swap;
> +       u16 reg_debug_addr;
> +       u8 reg_debug_size;
> +#endif
> +};

(...)
> +struct rmi_f11_virtualbutton_map {
> +       u8 buttons;
> +       struct virtualbutton_map *map;
> +};

<newline> (nitpick)

> +struct rmi_device_platform_data_spi {
> +       int block_delay_us;
> +       int split_read_block_delay_us;
> +       int read_delay_us;
> +       int write_delay_us;
> +       int split_read_byte_delay_us;
> +       int pre_delay_us;
> +       int post_delay_us;
> +
> +       void *cs_assert_data;
> +       int (*cs_assert) (const void *cs_assert_data, const bool assert);
> +};
> +
> +/**
> + * struct rmi_device_platform_data - system specific configuration info.
> + *
> + * @firmware_name - if specified will override default firmware name,
> + * for reflashing.
> + *
> + * @f11_type_b - Force F11 to treat the sensor as a multitouch Type B sensor.
> + * This is useful for older Type B RMI4 sensors that can't be queried for this
> + * information, but will cause problems if you force a Type A sensor to be
> + * treated as type_b.
> + *
> + * @pre_suspend - this will be called before any other suspend operations are
> + * done.
> + * @post_suspend - this will be called after all suspend operations are
> + * completed.  This is the ONLY safe place to power off an RMI sensor
> + * during the suspend process.
> + * @pre_resume - this is called before any other resume operations.  If you
> + * powered off the RMI4 sensor in post_suspend(), then you MUST power it back
> + * here, and you MUST wait an appropriate time for the ASIC to come up
> + * (100ms to 200ms, depending on the sensor) before returning.
> + * @pm_data - this will be passed to the various (pre|post)_(suspend/resume)
> + * functions.

This kerneldoc is incomplete, maybe intentional, I don't know...

(...)
> +struct rmi_function_handler {
> +       int func;
> +       int (*init)(struct rmi_function_container *fc);
> +       int (*config)(struct rmi_function_container *fc);
> +       int (*reset)(struct rmi_function_container *fc);
> +       int (*attention)(struct rmi_function_container *fc, u8 *irq_bits);
> +#ifdef CONFIG_PM
> +       int (*suspend)(struct rmi_function_container *fc);
> +       int (*resume)(struct rmi_function_container *fc);
> +#if defined(CONFIG_HAS_EARLYSUSPEND) && \
> +               !defined(CONFIG_RMI4_SPECIAL_EARLYSUSPEND)

There is this earlysuspend again, now with some turbo-version
which is "special" as well :-)

> +       int (*early_suspend)(struct rmi_function_container *fc);
> +       int (*late_resume)(struct rmi_function_container *fc);
> +#endif
> +#endif
> +       void (*remove)(struct rmi_function_container *fc);
> +};

(...)
> +/**
> + * struct rmi_device - represents an RMI device
> + * @dev: The device created for the RMI bus
> + * @number: Unique number for the device on the bus.
> + * @driver: Pointer to associated driver
> + * @phys: Pointer to the physical interface
> + * @early_suspend_handler: Pointers to early_suspend and late_resume, if
> + * configured.
> + *
> + * This structs represent an RMI device.
> + *
> + */
> +struct rmi_device {
> +       struct device dev;
> +       int number;
> +
> +       struct rmi_driver *driver;
> +       struct rmi_phys_device *phys;
> +
> +#ifdef CONFIG_HAS_EARLYSUSPEND
> +       struct early_suspend early_suspend_handler;
> +#endif

...and here...

> +#ifdef CONFIG_RMI4_DEBUG
> +       struct dentry *debugfs_root;
> +#endif
> +};

(...)
> +/* Helper fn to convert a byte array representing a short in the RMI
> + * endian-ness to a short in the native processor's specific endianness.
> + * We don't use ntohs/htons here because, well, we're not dealing with
> + * a pair of shorts. And casting dest to short* wouldn't work, because
> + * that would imply knowing the byte order of short in the first place.
> + */
> +static inline void batohs(unsigned short *dest, unsigned char *src)

Change this prototype to use kernel-internal types, and what is
wron with using the return value??

static inline void batohs(unsigned short *dest, unsigned char *src)

> +{
> +       *dest = src[1] * 0x100 + src[0];
> +}

Doing this instead of casting is good style. But I would have done like this:

static inline u16 batohs(u8 *src)
{
   return src[1] << 8 | src[0];
}

Which the compiler *hopufully will emit, but since we're doing
arithmetic, not maths, please use shifts and OR:s.

And at the call sites, instead of

unsigned short mr16;
unsigned char mr_arrary[2];
batohs(&mr16, &mr_array[0]);

Just:

u16 mr16;
u8 mr_array[2];

mr16 = batohs(&mr_array[0]);

> +/* Helper function to convert a short (in host processor endianess) to
> + * a byte array in the RMI endianess for shorts.  See above comment for
> + * why we dont us htons or something like that.
> + */
> +static inline void hstoba(unsigned char *dest, unsigned short src)

Here yuou need to pass a pointer to the destination for sure.

> +{
> +       dest[0] = src % 0x100;
> +       dest[1] = src / 0x100;
> +}

I would have done the above like this:

static inline void hstoba(u8 *dest, u16 src)
{
  dest[0] = src & 0xFF;
  dest[1] = src >> 8;
}

(...)
> +/* utility function for bit access of u8*'s */

No, do not reinvent the wheel. Use
#include <linux/bitmap.h>
(which in turn will include <linux/bitops.h> for you)
Those bitmaps use unsigned int but that does not
really matter, they should work just fine using the lower
8 bits.

> +void u8_set_bit(u8 *target, int pos);

Use set_bit() from <linux/bitops.h>

> +void u8_clear_bit(u8 *target, int pos);

Use clear_bit() from <linux/bitops.h>

> +bool u8_is_set(u8 *target, int pos);

Use test_bit() from <linux/bitops.h>

> +bool u8_is_any_set(u8 *target, int size);

Use find_first_bit(), if this returns 0 none is set, !find_first_bit() for
example serves the same purpose.

> +void u8_or(u8 *dest, u8* target1, u8* target2, int size);

Use bitmap_or()

> +void u8_and(u8 *dest, u8* target1, u8* target2, int size);

Use bitmap_and()

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/