[v3,2/3] lib/vsprintf: introduce OF_DEVICE_NODE_FLAG_MAX
diff mbox series

Message ID 2d432e67cab2eb51f36f5b2e904a185ef48df6e0.1579423564.git.lijiazi@xiaomi.com
State Changes Requested
Headers show
Series
  • Untitled series #154227
Related show

Checks

Context Check Description
robh/checkpatch warning "total: 0 errors, 1 warnings, 13 lines checked"

Commit Message

lijiazi Jan. 20, 2020, 11:38 a.m. UTC
introduce OF_DEVICE_NODE_FLAG_MAX for %pOF related printk.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: lijiazi <lijiazi@xiaomi.com>
---
Changes in v3:
 - fix incorrect multi-line comment style.
 - split v2 to 3 patches.
---
 include/linux/of.h | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Sergey Senozhatsky Jan. 20, 2020, 1:38 p.m. UTC | #1
On (20/01/20 19:38), lijiazi wrote:
[..]
> Changes in v3:
>  - fix incorrect multi-line comment style.
>  - split v2 to 3 patches.
> ---
>  include/linux/of.h | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/include/linux/of.h b/include/linux/of.h
> index c669c0a..c5bbfa6 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -147,6 +147,13 @@ extern raw_spinlock_t devtree_lock;
>  #define OF_OVERLAY		5 /* allocated for an overlay */
>  #define OF_OVERLAY_FREE_CSET	6 /* in overlay cset being freed */
>  
> +/*
> + * add OF_DEVICE_NODE_FLAG_MAX for %pOF related printk.
> + * if there is any change on these flags, please synchronize the change
> + * to device_node_string function in lib/vsprintf.c.

So maybe the flags can become enum then?

> + */
> +#define OF_DEVICE_NODE_FLAG_MAX 6 /* Maximum available flags */

	-ss
Andy Shevchenko Jan. 20, 2020, 2:16 p.m. UTC | #2
On Mon, Jan 20, 2020 at 07:38:28PM +0800, lijiazi wrote:
> introduce OF_DEVICE_NODE_FLAG_MAX for %pOF related printk.

introduce -> Introduce

> @@ -147,6 +147,13 @@ extern raw_spinlock_t devtree_lock;
>  #define OF_OVERLAY		5 /* allocated for an overlay */
>  #define OF_OVERLAY_FREE_CSET	6 /* in overlay cset being freed */

> +/*
> + * add OF_DEVICE_NODE_FLAG_MAX for %pOF related printk.
> + * if there is any change on these flags, please synchronize the change
> + * to device_node_string function in lib/vsprintf.c.

This is doubtful comment. If we don't want to print some flags in printf()?
And in general this is orthogonal to the printf() changes.

> + */
> +#define OF_DEVICE_NODE_FLAG_MAX 6 /* Maximum available flags */
Andy Shevchenko Jan. 20, 2020, 2:19 p.m. UTC | #3
On Mon, Jan 20, 2020 at 07:38:29PM +0800, lijiazi wrote:
> Add two device node flags, and use OF_DEVICE_NODE_FLAG_MAX instead
> of sizeof("xxxx").

...

>  			tbuf[1] = of_node_check_flag(dn, OF_DETACHED) ? 'd' : '-';
>  			tbuf[2] = of_node_check_flag(dn, OF_POPULATED) ? 'P' : '-';
>  			tbuf[3] = of_node_check_flag(dn, OF_POPULATED_BUS) ? 'B' : '-';

> -			tbuf[4] = 0;

This is fine to leave untouched. See below.

> +			tbuf[4] = of_node_check_flag(dn, OF_OVERLAY) ? 'O' : '-';
> +			tbuf[5] = of_node_check_flag(dn, OF_OVERLAY_FREE_CSET) ? 'F' : '-';

These two should be part of patch 1, which in turn should be last in the series.

> +			tbuf[OF_DEVICE_NODE_FLAG_MAX] = 0;

This one also, but in a form of explicit number, if you afraid of problems
here, we may add something like

	BUILD_BUG_ON(OF_DEVICE_NODE_FLAG_MAX < ...);

where ... depends on amount of flags we print here.
Petr Mladek Feb. 10, 2020, 11:42 a.m. UTC | #4
On Mon 2020-01-20 16:19:51, Andy Shevchenko wrote:
> On Mon, Jan 20, 2020 at 07:38:29PM +0800, lijiazi wrote:
> > Add two device node flags, and use OF_DEVICE_NODE_FLAG_MAX instead
> > of sizeof("xxxx").
> 
> ...
> 
> >  			tbuf[1] = of_node_check_flag(dn, OF_DETACHED) ? 'd' : '-';
> >  			tbuf[2] = of_node_check_flag(dn, OF_POPULATED) ? 'P' : '-';
> >  			tbuf[3] = of_node_check_flag(dn, OF_POPULATED_BUS) ? 'B' : '-';
> 
> > -			tbuf[4] = 0;
> 
> This is fine to leave untouched. See below.
> 
> > +			tbuf[4] = of_node_check_flag(dn, OF_OVERLAY) ? 'O' : '-';
> > +			tbuf[5] = of_node_check_flag(dn, OF_OVERLAY_FREE_CSET) ? 'F' : '-';
> 
> These two should be part of patch 1, which in turn should be last in the series.
> 
> > +			tbuf[OF_DEVICE_NODE_FLAG_MAX] = 0;
> 
> This one also, but in a form of explicit number, if you afraid of problems
> here, we may add something like

I agree that explicit number would be better here:

			tbuf[6] = 0;

And I like the build check. I would even check for the exact number.
It would help to keep the code updated when the number is modified.

	BUILD_BUG_ON(OF_DEVICE_NODE_FLAG_MAX != 6);

Finally, I would put all three changes into the same patch. And
remove the comment above OF_DEVICE_NODE_FLAG_MAX definition
completely.

Best Regards,
Petr

Patch
diff mbox series

diff --git a/include/linux/of.h b/include/linux/of.h
index c669c0a..c5bbfa6 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -147,6 +147,13 @@  extern raw_spinlock_t devtree_lock;
 #define OF_OVERLAY		5 /* allocated for an overlay */
 #define OF_OVERLAY_FREE_CSET	6 /* in overlay cset being freed */
 
+/*
+ * add OF_DEVICE_NODE_FLAG_MAX for %pOF related printk.
+ * if there is any change on these flags, please synchronize the change
+ * to device_node_string function in lib/vsprintf.c.
+ */
+#define OF_DEVICE_NODE_FLAG_MAX 6 /* Maximum available flags */
+
 #define OF_BAD_ADDR	((u64)-1)
 
 #ifdef CONFIG_OF