On 16.12.2021 20:55, Andy Shevchenko wrote:
+/*
+ * Copyright (C) 2021 Rafał Miłecki <rafal@xxxxxxxxxx>
+ */
One line?
I don't think there's a rule for that. Not in coding-style.rst as much
as I'm aware of. checkpatch.pl also doesn't complain.
+#include <linux/pinctrl/pinconf-generic.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
Can you move this group...
+#include <linux/platform_device.h>
+#include <linux/slab.h>
...here?
Any reason for that? For most of the time I keep my includes sorted
alphabetically. Now I checked coding-style.rst is actually seems to
recomment "clang-format" for the same reason: sorting includes.
+#define TEST_PORT_BLOCK_EN_LSB 0x00
+#define TEST_PORT_BLOCK_DATA_MSB 0x04
+#define TEST_PORT_BLOCK_DATA_LSB 0x08
+#define TEST_PORT_LSB_PINMUX_DATA_SHIFT 12
+#define TEST_PORT_COMMAND 0x0c
+#define TEST_PORT_CMD_LOAD_MUX_REG 0x00000021
The prefix of all above doesn't match the module name.
Those are register names as in Broadcom's documentation. I don't think
those names can conflict with any included header defines but I can
change it.
+struct bcm4908_pinctrl_grp {
+ const char *name;
+ const struct bcm4908_pinctrl_pin_setup *pins;
+ const unsigned int num_pins;
+};
Why not to (re)use struct group_desc?
It's because "struct group_desc" has @pins that I can't fill statically
as I need struct instead of int.
I also need struct field for "const struct bcm4908_pinctrl_pin_setup"
and "void *data" doesn't fit that purpsose 100% because:
1. It's a void
2. It's not static
+ /* Set pinctrl properties */
+
Here and everywhere else, please drop redundant blank line.
No clear kernel rule for that.
I use blank line to indicate / suggest that comment applies to more than
just a single line that follows.
+static struct platform_driver bcm4908_pinctrl_driver = {
+ .probe = bcm4908_pinctrl_probe,
+ .driver = {
+ .name = "bcm4908-pinctrl",
+ .of_match_table = bcm4908_pinctrl_of_match_table,
+ },
+};
+
No need.
+module_platform_driver(bcm4908_pinctrl_driver);
You have 1344 other source files with empty line above
module_platform_driver(). coding-style.rst says to "separate functions
with one blank line". Are we supposed to argue now whether a macro can
be considered a functio nor not?
grep -B 1 -r "module_platform_driver" drivers/* | egrep -c "\.c-$"
1344
