Add some helpers under lib/string_helpers.h so they can be used
throughout the kernel. When I started doing this there were 2 other
previous attempts I know of, not counting the iterations each of them
had:
1) https://lore.kernel.org/all/[email protected]/
2) https://lore.kernel.org/all/[email protected]/#t
Going through the comments I tried to find some common ground and
justification for what is in here, addressing some of the concerns
raised.
a. This version should be a drop-in replacement for what is currently in
the tree, with no change in behavior or binary size. For binary
size what I checked wat that the linked objects in the end have the
same size (gcc 11). From comments in the previous attempts this seems
also the case for earlier compiler versions
b. I didn't change the function name to choice_* as suggested by Andrew
Morton in [email protected]
because other people argumented in favor of shorter names for these
simple helpers - if they are long and people simply not use due to
that, we failed
c. Use string_helper.h for these helpers - pulling string.h in the
compilations units was one of the concerns and I think re-using this
already existing header is better than creating a new string-choice.h
d. This doesn't bring onoff() helper as there are some places in the
kernel with onoff as variable - another name is probably needed for
this function in order not to shadow the variable, or those variables
could be renamed. Or if people wanting <someprefix>
try to find a short one
e. One alternative to all of this suggested by Christian König
([email protected]) would be to add a
printk format. But besides the comment, he also seemed to like
the common function. This brought the argument from others that the
simple yesno()/enabledisable() already used in the code is easier to
remember and use than e.g. %py[DOY]
Last patch also has some additional conversion of open coded cases. I
preferred starting with drm/ since this is "closer to home".
I hope this is a good summary of the previous attempts and a way we can
move forward.
Andrew Morton, Petr Mladek, Andy Shevchenko: if this is accepted, my
proposal is to take first 2 patches either through mm tree or maybe
vsprintf. Last patch can be taken later through drm.
thanks
Lucas De Marchi
Cc: Alex Deucher <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Cc: Ben Skeggs <[email protected]>
Cc: Christian König <[email protected]>
Cc: Chris Wilson <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: David Airlie <[email protected]>
Cc: David S. Miller <[email protected]>
Cc: Emma Anholt <[email protected]>
Cc: Eryk Brol <[email protected]>
Cc: Francis Laniel <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Harry Wentland <[email protected]>
Cc: Jakub Kicinski <[email protected]>
Cc: Jani Nikula <[email protected]>
Cc: Joonas Lahtinen <[email protected]>
Cc: Julia Lawall <[email protected]>
Cc: Kentaro Takeda <[email protected]>
Cc: Leo Li <[email protected]>
Cc: Mikita Lipski <[email protected]>
Cc: Petr Mladek <[email protected]>
Cc: Rahul Lakkireddy <[email protected]>
Cc: Raju Rangoju <[email protected]>
Cc: Rasmus Villemoes <[email protected]>
Cc: Rodrigo Vivi <[email protected]>
Cc: Sakari Ailus <[email protected]>
Cc: Sergey Senozhatsky <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Vishal Kulkarni <[email protected]>
Lucas De Marchi (3):
lib/string_helpers: Consolidate yesno() implementation
lib/string_helpers: Add helpers for enable[d]/disable[d]
drm: Convert open yes/no strings to yesno()
drivers/gpu/drm/amd/amdgpu/atom.c | 3 ++-
.../amd/display/amdgpu_dm/amdgpu_dm_debugfs.c | 6 +-----
drivers/gpu/drm/drm_client_modeset.c | 3 ++-
drivers/gpu/drm/drm_dp_helper.c | 3 ++-
drivers/gpu/drm/drm_gem.c | 3 ++-
drivers/gpu/drm/i915/i915_utils.h | 15 ---------------
drivers/gpu/drm/nouveau/nvkm/subdev/i2c/aux.c | 4 +++-
drivers/gpu/drm/radeon/atom.c | 3 ++-
drivers/gpu/drm/v3d/v3d_debugfs.c | 11 ++++++-----
drivers/gpu/drm/virtio/virtgpu_debugfs.c | 3 ++-
.../net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c | 11 -----------
include/linux/string_helpers.h | 4 ++++
security/tomoyo/audit.c | 2 +-
security/tomoyo/common.c | 18 ++++--------------
security/tomoyo/common.h | 1 -
15 files changed, 31 insertions(+), 59 deletions(-)
--
2.34.1
Follow the yes/no logic and add helpers for enabled/disabled and
enable/disable - those are not so common throughout the kernel,
but they give a nice way to reuse the strings to log things as
enabled/disabled or enable/disable.
Signed-off-by: Lucas De Marchi <[email protected]>
---
drivers/gpu/drm/i915/i915_utils.h | 10 ----------
include/linux/string_helpers.h | 2 ++
2 files changed, 2 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h
index 2a8781cc648b..cbec79bae0d2 100644
--- a/drivers/gpu/drm/i915/i915_utils.h
+++ b/drivers/gpu/drm/i915/i915_utils.h
@@ -419,16 +419,6 @@ static inline const char *onoff(bool v)
return v ? "on" : "off";
}
-static inline const char *enabledisable(bool v)
-{
- return v ? "enable" : "disable";
-}
-
-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_helpers.h b/include/linux/string_helpers.h
index e980dec05d31..e4b82f364ee1 100644
--- a/include/linux/string_helpers.h
+++ b/include/linux/string_helpers.h
@@ -103,5 +103,7 @@ char *kstrdup_quotable_file(struct file *file, gfp_t gfp);
void kfree_strarray(char **array, size_t n);
static inline const char *yesno(bool v) { return v ? "yes" : "no"; }
+static inline const char *enabledisable(bool v) { return v ? "enable" : "disable"; }
+static inline const char *enableddisabled(bool v) { return v ? "enabled" : "disabled"; }
#endif
--
2.34.1
linux/string_helpers.h provides a helper to return "yes"/"no"
strings. Replace the open coded versions with yesno(). The places were
identified with the following semantic patch:
@@
expression b;
@@
- b ? "yes" : "no"
+ yesno(b)
Then the includes were added, so we include-what-we-use, and parenthesis
adjusted in drivers/gpu/drm/v3d/v3d_debugfs.c. After the conversion we
still see the same binary sizes:
text data bss dec hex filename
1442171 60344 800 1503315 16f053 ./drivers/gpu/drm/radeon/radeon.ko
1442171 60344 800 1503315 16f053 ./drivers/gpu/drm/radeon/radeon.ko.old
5985991 324439 33808 6344238 60ce2e ./drivers/gpu/drm/amd/amdgpu/amdgpu.ko
5985991 324439 33808 6344238 60ce2e ./drivers/gpu/drm/amd/amdgpu/amdgpu.ko.old
411986 10490 6176 428652 68a6c ./drivers/gpu/drm/drm.ko
411986 10490 6176 428652 68a6c ./drivers/gpu/drm/drm.ko.old
1970292 109515 2352 2082159 1fc56f ./drivers/gpu/drm/nouveau/nouveau.ko
1970292 109515 2352 2082159 1fc56f ./drivers/gpu/drm/nouveau/nouveau.ko.old
Signed-off-by: Lucas De Marchi <[email protected]>
---
drivers/gpu/drm/amd/amdgpu/atom.c | 3 ++-
drivers/gpu/drm/drm_client_modeset.c | 3 ++-
drivers/gpu/drm/drm_dp_helper.c | 3 ++-
drivers/gpu/drm/drm_gem.c | 3 ++-
drivers/gpu/drm/nouveau/nvkm/subdev/i2c/aux.c | 4 +++-
drivers/gpu/drm/radeon/atom.c | 3 ++-
drivers/gpu/drm/v3d/v3d_debugfs.c | 11 ++++++-----
drivers/gpu/drm/virtio/virtgpu_debugfs.c | 3 ++-
8 files changed, 21 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/atom.c b/drivers/gpu/drm/amd/amdgpu/atom.c
index 6fa2229b7229..3d7d0f4cfc05 100644
--- a/drivers/gpu/drm/amd/amdgpu/atom.c
+++ b/drivers/gpu/drm/amd/amdgpu/atom.c
@@ -25,6 +25,7 @@
#include <linux/module.h>
#include <linux/sched.h>
#include <linux/slab.h>
+#include <linux/string_helpers.h>
#include <asm/unaligned.h>
#include <drm/drm_util.h>
@@ -740,7 +741,7 @@ static void atom_op_jump(atom_exec_context *ctx, int *ptr, int arg)
break;
}
if (arg != ATOM_COND_ALWAYS)
- SDEBUG(" taken: %s\n", execute ? "yes" : "no");
+ SDEBUG(" taken: %s\n", yesno(execute));
SDEBUG(" target: 0x%04X\n", target);
if (execute) {
if (ctx->last_jump == (ctx->start + target)) {
diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
index ced09c7c06f9..3c55156a75fa 100644
--- a/drivers/gpu/drm/drm_client_modeset.c
+++ b/drivers/gpu/drm/drm_client_modeset.c
@@ -11,6 +11,7 @@
#include <linux/module.h>
#include <linux/mutex.h>
#include <linux/slab.h>
+#include <linux/string_helpers.h>
#include <drm/drm_atomic.h>
#include <drm/drm_client.h>
@@ -241,7 +242,7 @@ static void drm_client_connectors_enabled(struct drm_connector **connectors,
connector = connectors[i];
enabled[i] = drm_connector_enabled(connector, true);
DRM_DEBUG_KMS("connector %d enabled? %s\n", connector->base.id,
- connector->display_info.non_desktop ? "non desktop" : enabled[i] ? "yes" : "no");
+ connector->display_info.non_desktop ? "non desktop" : yesno(enabled[i]));
any_enabled |= enabled[i];
}
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 4d0d1e8e51fa..f600616839f3 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -28,6 +28,7 @@
#include <linux/module.h>
#include <linux/sched.h>
#include <linux/seq_file.h>
+#include <linux/string_helpers.h>
#include <drm/drm_dp_helper.h>
#include <drm/drm_print.h>
@@ -1122,7 +1123,7 @@ void drm_dp_downstream_debug(struct seq_file *m,
bool branch_device = drm_dp_is_branch(dpcd);
seq_printf(m, "\tDP branch device present: %s\n",
- branch_device ? "yes" : "no");
+ yesno(branch_device));
if (!branch_device)
return;
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 4dcdec6487bb..6436876341bb 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -39,6 +39,7 @@
#include <linux/dma-buf-map.h>
#include <linux/mem_encrypt.h>
#include <linux/pagevec.h>
+#include <linux/string_helpers.h>
#include <drm/drm.h>
#include <drm/drm_device.h>
@@ -1145,7 +1146,7 @@ void drm_gem_print_info(struct drm_printer *p, unsigned int indent,
drm_vma_node_start(&obj->vma_node));
drm_printf_indent(p, indent, "size=%zu\n", obj->size);
drm_printf_indent(p, indent, "imported=%s\n",
- obj->import_attach ? "yes" : "no");
+ yesno(obj->import_attach));
if (obj->funcs->print_info)
obj->funcs->print_info(p, indent, obj);
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/i2c/aux.c b/drivers/gpu/drm/nouveau/nvkm/subdev/i2c/aux.c
index a11637b0f6cc..d39a9c1a2a6e 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/i2c/aux.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/i2c/aux.c
@@ -21,6 +21,8 @@
*
* Authors: Ben Skeggs
*/
+#include <linux/string_helpers.h>
+
#include "aux.h"
#include "pad.h"
@@ -94,7 +96,7 @@ void
nvkm_i2c_aux_monitor(struct nvkm_i2c_aux *aux, bool monitor)
{
struct nvkm_i2c_pad *pad = aux->pad;
- AUX_TRACE(aux, "monitor: %s", monitor ? "yes" : "no");
+ AUX_TRACE(aux, "monitor: %s", yesno(monitor));
if (monitor)
nvkm_i2c_pad_mode(pad, NVKM_I2C_PAD_AUX);
else
diff --git a/drivers/gpu/drm/radeon/atom.c b/drivers/gpu/drm/radeon/atom.c
index f15b20da5315..77ef7d136530 100644
--- a/drivers/gpu/drm/radeon/atom.c
+++ b/drivers/gpu/drm/radeon/atom.c
@@ -25,6 +25,7 @@
#include <linux/module.h>
#include <linux/sched.h>
#include <linux/slab.h>
+#include <linux/string_helpers.h>
#include <asm/unaligned.h>
@@ -722,7 +723,7 @@ static void atom_op_jump(atom_exec_context *ctx, int *ptr, int arg)
break;
}
if (arg != ATOM_COND_ALWAYS)
- SDEBUG(" taken: %s\n", execute ? "yes" : "no");
+ SDEBUG(" taken: %s\n", yesno(execute));
SDEBUG(" target: 0x%04X\n", target);
if (execute) {
if (ctx->last_jump == (ctx->start + target)) {
diff --git a/drivers/gpu/drm/v3d/v3d_debugfs.c b/drivers/gpu/drm/v3d/v3d_debugfs.c
index e76b24bb8828..22c23f3a691e 100644
--- a/drivers/gpu/drm/v3d/v3d_debugfs.c
+++ b/drivers/gpu/drm/v3d/v3d_debugfs.c
@@ -6,6 +6,7 @@
#include <linux/debugfs.h>
#include <linux/pm_runtime.h>
#include <linux/seq_file.h>
+#include <linux/string_helpers.h>
#include <drm/drm_debugfs.h>
@@ -148,15 +149,15 @@ static int v3d_v3d_debugfs_ident(struct seq_file *m, void *unused)
V3D_GET_FIELD(ident3, V3D_HUB_IDENT3_IPREV),
V3D_GET_FIELD(ident3, V3D_HUB_IDENT3_IPIDX));
seq_printf(m, "MMU: %s\n",
- (ident2 & V3D_HUB_IDENT2_WITH_MMU) ? "yes" : "no");
+ yesno(ident2 & V3D_HUB_IDENT2_WITH_MMU));
seq_printf(m, "TFU: %s\n",
- (ident1 & V3D_HUB_IDENT1_WITH_TFU) ? "yes" : "no");
+ yesno(ident1 & V3D_HUB_IDENT1_WITH_TFU));
seq_printf(m, "TSY: %s\n",
- (ident1 & V3D_HUB_IDENT1_WITH_TSY) ? "yes" : "no");
+ yesno(ident1 & V3D_HUB_IDENT1_WITH_TSY));
seq_printf(m, "MSO: %s\n",
- (ident1 & V3D_HUB_IDENT1_WITH_MSO) ? "yes" : "no");
+ yesno(ident1 & V3D_HUB_IDENT1_WITH_MSO));
seq_printf(m, "L3C: %s (%dkb)\n",
- (ident1 & V3D_HUB_IDENT1_WITH_L3C) ? "yes" : "no",
+ yesno(ident1 & V3D_HUB_IDENT1_WITH_L3C),
V3D_GET_FIELD(ident2, V3D_HUB_IDENT2_L3C_NKB));
for (core = 0; core < cores; core++) {
diff --git a/drivers/gpu/drm/virtio/virtgpu_debugfs.c b/drivers/gpu/drm/virtio/virtgpu_debugfs.c
index b6954e2f75e6..c7f675721840 100644
--- a/drivers/gpu/drm/virtio/virtgpu_debugfs.c
+++ b/drivers/gpu/drm/virtio/virtgpu_debugfs.c
@@ -22,6 +22,7 @@
* OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
* WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
*/
+#include <linux/string_helpers.h>
#include <drm/drm_debugfs.h>
#include <drm/drm_file.h>
@@ -31,7 +32,7 @@
static void virtio_gpu_add_bool(struct seq_file *m, const char *name,
bool value)
{
- seq_printf(m, "%-16s : %s\n", name, value ? "yes" : "no");
+ seq_printf(m, "%-16s : %s\n", name, yesno(value));
}
static void virtio_gpu_add_int(struct seq_file *m, const char *name, int value)
--
2.34.1
On Tue, 18 Jan 2022, Lucas De Marchi <[email protected]> wrote:
> Add some helpers under lib/string_helpers.h so they can be used
> throughout the kernel. When I started doing this there were 2 other
> previous attempts I know of, not counting the iterations each of them
> had:
>
> 1) https://lore.kernel.org/all/[email protected]/
> 2) https://lore.kernel.org/all/[email protected]/#t
>
> Going through the comments I tried to find some common ground and
> justification for what is in here, addressing some of the concerns
> raised.
>
> a. This version should be a drop-in replacement for what is currently in
> the tree, with no change in behavior or binary size. For binary
> size what I checked wat that the linked objects in the end have the
> same size (gcc 11). From comments in the previous attempts this seems
> also the case for earlier compiler versions
>
> b. I didn't change the function name to choice_* as suggested by Andrew
> Morton in [email protected]
> because other people argumented in favor of shorter names for these
> simple helpers - if they are long and people simply not use due to
> that, we failed
>
> c. Use string_helper.h for these helpers - pulling string.h in the
> compilations units was one of the concerns and I think re-using this
> already existing header is better than creating a new string-choice.h
>
> d. This doesn't bring onoff() helper as there are some places in the
> kernel with onoff as variable - another name is probably needed for
> this function in order not to shadow the variable, or those variables
> could be renamed. Or if people wanting <someprefix>
> try to find a short one
>
> e. One alternative to all of this suggested by Christian König
> ([email protected]) would be to add a
> printk format. But besides the comment, he also seemed to like
> the common function. This brought the argument from others that the
> simple yesno()/enabledisable() already used in the code is easier to
> remember and use than e.g. %py[DOY]
>
> Last patch also has some additional conversion of open coded cases. I
> preferred starting with drm/ since this is "closer to home".
>
> I hope this is a good summary of the previous attempts and a way we can
> move forward.
Thanks for picking this up again. I agree with the approach here.
Acked-by: Jani Nikula <[email protected]>
>
> Andrew Morton, Petr Mladek, Andy Shevchenko: if this is accepted, my
> proposal is to take first 2 patches either through mm tree or maybe
> vsprintf. Last patch can be taken later through drm.
>
> thanks
> Lucas De Marchi
>
> Cc: Alex Deucher <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Andy Shevchenko <[email protected]>
> Cc: Andy Shevchenko <[email protected]>
> Cc: Ben Skeggs <[email protected]>
> Cc: Christian König <[email protected]>
> Cc: Chris Wilson <[email protected]>
> Cc: Daniel Vetter <[email protected]>
> Cc: David Airlie <[email protected]>
> Cc: David S. Miller <[email protected]>
> Cc: Emma Anholt <[email protected]>
> Cc: Eryk Brol <[email protected]>
> Cc: Francis Laniel <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Harry Wentland <[email protected]>
> Cc: Jakub Kicinski <[email protected]>
> Cc: Jani Nikula <[email protected]>
> Cc: Joonas Lahtinen <[email protected]>
> Cc: Julia Lawall <[email protected]>
> Cc: Kentaro Takeda <[email protected]>
> Cc: Leo Li <[email protected]>
> Cc: Mikita Lipski <[email protected]>
> Cc: Petr Mladek <[email protected]>
> Cc: Rahul Lakkireddy <[email protected]>
> Cc: Raju Rangoju <[email protected]>
> Cc: Rasmus Villemoes <[email protected]>
> Cc: Rodrigo Vivi <[email protected]>
> Cc: Sakari Ailus <[email protected]>
> Cc: Sergey Senozhatsky <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: Vishal Kulkarni <[email protected]>
>
> Lucas De Marchi (3):
> lib/string_helpers: Consolidate yesno() implementation
> lib/string_helpers: Add helpers for enable[d]/disable[d]
> drm: Convert open yes/no strings to yesno()
>
> drivers/gpu/drm/amd/amdgpu/atom.c | 3 ++-
> .../amd/display/amdgpu_dm/amdgpu_dm_debugfs.c | 6 +-----
> drivers/gpu/drm/drm_client_modeset.c | 3 ++-
> drivers/gpu/drm/drm_dp_helper.c | 3 ++-
> drivers/gpu/drm/drm_gem.c | 3 ++-
> drivers/gpu/drm/i915/i915_utils.h | 15 ---------------
> drivers/gpu/drm/nouveau/nvkm/subdev/i2c/aux.c | 4 +++-
> drivers/gpu/drm/radeon/atom.c | 3 ++-
> drivers/gpu/drm/v3d/v3d_debugfs.c | 11 ++++++-----
> drivers/gpu/drm/virtio/virtgpu_debugfs.c | 3 ++-
> .../net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c | 11 -----------
> include/linux/string_helpers.h | 4 ++++
> security/tomoyo/audit.c | 2 +-
> security/tomoyo/common.c | 18 ++++--------------
> security/tomoyo/common.h | 1 -
> 15 files changed, 31 insertions(+), 59 deletions(-)
--
Jani Nikula, Intel Open Source Graphics Center
On Wed, Jan 19, 2022 at 11:20:38AM +0200, Andy Shevchenko wrote:
>On Wednesday, January 19, 2022, Lucas De Marchi <[email protected]>
>wrote:
>
>> Follow the yes/no logic and add helpers for enabled/disabled and
>> enable/disable - those are not so common throughout the kernel,
>> but they give a nice way to reuse the strings to log things as
>> enabled/disabled or enable/disable.
>>
>> Signed-off-by: Lucas De Marchi <[email protected]>
>> ---
>> drivers/gpu/drm/i915/i915_utils.h | 10 ----------
>> include/linux/string_helpers.h | 2 ++
>> 2 files changed, 2 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_utils.h
>> b/drivers/gpu/drm/i915/i915_utils.h
>> index 2a8781cc648b..cbec79bae0d2 100644
>> --- a/drivers/gpu/drm/i915/i915_utils.h
>> +++ b/drivers/gpu/drm/i915/i915_utils.h
>> @@ -419,16 +419,6 @@ static inline const char *onoff(bool v)
>> return v ? "on" : "off";
>> }
>>
>> -static inline const char *enabledisable(bool v)
>> -{
>> - return v ? "enable" : "disable";
>> -}
>> -
>> -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_helpers.h b/include/linux/string_
>> helpers.h
>> index e980dec05d31..e4b82f364ee1 100644
>> --- a/include/linux/string_helpers.h
>> +++ b/include/linux/string_helpers.h
>> @@ -103,5 +103,7 @@ char *kstrdup_quotable_file(struct file *file, gfp_t
>> gfp);
>> void kfree_strarray(char **array, size_t n);
>>
>> static inline const char *yesno(bool v) { return v ? "yes" : "no"; }
>> +static inline const char *enabledisable(bool v) { return v ? "enable" :
>> "disable"; }
>> +static inline const char *enableddisabled(bool v) { return v ? "enabled"
>> : "disabled"; }
>
>
>Looks not readable even if takes 80 characters. Please, keep original style.
>
>
>I believe you wanted to have nice negative statistics from day 1, then you
>may add more patches in the series to cleanup more users.
not really the reason... it was just "this is small enough and
checkpatch doesn't complain" (it checks for 100 chars nowadays). But yes,
I can keep it in 4 lines.
thanks
Lucas De Marchi
On Tue 2022-01-18 23:24:47, Lucas De Marchi wrote:
> Add some helpers under lib/string_helpers.h so they can be used
> throughout the kernel. When I started doing this there were 2 other
> previous attempts I know of, not counting the iterations each of them
> had:
>
> 1) https://lore.kernel.org/all/[email protected]/
> 2) https://lore.kernel.org/all/[email protected]/#t
>
> Going through the comments I tried to find some common ground and
> justification for what is in here, addressing some of the concerns
> raised.
>
> d. This doesn't bring onoff() helper as there are some places in the
> kernel with onoff as variable - another name is probably needed for
> this function in order not to shadow the variable, or those variables
> could be renamed. Or if people wanting <someprefix>
> try to find a short one
I would call it str_on_off().
And I would actually suggest to use the same style also for
the other helpers.
The "str_" prefix would make it clear that it is something with
string. There are other <prefix>_on_off() that affect some
functionality, e.g. mute_led_on_off(), e1000_vlan_filter_on_off().
The dash '_' would significantly help to parse the name. yesno() and
onoff() are nicely short and kind of acceptable. But "enabledisable()"
is a puzzle.
IMHO, str_yes_no(), str_on_off(), str_enable_disable() are a good
compromise.
The main motivation should be code readability. You write the
code once. But many people will read it many times. Open coding
is sometimes better than misleading macro names.
That said, I do not want to block this patchset. If others like
it... ;-)
> e. One alternative to all of this suggested by Christian K?nig
> ([email protected]) would be to add a
> printk format. But besides the comment, he also seemed to like
> the common function. This brought the argument from others that the
> simple yesno()/enabledisable() already used in the code is easier to
> remember and use than e.g. %py[DOY]
Thanks for not going this way :-)
> Last patch also has some additional conversion of open coded cases. I
> preferred starting with drm/ since this is "closer to home".
>
> I hope this is a good summary of the previous attempts and a way we can
> move forward.
>
> Andrew Morton, Petr Mladek, Andy Shevchenko: if this is accepted, my
> proposal is to take first 2 patches either through mm tree or maybe
> vsprintf. Last patch can be taken later through drm.
I agree with Andy that it should go via drm tree. It would make it
easier to handle potential conflicts.
Just in case, you decide to go with str_yes_no() or something similar.
Mass changes are typically done at the end on the merge window.
The best solution is when it can be done by a script.
Best Regards,
Petr
On Wed, 19 Jan 2022, Petr Mladek <[email protected]> wrote:
> On Tue 2022-01-18 23:24:47, Lucas De Marchi wrote:
>> Add some helpers under lib/string_helpers.h so they can be used
>> throughout the kernel. When I started doing this there were 2 other
>> previous attempts I know of, not counting the iterations each of them
>> had:
>>
>> 1) https://lore.kernel.org/all/[email protected]/
>> 2) https://lore.kernel.org/all/[email protected]/#t
>>
>> Going through the comments I tried to find some common ground and
>> justification for what is in here, addressing some of the concerns
>> raised.
>>
>> d. This doesn't bring onoff() helper as there are some places in the
>> kernel with onoff as variable - another name is probably needed for
>> this function in order not to shadow the variable, or those variables
>> could be renamed. Or if people wanting <someprefix>
>> try to find a short one
>
> I would call it str_on_off().
>
> And I would actually suggest to use the same style also for
> the other helpers.
>
> The "str_" prefix would make it clear that it is something with
> string. There are other <prefix>_on_off() that affect some
> functionality, e.g. mute_led_on_off(), e1000_vlan_filter_on_off().
>
> The dash '_' would significantly help to parse the name. yesno() and
> onoff() are nicely short and kind of acceptable. But "enabledisable()"
> is a puzzle.
>
> IMHO, str_yes_no(), str_on_off(), str_enable_disable() are a good
> compromise.
>
> The main motivation should be code readability. You write the
> code once. But many people will read it many times. Open coding
> is sometimes better than misleading macro names.
>
> That said, I do not want to block this patchset. If others like
> it... ;-)
I don't mind the names either way. Adding the prefix and dashes is
helpful in that it's possible to add the functions first and convert
users at leisure, though with a bunch of churn, while using names that
collide with existing ones requires the changes to happen in one go.
What I do mind is grinding this series to a halt once again. I sent a
handful of versions of this three years ago, with inconclusive
bikeshedding back and forth, eventually threw my hands up in disgust,
and walked away.
>
>
>> e. One alternative to all of this suggested by Christian König
>> ([email protected]) would be to add a
>> printk format. But besides the comment, he also seemed to like
>> the common function. This brought the argument from others that the
>> simple yesno()/enabledisable() already used in the code is easier to
>> remember and use than e.g. %py[DOY]
>
> Thanks for not going this way :-)
>
>> Last patch also has some additional conversion of open coded cases. I
>> preferred starting with drm/ since this is "closer to home".
>>
>> I hope this is a good summary of the previous attempts and a way we can
>> move forward.
>>
>> Andrew Morton, Petr Mladek, Andy Shevchenko: if this is accepted, my
>> proposal is to take first 2 patches either through mm tree or maybe
>> vsprintf. Last patch can be taken later through drm.
>
> I agree with Andy that it should go via drm tree. It would make it
> easier to handle potential conflicts.
>
> Just in case, you decide to go with str_yes_no() or something similar.
> Mass changes are typically done at the end on the merge window.
> The best solution is when it can be done by a script.
>
> Best Regards,
> Petr
--
Jani Nikula, Intel Open Source Graphics Center
On Wed, Jan 19, 2022 at 04:16:12PM +0200, Jani Nikula wrote:
> On Wed, 19 Jan 2022, Petr Mladek <[email protected]> wrote:
> > On Tue 2022-01-18 23:24:47, Lucas De Marchi wrote:
> >> Add some helpers under lib/string_helpers.h so they can be used
> >> throughout the kernel. When I started doing this there were 2 other
> >> previous attempts I know of, not counting the iterations each of them
> >> had:
> >>
> >> 1) https://lore.kernel.org/all/[email protected]/
> >> 2) https://lore.kernel.org/all/[email protected]/#t
> >>
> >> Going through the comments I tried to find some common ground and
> >> justification for what is in here, addressing some of the concerns
> >> raised.
> >>
> >> d. This doesn't bring onoff() helper as there are some places in the
> >> kernel with onoff as variable - another name is probably needed for
> >> this function in order not to shadow the variable, or those variables
> >> could be renamed. Or if people wanting <someprefix>
> >> try to find a short one
> >
> > I would call it str_on_off().
> >
> > And I would actually suggest to use the same style also for
> > the other helpers.
> >
> > The "str_" prefix would make it clear that it is something with
> > string. There are other <prefix>_on_off() that affect some
> > functionality, e.g. mute_led_on_off(), e1000_vlan_filter_on_off().
> >
> > The dash '_' would significantly help to parse the name. yesno() and
> > onoff() are nicely short and kind of acceptable. But "enabledisable()"
> > is a puzzle.
> >
> > IMHO, str_yes_no(), str_on_off(), str_enable_disable() are a good
> > compromise.
> >
> > The main motivation should be code readability. You write the
> > code once. But many people will read it many times. Open coding
> > is sometimes better than misleading macro names.
> >
> > That said, I do not want to block this patchset. If others like
> > it... ;-)
>
> I don't mind the names either way. Adding the prefix and dashes is
> helpful in that it's possible to add the functions first and convert
> users at leisure, though with a bunch of churn, while using names that
> collide with existing ones requires the changes to happen in one go.
>
> What I do mind is grinding this series to a halt once again. I sent a
> handful of versions of this three years ago, with inconclusive
> bikeshedding back and forth, eventually threw my hands up in disgust,
> and walked away.
Yeah we can sed this anytime later we want to, but we need to get the foot
in the door. There's also a pile more of these all over.
Acked-by: Daniel Vetter <[email protected]>
on the series, maybe it helps? And yes let's merge this through drm-misc.
-Daniel
>
> >
> >
> >> e. One alternative to all of this suggested by Christian K?nig
> >> ([email protected]) would be to add a
> >> printk format. But besides the comment, he also seemed to like
> >> the common function. This brought the argument from others that the
> >> simple yesno()/enabledisable() already used in the code is easier to
> >> remember and use than e.g. %py[DOY]
> >
> > Thanks for not going this way :-)
> >
> >> Last patch also has some additional conversion of open coded cases. I
> >> preferred starting with drm/ since this is "closer to home".
> >>
> >> I hope this is a good summary of the previous attempts and a way we can
> >> move forward.
> >>
> >> Andrew Morton, Petr Mladek, Andy Shevchenko: if this is accepted, my
> >> proposal is to take first 2 patches either through mm tree or maybe
> >> vsprintf. Last patch can be taken later through drm.
> >
> > I agree with Andy that it should go via drm tree. It would make it
> > easier to handle potential conflicts.
> >
> > Just in case, you decide to go with str_yes_no() or something similar.
> > Mass changes are typically done at the end on the merge window.
> > The best solution is when it can be done by a script.
> >
> > Best Regards,
> > Petr
>
> --
> Jani Nikula, Intel Open Source Graphics Center
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Tue, Jan 18, 2022 at 11:24:50PM -0800, Lucas De Marchi wrote:
> linux/string_helpers.h provides a helper to return "yes"/"no"
> strings. Replace the open coded versions with yesno(). The places were
> identified with the following semantic patch:
>
> @@
> expression b;
> @@
>
> - b ? "yes" : "no"
> + yesno(b)
>
> Then the includes were added, so we include-what-we-use, and parenthesis
> adjusted in drivers/gpu/drm/v3d/v3d_debugfs.c. After the conversion we
> still see the same binary sizes:
>
> text data bss dec hex filename
> 1442171 60344 800 1503315 16f053 ./drivers/gpu/drm/radeon/radeon.ko
> 1442171 60344 800 1503315 16f053 ./drivers/gpu/drm/radeon/radeon.ko.old
> 5985991 324439 33808 6344238 60ce2e ./drivers/gpu/drm/amd/amdgpu/amdgpu.ko
> 5985991 324439 33808 6344238 60ce2e ./drivers/gpu/drm/amd/amdgpu/amdgpu.ko.old
> 411986 10490 6176 428652 68a6c ./drivers/gpu/drm/drm.ko
> 411986 10490 6176 428652 68a6c ./drivers/gpu/drm/drm.ko.old
> 1970292 109515 2352 2082159 1fc56f ./drivers/gpu/drm/nouveau/nouveau.ko
> 1970292 109515 2352 2082159 1fc56f ./drivers/gpu/drm/nouveau/nouveau.ko.old
...
> #include <linux/module.h>
> #include <linux/sched.h>
> #include <linux/slab.h>
> +#include <linux/string_helpers.h>
+ blank line?
> +#include <linux/string_helpers.h>
...
> seq_printf(m, "\tDP branch device present: %s\n",
> - branch_device ? "yes" : "no");
> + yesno(branch_device));
Now it's possible to keep this on one line.
...
> drm_printf_indent(p, indent, "imported=%s\n",
> - obj->import_attach ? "yes" : "no");
> + yesno(obj->import_attach));
81 here, but anyway, ditto!
...
> */
+blank line here?
> +#include <linux/string_helpers.h>
> +
> #include "aux.h"
> #include "pad.h"
...
> seq_printf(m, "MMU: %s\n",
> - (ident2 & V3D_HUB_IDENT2_WITH_MMU) ? "yes" : "no");
> + yesno(ident2 & V3D_HUB_IDENT2_WITH_MMU));
> seq_printf(m, "TFU: %s\n",
> - (ident1 & V3D_HUB_IDENT1_WITH_TFU) ? "yes" : "no");
> + yesno(ident1 & V3D_HUB_IDENT1_WITH_TFU));
> seq_printf(m, "TSY: %s\n",
> - (ident1 & V3D_HUB_IDENT1_WITH_TSY) ? "yes" : "no");
> + yesno(ident1 & V3D_HUB_IDENT1_WITH_TSY));
> seq_printf(m, "MSO: %s\n",
> - (ident1 & V3D_HUB_IDENT1_WITH_MSO) ? "yes" : "no");
> + yesno(ident1 & V3D_HUB_IDENT1_WITH_MSO));
> seq_printf(m, "L3C: %s (%dkb)\n",
> - (ident1 & V3D_HUB_IDENT1_WITH_L3C) ? "yes" : "no",
> + yesno(ident1 & V3D_HUB_IDENT1_WITH_L3C),
> V3D_GET_FIELD(ident2, V3D_HUB_IDENT2_L3C_NKB));
I believe it's fine to join back to have less LOCs (yes, it will be 83 or so,
but I believe in these cases it's very much okay).
--
With Best Regards,
Andy Shevchenko
On Wed, Jan 19, 2022 at 05:15:02PM +0100, Daniel Vetter wrote:
>On Wed, Jan 19, 2022 at 04:16:12PM +0200, Jani Nikula wrote:
>> On Wed, 19 Jan 2022, Petr Mladek <[email protected]> wrote:
>> > On Tue 2022-01-18 23:24:47, Lucas De Marchi wrote:
>> >> Add some helpers under lib/string_helpers.h so they can be used
>> >> throughout the kernel. When I started doing this there were 2 other
>> >> previous attempts I know of, not counting the iterations each of them
>> >> had:
>> >>
>> >> 1) https://lore.kernel.org/all/[email protected]/
>> >> 2) https://lore.kernel.org/all/[email protected]/#t
>> >>
>> >> Going through the comments I tried to find some common ground and
>> >> justification for what is in here, addressing some of the concerns
>> >> raised.
>> >>
>> >> d. This doesn't bring onoff() helper as there are some places in the
>> >> kernel with onoff as variable - another name is probably needed for
>> >> this function in order not to shadow the variable, or those variables
>> >> could be renamed. Or if people wanting <someprefix>
>> >> try to find a short one
>> >
>> > I would call it str_on_off().
>> >
>> > And I would actually suggest to use the same style also for
>> > the other helpers.
>> >
>> > The "str_" prefix would make it clear that it is something with
>> > string. There are other <prefix>_on_off() that affect some
>> > functionality, e.g. mute_led_on_off(), e1000_vlan_filter_on_off().
>> >
>> > The dash '_' would significantly help to parse the name. yesno() and
>> > onoff() are nicely short and kind of acceptable. But "enabledisable()"
>> > is a puzzle.
>> >
>> > IMHO, str_yes_no(), str_on_off(), str_enable_disable() are a good
>> > compromise.
>> >
>> > The main motivation should be code readability. You write the
>> > code once. But many people will read it many times. Open coding
>> > is sometimes better than misleading macro names.
>> >
>> > That said, I do not want to block this patchset. If others like
>> > it... ;-)
>>
>> I don't mind the names either way. Adding the prefix and dashes is
>> helpful in that it's possible to add the functions first and convert
>> users at leisure, though with a bunch of churn, while using names that
>> collide with existing ones requires the changes to happen in one go.
>>
>> What I do mind is grinding this series to a halt once again. I sent a
>> handful of versions of this three years ago, with inconclusive
>> bikeshedding back and forth, eventually threw my hands up in disgust,
>> and walked away.
>
>Yeah we can sed this anytime later we want to, but we need to get the foot
>in the door. There's also a pile more of these all over.
>
>Acked-by: Daniel Vetter <[email protected]>
>
>on the series, maybe it helps? And yes let's merge this through drm-misc.
Ok, it seems we are reaching some agreement here then:
- Change it to use str_ prefix
- Wait -rc1 to avoid conflict
- Merge through drm-misc
I will re-send the series again soon.
Lucas De Marchi
On Wed 2022-01-19 16:16:12, Jani Nikula wrote:
> On Wed, 19 Jan 2022, Petr Mladek <[email protected]> wrote:
> > On Tue 2022-01-18 23:24:47, Lucas De Marchi wrote:
> >> d. This doesn't bring onoff() helper as there are some places in the
> >> kernel with onoff as variable - another name is probably needed for
> >> this function in order not to shadow the variable, or those variables
> >> could be renamed. Or if people wanting <someprefix>
> >> try to find a short one
> >
> > I would call it str_on_off().
> >
> > And I would actually suggest to use the same style also for
> > the other helpers.
> >
> > The "str_" prefix would make it clear that it is something with
> > string. There are other <prefix>_on_off() that affect some
> > functionality, e.g. mute_led_on_off(), e1000_vlan_filter_on_off().
> >
> > The dash '_' would significantly help to parse the name. yesno() and
> > onoff() are nicely short and kind of acceptable. But "enabledisable()"
> > is a puzzle.
> >
> > IMHO, str_yes_no(), str_on_off(), str_enable_disable() are a good
> > compromise.
> >
> > The main motivation should be code readability. You write the
> > code once. But many people will read it many times. Open coding
> > is sometimes better than misleading macro names.
> >
> > That said, I do not want to block this patchset. If others like
> > it... ;-)
>
> I don't mind the names either way. Adding the prefix and dashes is
> helpful in that it's possible to add the functions first and convert
> users at leisure, though with a bunch of churn, while using names that
> collide with existing ones requires the changes to happen in one go.
It is also possible to support both notations at the beginning.
And convert the existing users in the 2nd step.
> What I do mind is grinding this series to a halt once again. I sent a
> handful of versions of this three years ago, with inconclusive
> bikeshedding back and forth, eventually threw my hands up in disgust,
> and walked away.
Yeah, and I am sorry for bikeshedding. Honestly, I do not know what is
better. This is why I do not want to block this series when others
like this.
My main motivation is to point out that:
enabledisable(enable)
might be, for some people, more eye bleeding than
enable ? "enable" : "disable"
The problem is not that visible with yesno() and onoff(). But as you said,
onoff() confliscts with variable names. And enabledisable() sucks.
As a result, there is a non-trivial risk of two mass changes:
now:
- contition ? "yes" : "no"
+ yesno(condition)
a few moths later:
- yesno(condition)
+ str_yes_no(condition)
Best Regards,
Petr
...
> Yeah, and I am sorry for bikeshedding. Honestly, I do not know what is
> better. This is why I do not want to block this series when others
> like this.
>
> My main motivation is to point out that:
>
> enabledisable(enable)
>
> might be, for some people, more eye bleeding than
>
> enable ? "enable" : "disable"
Indeed - you need to look the former up, wasting brain time.
> The problem is not that visible with yesno() and onoff(). But as you said,
> onoff() confliscts with variable names. And enabledisable() sucks.
> As a result, there is a non-trivial risk of two mass changes:
>
> now:
>
> - contition ? "yes" : "no"
> + yesno(condition)
>
> a few moths later:
>
> - yesno(condition)
> + str_yes_no(condition)
Followed by:
- str_yes_no(x)
+ no_yes_str(x)
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Thu, 20 Jan 2022, Petr Mladek <[email protected]> wrote:
> On Wed 2022-01-19 16:16:12, Jani Nikula wrote:
>> On Wed, 19 Jan 2022, Petr Mladek <[email protected]> wrote:
>> > On Tue 2022-01-18 23:24:47, Lucas De Marchi wrote:
>> >> d. This doesn't bring onoff() helper as there are some places in the
>> >> kernel with onoff as variable - another name is probably needed for
>> >> this function in order not to shadow the variable, or those variables
>> >> could be renamed. Or if people wanting <someprefix>
>> >> try to find a short one
>> >
>> > I would call it str_on_off().
>> >
>> > And I would actually suggest to use the same style also for
>> > the other helpers.
>> >
>> > The "str_" prefix would make it clear that it is something with
>> > string. There are other <prefix>_on_off() that affect some
>> > functionality, e.g. mute_led_on_off(), e1000_vlan_filter_on_off().
>> >
>> > The dash '_' would significantly help to parse the name. yesno() and
>> > onoff() are nicely short and kind of acceptable. But "enabledisable()"
>> > is a puzzle.
>> >
>> > IMHO, str_yes_no(), str_on_off(), str_enable_disable() are a good
>> > compromise.
>> >
>> > The main motivation should be code readability. You write the
>> > code once. But many people will read it many times. Open coding
>> > is sometimes better than misleading macro names.
>> >
>> > That said, I do not want to block this patchset. If others like
>> > it... ;-)
>>
>> I don't mind the names either way. Adding the prefix and dashes is
>> helpful in that it's possible to add the functions first and convert
>> users at leisure, though with a bunch of churn, while using names that
>> collide with existing ones requires the changes to happen in one go.
>
> It is also possible to support both notations at the beginning.
> And convert the existing users in the 2nd step.
>
>> What I do mind is grinding this series to a halt once again. I sent a
>> handful of versions of this three years ago, with inconclusive
>> bikeshedding back and forth, eventually threw my hands up in disgust,
>> and walked away.
>
> Yeah, and I am sorry for bikeshedding. Honestly, I do not know what is
> better. This is why I do not want to block this series when others
> like this.
>
> My main motivation is to point out that:
>
> enabledisable(enable)
>
> might be, for some people, more eye bleeding than
>
> enable ? "enable" : "disable"
>
>
> The problem is not that visible with yesno() and onoff(). But as you said,
> onoff() confliscts with variable names. And enabledisable() sucks.
> As a result, there is a non-trivial risk of two mass changes:
My point is, in the past three years we could have churned through more
than two mass renames just fine, if needed, *if* we had just managed to
merge something for a start!
BR,
Jani.
>
> now:
>
> - contition ? "yes" : "no"
> + yesno(condition)
>
> a few moths later:
>
> - yesno(condition)
> + str_yes_no(condition)
>
>
> Best Regards,
> Petr
--
Jani Nikula, Intel Open Source Graphics Center
On Thu 2022-01-20 11:12:27, Jani Nikula wrote:
> On Thu, 20 Jan 2022, Petr Mladek <[email protected]> wrote:
> > The problem is not that visible with yesno() and onoff(). But as you said,
> > onoff() confliscts with variable names. And enabledisable() sucks.
> > As a result, there is a non-trivial risk of two mass changes:
>
> My point is, in the past three years we could have churned through more
> than two mass renames just fine, if needed, *if* we had just managed to
> merge something for a start!
Huh, this sound alarming.
Cosmetic changes just complicate history. They make "git blame" useless.
They also complicate backports. I know that it is not problem for
mainline. But there are supported stable branches, ...
There should be a good reason for such changes. They should not be
done light-heartedly.
Best Regards,
Petr
On Wed, Jan 19, 2022 at 09:30:47PM +0200, Andy Shevchenko wrote:
>On Tue, Jan 18, 2022 at 11:24:50PM -0800, Lucas De Marchi wrote:
>> linux/string_helpers.h provides a helper to return "yes"/"no"
>> strings. Replace the open coded versions with yesno(). The places were
>> identified with the following semantic patch:
>>
>> @@
>> expression b;
>> @@
>>
>> - b ? "yes" : "no"
>> + yesno(b)
>>
>> Then the includes were added, so we include-what-we-use, and parenthesis
>> adjusted in drivers/gpu/drm/v3d/v3d_debugfs.c. After the conversion we
>> still see the same binary sizes:
>>
>> text data bss dec hex filename
>> 1442171 60344 800 1503315 16f053 ./drivers/gpu/drm/radeon/radeon.ko
>> 1442171 60344 800 1503315 16f053 ./drivers/gpu/drm/radeon/radeon.ko.old
>> 5985991 324439 33808 6344238 60ce2e ./drivers/gpu/drm/amd/amdgpu/amdgpu.ko
>> 5985991 324439 33808 6344238 60ce2e ./drivers/gpu/drm/amd/amdgpu/amdgpu.ko.old
>> 411986 10490 6176 428652 68a6c ./drivers/gpu/drm/drm.ko
>> 411986 10490 6176 428652 68a6c ./drivers/gpu/drm/drm.ko.old
>> 1970292 109515 2352 2082159 1fc56f ./drivers/gpu/drm/nouveau/nouveau.ko
>> 1970292 109515 2352 2082159 1fc56f ./drivers/gpu/drm/nouveau/nouveau.ko.old
>
>...
>
>> #include <linux/module.h>
>> #include <linux/sched.h>
>> #include <linux/slab.h>
>> +#include <linux/string_helpers.h>
>
>+ blank line?
>
>> +#include <linux/string_helpers.h>
>
>...
>
>> seq_printf(m, "\tDP branch device present: %s\n",
>> - branch_device ? "yes" : "no");
>> + yesno(branch_device));
>
>Now it's possible to keep this on one line.
>
>...
>
>> drm_printf_indent(p, indent, "imported=%s\n",
>> - obj->import_attach ? "yes" : "no");
>> + yesno(obj->import_attach));
>
>81 here, but anyway, ditto!
>
>...
>
>> */
>
>+blank line here?
>
>> +#include <linux/string_helpers.h>
>> +
>> #include "aux.h"
>> #include "pad.h"
>
>...
>
>> seq_printf(m, "MMU: %s\n",
>> - (ident2 & V3D_HUB_IDENT2_WITH_MMU) ? "yes" : "no");
>> + yesno(ident2 & V3D_HUB_IDENT2_WITH_MMU));
>> seq_printf(m, "TFU: %s\n",
>> - (ident1 & V3D_HUB_IDENT1_WITH_TFU) ? "yes" : "no");
>> + yesno(ident1 & V3D_HUB_IDENT1_WITH_TFU));
>> seq_printf(m, "TSY: %s\n",
>> - (ident1 & V3D_HUB_IDENT1_WITH_TSY) ? "yes" : "no");
>> + yesno(ident1 & V3D_HUB_IDENT1_WITH_TSY));
>> seq_printf(m, "MSO: %s\n",
>> - (ident1 & V3D_HUB_IDENT1_WITH_MSO) ? "yes" : "no");
>> + yesno(ident1 & V3D_HUB_IDENT1_WITH_MSO));
>> seq_printf(m, "L3C: %s (%dkb)\n",
>> - (ident1 & V3D_HUB_IDENT1_WITH_L3C) ? "yes" : "no",
>> + yesno(ident1 & V3D_HUB_IDENT1_WITH_L3C),
>> V3D_GET_FIELD(ident2, V3D_HUB_IDENT2_L3C_NKB));
>
>I believe it's fine to join back to have less LOCs (yes, it will be 83 or so,
>but I believe in these cases it's very much okay).
now that we are converting to str_yes_no(), we will have a few more
chars. Some maintainers may be more strict on the 80 or 100 chars. I
will assume whatever is in the code base is the preferred form.
thanks
Lucas De Marchi
>
>--
>With Best Regards,
>Andy Shevchenko
>
>