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:
>> +/* 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?
>> +/* debug helper macro */
>> +#define get_dev(x) (&(x->usbdev->dev))
> Just stick that dev in sur40_state and then use sur40->dev throughout.
OK.
>> + struct sur40_header *header = &(sur40->bulk_in_buffer->header);
> No need to have parenthesis around & operator.
>> + 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.
>> + 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
signature.asc
Description: OpenPGP digital signature
