| Message ID | 20170424020211.20690-4-sjg@chromium.org |
|---|---|
| State | Accepted |
| Delegated to: | Simon Glass |
| Headers | show |
On Mon, Apr 24, 2017 at 5:02 AM, Simon Glass <sjg@chromium.org> wrote: > At present this code is inline. Move it into a function to allow it to > be used elsewhere. > > Signed-off-by: Simon Glass <sjg@chromium.org> > +static int blk_next_free_devnum(enum if_type if_type) > +{ > + int ret; > + > + ret = blk_find_max_devnum(if_type); > + if (ret == -ENODEV) > + return 0; > + else if (ret < 0) Useless 'else'. > + return ret; > + else Ditto. > + return ret + 1; > +}
Hi Andy, On 24 April 2017 at 02:04, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, Apr 24, 2017 at 5:02 AM, Simon Glass <sjg@chromium.org> wrote: >> At present this code is inline. Move it into a function to allow it to >> be used elsewhere. >> >> Signed-off-by: Simon Glass <sjg@chromium.org> > >> +static int blk_next_free_devnum(enum if_type if_type) >> +{ >> + int ret; >> + >> + ret = blk_find_max_devnum(if_type); >> + if (ret == -ENODEV) >> + return 0; > >> + else if (ret < 0) > > Useless 'else'. > >> + return ret; >> + else > > Ditto. > >> + return ret + 1; >> +} I think it is clearer with these. At least, I would ask for them to be added if they were missing. What do you think of this series as a whole? Regards, Simon
On Wed, May 3, 2017 at 5:36 AM, Simon Glass <sjg@chromium.org> wrote: > Hi Andy, > > On 24 April 2017 at 02:04, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: >> On Mon, Apr 24, 2017 at 5:02 AM, Simon Glass <sjg@chromium.org> wrote: >>> At present this code is inline. Move it into a function to allow it to >>> be used elsewhere. >>> >>> Signed-off-by: Simon Glass <sjg@chromium.org> >> >>> +static int blk_next_free_devnum(enum if_type if_type) >>> +{ >>> + int ret; >>> + >>> + ret = blk_find_max_devnum(if_type); >>> + if (ret == -ENODEV) >>> + return 0; >> >>> + else if (ret < 0) >> >> Useless 'else'. >> >>> + return ret; >>> + else >> >> Ditto. >> >>> + return ret + 1; >>> +} > > I think it is clearer with these. At least, I would ask for them to be > added if they were missing. Really? It's just noise in the code which makes it harder to read. In Linux kernel we remove that. If U-Boot has the above style kinda mandatory I would be really surprised. > What do you think of this series as a whole? It was a while ago. So, if there were no comments from me, that means I have nothing to argue of.
On Wed, May 3, 2017 at 5:36 AM, Simon Glass <sjg@chromium.org> wrote: > Hi Andy, > > On 24 April 2017 at 02:04, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: >> On Mon, Apr 24, 2017 at 5:02 AM, Simon Glass <sjg@chromium.org> wrote: >>> At present this code is inline. Move it into a function to allow it to >>> be used elsewhere. >>> >>> Signed-off-by: Simon Glass <sjg@chromium.org> >> >>> +static int blk_next_free_devnum(enum if_type if_type) >>> +{ >>> + int ret; >>> + >>> + ret = blk_find_max_devnum(if_type); >>> + if (ret == -ENODEV) >>> + return 0; >> >>> + else if (ret < 0) >> >> Useless 'else'. >> >>> + return ret; >>> + else >> >> Ditto. >> >>> + return ret + 1; >>> +} > > I think it is clearer with these. At least, I would ask for them to be > added if they were missing. Really? It's just noise in the code which makes it harder to read. In Linux kernel we remove that. If U-Boot has the above style kinda mandatory I would be really surprised. > What do you think of this series as a whole? It was a while ago. So, if there were no comments from me, that means I have nothing to argue of.
Hi Andy, On 18 May 2017 at 10:01, <sjg@google.com> wrote: > On Wed, May 3, 2017 at 5:36 AM, Simon Glass <sjg@chromium.org> wrote: >> Hi Andy, >> >> On 24 April 2017 at 02:04, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: >>> On Mon, Apr 24, 2017 at 5:02 AM, Simon Glass <sjg@chromium.org> wrote: >>>> At present this code is inline. Move it into a function to allow it to >>>> be used elsewhere. >>>> >>>> Signed-off-by: Simon Glass <sjg@chromium.org> >>> >>>> +static int blk_next_free_devnum(enum if_type if_type) >>>> +{ >>>> + int ret; >>>> + >>>> + ret = blk_find_max_devnum(if_type); >>>> + if (ret == -ENODEV) >>>> + return 0; >>> >>>> + else if (ret < 0) >>> >>> Useless 'else'. >>> >>>> + return ret; >>>> + else >>> >>> Ditto. I updated these when applying. - Simon
diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c index 8b6b28d890..7947ca6f22 100644 --- a/drivers/block/blk-uclass.c +++ b/drivers/block/blk-uclass.c @@ -473,6 +473,19 @@ int blk_find_max_devnum(enum if_type if_type) return max_devnum; } +static int blk_next_free_devnum(enum if_type if_type) +{ + int ret; + + ret = blk_find_max_devnum(if_type); + if (ret == -ENODEV) + return 0; + else if (ret < 0) + return ret; + else + return ret + 1; +} + int blk_create_device(struct udevice *parent, const char *drv_name, const char *name, int if_type, int devnum, int blksz, lbaint_t size, struct udevice **devp) @@ -482,13 +495,10 @@ int blk_create_device(struct udevice *parent, const char *drv_name, int ret; if (devnum == -1) { - ret = blk_find_max_devnum(if_type); - if (ret == -ENODEV) - devnum = 0; - else if (ret < 0) + ret = blk_next_free_devnum(if_type); + if (ret < 0) return ret; - else - devnum = ret + 1; + devnum = ret; } ret = device_bind_driver(parent, drv_name, name, &dev); if (ret)
At present this code is inline. Move it into a function to allow it to be used elsewhere. Signed-off-by: Simon Glass <sjg@chromium.org> --- drivers/block/blk-uclass.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-)