Re: [PATCH v2 1/3] firmware: Add boot information to sysfs

On Thu, 3 Feb 2022 at 12:44, Greg Kroah-Hartman
[off-list ref] wrote:

On Thu, Feb 03, 2022 at 10:23:42PM +1030, Joel Stanley wrote:

quoted

diff --git a/include/linux/firmware_bootinfo.h b/include/linux/firmware_bootinfo.h
new file mode 100644
index 000000000000..3fe630b061b9
--- /dev/null
+++ b/include/linux/firmware_bootinfo.h

@@ -0,0 +1,22 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */

I have to ask, do you really mean "or later"?

Yeah. That's what we're told we should use.

quoted

+/* Copyright 2022 IBM Corp. */
+
+#include <linux/sysfs.h>
+#include <linux/init.h>
+
+#define BOOTINFO_SET(b, n, v) b.n.en = true; b.n.val = v

Please use a while {} loop around these two statements.

Didn't checkpatch warn you about that?

No, it didn't. I'll add it.

quoted

+struct bootinfo_entry {
+     bool en;

What does "en" mean?  "enabled"?  If so, please spell it out.

quoted

+     bool val;

How can a "value" have a boolean?  Is this "valid"?  Again, please spell
it out, we have no lack of letters to use here to keep people from being
confused.

I meant value. I think it's reasonable for a value to be true or
false. I'll make the names clearer with docs as you suggest.

Can you please use kernel-doc comments to describe this structure?

quoted

+};
+
+struct bootinfo {
+     struct bootinfo_entry abr_image;
+     struct bootinfo_entry low_security_key;
+     struct bootinfo_entry otp_protected;
+     struct bootinfo_entry secure_boot;
+     struct bootinfo_entry uart_boot;
+};

Same here, please use kernel-doc

quoted

+
+int __init firmware_bootinfo_init(struct bootinfo *bootinfo_init);

__init is not needed on a function definition like this.

ack.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel