Re: [PATCH v2] lib/vsprintf: introduce OF_DEVICE_NODE_FLAG_MAX for %pOF

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Fri, Jan 17, 2020 at 07:15:19PM +0800, lijiazi wrote:
> Introduce OF_DEVICE_NODE_FLAG_MAX for device node printk, if add
> new device node flag, please add the corresponding flag abbreviation
> to tbuf in device_node_string.

Thank you for an update!
My comments below.

...

> +                                                        O - Overlay
> +                                                        F - Overlay free cset

Perhaps this would be a separate patch.

> +/* Add flag max for %pOF related printk, if add new flag, please
> + * increase following marco, and add abbreviations to tbuf in
> + * device_node_string function.
> + */

I'm not sure it's correct style for multi-line comments.

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

Altogether I think this would be done as a separate patch (with corresponding
Suggested-by tag).

> -	char tbuf[sizeof("xxxx") + 1];
> +	char tbuf[OF_DEVICE_NODE_FLAG_MAX + 1] = { "DdPBOF" };

This trick is interesting. Had you checked what code is being produced
out of it (additional memcpy()? or constants assignment on the stack?) ?

> -			tbuf[0] = of_node_check_flag(dn, OF_DYNAMIC) ? 'D' : '-';
> -			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;

> +			for (i = 0; i < OF_DEVICE_NODE_FLAG_MAX; i++) {
> +				if (!of_node_check_flag(dn, OF_DYNAMIC + i))
> +					tbuf[i] = '-';
> +			}

For my personal opinion the former is clearer than the latter since we have no
exported iterator over the OF device node flags. What if we decide not to use
one in the middle?

> +			tbuf[OF_DEVICE_NODE_FLAG_MAX] = 0;

This line completely depends on how we assign the initial array (see above).

-- 
With Best Regards,
Andy Shevchenko