On Sat, Jun 01, 2013 at 02:07:18AM +0300, Xenia Ragiadakou wrote:
> On 06/01/2013 01:47 AM, Dan Carpenter wrote:
> >On Fri, May 31, 2013 at 08:10:52PM +0300, Xenia Ragiadakou wrote:
> >>This patch fixes identation and alignment in r8192U_core.c.
> >>Also, removes spaces from idents when applicable.
> >>
> >>Signed-off-by: Xenia Ragiadakou<burzalodowa@xxxxxxxxx>
> >>---
> >>
> >>Please take a look at changes in stage:
> >>@@-2686,35 +2688,35 @@ static void rtl8192_read_eeprom_info(struct net_device *dev)
> >>There are some changes that i don't know where they
> >>came from. They do not alter the code though.
> >>I am referring to the following sub-add pairs:
> >>patch lines 922 and 923
> >>patch lines 925 and 926
> >>patch lines 928 and 945
> >>
> >I am worried about this but I don't understand it. What do those
> >line numbers mean? Please explain again.
>
> They are the lines in the patch file where I saw the changes when
> I was reviewing the patch.
>
> >Don't put RFC. It's sort of cowardly. Be fearless!
>
> I do not revert never ever again!
>
No no. Reverting a patch is like graduating. You're not a real
kernel hacker until you fix one of your own bugs. :)
> >
> >patch 2/2 will have to be redone. so my guess is that 3-5 won't
> >apply after 2/2 is redone.
> >
> >Patches 1, 3 and 4 are great. Patch 5 is huge but I don't see a
> >clear way to break it into smaller patches, so I guess it's fine.
> >I have scripts to review that kind of patch so it's not a problem.
> >
> >>@@ -236,12 +236,12 @@ static void rtl819x_set_channel_map(u8 channel_plan, struct r8192_priv *priv)
> >> }
> >>
> >>
> >>-#define rx_hal_is_cck_rate(_pdrvinfo)\
> >>- (_pdrvinfo->RxRate == DESC90_RATE1M ||\
> >>- _pdrvinfo->RxRate == DESC90_RATE2M ||\
> >>- _pdrvinfo->RxRate == DESC90_RATE5_5M ||\
> >>- _pdrvinfo->RxRate == DESC90_RATE11M)&&\
> >>- !_pdrvinfo->RxHT\
> >>+#define rx_hal_is_cck_rate(_pdrvinfo) \
> >>+ (_pdrvinfo->RxRate == DESC90_RATE1M || \
> >>+ _pdrvinfo->RxRate == DESC90_RATE2M || \
> >>+ _pdrvinfo->RxRate == DESC90_RATE5_5M || \
> >>+ _pdrvinfo->RxRate == DESC90_RATE11M)&& \
> >>+ !_pdrvinfo->RxHT \
> >>
> >This macro is disgusting. It should be a function.
> >
> >bool rx_hal_is_cck_rate(struct rx_drvinfo_819x_usb *pdrvinfo)
> >{
> > if (pdrvinfo->RxHT)
> > return false;
> >
> > switch (pdrvinfo->RxRate) {
> > case DESC90_RATE1M:
> > case DESC90_RATE2M:
> > case DESC90_RATE5_5M:
> > case DESC90_RATE11M:
> > return true;
> > default:
> > return false;
> > }
> >}
> >
> >regards,
> >dan carpenter
>
> To be honest i don't know the comparative advantages
> of the two styles (not the visual ones).
In general, you should use functions anywhere it is possible.
>
> (irrelevant)
> I have a problem with ifdefed code. I am not sure as far
> as which ifdefs should be removed completely.
> Even the TODO and debug ifdefs appear useful to me
> if I was a developer that works on this driver.
> Is this the right thing to delete them?
The dead code is still there in the git history if we need it.
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
