[PATCH v1 1/3] string: Consolidate yesno() helpers under string.h hood

We have already few similar implementation and a lot of code that can benefit
of the yesno() helper.  Consolidate yesno() helpers under string.h hood.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 .../drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c    |  6 +-----
 drivers/gpu/drm/i915/i915_utils.h                    |  6 +-----
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c   | 12 +-----------
 include/linux/string.h                               |  5 +++++
 4 files changed, 8 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c
index 360952129b6d..7fde4f90e513 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c

@@ -23,6 +23,7 @@
  *
  */
 
+#include <linux/string.h>
 #include <linux/uaccess.h>
 
 #include <drm/drm_debugfs.h>

@@ -49,11 +50,6 @@ struct dmub_debugfs_trace_entry {
 	uint32_t param1;
 };
 
-static inline const char *yesno(bool v)
-{
-	return v ? "yes" : "no";
-}
-
 /* parse_write_buffer_into_params - Helper function to parse debugfs write buffer into an array
  *
  * Function takes in attributes passed to debugfs write entry

diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h
index abd4dcd9f79c..e6da5a951132 100644
--- a/drivers/gpu/drm/i915/i915_utils.h
+++ b/drivers/gpu/drm/i915/i915_utils.h

@@ -27,6 +27,7 @@
 
 #include <linux/list.h>
 #include <linux/overflow.h>
+#include <linux/string.h>
 #include <linux/sched.h>
 #include <linux/types.h>
 #include <linux/workqueue.h>

@@ -408,11 +409,6 @@ wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms)
 #define MBps(x) KBps(1000 * (x))
 #define GBps(x) ((u64)1000 * MBps((x)))
 
-static inline const char *yesno(bool v)
-{
-	return v ? "yes" : "no";
-}
-
 static inline const char *onoff(bool v)
 {
 	return v ? "on" : "off";

diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c
index 7d49fd4edc9e..c857d73abbd7 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c

@@ -34,6 +34,7 @@
 
 #include <linux/seq_file.h>
 #include <linux/debugfs.h>
+#include <linux/string.h>
 #include <linux/string_helpers.h>
 #include <linux/sort.h>
 #include <linux/ctype.h>

@@ -2015,17 +2016,6 @@ static const struct file_operations rss_debugfs_fops = {
 /* RSS Configuration.
  */
 
-/* Small utility function to return the strings "yes" or "no" if the supplied
- * argument is non-zero.
- */
-static const char *yesno(int x)
-{
-	static const char *yes = "yes";
-	static const char *no = "no";
-
-	return x ? yes : no;
-}
-
 static int rss_config_show(struct seq_file *seq, void *v)
 {
 	struct adapter *adapter = seq->private;

diff --git a/include/linux/string.h b/include/linux/string.h
index 9521d8cab18e..fd946a5e18c8 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h

@@ -308,4 +308,9 @@ static __always_inline size_t str_has_prefix(const char *str, const char *prefix
 	return strncmp(str, prefix, len) == 0 ? len : 0;
 }
 
+static inline const char *yesno(bool yes)
+{
+	return yes ? "yes" : "no";
+}
+
 #endif /* _LINUX_STRING_H_ */

-- 
2.30.0

We have already an implementation and a lot of code that can benefit
of the onoff() helper. Move it under string.h hood.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_utils.h | 5 -----
 include/linux/string.h            | 5 +++++
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h
index e6da5a951132..d2ac357896d4 100644
--- a/drivers/gpu/drm/i915/i915_utils.h
+++ b/drivers/gpu/drm/i915/i915_utils.h

@@ -409,11 +409,6 @@ wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms)
 #define MBps(x) KBps(1000 * (x))
 #define GBps(x) ((u64)1000 * MBps((x)))
 
-static inline const char *onoff(bool v)
-{
-	return v ? "on" : "off";
-}
-
 static inline const char *enableddisabled(bool v)
 {
 	return v ? "enabled" : "disabled";

diff --git a/include/linux/string.h b/include/linux/string.h
index fd946a5e18c8..2a0589e945d9 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h

@@ -313,4 +313,9 @@ static inline const char *yesno(bool yes)
 	return yes ? "yes" : "no";
 }
 
+static inline const char *onoff(bool on)
+{
+	return on ? "on" : "off";
+}
+
 #endif /* _LINUX_STRING_H_ */

-- 
2.30.0

We have already an implementation and a lot of code that can benefit
of the enableddisabled() helper. Move it under string.h hood.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_utils.h | 5 -----
 include/linux/string.h            | 5 +++++
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h
index d2ac357896d4..b05d72b4dd93 100644
--- a/drivers/gpu/drm/i915/i915_utils.h
+++ b/drivers/gpu/drm/i915/i915_utils.h

@@ -409,11 +409,6 @@ wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms)
 #define MBps(x) KBps(1000 * (x))
 #define GBps(x) ((u64)1000 * MBps((x)))
 
-static inline const char *enableddisabled(bool v)
-{
-	return v ? "enabled" : "disabled";
-}
-
 void add_taint_for_CI(struct drm_i915_private *i915, unsigned int taint);
 static inline void __add_taint_for_CI(unsigned int taint)
 {

diff --git a/include/linux/string.h b/include/linux/string.h
index 2a0589e945d9..25f055aa4c31 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h

@@ -318,4 +318,9 @@ static inline const char *onoff(bool on)
 	return on ? "on" : "off";
 }
 
+static inline const char *enableddisabled(bool enabled)
+{
+	return enabled ? "enabled" : "disabled";
+}
+
 #endif /* _LINUX_STRING_H_ */

-- 
2.30.0

Am 15.02.21 um 15:21 schrieb Andy Shevchenko:

We have already few similar implementation and a lot of code that can benefit
of the yesno() helper.  Consolidate yesno() helpers under string.h hood.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Looks like a good idea to me, feel free to add an Acked-by: Christian 
König [off-list ref] to the series.

But looking at the use cases for this, wouldn't it make more sense to 
teach kprintf some new format modifier for this?

Christian.

On Mon, 15 Feb 2021, Andy Shevchenko [off-list ref] wrote:

We have already few similar implementation and a lot of code that can benefit
of the yesno() helper.  Consolidate yesno() helpers under string.h hood.

Good luck. I gave up after just four versions. [1]

Acked-by: Jani Nikula <redacted>


BR,
Jani.


[1] http://lore.kernel.org/r/20191023131308.9420-1-jani.nikula@intel.com

-- 
Jani Nikula, Intel Open Source Graphics Center

+Cc: Sakari and printk people

On Mon, Feb 15, 2021 at 4:28 PM Christian König
[off-list ref] wrote:

Am 15.02.21 um 15:21 schrieb Andy Shevchenko:

quoted

We have already few similar implementation and a lot of code that can benefit
of the yesno() helper.  Consolidate yesno() helpers under string.h hood.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Looks like a good idea to me, feel free to add an Acked-by: Christian
König [off-list ref] to the series.

Thanks.

But looking at the use cases for this, wouldn't it make more sense to
teach kprintf some new format modifier for this?

As a next step? IIRC Sakari has at some point the series converted
yesno and Co. to something which I don't remember the details of.

Guys, what do you think?

-- 
With Best Regards,
Andy Shevchenko

On Mon, Feb 15, 2021 at 04:37:50PM +0200, Jani Nikula wrote:

On Mon, 15 Feb 2021, Andy Shevchenko [off-list ref] wrote:

quoted

We have already few similar implementation and a lot of code that can benefit
of the yesno() helper.  Consolidate yesno() helpers under string.h hood.

Good luck. I gave up after just four versions. [1]

Thanks for a pointer! I like your version, but here we also discussing a
possibility to do something like %py[DOY]. It will consolidate all those RO or
whatever sections inside one data structure.

