On 16.12.2021 20:55, Andy Shevchenko wrote:

quoted

+/*
+ * Copyright (C) 2021 Rafał Miłecki [off-list ref]
+ */

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.

quoted

+#include <linux/pinctrl/pinconf-generic.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>

Can you move this group...

quoted

+#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.

quoted

+#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.

quoted

+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

quoted

+       /* 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.

quoted

+static struct platform_driver bcm4908_pinctrl_driver = {
+       .probe = bcm4908_pinctrl_probe,
+       .driver = {
+               .name = "bcm4908-pinctrl",
+               .of_match_table = bcm4908_pinctrl_of_match_table,
+       },
+};

quoted

+

No need.

quoted

+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