Re: [PATCH] Eagle and ADI 930 usb adsl modem driver

[matthieu castet <castet.matthieu@free.fr>]

Hi Greg,

thanks for your review.

Greg KH wrote:
> On Sun, Oct 30, 2005 at 12:37:41AM +0200, matthieu castet wrote:
> 
>>Please comment and consider for inclusion.
> 
> 
> I need a "Signed-off-by:" line in order to be able to add it.  Care to
> redo things based on the comments you have had and resend it with this
> line?
> 
> 
no problem ;)
>>+ *
>>+ * This software is available to you under a choice of one of two
>>+ * licenses. You may choose to be licensed under the terms of the GNU
>>+ * General Public License (GPL) Version 2, available from the file
>>+ * COPYING in the main directory of this source tree, or the
>>+ * BSD license below:
>>+ *
>>+ * Redistribution and use in source and binary forms, with or without
>>+ * modification, are permitted provided that the following conditions
>>+ * are met:
> 
> 
> <snip>  You don't need the whole GPL 2 copy here, just put the first
> paragraph you have before this one in.
> 
The paragraph you quote is the BSD licence, and point 1 is :
Redistributions of source code must retain the above copyright
  *    notice unmodified, this list of conditions, and the following
  *    disclaimer

So could I remove it ?


>>+/*
>>+ * sometime hotplug don't have time to give the firmware the
>>+ * first time, retry it.
>>+ */
>>+static int sleepy_request_firmware(const struct firmware **fw, 
>>+		const char *name, struct device *dev)
>>+{
>>+	if (request_firmware(fw, name, dev) == 0)
>>+		return 0;
>>+	msleep(1000);
>>+	return request_firmware(fw, name, dev);
>>+}
> 
> 
> No, use the async firmware download mode instead of this.  That will
> solve all of your problems.
> 
> 
Thanks, but does userspace will retry if it fails the first time ?
The device needs the firmware quickly and after 3-5 seconds without it, 
it goes berserk.


>>+/* we need to use semaphore until sysfs and removable devices is fixed
>>+ * the problem is explained on http://marc.theaimsgroup.com/?t=112006484100003
>>+ */
> 
> 
> This is the proper fix, why do you think it should be fixed in the
> driver core?
> 
I don't remember, but aren't any possible race in sysfs code ?
In the read, after the up, the usb_disconnect is scheduled, and call 
device_remove_file. Is that ok for the sysfs code to be still in the 
read code ?
An other case : a process open a file, and start a read. Before the down 
, the usb_disconnect is scheduled and the module is removed.
What will do the read, run code from the removed code ?



> 
>>diff -rNu -x '*.ko*' -x '*.mod*' -x '*.o*' linux-2.6.14/drivers/usb/atm.old/ueagle-atm.h linux-2.6.14/drivers/usb/atm/ueagle-atm.h
>>--- linux-2.6.14/drivers/usb/atm.old/ueagle-atm.h	1970-01-01 01:00:00.000000000 +0100
>>+++ linux-2.6.14/drivers/usb/atm/ueagle-atm.h	2005-10-30 00:25:27.000000000 +0200
> 
> 
> Why do you need a header file for a single .c file?
> 
I think it makes things cleaner. I even like the bsd style where there 
is an header for reg (hardware values) and an other for val (driver 
structures).

 >>+#define PACKED __attribute__ ((packed))
 >
 >
 > No, spell it out please.
 >
 >
 >>+/* structure describing a block within a DSP page */
 >>+typedef struct {
 >>+	__le16 wHdr;
 >>+#define UEA_BIHDR 0xabcd
 >>+	__le16 wAddress;
 >>+	__le16 wSize;
 >>+	__le16 wOvlOffset;
 >>+	__le16 wOvl;		/* overlay */
 >>+	__le16 wLast;
 >>+} PACKED block_info_t;
 >
 >
 > Do not create new typedefs.  Please get rid of all of them.
It comes from bsd driver, it will be cleaned.

thanks,

Matthieu