| Message ID | 20230306195556.55475-1-andriy.shevchenko@linux.intel.com |
|---|---|
| Headers | show |
| Series | gpio: Use string_choices.h | expand |
On Mon, Mar 6, 2023 at 8:55 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > Use string_choices.h in the GPIO drivers and library. > It has been tested on x86_64 and (semi-)compile tested > over all. > > Andy Shevchenko (16): > lib/string_helpers: Add missing header files to MAINTAINERS database > lib/string_helpers: Split out string_choices.h > lib/string_choices: Add str_high_low() helper > lib/string_choices: Add str_input_output() helper > gpiolib: Utilize helpers from string_choices.h > gpio: adnp: Utilize helpers from string_choices.h > gpio: brcmstb: Utilize helpers from string_choices.h > gpio: crystalcove: Utilize helpers from string_choices.h > gpio: grgpio: Utilize helpers from string_choices.h > gpio: mvebu: Utilize helpers from string_choices.h > gpio: pl061: Utilize helpers from string_choices.h > gpio: stmpe: Utilize helpers from string_choices.h > gpio: wcove: Utilize helpers from string_choices.h > gpio: wm831x: Utilize helpers from string_choices.h > gpio: wm8994: Utilize helpers from string_choices.h > gpio: xra1403: Utilize helpers from string_choices.h > > MAINTAINERS | 3 ++ > drivers/gpio/gpio-adnp.c | 24 ++++---------- > drivers/gpio/gpio-brcmstb.c | 3 +- > drivers/gpio/gpio-crystalcove.c | 17 +++++----- > drivers/gpio/gpio-grgpio.c | 3 +- > drivers/gpio/gpio-mvebu.c | 27 +++++++--------- > drivers/gpio/gpio-pl061.c | 4 +-- > drivers/gpio/gpio-stmpe.c | 19 +++++------ > drivers/gpio/gpio-wcove.c | 15 ++++----- > drivers/gpio/gpio-wm831x.c | 5 +-- > drivers/gpio/gpio-wm8994.c | 6 ++-- > drivers/gpio/gpio-xra1403.c | 5 +-- > drivers/gpio/gpiolib-sysfs.c | 3 +- > drivers/gpio/gpiolib.c | 13 ++++---- > include/linux/string_choices.h | 56 +++++++++++++++++++++++++++++++++ > include/linux/string_helpers.h | 26 +-------------- > 16 files changed, 125 insertions(+), 104 deletions(-) > create mode 100644 include/linux/string_choices.h > > -- > 2.39.1 > I've been thinking about this and I must say it doesn't make much sense to me. Not only does it NOT reduce the code size (even if we assume the unlikely case where we'd build all those modules that use the helpers) but also decreases the readability for anyone not familiar with the new interfaces (meaning time spent looking up the new function). The "%s", x ? "if" : "else" statement is concise and clear already, I don't see much improvement with this series. And I'm saying it from the position of someone who loves factoring out common code. :) I'll wait to hear what others have to say but if it were up to me, I'd politely say no. (I mean: I guess, in the end it is up to me, but I'm open to arguments.) :) Bart
Hi Bart, hi Andy, On Thu, Mar 09, 2023 at 04:22:19PM +0100, Bartosz Golaszewski wrote: > I've been thinking about this and I must say it doesn't make much > sense to me. Not only does it NOT reduce the code size (even if we > assume the unlikely case where we'd build all those modules that use > the helpers) but also decreases the readability for anyone not > familiar with the new interfaces (meaning time spent looking up the > new function). The "%s", x ? "if" : "else" statement is concise and > clear already, I don't see much improvement with this series. And I'm > saying it from the position of someone who loves factoring out common > code. :) > > I'll wait to hear what others have to say but if it were up to me, I'd > politely say no. Interpreting this as request to share my view: I'm having the same doubts. While I'm not a big fan of the ?: operator, it's semantic is more obvious here. What I find most difficult about str_high_low(plr & BIT(j)) (from patch #6) is: Does this give me "high" or "low" if the argument is zero? You could tell me, and judging from the patch I'd hope that it would give me "low". But if I stumble over this code in two weeks I have probably forgotten and have to look it up again. Best regards Uwe