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.
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.
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) ;
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.
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);
> 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".
> #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!
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
