Re: [PATCH 2/2] pinctrl: bcm: add driver for BCM4908 pinmux

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Wed, Dec 22, 2021 at 12:19 PM Rafał Miłecki <rafal@milecki.pl> wrote:
> On 16.12.2021 20:55, Andy Shevchenko wrote:
> >> +/*
> >> + * Copyright (C) 2021 Rafał Miłecki <rafal@milecki.pl>
> >> + */
> >
> > 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