Hello Dmitry,

thanks for your quick feedback, a few questions below:

On 21.10.2013 18:20, Dmitry Torokhov wrote:

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?

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

+	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.

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;


Best regards, Florian
-- 
SENT FROM MY DEC VT50 TERMINAL