Acked-by: Jani Nikula <redacted>

[1] http://lore.kernel.org/r/20191023131308.9420-1-jani.nikula@intel.com

-- 
With Best Regards,
Andy Shevchenko

On Mon 2021-02-15 16:39:26, Andy Shevchenko wrote:

+Cc: Sakari and printk people

On Mon, Feb 15, 2021 at 4:28 PM Christian König
[off-list ref] wrote:

quoted

Am 15.02.21 um 15:21 schrieb Andy Shevchenko:

quoted

We have already few similar implementation and a lot of code that can benefit
of the yesno() helper.  Consolidate yesno() helpers under string.h hood.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Looks like a good idea to me, feel free to add an Acked-by: Christian
König [off-list ref] to the series.

Thanks.

quoted

But looking at the use cases for this, wouldn't it make more sense to
teach kprintf some new format modifier for this?

As a next step? IIRC Sakari has at some point the series converted
yesno and Co. to something which I don't remember the details of.

Guys, what do you think?

Honestly, I think that yesno() is much easier to understand than %py.
And %py[DOY] looks really scary. It has been suggested at
https://lore.kernel.org/lkml/YCqaNnr7ynRydczE@smile.fi.intel.com/#t

Yes, enabledisable() is hard to parse but it is still self-explaining
and can be found easily by cscope. On the contrary, %pyD will likely
print some python code and it is not clear if it would be compatible
with v3. I am just kidding but you get the picture.

Best Regards,
Petr

On Wed, 17 Feb 2021, Petr Mladek [off-list ref] wrote:

On Mon 2021-02-15 16:39:26, Andy Shevchenko wrote:

quoted

+Cc: Sakari and printk people

On Mon, Feb 15, 2021 at 4:28 PM Christian König
[off-list ref] wrote:

quoted

Am 15.02.21 um 15:21 schrieb Andy Shevchenko:

quoted

We have already few similar implementation and a lot of code that can benefit
of the yesno() helper.  Consolidate yesno() helpers under string.h hood.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Looks like a good idea to me, feel free to add an Acked-by: Christian
König [off-list ref] to the series.

Thanks.

quoted

But looking at the use cases for this, wouldn't it make more sense to
teach kprintf some new format modifier for this?

As a next step? IIRC Sakari has at some point the series converted
yesno and Co. to something which I don't remember the details of.

Guys, what do you think?

Honestly, I think that yesno() is much easier to understand than %py.
And %py[DOY] looks really scary. It has been suggested at
https://lore.kernel.org/lkml/YCqaNnr7ynRydczE@smile.fi.intel.com/#t

Yes, enabledisable() is hard to parse but it is still self-explaining
and can be found easily by cscope. On the contrary, %pyD will likely
print some python code and it is not clear if it would be compatible
with v3. I am just kidding but you get the picture.

Personally I prefer %s and the functions.

I think the format specifiers have become unwieldy. I don't remember any
of the kernel specific ones by heart, I always look them up or just
cargo-cult. I think the fourcc format specifiers are a nice cleanup, but
I don't remember them either. I'd like something like %foo{yesno} where,
if you remember the %foo part, you could actually also remember the
rest.

But really if you get *any* version accepted, I'm not going to argue
against it, and you can disregard this as meaningless bikeshedding.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center

On Mon, Feb 15, 2021 at 04:21:35PM +0200, Andy Shevchenko wrote:

We have already few similar implementation and a lot of code that can benefit
of the yesno() helper.  Consolidate yesno() helpers under string.h hood.

I was taking a look on i915_utils.h to reduce it and move some of it
elsewhere to be shared with others.  I was starting with these helpers
and had [1] done, then Jani pointed me to this thread and also his
previous tentative. I thought the natural place for this would be
include/linux/string_helpers.h, but I will leave it up to you.

After reading the threads, I don't see real opposition to it.
Is there a tree you plan to take this through?

thanks
Lucas De Marchi

