On Wed, Dec 22, 2021 at 12:19 PM Rafał Miłecki [off-list ref] wrote:

On 16.12.2021 20:55, Andy Shevchenko wrote:

quoted

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.

There is no rule, but common sense. Why occupy 3 LOCs instead of 1 LOC?

...

quoted

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.

Yes. The reason is simple. With moving this group separately you
follow the rule of going from most generic to most particular headers
in the block.  Grouping like this will show better that this code has
tighten relations with the pin control subsystem.

...

quoted

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.

They may easily conflict through headers with something more generic
not related to your driver or even GPIO. The TEST_PORT_COMMAND seems
one of this kind that might potentially collide.

...

quoted

quoted

+

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.

Maybe these comments are not so useful after all?

...

quoted

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

Same as above, common sense and the tight relationship between two.

-- 
With Best Regards,
Andy Shevchenko