Hans de Goede wrote:
> Hi,
>
> Thanks for your continued work on this. I'm afraid I found
> one thing which needs fixing (can be fixed with
> a separate patch after merging, but that is up to
> Jean-Francois).
>
> See my comments inline.
Thanks for the feedback, I hope the quality of the software increases
during this review phase.
> On 01/17/2010 02:08 PM, Németh Márton wrote:
[snip]
>> +#ifdef CONFIG_INPUT
>> +#if LINUX_VERSION_CODE< KERNEL_VERSION(2, 6, 19)
>> +static void int_irq(struct urb *urb, struct pt_regs *regs)
>> +#else
>> +static void int_irq(struct urb *urb)
>> +#endif
>> +{
>> + struct gspca_dev *gspca_dev = (struct gspca_dev *) urb->context;
>> + int ret;
>> +
>> + if (urb->status == 0) {
>> + if (gspca_dev->sd_desc->int_pkt_scan(gspca_dev,
>> + urb->transfer_buffer, urb->actual_length)< 0) {
>> + PDEBUG(D_ERR, "Unknown packet received");
>> + }
>> +
>> + ret = usb_submit_urb(urb, GFP_ATOMIC);
>> + if (ret< 0)
>> + PDEBUG(D_ERR, "Resubmit URB failed with error %i", ret);
>> + }
>> +}
>> +
>
> If the status is not 0 you should print an error message, and
> reset the status and still resubmit the urb, if you don't resubmit
> on error, after one single usb glitch, the button will stop working.
I rewrote this function, see the following patch version.
[snip]
>> @@ -2137,6 +2308,11 @@
>>
>> usb_set_intfdata(intf, gspca_dev);
>> PDEBUG(D_PROBE, "%s created", video_device_node_name(&gspca_dev->vdev));
>> +
>> + ret = gspca_input_connect(gspca_dev);
>> + if (0<= ret)
>> + ret = gspca_input_create_urb(gspca_dev);
>> +
>
> I don't like this reverse psychology if. Why not just write:
> if (ret == 0) ?
I changed this line to "if (ret == 0)".
When I use a not-equal relation I try to put the smaller value on the left
hand side and the bigger one to the right hand side. In this case a range
which is expressed in mathematical notation like 10 < a <= 16 can be written
in C like (10 < a) && (a <= 16).
But it is a different question what people like and what not, so I've changed
that line.
> Otherwise it looks good.
Thanks.
I'm looking forward to receive more comments on the new version of this patch.
Regars,
Márton Németh
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
