| Message ID | 20231130134630.18198-10-brgl@bgdev.pl |
|---|---|
| State | New |
| Headers | show |
| Series | gpio/pinctrl: replace gpiochip_is_requested() with a safer interface | expand |
On Thu, Nov 30, 2023 at 02:46:29PM +0100, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > Rework for_each_requested_gpio_in_range() to use the new helper to > retrieve a dynamically allocated copy of the descriptor label and free > it at the end of each iteration. We need to leverage the CLASS()' > destructor to make sure that the label is freed even when breaking out > of the loop. ... > const char *gpiochip_is_requested(struct gpio_chip *gc, unsigned int offset); > char *gpiochip_dup_line_label(struct gpio_chip *gc, unsigned int offset); > > + One blank line is enough. > +struct _gpiochip_for_each_data { > + const char **label; > + int *i; Why is this a signed? > +}; ... > +DEFINE_CLASS(_gpiochip_for_each_data, > + struct _gpiochip_for_each_data, > + if (*_T.label) kfree(*_T.label), > + ({ struct _gpiochip_for_each_data _data = { label, i }; > + *_data.i = 0; > + _data; }), To me indentation of ({}) is quite weird. Where is this style being used instead of more readable ({ ... }) ?
On Thu, Nov 30, 2023 at 5:40 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Nov 30, 2023 at 02:46:29PM +0100, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > Rework for_each_requested_gpio_in_range() to use the new helper to > > retrieve a dynamically allocated copy of the descriptor label and free > > it at the end of each iteration. We need to leverage the CLASS()' > > destructor to make sure that the label is freed even when breaking out > > of the loop. > > ... > > > const char *gpiochip_is_requested(struct gpio_chip *gc, unsigned int offset); > > char *gpiochip_dup_line_label(struct gpio_chip *gc, unsigned int offset); > > > > + > > One blank line is enough. > > > +struct _gpiochip_for_each_data { > > + const char **label; > > + int *i; > > Why is this a signed? > Some users use signed, others use unsigned. It doesn't matter as we can't overflow it with the limit on the lines we have. Bart > > +}; > > ... > > > +DEFINE_CLASS(_gpiochip_for_each_data, > > + struct _gpiochip_for_each_data, > > + if (*_T.label) kfree(*_T.label), > > + ({ struct _gpiochip_for_each_data _data = { label, i }; > > + *_data.i = 0; > > + _data; }), > > To me indentation of ({}) is quite weird. Where is this style being used > instead of more readable > There are no guidelines for this type of C abuse AFAIK. The macro may be ugly but at least it hides the details from users which look nice instead. Bart > ({ > ... > }) > > ? > > -- > With Best Regards, > Andy Shevchenko > >
On Thu, Nov 30, 2023 at 06:42:37PM +0100, Bartosz Golaszewski wrote: > On Thu, Nov 30, 2023 at 5:40 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Nov 30, 2023 at 02:46:29PM +0100, Bartosz Golaszewski wrote: ... > > > const char *gpiochip_is_requested(struct gpio_chip *gc, unsigned int offset); > > > char *gpiochip_dup_line_label(struct gpio_chip *gc, unsigned int offset); > > > > > > + > > > > One blank line is enough. > > > > > +struct _gpiochip_for_each_data { > > > + const char **label; > > > + int *i; > > > > Why is this a signed? > > Some users use signed, others use unsigned. It doesn't matter as we > can't overflow it with the limit on the lines we have. What's the problem to make it unsigned and be done with that for good? > > > +}; ... > > > +DEFINE_CLASS(_gpiochip_for_each_data, > > > + struct _gpiochip_for_each_data, > > > + if (*_T.label) kfree(*_T.label), > > > + ({ struct _gpiochip_for_each_data _data = { label, i }; > > > + *_data.i = 0; > > > + _data; }), > > > > To me indentation of ({}) is quite weird. Where is this style being used > > instead of more readable > > There are no guidelines for this type of C abuse AFAIK. The macro may > be ugly but at least it hides the details from users which look nice > instead. If we can make it more readable for free, why not doing that way? > > ({ > > ... > > }) > > > > ?
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h index 9796a34e2fee..b1ed501e9ee0 100644 --- a/include/linux/gpio/driver.h +++ b/include/linux/gpio/driver.h @@ -534,17 +534,36 @@ struct gpio_chip { const char *gpiochip_is_requested(struct gpio_chip *gc, unsigned int offset); char *gpiochip_dup_line_label(struct gpio_chip *gc, unsigned int offset); + +struct _gpiochip_for_each_data { + const char **label; + int *i; +}; + +DEFINE_CLASS(_gpiochip_for_each_data, + struct _gpiochip_for_each_data, + if (*_T.label) kfree(*_T.label), + ({ struct _gpiochip_for_each_data _data = { label, i }; + *_data.i = 0; + _data; }), + const char **label, int *i) + /** * for_each_requested_gpio_in_range - iterates over requested GPIOs in a given range - * @chip: the chip to query - * @i: loop variable - * @base: first GPIO in the range - * @size: amount of GPIOs to check starting from @base - * @label: label of current GPIO + * @_chip: the chip to query + * @_i: loop variable + * @_base: first GPIO in the range + * @_size: amount of GPIOs to check starting from @base + * @_label: label of current GPIO */ -#define for_each_requested_gpio_in_range(chip, i, base, size, label) \ - for (i = 0; i < size; i++) \ - if ((label = gpiochip_is_requested(chip, base + i)) == NULL) {} else +#define for_each_requested_gpio_in_range(_chip, _i, _base, _size, _label) \ + for (CLASS(_gpiochip_for_each_data, _data)(&_label, &_i); \ + *_data.i < _size; \ + (*_data.i)++, kfree(*(_data.label)), *_data.label = NULL) \ + if ((*_data.label = \ + gpiochip_dup_line_label(_chip, _base + *_data.i)) == NULL) {} \ + else if (IS_ERR(*_data.label)) {} \ + else /* Iterates over all requested GPIO of the given @chip */ #define for_each_requested_gpio(chip, i, label) \