On Wed, Dec 22, 2021 at 12:19 PM Rafał Miłecki <rafal@xxxxxxxxxx> wrote: > 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. There is no rule, but common sense. Why occupy 3 LOCs instead of 1 LOC? ... > >> +#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. 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. ... > >> +#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. ... > >> + > > > > 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? ... > >> + > > > > 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 Same as above, common sense and the tight relationship between two. -- With Best Regards, Andy Shevchenko
