On Fri, Apr 1, 2022 at 6:06 AM unSimple <unsimple1993@xxxxxxx> wrote: > > The main consideration of the 'continue' in the patch is that those statements are inner a loop. > > The next allocation may be successful so I think it is better to use 'continue' here. Please, do not top-post! What you explained is logical from APIs point of view, what I was asking is about functional point of view. Why do you think the skipping iteration is fine? You need to explain this in the code/commit message. > At 2022-03-29 19:45:50, "Andy Shevchenko" <andy.shevchenko@xxxxxxxxx> wrote: > >On Tue, Mar 29, 2022 at 11:36 AM QintaoShen <unSimple1993@xxxxxxx> wrote: > >> > >> The memory allocation function devm_kcalloc() may return NULL pointer, > > > >may --> might > > > >> so it is better to add a check for 'p->func[i]->pins' to avoid possible > >> NULL pointer dereference. ... > >> @@ -266,6 +266,10 @@ static int rt2880_pinmux_pins(struct rt2880_priv *p) > >> p->func[i]->pin_count, > >> sizeof(int), > >> GFP_KERNEL); > > > >> + > > > >Stray change. Also it seems it has trailing space character(s). > > > >> + if (!p->func[i]->pins) > > > >> + continue; > > > >Why is 'continue' the proper choice here? No clarification is given in > >the commit message. > > > >> for (j = 0; j < p->func[i]->pin_count; j++) > >> p->func[i]->pins[j] = p->func[i]->pin_first + j; -- With Best Regards, Andy Shevchenko
