On Wed, Jul 29, 2015 at 02:04:42AM +0300, Dan Carpenter wrote:
> On Tue, Jul 28, 2015 at 11:52:42PM +0200, Mateusz Kulikowski wrote:
>
> > diff --git a/drivers/staging/rtl8192e/rtl8192e/rtl_eeprom.c b/drivers/staging/rtl8192e/rtl8192e/rtl_eeprom.c
> > index ed54193..fe4e282 100644
> > --- a/drivers/staging/rtl8192e/rtl8192e/rtl_eeprom.c
> > +++ b/drivers/staging/rtl8192e/rtl8192e/rtl_eeprom.c
> > @@ -25,115 +25,72 @@
> > #include "rtl_core.h"
> > #include "rtl_eeprom.h"
> >
> > -static void eprom_cs(struct net_device *dev, short bit)
> > +static void _rtl92e_gpio_set(struct net_device *dev, int no, int val)
>
> I don't like the new name very much. I don't like the underscore. Also
> set kind of implies set vs clear. Maybe:
>
> static void rtl92e_gpio_write_bit(struct net_device *dev, int bit, bool val)
The underscore is notation used in rtlwifi - they use _rtl92<something> for
static functions and rtl92<something> for the rest.
We may like it or not, but IMHO it's better to have unification if this driver
is ever going to be unstaged.
As for your proposed name - except for prefix part it's fine.
> > -static void eprom_ck_cycle(struct net_device *dev)
> > +static int _rtl92e_gpio_get(struct net_device *dev, int no)
>
> static bool rtl92e_gpio_get_bit(struct net_device *dev, int bit)
As above
>
> > {
> > - rtl92e_writeb(dev, EPROM_CMD,
> > - (1<<EPROM_CK_SHIFT) | rtl92e_readb(dev, EPROM_CMD));
> > - udelay(EPROM_DELAY);
> > - rtl92e_writeb(dev, EPROM_CMD,
> > - rtl92e_readb(dev, EPROM_CMD) & ~(1<<EPROM_CK_SHIFT));
> > - udelay(EPROM_DELAY);
> > -}
> > + u8 reg = rtl92e_readb(dev, EPROM_CMD);
> >
> > + return (reg >> no) & 0x1;
> > +}
> >
> > -static void eprom_w(struct net_device *dev, short bit)
> > +static void _rtl92e_eeprom_ck_cycle(struct net_device *dev)
> > {
> > - if (bit)
> > - rtl92e_writeb(dev, EPROM_CMD, (1<<EPROM_W_SHIFT) |
> > - rtl92e_readb(dev, EPROM_CMD));
> > - else
> > - rtl92e_writeb(dev, EPROM_CMD,
> > - rtl92e_readb(dev, EPROM_CMD) &
> > - ~(1<<EPROM_W_SHIFT));
> > -
> > - udelay(EPROM_DELAY);
> > + _rtl92e_gpio_set(dev, EPROM_CK_BIT, 1);
> > + _rtl92e_gpio_set(dev, EPROM_CK_BIT, 0);
>
> The old cycle function had some delays built in. You're probably right
> that they aren't needed, but why do you think so?
Delays are now built-in to gpio_set function to give device time to
respond.
I have removed delays during read operation as we wait after write.
Reading is just reading gpio register - I see reason to wait.
Of course I've tested that - with no delays at all or other bugs
driver just fails to probe device.
As for other comments - could you take look at "Notes" from patch?
This was one of main concerns for me - Wouldn't it be better
if I rewrite that code to use GPIO/SPI/EEPROM subsystems?
Although code sie will probably not decrease and we will
be dependent on other modules, but it seems more 'proper'
solution.
Regards,
Mateusz
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
