| Message ID | 1418735363-5851-1-git-send-email-marex@denx.de |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Stefano Babic |
| Headers | show |
2014-12-16 14:09 GMT+01:00 Marek Vasut <marex@denx.de>: > The malloc() calls are unnecessary, just allocate the stuff on stack. > While at it, reorder the code a little, so that only one variable is > used for the text, use snprintf() instead of sprintf() and use %01d > as a formatting string to avoid any possible overflows. > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Igor Grinberg <grinberg@compulab.co.il> > Cc: Nikita Kiryanov <nikita@compulab.co.il> > Cc: Sean Cross <xobs@kosagi.com> > Cc: Simon Glass <sjg@chromium.org> > Cc: Stefano Babic <sbabic@denx.de> > Cc: Tim Harvey <tharvey@gateworks.com> > --- > arch/arm/imx-common/i2c-mxv7.c | 24 ++++++++---------------- > 1 file changed, 8 insertions(+), 16 deletions(-) > > diff --git a/arch/arm/imx-common/i2c-mxv7.c b/arch/arm/imx-common/i2c-mxv7.c > index 34f5387..1a632e7 100644 > --- a/arch/arm/imx-common/i2c-mxv7.c > +++ b/arch/arm/imx-common/i2c-mxv7.c > @@ -73,26 +73,21 @@ static void * const i2c_bases[] = { > int setup_i2c(unsigned i2c_index, int speed, int slave_addr, > struct i2c_pads_info *p) > { > - char *name1, *name2; > + char name[9]; > int ret; > > if (i2c_index >= ARRAY_SIZE(i2c_bases)) > return -EINVAL; > > - name1 = malloc(9); > - name2 = malloc(9); > - if (!name1 || !name2) > - return -ENOMEM; > - > - sprintf(name1, "i2c_sda%d", i2c_index); > - sprintf(name2, "i2c_scl%d", i2c_index); > - ret = gpio_request(p->sda.gp, name1); > + snprintf(name, sizeof(name), "i2c_sda%01d", i2c_index); > + ret = gpio_request(p->sda.gp, name); > if (ret) > - goto err_req1; > + return ret; > > - ret = gpio_request(p->scl.gp, name2); > + snprintf(name, sizeof(name), "i2c_scl%01d", i2c_index); > + ret = gpio_request(p->scl.gp, name); > if (ret) > - goto err_req2; > + goto err_req; > > /* Enable i2c clock */ > ret = enable_i2c_clk(1, i2c_index); > @@ -112,11 +107,8 @@ int setup_i2c(unsigned i2c_index, int speed, int slave_addr, > err_idle: > err_clk: > gpio_free(p->scl.gp); > -err_req2: > +err_req: > gpio_free(p->sda.gp); > -err_req1: > - free(name1); > - free(name2); > > return ret; > } > -- > 2.1.3 > Reviewed-by: Christian Gmeiner <christian.gmeiner@gmail.com>
Hi Marek, On 16 December 2014 at 06:09, Marek Vasut <marex@denx.de> wrote: > The malloc() calls are unnecessary, just allocate the stuff on stack. > While at it, reorder the code a little, so that only one variable is > used for the text, use snprintf() instead of sprintf() and use %01d > as a formatting string to avoid any possible overflows. > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Igor Grinberg <grinberg@compulab.co.il> > Cc: Nikita Kiryanov <nikita@compulab.co.il> > Cc: Sean Cross <xobs@kosagi.com> > Cc: Simon Glass <sjg@chromium.org> > Cc: Stefano Babic <sbabic@denx.de> > Cc: Tim Harvey <tharvey@gateworks.com> > --- > arch/arm/imx-common/i2c-mxv7.c | 24 ++++++++---------------- > 1 file changed, 8 insertions(+), 16 deletions(-) > > diff --git a/arch/arm/imx-common/i2c-mxv7.c b/arch/arm/imx-common/i2c-mxv7.c > index 34f5387..1a632e7 100644 > --- a/arch/arm/imx-common/i2c-mxv7.c > +++ b/arch/arm/imx-common/i2c-mxv7.c > @@ -73,26 +73,21 @@ static void * const i2c_bases[] = { > int setup_i2c(unsigned i2c_index, int speed, int slave_addr, > struct i2c_pads_info *p) > { > - char *name1, *name2; > + char name[9]; > int ret; > > if (i2c_index >= ARRAY_SIZE(i2c_bases)) > return -EINVAL; > > - name1 = malloc(9); > - name2 = malloc(9); > - if (!name1 || !name2) > - return -ENOMEM; > - > - sprintf(name1, "i2c_sda%d", i2c_index); > - sprintf(name2, "i2c_scl%d", i2c_index); > - ret = gpio_request(p->sda.gp, name1); > + snprintf(name, sizeof(name), "i2c_sda%01d", i2c_index); > + ret = gpio_request(p->sda.gp, name); Does this board use driver model? If not it should be easy to convert since one MX6 board supports it. With driver model there is gpio_requestf("i2c_sda%01d", i2c_index); > if (ret) > - goto err_req1; > + return ret; > > - ret = gpio_request(p->scl.gp, name2); > + snprintf(name, sizeof(name), "i2c_scl%01d", i2c_index); > + ret = gpio_request(p->scl.gp, name); > if (ret) > - goto err_req2; > + goto err_req; > > /* Enable i2c clock */ > ret = enable_i2c_clk(1, i2c_index); > @@ -112,11 +107,8 @@ int setup_i2c(unsigned i2c_index, int speed, int slave_addr, > err_idle: > err_clk: > gpio_free(p->scl.gp); > -err_req2: > +err_req: > gpio_free(p->sda.gp); > -err_req1: > - free(name1); > - free(name2); > > return ret; > } > -- > 2.1.3 > Regards, Simon
On Tuesday, December 16, 2014 at 05:27:53 PM, Simon Glass wrote: > Hi Marek, > > On 16 December 2014 at 06:09, Marek Vasut <marex@denx.de> wrote: > > The malloc() calls are unnecessary, just allocate the stuff on stack. > > While at it, reorder the code a little, so that only one variable is > > used for the text, use snprintf() instead of sprintf() and use %01d > > as a formatting string to avoid any possible overflows. > > > > Signed-off-by: Marek Vasut <marex@denx.de> > > Cc: Igor Grinberg <grinberg@compulab.co.il> > > Cc: Nikita Kiryanov <nikita@compulab.co.il> > > Cc: Sean Cross <xobs@kosagi.com> > > Cc: Simon Glass <sjg@chromium.org> > > Cc: Stefano Babic <sbabic@denx.de> > > Cc: Tim Harvey <tharvey@gateworks.com> > > --- > > > > arch/arm/imx-common/i2c-mxv7.c | 24 ++++++++---------------- > > 1 file changed, 8 insertions(+), 16 deletions(-) > > > > diff --git a/arch/arm/imx-common/i2c-mxv7.c > > b/arch/arm/imx-common/i2c-mxv7.c index 34f5387..1a632e7 100644 > > --- a/arch/arm/imx-common/i2c-mxv7.c > > +++ b/arch/arm/imx-common/i2c-mxv7.c > > @@ -73,26 +73,21 @@ static void * const i2c_bases[] = { > > > > int setup_i2c(unsigned i2c_index, int speed, int slave_addr, > > > > struct i2c_pads_info *p) > > > > { > > > > - char *name1, *name2; > > + char name[9]; > > > > int ret; > > > > if (i2c_index >= ARRAY_SIZE(i2c_bases)) > > > > return -EINVAL; > > > > - name1 = malloc(9); > > - name2 = malloc(9); > > - if (!name1 || !name2) > > - return -ENOMEM; > > - > > - sprintf(name1, "i2c_sda%d", i2c_index); > > - sprintf(name2, "i2c_scl%d", i2c_index); > > - ret = gpio_request(p->sda.gp, name1); > > + snprintf(name, sizeof(name), "i2c_sda%01d", i2c_index); > > + ret = gpio_request(p->sda.gp, name); > > Does this board use driver model? If not it should be easy to convert > since one MX6 board supports it. With driver model there is > gpio_requestf("i2c_sda%01d", i2c_index); No, not yet, but it's in the pipeline. I am already keeping an eye on a few conversion patches to get this done.
On 16/12/2014 14:09, Marek Vasut wrote: > The malloc() calls are unnecessary, just allocate the stuff on stack. > While at it, reorder the code a little, so that only one variable is > used for the text, use snprintf() instead of sprintf() and use %01d > as a formatting string to avoid any possible overflows. > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Igor Grinberg <grinberg@compulab.co.il> > Cc: Nikita Kiryanov <nikita@compulab.co.il> > Cc: Sean Cross <xobs@kosagi.com> > Cc: Simon Glass <sjg@chromium.org> > Cc: Stefano Babic <sbabic@denx.de> > Cc: Tim Harvey <tharvey@gateworks.com> > --- Applied to u-boot-imx, thanks ! Best regards, Stefano Babic
On Tuesday, December 30, 2014 at 02:34:47 PM, Stefano Babic wrote: > On 16/12/2014 14:09, Marek Vasut wrote: > > The malloc() calls are unnecessary, just allocate the stuff on stack. > > While at it, reorder the code a little, so that only one variable is > > used for the text, use snprintf() instead of sprintf() and use %01d > > as a formatting string to avoid any possible overflows. > > > > Signed-off-by: Marek Vasut <marex@denx.de> > > Cc: Igor Grinberg <grinberg@compulab.co.il> > > Cc: Nikita Kiryanov <nikita@compulab.co.il> > > Cc: Sean Cross <xobs@kosagi.com> > > Cc: Simon Glass <sjg@chromium.org> > > Cc: Stefano Babic <sbabic@denx.de> > > Cc: Tim Harvey <tharvey@gateworks.com> > > --- > > Applied to u-boot-imx, thanks ! Hey! hope you had a nice holiday :) You might want to apply 2/8 and 3/8 to current codebase and send it to Tom, since they fix real problem and the board doesn't boot without this. I should have separated them out, sorry. Best regards, Marek Vasut
diff --git a/arch/arm/imx-common/i2c-mxv7.c b/arch/arm/imx-common/i2c-mxv7.c index 34f5387..1a632e7 100644 --- a/arch/arm/imx-common/i2c-mxv7.c +++ b/arch/arm/imx-common/i2c-mxv7.c @@ -73,26 +73,21 @@ static void * const i2c_bases[] = { int setup_i2c(unsigned i2c_index, int speed, int slave_addr, struct i2c_pads_info *p) { - char *name1, *name2; + char name[9]; int ret; if (i2c_index >= ARRAY_SIZE(i2c_bases)) return -EINVAL; - name1 = malloc(9); - name2 = malloc(9); - if (!name1 || !name2) - return -ENOMEM; - - sprintf(name1, "i2c_sda%d", i2c_index); - sprintf(name2, "i2c_scl%d", i2c_index); - ret = gpio_request(p->sda.gp, name1); + snprintf(name, sizeof(name), "i2c_sda%01d", i2c_index); + ret = gpio_request(p->sda.gp, name); if (ret) - goto err_req1; + return ret; - ret = gpio_request(p->scl.gp, name2); + snprintf(name, sizeof(name), "i2c_scl%01d", i2c_index); + ret = gpio_request(p->scl.gp, name); if (ret) - goto err_req2; + goto err_req; /* Enable i2c clock */ ret = enable_i2c_clk(1, i2c_index); @@ -112,11 +107,8 @@ int setup_i2c(unsigned i2c_index, int speed, int slave_addr, err_idle: err_clk: gpio_free(p->scl.gp); -err_req2: +err_req: gpio_free(p->sda.gp); -err_req1: - free(name1); - free(name2); return ret; }
The malloc() calls are unnecessary, just allocate the stuff on stack. While at it, reorder the code a little, so that only one variable is used for the text, use snprintf() instead of sprintf() and use %01d as a formatting string to avoid any possible overflows. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Igor Grinberg <grinberg@compulab.co.il> Cc: Nikita Kiryanov <nikita@compulab.co.il> Cc: Sean Cross <xobs@kosagi.com> Cc: Simon Glass <sjg@chromium.org> Cc: Stefano Babic <sbabic@denx.de> Cc: Tim Harvey <tharvey@gateworks.com> --- arch/arm/imx-common/i2c-mxv7.c | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-)