[v5,09/11] lib/vsprintf: OF nodes are first and foremost, struct device_nodes
diff mbox series

Message ID 20190902135732.23455-10-sakari.ailus@linux.intel.com
State Superseded, archived
Headers show
Series
  • Device property improvements, add %pfw format specifier
Related show

Commit Message

Sakari Ailus Sept. 2, 2019, 1:57 p.m. UTC
Factor out static kobject_string() function that simply calls
device_node_string(), and thus remove references to kobjects (as these are
struct device_node).

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/vsprintf.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

Comments

Petr Mladek Sept. 3, 2019, 8:52 a.m. UTC | #1
On Mon 2019-09-02 16:57:30, Sakari Ailus wrote:
> Factor out static kobject_string() function that simply calls
> device_node_string(), and thus remove references to kobjects (as these are
> struct device_node).
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  lib/vsprintf.c | 16 ++++------------
>  1 file changed, 4 insertions(+), 12 deletions(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index a04a2167101ef..4ad9332d54ba6 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1905,6 +1905,9 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
>  	struct printf_spec str_spec = spec;
>  	str_spec.field_width = -1;
>  
> +	if (fmt[0] != 'F')
> +		return error_string(buf, end, "(%pO?)", spec);
> +
>  	if (!IS_ENABLED(CONFIG_OF))
>  		return error_string(buf, end, "(%pOF?)", spec);
>  
> @@ -1978,17 +1981,6 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
>  	return widen_string(buf, buf - buf_start, end, spec);
>  }
>  
> -static char *kobject_string(char *buf, char *end, void *ptr,
> -			    struct printf_spec spec, const char *fmt)
> -{
> -	switch (fmt[1]) {
> -	case 'F':
> -		return device_node_string(buf, end, ptr, spec, fmt + 1);
> -	}
> -
> -	return error_string(buf, end, "(%pO?)", spec);
> -}
> -
>  /*
>   * Show a '%p' thing.  A kernel extension is that the '%p' is followed
>   * by an extra set of alphanumeric characters that are extended format
> @@ -2167,7 +2159,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>  	case 'G':
>  		return flags_string(buf, end, ptr, spec, fmt);
>  	case 'O':
> -		return kobject_string(buf, end, ptr, spec, fmt);
> +		return device_node_string(buf, end, ptr, spec, fmt + 1);

I know that this come from from kobject_string(). But please, modify
it to follow the style used by other %p modifiers. I mean to pass
"fmt" as is and then use:

	if (fmt[1] != 'F')

With the above change:

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr
Petr Mladek Sept. 3, 2019, 9:28 a.m. UTC | #2
On Tue 2019-09-03 10:52:33, Petr Mladek wrote:
> On Mon 2019-09-02 16:57:30, Sakari Ailus wrote:
> > Factor out static kobject_string() function that simply calls
> > device_node_string(), and thus remove references to kobjects (as these are
> > struct device_node).
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  lib/vsprintf.c | 16 ++++------------
> >  1 file changed, 4 insertions(+), 12 deletions(-)
> > 
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index a04a2167101ef..4ad9332d54ba6 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -1905,6 +1905,9 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
> >  	struct printf_spec str_spec = spec;
> >  	str_spec.field_width = -1;
> >  
> > +	if (fmt[0] != 'F')
> > +		return error_string(buf, end, "(%pO?)", spec);
> > +
> >  	if (!IS_ENABLED(CONFIG_OF))
> >  		return error_string(buf, end, "(%pOF?)", spec);
> >  
> > @@ -1978,17 +1981,6 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
> >  	return widen_string(buf, buf - buf_start, end, spec);
> >  }
> >  
> > -static char *kobject_string(char *buf, char *end, void *ptr,
> > -			    struct printf_spec spec, const char *fmt)
> > -{
> > -	switch (fmt[1]) {
> > -	case 'F':
> > -		return device_node_string(buf, end, ptr, spec, fmt + 1);
> > -	}
> > -
> > -	return error_string(buf, end, "(%pO?)", spec);
> > -}
> > -
> >  /*
> >   * Show a '%p' thing.  A kernel extension is that the '%p' is followed
> >   * by an extra set of alphanumeric characters that are extended format
> > @@ -2167,7 +2159,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
> >  	case 'G':
> >  		return flags_string(buf, end, ptr, spec, fmt);
> >  	case 'O':
> > -		return kobject_string(buf, end, ptr, spec, fmt);
> > +		return device_node_string(buf, end, ptr, spec, fmt + 1);
> 
> I know that this come from from kobject_string(). But please, modify
> it to follow the style used by other %p modifiers. I mean to pass
> "fmt" as is and then use:
> 
> 	if (fmt[1] != 'F')

Ah, I see that it would need more changes in device_node_string().
OK, let's leave the patch as is. I am sorry for the noise.

Best Regards,
Petr
Andy Shevchenko Sept. 3, 2019, 11:28 a.m. UTC | #3
On Tue, Sep 03, 2019 at 11:28:16AM +0200, Petr Mladek wrote:
> On Tue 2019-09-03 10:52:33, Petr Mladek wrote:
> > On Mon 2019-09-02 16:57:30, Sakari Ailus wrote:
> > > Factor out static kobject_string() function that simply calls
> > > device_node_string(), and thus remove references to kobjects (as these are
> > > struct device_node).

> > > -		return kobject_string(buf, end, ptr, spec, fmt);
> > > +		return device_node_string(buf, end, ptr, spec, fmt + 1);
> > 
> > I know that this come from from kobject_string(). But please, modify
> > it to follow the style used by other %p modifiers. I mean to pass
> > "fmt" as is and then use:
> > 
> > 	if (fmt[1] != 'F')
> 
> Ah, I see that it would need more changes in device_node_string().
> OK, let's leave the patch as is. I am sorry for the noise.

I came to the same conclusions, though can we consider to drop this patch?
Sakari Ailus Sept. 6, 2019, 7:04 a.m. UTC | #4
On Tue, Sep 03, 2019 at 02:28:00PM +0300, Andy Shevchenko wrote:
> On Tue, Sep 03, 2019 at 11:28:16AM +0200, Petr Mladek wrote:
> > On Tue 2019-09-03 10:52:33, Petr Mladek wrote:
> > > On Mon 2019-09-02 16:57:30, Sakari Ailus wrote:
> > > > Factor out static kobject_string() function that simply calls
> > > > device_node_string(), and thus remove references to kobjects (as these are
> > > > struct device_node).
> 
> > > > -		return kobject_string(buf, end, ptr, spec, fmt);
> > > > +		return device_node_string(buf, end, ptr, spec, fmt + 1);
> > > 
> > > I know that this come from from kobject_string(). But please, modify
> > > it to follow the style used by other %p modifiers. I mean to pass
> > > "fmt" as is and then use:
> > > 
> > > 	if (fmt[1] != 'F')
> > 
> > Ah, I see that it would need more changes in device_node_string().
> > OK, let's leave the patch as is. I am sorry for the noise.
> 
> I came to the same conclusions, though can we consider to drop this patch?

It's a cleanup. I'd prefer to keep the patch.

Albeit fmt++; in device_node_string() would do the trick of avoiding fmt +
1 in the caller. That said, I'd prefer to keep the original patch as-is.

Patch
diff mbox series

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index a04a2167101ef..4ad9332d54ba6 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1905,6 +1905,9 @@  char *device_node_string(char *buf, char *end, struct device_node *dn,
 	struct printf_spec str_spec = spec;
 	str_spec.field_width = -1;
 
+	if (fmt[0] != 'F')
+		return error_string(buf, end, "(%pO?)", spec);
+
 	if (!IS_ENABLED(CONFIG_OF))
 		return error_string(buf, end, "(%pOF?)", spec);
 
@@ -1978,17 +1981,6 @@  char *device_node_string(char *buf, char *end, struct device_node *dn,
 	return widen_string(buf, buf - buf_start, end, spec);
 }
 
-static char *kobject_string(char *buf, char *end, void *ptr,
-			    struct printf_spec spec, const char *fmt)
-{
-	switch (fmt[1]) {
-	case 'F':
-		return device_node_string(buf, end, ptr, spec, fmt + 1);
-	}
-
-	return error_string(buf, end, "(%pO?)", spec);
-}
-
 /*
  * Show a '%p' thing.  A kernel extension is that the '%p' is followed
  * by an extra set of alphanumeric characters that are extended format
@@ -2167,7 +2159,7 @@  char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 	case 'G':
 		return flags_string(buf, end, ptr, spec, fmt);
 	case 'O':
-		return kobject_string(buf, end, ptr, spec, fmt);
+		return device_node_string(buf, end, ptr, spec, fmt + 1);
 	case 'x':
 		return pointer_string(buf, end, ptr, spec);
 	}