Hi

On Tue, Oct 22, 2013 at 8:08 AM, Florian Echtler [off-list ref] wrote:

Hello Dmitry,

thanks for your quick feedback, a few questions below:

On 21.10.2013 18:20, Dmitry Torokhov wrote:

quoted

On Sun, Oct 20, 2013 at 06:49:11PM +0200, Florian Echtler wrote:

quoted

+/* read 512 bytes from endpoint 0x86 -> get header + blobs */
+struct sur40_header {
+
+    uint16_t type;       /* always 0x0001 */
+    uint16_t count;      /* count of blobs (if 0: continue prev. packet) */
+
+    uint32_t packet_id;
+
+    uint32_t timestamp;  /* milliseconds (inc. by 16 or 17 each frame) */
+    uint32_t unknown;    /* "epoch?" always 02/03 00 00 00 */

Proper internal kernel types are u8, u16, u32. For user-facing APIs
__u8, __u16, and __u32 should be used. Also, since this is data coming
directly off the wire, you should be using __le16, __le32, etc, and then
do __leXX_to_cpu() conversion before using it in calculations.

OK, I'll switch to u32 throughout (also for the float, I'll explain in a
commment). However, I haven't found a single other touchscreen driver
which uses __le32, even though they all probably process raw wire data -
can you suggest an example?

These are probably all broken or the hardware guarantees
cpu-byte-order. Anyway, what you should do is use __le16/32 for your
types which represent data from the device. Then call le16_to_cpu() on
these values to convert it to host byte-order. Something like this:

struct sur40_raw_header {
  __le16 type;
  __le32 unused;
  __le8 count;
} __packed;

struct sur40_header {
  u16 type;
  u8 count;
};

static void parse_data(const struct sur40_raw_header *h)
{
  struct sur40_header d;

  d.type = __le16_to_cpu(h->type);
  d.count = h->count;
  do_something(&d);
}

quoted

quoted

+/* debug helper macro */
+#define get_dev(x) (&(x->usbdev->dev))

Just stick that dev in sur40_state and then use sur40->dev throughout.

OK.

quoted

quoted

+    struct sur40_header *header = &(sur40->bulk_in_buffer->header);

No need to have parenthesis around & operator.

quoted

+    struct sur40_blob *inblob = &(sur40->bulk_in_buffer->blobs[0]);

Same here.

Intention seems clearer to me with parentheses, but if this doesn't
conform to coding style, I'll fix it.

We never use parentheses for that. You will get used to it ;)

quoted

quoted

+    if (!sur40->bulk_in_buffer) {
+            dev_err(&interface->dev, "Unable to allocate input buffer.");
+            sur40_delete(sur40);

Would prefer standard kernel error unwinding style (gotos to proper
unwinding point).

Something like this example from ucb1400_ts?

        error = input_register_device(ucb->ts_idev);
        if (error)
                goto err_free_irq;

        return 0;

err_free_irq:
        free_irq(ucb->irq, ucb);
err_free_devs:
        input_free_device(ucb->ts_idev);
err:
        return error;

Yepp, exactly.

David