On 06/05/2013 11:06 AM, Dan Carpenter wrote:
My main concern here is that in eprom_ck_cycle() the original code
read twice and in the new code it only reads one time.
What you have done is *probably* what we want to do and it's
*probably* an improvement over the original but we can't know
without testing it and I don't think you have the hardware to test
this. So let's put two reads back in and add a comment.
I did that unintentionally. I think its wrong. I will put back the second
read. It is possible for the register to change after setting sck=1
and that will bug the subsequent write to it when deassering sck.
I will look for a spec for this type of eeprom and if nothing changes
during toggling the sck i will mention it in the changelog.
void eprom_ck_cycle(struct net_device *dev)
{
u8 cmdreg;
int ret;
ret = read_nic_byte_E(dev, EPROM_CMD,&cmdreg);
if (ret)
return;
write_nic_byte_E(dev, EPROM_CMD, cmdreg | (1<<EPROM_CK_SHIFT));
force_pci_posting(dev);
udelay(EPROM_DELAY);
/* I'm not sure this second read is needed but I can't test it */
ret = read_nic_byte_E(dev, EPROM_CMD,&cmdreg);
if (ret)
return;
write_nic_byte_E(dev, EPROM_CMD, cmdreg& ~(1<<EPROM_CK_SHIFT));
force_pci_posting(dev);
udelay(EPROM_DELAY);
}
Also read_nic_byte_E() now returns an error code but we don't check
it so we've just moved the uninitialized code problem around a bit
but not fixed it. The error handling could be added in a follow on
patch because that doesn't introduce any bugs that weren't there in
the original code.
Yes, i will do it in a following patch but there are a lot of calls
to read_nic_* (maybe i will put if (unlikely(ret)))
The other minor style issue is that there should be a blank line
between declarations and code.
int read_nic_byte(struct net_device *dev, int x, u8 *data);
int read_nic_byte_E(struct net_device *dev, int x, u8 *data);
int read_nic_dword(struct net_device *dev, int x, u32 *data);
int read_nic_word(struct net_device *dev, int x, u16 *data) ;
I will fix this (and the space before ';' that i overlooked)
These should be renamed to read_nic_u8(), read_nic_u8_E(),
read_nic_u32() and read_nic_u16(). What does the "_E" mean? That's
actually a terrible name, obviously.
I suspect it refers to endpoint 0, not sure though. And i cannot
find a spec for this card.
Also btw, the (1<<EPROM_CK_SHIFT) could be replaced with:
#define EPROM_CK_BIT BIT(2)
write_nic_byte_E(dev, EPROM_CMD, cmdreg | EPROM_CK_BIT);
and
write_nic_byte_E(dev, EPROM_CMD, cmdreg& ~EPROM_CK_BIT);
Yes, it is better to use a bitmask than shifting all the time. I will
fix this.
In function eprom_r(), defined in r8180_93cx6.c,
a value with type 'u8' is assigned to a variable of
type 'short'. I cannot think of a reason to do that,
given the context, but I left it as it is.
There isn't any reason for it. Feel free to change it if you want.
-u8 read_nic_byte(struct net_device *dev, int indx)
+int read_nic_byte(struct net_device *dev, int indx, u8 *data)
{
- u8 data;
int status;
struct r8192_priv *priv = (struct r8192_priv *)ieee80211_priv(dev);
struct usb_device *udev = priv->udev;
status = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
RTL8187_REQ_GET_REGS, RTL8187_REQT_READ,
- (indx&0xff)|0xff00, (indx>>8)&0x0f,&data, 1, HZ / 2);
(indx& 0xff) | 0xff00, (indx>> 8)& 0x0f,
data, 1, HZ / 2);
Also "indx" is a weird way to abreviate. Normally it would be "idx"
or spelled out all the way "index".
indx maybe wants to show a relation with the wIndex field of the setup
packet (sent
when performing a control transfer).
Here, there is a bug also, introduced by me (&data should be data), i
overlooked it (sorry!!)
#ifdef DEBUG_RX
- DMESG("rxconf: %x %x", rxconf, read_nic_dword(dev, RCR));
+ {
+ u32 tmp;
+ read_nic_dword(dev, RCR,&tmp);
+ DMESG("rxconf: %x %x", rxconf, tmp);
+ }
#endif
Delete!
Ok. Calm down, i will delete everything that upsets you. Promise ;)
regards,
dan carpenter
Thanks for your time I appreciate it a lot.
Regards
Ksenia
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
