by Álvaro Fernández Rojas

[permalink] [raw]

Subject: Re: [PATCH v7 05/22] dt-bindings: add BCM6328 pincontroller binding documentation

Hi Rob,

> El 16 mar 2021, a las 21:59, Rob Herring <[email protected]> escribió:
>
> On Mon, Mar 15, 2021 at 12:41:57PM +0100, Álvaro Fernández Rojas wrote:
>> Add binding documentation for the pincontrol core found in BCM6328 SoCs.
>>
>> Co-developed-by: Jonas Gorski <[email protected]>
>> Signed-off-by: Jonas Gorski <[email protected]>
>> Signed-off-by: Álvaro Fernández Rojas <[email protected]>
>> ---
>> v7: add changes suggested by Rob Herring
>> v6: add changes suggested by Rob Herring
>> v5: change Documentation to dt-bindings in commit title
>> v4: no changes
>> v3: add new gpio node
>> v2: remove interrupts
>>
>> .../pinctrl/brcm,bcm6328-pinctrl.yaml | 132 ++++++++++++++++++
>> 1 file changed, 132 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml
>> new file mode 100644
>> index 000000000000..e1ecdc578f32
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml
>> @@ -0,0 +1,132 @@
>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/pinctrl/brcm,bcm6328-pinctrl.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Broadcom BCM6328 pin controller
>> +
>> +maintainers:
>> + - Álvaro Fernández Rojas <[email protected]>
>> + - Jonas Gorski <[email protected]>
>> +
>> +description: |+
>> + The pin controller node should be the child of a syscon node.
>> +
>> + Refer to the the bindings described in
>> + Documentation/devicetree/bindings/mfd/syscon.yaml
>> +
>> +properties:
>> + compatible:
>> + const: brcm,bcm6328-pinctrl
>> +
>> + reg:
>> + maxItems: 1
>> +
>> +patternProperties:
>> + '^.*-pins$':
>
> '-pins$' is equivalent.
>
>> + if:
>> + type: object
>> + then:
>
> You don't need this if/then. This should be:
>
> '-pins$':
> type: object
> $ref: pinmux-node.yaml#
>
> additionalProperties: false
> properties:
> ...

If I add "additionalProperties: false" I get the following error:
/home/noltari/workspace/linux/Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.example.dt.yaml: pinctrl@18: serial_led-pins: 'serial_led_clk-pins', 'serial_led_data-pins' do not match any of the regexes: 'pinctrl-[0-9]+'
From schema: /home/noltari/workspace/linux/Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml

>
>> + properties:
>> + function:
>> + $ref: "pinmux-node.yaml#/properties/function"
>
> Drop this as you need the $ref up a level.
>
>> + enum: [ serial_led_data, serial_led_clk, inet_act_led, pcie_clkreq,
>> + led, ephy0_act_led, ephy1_act_led, ephy2_act_led,
>> + ephy3_act_led, hsspi_cs1, usb_device_port, usb_host_port ]
>> +
>> + pins:
>> + $ref: "pinmux-node.yaml#/properties/pins"
>> + enum: [ gpio6, gpio7, gpio11, gpio16, gpio17, gpio18, gpio19,
>> + gpio20, gpio25, gpio26, gpio27, gpio28, hsspi_cs1,
>> + usb_port1 ]
>> +
>> +required:
>> + - compatible
>> + - reg
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> + - |
>> + pinctrl@18 {
>> + compatible = "brcm,bcm6328-pinctrl";
>> + reg = <0x18 0x10>;
>> +
>> + pinctrl_serial_led: serial_led-pins {
>> + pinctrl_serial_led_data: serial_led_data-pins {
>> + function = "serial_led_data";
>> + pins = "gpio6";
>> + };
>> +
>> + pinctrl_serial_led_clk: serial_led_clk-pins {
>> + function = "serial_led_clk";
>> + pins = "gpio7";
>> + };
>> + };
>> +
>> + pinctrl_inet_act_led: inet_act_led-pins {
>> + function = "inet_act_led";
>> + pins = "gpio11";
>> + };
>> +
>> + pinctrl_pcie_clkreq: pcie_clkreq-pins {
>> + function = "pcie_clkreq";
>> + pins = "gpio16";
>> + };
>> +
>> + pinctrl_ephy0_spd_led: ephy0_spd_led-pins {
>> + function = "led";
>> + pins = "gpio17";
>> + };
>> +
>> + pinctrl_ephy1_spd_led: ephy1_spd_led-pins {
>> + function = "led";
>> + pins = "gpio18";
>> + };
>> +
>> + pinctrl_ephy2_spd_led: ephy2_spd_led-pins {
>> + function = "led";
>> + pins = "gpio19";
>> + };
>> +
>> + pinctrl_ephy3_spd_led: ephy3_spd_led-pins {
>> + function = "led";
>> + pins = "gpio20";
>> + };
>> +
>> + pinctrl_ephy0_act_led: ephy0_act_led-pins {
>> + function = "ephy0_act_led";
>> + pins = "gpio25";
>> + };
>> +
>> + pinctrl_ephy1_act_led: ephy1_act_led-pins {
>> + function = "ephy1_act_led";
>> + pins = "gpio26";
>> + };
>> +
>> + pinctrl_ephy2_act_led: ephy2_act_led-pins {
>> + function = "ephy2_act_led";
>> + pins = "gpio27";
>> + };
>> +
>> + pinctrl_ephy3_act_led: ephy3_act_led-pins {
>> + function = "ephy3_act_led";
>> + pins = "gpio28";
>> + };
>> +
>> + pinctrl_hsspi_cs1: hsspi_cs1-pins {
>> + function = "hsspi_cs1";
>> + pins = "hsspi_cs1";
>> + };
>> +
>> + pinctrl_usb_port1_device: usb_port1_device-pins {
>> + function = "usb_device_port";
>> + pins = "usb_port1";
>> + };
>> +
>> + pinctrl_usb_port1_host: usb_port1_host-pins {
>> + function = "usb_host_port";
>> + pins = "usb_port1";
>> + };
>> + };
>> --
>> 2.20.1

2021-03-20 11:39:51

by Linus Walleij

[permalink] [raw]

Subject: Re: [PATCH v7 00/22] pinctrl: add BCM63XX pincontrol support

On Wed, Mar 17, 2021 at 12:20 PM Álvaro Fernández Rojas
<[email protected]> wrote:

> I appreciate that, but I’m having a hard time in understanding what
> Rob wants and since there are no examples available on most of the
> stuff he’s requesting this is really frustrating...

I am sorry that the situation can be stressful.

This is not Rob's fault, at least it's also mine.

The real problem we have is lack of hardware people
reviewing hardware descriptions, to put it simple.
As reviewers we get a bit confused, then we try to make
a mind map of the hardware as most driver developers do,
but as we are not chip designers we will make
mistakes and get confused and there will be a bit
of back-and-forth and inconsistencies.

The bindings have very high ambitions (to describe all
hardware) but it's a bit like food: the less you know
about how it's produced, the better the taste.
In fact it is a best effort and involves a bit of guesswork
and group effort and you are part of the group effort
now :)

Yours,
Linus Walleij