| Message ID | 1496134720-5363-5-git-send-email-brgl@bgdev.pl |
|---|---|
| State | New |
| Headers | show |
On Tue, May 30, 2017 at 11:58 AM, Bartosz Golaszewski <brgl@bgdev.pl> wrote: > We currently shift bits here and there without actually explaining > what we're doing. Add some helper variables with names indicating > their purpose to improve the code readability. > + /* Each chip is described by two values. */ > + num_chips = gpio_mockup_params_nr / 2; > + > + chips = devm_kzalloc(dev, sizeof(*chips) * num_chips, GFP_KERNEL); It's effectively devm_kcalloc() or devm_kmalloc_array() depending on the requirement of zeroing a memory chunks.
2017-05-30 20:55 GMT+02:00 Andy Shevchenko <andy.shevchenko@gmail.com>: > On Tue, May 30, 2017 at 11:58 AM, Bartosz Golaszewski <brgl@bgdev.pl> wrote: >> We currently shift bits here and there without actually explaining >> what we're doing. Add some helper variables with names indicating >> their purpose to improve the code readability. > >> + /* Each chip is described by two values. */ >> + num_chips = gpio_mockup_params_nr / 2; >> + >> + chips = devm_kzalloc(dev, sizeof(*chips) * num_chips, GFP_KERNEL); > > It's effectively > devm_kcalloc() > or > devm_kmalloc_array() > depending on the requirement of zeroing a memory chunks. > Is there any advantage to using one of these here? Thanks, Bartosz -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 31, 2017 at 1:53 PM, Bartosz Golaszewski <brgl@bgdev.pl> wrote: > 2017-05-30 20:55 GMT+02:00 Andy Shevchenko <andy.shevchenko@gmail.com>: >> On Tue, May 30, 2017 at 11:58 AM, Bartosz Golaszewski <brgl@bgdev.pl> wrote: >>> We currently shift bits here and there without actually explaining >>> what we're doing. Add some helper variables with names indicating >>> their purpose to improve the code readability. >> >>> + /* Each chip is described by two values. */ >>> + num_chips = gpio_mockup_params_nr / 2; >>> + >>> + chips = devm_kzalloc(dev, sizeof(*chips) * num_chips, GFP_KERNEL); >> >> It's effectively >> devm_kcalloc() >> or >> devm_kmalloc_array() >> depending on the requirement of zeroing a memory chunks. > Is there any advantage to using one of these here? Yes, though subtle one in this case. The caller will not care about (possible) overflow in multiplication. Other (micro)optimizations might be in place in the future as well, but I dunno about this. I would suggest to change. P.S. And of course if you don't need zeroed area it would be (slight?) performance impact I suppose.
2017-05-31 16:57 GMT+02:00 Andy Shevchenko <andy.shevchenko@gmail.com>: > On Wed, May 31, 2017 at 1:53 PM, Bartosz Golaszewski <brgl@bgdev.pl> wrote: >> 2017-05-30 20:55 GMT+02:00 Andy Shevchenko <andy.shevchenko@gmail.com>: >>> On Tue, May 30, 2017 at 11:58 AM, Bartosz Golaszewski <brgl@bgdev.pl> wrote: >>>> We currently shift bits here and there without actually explaining >>>> what we're doing. Add some helper variables with names indicating >>>> their purpose to improve the code readability. >>> >>>> + /* Each chip is described by two values. */ >>>> + num_chips = gpio_mockup_params_nr / 2; >>>> + >>>> + chips = devm_kzalloc(dev, sizeof(*chips) * num_chips, GFP_KERNEL); >>> >>> It's effectively >>> devm_kcalloc() >>> or >>> devm_kmalloc_array() >>> depending on the requirement of zeroing a memory chunks. > >> Is there any advantage to using one of these here? > > Yes, though subtle one in this case. The caller will not care about > (possible) overflow in multiplication. > Other (micro)optimizations might be in place in the future as well, > but I dunno about this. > > I would suggest to change. > Ok, I'll change it both for the lines and for the chips arrays and add a separate patch. Thanks, Bartosz -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c index d95d37a..0cb6cba 100644 --- a/drivers/gpio/gpio-mockup.c +++ b/drivers/gpio/gpio-mockup.c @@ -27,6 +27,11 @@ #define GPIO_MOCKUP_NAME "gpio-mockup" #define GPIO_MOCKUP_MAX_GC 10 +/* + * We're storing two values per chip: the GPIO base and the number + * of GPIO lines. + */ +#define GPIO_MOCKUP_MAX_RANGES (GPIO_MOCKUP_MAX_GC * 2) enum { GPIO_MOCKUP_DIR_OUT = 0, @@ -62,7 +67,7 @@ struct gpio_mockup_dbgfs_private { int offset; }; -static int gpio_mockup_ranges[GPIO_MOCKUP_MAX_GC << 1]; +static int gpio_mockup_ranges[GPIO_MOCKUP_MAX_RANGES]; static int gpio_mockup_params_nr; module_param_array(gpio_mockup_ranges, int, &gpio_mockup_params_nr, 0400); @@ -329,23 +334,24 @@ static int gpio_mockup_add(struct device *dev, static int gpio_mockup_probe(struct platform_device *pdev) { - struct gpio_mockup_chip *chips; + int ret, i, base, ngpio, num_chips; struct device *dev = &pdev->dev; - int ret, i, base, ngpio; + struct gpio_mockup_chip *chips; char *chip_name; if (gpio_mockup_params_nr < 2 || (gpio_mockup_params_nr % 2)) return -EINVAL; - chips = devm_kzalloc(dev, - sizeof(*chips) * (gpio_mockup_params_nr >> 1), - GFP_KERNEL); + /* Each chip is described by two values. */ + num_chips = gpio_mockup_params_nr / 2; + + chips = devm_kzalloc(dev, sizeof(*chips) * num_chips, GFP_KERNEL); if (!chips) return -ENOMEM; platform_set_drvdata(pdev, chips); - for (i = 0; i < gpio_mockup_params_nr >> 1; i++) { + for (i = 0; i < num_chips; i++) { base = gpio_mockup_ranges[i * 2]; if (base == -1)
We currently shift bits here and there without actually explaining what we're doing. Add some helper variables with names indicating their purpose to improve the code readability. Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl> --- drivers/gpio/gpio-mockup.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-)