| Message ID | 20211215204753.5956-1-zajec5@gmail.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [1/2] dt-bindings: pinctrl: Add binding for BCM4908 pinctrl | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/dtbs-check | success | |
| robh/dt-meta-schema | fail | build log |
On Wed, 15 Dec 2021 21:47:52 +0100, Rafał Miłecki wrote: > From: Rafał Miłecki <rafal@milecki.pl> > > It's hardware block that is part of every SoC from BCM4908 family. > > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > --- > .../pinctrl/brcm,bcm4908-pinctrl.yaml | 72 +++++++++++++++++++ > MAINTAINERS | 7 ++ > 2 files changed, 79 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pinctrl/brcm,bcm4908-pinctrl.yaml > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: Unknown file referenced: [Errno 2] No such file or directory: '/usr/local/lib/python3.8/dist-packages/dtschema/schemas/pinctrl/pinctrl.yaml' xargs: dt-doc-validate: exited with status 255; aborting make[1]: *** Deleting file 'Documentation/devicetree/bindings/pinctrl/brcm,bcm4908-pinctrl.example.dt.yaml' Unknown file referenced: [Errno 2] No such file or directory: '/usr/local/lib/python3.8/dist-packages/dtschema/schemas/pinctrl/pinctrl.yaml' make[1]: *** [scripts/Makefile.lib:373: Documentation/devicetree/bindings/pinctrl/brcm,bcm4908-pinctrl.example.dt.yaml] Error 255 make[1]: *** Waiting for unfinished jobs.... make: *** [Makefile:1413: dt_binding_check] Error 2 doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/patch/1568760 This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit.
Hi Rob, On 15.12.2021 23:27, Rob Herring wrote: > On Wed, 15 Dec 2021 21:47:52 +0100, Rafał Miłecki wrote: >> From: Rafał Miłecki <rafal@milecki.pl> >> >> It's hardware block that is part of every SoC from BCM4908 family. >> >> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> >> --- >> .../pinctrl/brcm,bcm4908-pinctrl.yaml | 72 +++++++++++++++++++ >> MAINTAINERS | 7 ++ >> 2 files changed, 79 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/pinctrl/brcm,bcm4908-pinctrl.yaml >> > > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' > on your patch (DT_CHECKER_FLAGS is new in v5.13): > > yamllint warnings/errors: > > dtschema/dtc warnings/errors: > Unknown file referenced: [Errno 2] No such file or directory: '/usr/local/lib/python3.8/dist-packages/dtschema/schemas/pinctrl/pinctrl.yaml' > xargs: dt-doc-validate: exited with status 255; aborting > make[1]: *** Deleting file 'Documentation/devicetree/bindings/pinctrl/brcm,bcm4908-pinctrl.example.dt.yaml' > Unknown file referenced: [Errno 2] No such file or directory: '/usr/local/lib/python3.8/dist-packages/dtschema/schemas/pinctrl/pinctrl.yaml' > make[1]: *** [scripts/Makefile.lib:373: Documentation/devicetree/bindings/pinctrl/brcm,bcm4908-pinctrl.example.dt.yaml] Error 255 > make[1]: *** Waiting for unfinished jobs.... > make: *** [Makefile:1413: dt_binding_check] Error 2 this patch targets Linus's git tree: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git/log/?h=for-next Above tree contains 896568e5b9c8 ("dt-bindings: pinctrl: convert controller description to the json-schema") which provides pinctrl.yaml.
On Wed, Dec 15, 2021 at 11:35:01PM +0100, Rafał Miłecki wrote: > Hi Rob, > > On 15.12.2021 23:27, Rob Herring wrote: > > On Wed, 15 Dec 2021 21:47:52 +0100, Rafał Miłecki wrote: > > > From: Rafał Miłecki <rafal@milecki.pl> > > > > > > It's hardware block that is part of every SoC from BCM4908 family. > > > > > > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > > > --- > > > .../pinctrl/brcm,bcm4908-pinctrl.yaml | 72 +++++++++++++++++++ > > > MAINTAINERS | 7 ++ > > > 2 files changed, 79 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/pinctrl/brcm,bcm4908-pinctrl.yaml > > > > > > > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' > > on your patch (DT_CHECKER_FLAGS is new in v5.13): > > > > yamllint warnings/errors: > > > > dtschema/dtc warnings/errors: > > Unknown file referenced: [Errno 2] No such file or directory: '/usr/local/lib/python3.8/dist-packages/dtschema/schemas/pinctrl/pinctrl.yaml' > > xargs: dt-doc-validate: exited with status 255; aborting > > make[1]: *** Deleting file 'Documentation/devicetree/bindings/pinctrl/brcm,bcm4908-pinctrl.example.dt.yaml' > > Unknown file referenced: [Errno 2] No such file or directory: '/usr/local/lib/python3.8/dist-packages/dtschema/schemas/pinctrl/pinctrl.yaml' > > make[1]: *** [scripts/Makefile.lib:373: Documentation/devicetree/bindings/pinctrl/brcm,bcm4908-pinctrl.example.dt.yaml] Error 255 > > make[1]: *** Waiting for unfinished jobs.... > > make: *** [Makefile:1413: dt_binding_check] Error 2 > > this patch targets Linus's git tree: > https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git/log/?h=for-next > > Above tree contains 896568e5b9c8 ("dt-bindings: pinctrl: convert > controller description to the json-schema") which provides pinctrl.yaml. Please note that in the patch if you don't want to get this message. Otherwise, this is the alert that there's some dependency. Rob
On Wed, 15 Dec 2021 21:47:52 +0100, Rafał Miłecki wrote: > From: Rafał Miłecki <rafal@milecki.pl> > > It's hardware block that is part of every SoC from BCM4908 family. > > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > --- > .../pinctrl/brcm,bcm4908-pinctrl.yaml | 72 +++++++++++++++++++ > MAINTAINERS | 7 ++ > 2 files changed, 79 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pinctrl/brcm,bcm4908-pinctrl.yaml > Reviewed-by: Rob Herring <robh@kernel.org>
On Thu, Dec 16, 2021 at 1:25 AM Rafał Miłecki <zajec5@gmail.com> wrote: > > From: Rafał Miłecki <rafal@milecki.pl> > > BCM4908 has its own pins layout so it needs a custom binding and a Linux > driver. ... > +config PINCTRL_BCM4908 > + bool "Broadcom BCM4908 pinmux driver" Why not tristate? > + depends on OF && (ARCH_BCM4908 || COMPILE_TEST) Is there really dependency to OF? > + select PINMUX > + select PINCONF > + select GENERIC_PINCONF > + select GENERIC_PINCTRL_GROUPS > + select GENERIC_PINMUX_FUNCTIONS > + default ARCH_BCM4908 > + help > + Say Y here to enable driver for BCM4908 family SoCs with integrated > + pin controller. With a module available it's good to mention its name here. ... > +/* > + * Copyright (C) 2021 Rafał Miłecki <rafal@milecki.pl> > + */ One line? ... > +#include <linux/err.h> > +#include <linux/io.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_device.h> No evidence of use of these headers. But missed mod_devicetable.h. > +#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? > +#include "../core.h" > +#include "../pinmux.h" ... > +#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. ... > +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? ... > + for (i = 0; i < group->num_pins; i++) { > + u32 lsb = 0; > + > + lsb |= group->pins[i].number; > + lsb |= group->pins[i].function << TEST_PORT_LSB_PINMUX_DATA_SHIFT; > + > + writel(0x0, bcm4908_pinctrl->base + TEST_PORT_BLOCK_DATA_MSB); > + writel(lsb, bcm4908_pinctrl->base + TEST_PORT_BLOCK_DATA_LSB); > + writel(TEST_PORT_CMD_LOAD_MUX_REG, bcm4908_pinctrl->base + TEST_PORT_COMMAND); > + } No serialization for IO, is it okay? ... > + bcm4908_pinctrl->base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(bcm4908_pinctrl->base)) { > + dev_err(dev, "Failed to map pinctrl regs\n"); > + return PTR_ERR(bcm4908_pinctrl->base); Besides of dev_err_probe(), why you duplicate message that already printed by core? > + } ... > + /* Set pinctrl properties */ > + Here and everywhere else, please drop redundant blank line. ... > + pins = devm_kcalloc(dev, BCM4908_NUM_PINS, > + sizeof(struct pinctrl_pin_desc), GFP_KERNEL); Please, use sizeof(*foo) form. Then put it on one line. > + if (!pins) > + return -ENOMEM; ... > + for (i = 0; i < BCM4908_NUM_PINS; i++) { > + pins[i].number = i; > + pins[i].name = devm_kasprintf(dev, GFP_KERNEL, "pin%d", i); > + if (!pins[i].name) > + return -ENOMEM; > + } Can you use the kasprintf_strarray() to make the style unified, please? ... > + bcm4908_pinctrl->pctldev = devm_pinctrl_register(dev, pctldesc, bcm4908_pinctrl); > + if (IS_ERR(bcm4908_pinctrl->pctldev)) { > + dev_err(dev, "Failed to register pinctrl\n"); > + return PTR_ERR(bcm4908_pinctrl->pctldev); return dev_err_probe(...); > + } ... > +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);
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. >> +#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
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.
diff --git a/Documentation/devicetree/bindings/pinctrl/brcm,bcm4908-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/brcm,bcm4908-pinctrl.yaml new file mode 100644 index 000000000000..175a992f15e1 --- /dev/null +++ b/Documentation/devicetree/bindings/pinctrl/brcm,bcm4908-pinctrl.yaml @@ -0,0 +1,72 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/pinctrl/brcm,bcm4908-pinctrl.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Broadcom BCM4908 pin controller + +maintainers: + - Rafał Miłecki <rafal@milecki.pl> + +description: + Binding for pin controller present on BCM4908 family SoCs. + +properties: + compatible: + const: brcm,bcm4908-pinctrl + + reg: + maxItems: 1 + +patternProperties: + '-pins$': + type: object + $ref: pinmux-node.yaml# + + properties: + function: + enum: [ led_0, led_1, led_2, led_3, led_4, led_5, led_6, led_7, led_8, + led_9, led_10, led_11, led_12, led_13, led_14, led_15, led_16, + led_17, led_18, led_19, led_20, led_21, led_22, led_23, led_24, + led_25, led_26, led_27, led_28, led_29, led_30, led_31, + hs_uart, i2c, i2s, nand_ctrl, nand_data, emmc_ctrl, usb0_pwr, + usb1_pwr ] + + groups: + minItems: 1 + maxItems: 2 + items: + enum: [ led_0_grp_a, led_1_grp_a, led_2_grp_a, led_3_grp_a, + led_4_grp_a, led_5_grp_a, led_6_grp_a, led_7_grp_a, + led_8_grp_a, led_9_grp_a, led_10_grp_a, led_10_grp_b, + led_11_grp_a, led_11_grp_b, led_12_grp_a, led_12_grp_b, + led_13_grp_a, led_13_grp_b, led_14_grp_a, led_15_grp_a, + led_16_grp_a, led_17_grp_a, led_18_grp_a, led_19_grp_a, + led_20_grp_a, led_21_grp_a, led_22_grp_a, led_23_grp_a, + led_24_grp_a, led_25_grp_a, led_26_grp_a, led_27_grp_a, + led_28_grp_a, led_29_grp_a, led_30_grp_a, led_31_grp_a, + led_31_grp_b, hs_uart_grp, i2c_grp_a, i2c_grp_b, i2s_grp, + nand_ctrl_grp, nand_data_grp, emmc_ctrl_grp, usb0_pwr_grp, + usb1_pwr_grp ] + +allOf: + - $ref: pinctrl.yaml# + +required: + - compatible + - reg + +unevaluatedProperties: false + +examples: + - | + pinctrl@ff800560 { + compatible = "brcm,bcm4908-pinctrl"; + reg = <0xff800560 0x10>; + + led_0-a-pins { + function = "led_0"; + groups = "led_0_grp_a"; + }; + }; diff --git a/MAINTAINERS b/MAINTAINERS index 52653c83de69..949cff4ee1e4 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -3666,6 +3666,13 @@ F: Documentation/devicetree/bindings/net/brcm,bcm4908-enet.yaml F: drivers/net/ethernet/broadcom/bcm4908_enet.* F: drivers/net/ethernet/broadcom/unimac.h +BROADCOM BCM4908 PINMUX DRIVER +M: Rafał Miłecki <rafal@milecki.pl> +M: bcm-kernel-feedback-list@broadcom.com +L: linux-gpio@vger.kernel.org +S: Maintained +F: Documentation/devicetree/bindings/pinctrl/brcm,bcm4908-pinctrl.yaml + BROADCOM BCM5301X ARM ARCHITECTURE M: Hauke Mehrtens <hauke@hauke-m.de> M: Rafał Miłecki <zajec5@gmail.com>