[1] https://lore.kernel.org/lkml/20211005212634.3223113-1-lucas.demarchi@intel.com/T/#u

On Tue, Oct 05, 2021 at 02:34:23PM -0700, Lucas De Marchi wrote:

On Mon, Feb 15, 2021 at 04:21:35PM +0200, Andy Shevchenko wrote:

quoted

We have already few similar implementation and a lot of code that can benefit
of the yesno() helper.  Consolidate yesno() helpers under string.h hood.

I was taking a look on i915_utils.h to reduce it and move some of it
elsewhere to be shared with others.  I was starting with these helpers
and had [1] done, then Jani pointed me to this thread and also his
previous tentative. I thought the natural place for this would be
include/linux/string_helpers.h, but I will leave it up to you.

Seems reasonable to use string_helpers (headers and/or C-file).

After reading the threads, I don't see real opposition to it.
Is there a tree you plan to take this through?

I rest my series in favour of Jani's approach, so I suppose there is no go
for _this_ series.

[1] https://lore.kernel.org/lkml/20211005212634.3223113-1-lucas.demarchi@intel.com/T/#u

-- 
With Best Regards,
Andy Shevchenko

On Mon, 15 Nov 2021, Andy Shevchenko [off-list ref] wrote:

On Tue, Oct 05, 2021 at 02:34:23PM -0700, Lucas De Marchi wrote:

quoted

On Mon, Feb 15, 2021 at 04:21:35PM +0200, Andy Shevchenko wrote:

quoted

We have already few similar implementation and a lot of code that can benefit
of the yesno() helper.  Consolidate yesno() helpers under string.h hood.

I was taking a look on i915_utils.h to reduce it and move some of it
elsewhere to be shared with others.  I was starting with these helpers
and had [1] done, then Jani pointed me to this thread and also his
previous tentative. I thought the natural place for this would be
include/linux/string_helpers.h, but I will leave it up to you.

Seems reasonable to use string_helpers (headers and/or C-file).

quoted

After reading the threads, I don't see real opposition to it.
Is there a tree you plan to take this through?

I rest my series in favour of Jani's approach, so I suppose there is no go
for _this_ series.

If you want to make it happen, please pick it up and drive it. I'm
thoroughly had enough of this.

BR,
Jani.

quoted

[1] https://lore.kernel.org/lkml/20211005212634.3223113-1-lucas.demarchi@intel.com/T/#u

-- 
Jani Nikula, Intel Open Source Graphics Center

On Mon, Nov 15, 2021 at 01:43:02PM +0200, Jani Nikula wrote:

On Mon, 15 Nov 2021, Andy Shevchenko [off-list ref] wrote:

quoted

On Tue, Oct 05, 2021 at 02:34:23PM -0700, Lucas De Marchi wrote:

quoted

On Mon, Feb 15, 2021 at 04:21:35PM +0200, Andy Shevchenko wrote:

quoted

We have already few similar implementation and a lot of code that can benefit
of the yesno() helper.  Consolidate yesno() helpers under string.h hood.

I was taking a look on i915_utils.h to reduce it and move some of it
elsewhere to be shared with others.  I was starting with these helpers
and had [1] done, then Jani pointed me to this thread and also his
previous tentative. I thought the natural place for this would be
include/linux/string_helpers.h, but I will leave it up to you.

Seems reasonable to use string_helpers (headers and/or C-file).

quoted

After reading the threads, I don't see real opposition to it.
Is there a tree you plan to take this through?

I rest my series in favour of Jani's approach, so I suppose there is no go
for _this_ series.

If you want to make it happen, please pick it up and drive it. I'm
thoroughly had enough of this.

My point is still the same, so it's more to Lucas.

I'm not going to drive this activity due to lack of time.

quoted

quoted

[1] https://lore.kernel.org/lkml/20211005212634.3223113-1-lucas.demarchi@intel.com/T/#u

-- 
With Best Regards,
Andy Shevchenko