| Message ID | 20211124230439.17531-1-zajec5@gmail.com |
|---|---|
| Headers | show |
| Series | pinctrl: support platform (e.g. DT) stored pins, groups & functions | expand |
Hi, * Rafał Miłecki <zajec5@gmail.com> [211124 23:05]: > --- a/Documentation/devicetree/bindings/pinctrl/pinctrl.yaml > +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl.yaml > @@ -42,4 +42,44 @@ properties: > This property can be set either globally for the pin controller or in > child nodes for individual pin group control. > > + pins: > + type: object > + > + patternProperties: > + "^.*$": > + type: object > + > + properties: > + number: > + description: Pin number > + $ref: /schemas/types.yaml#/definitions/uint32 > + > + additionalProperties: false Please don't introduce Linux kernel internal numbering here. It's like bringing back the interrupt numbers again. Just make this into a proper hardware offset from the controller base, so a reg property. Sure in some cases the reg property is just an index depending on the controller, we don't really care from the binding point of view. We already have #pinctrl-cells, so plase do something like the four ximaginary examples below: #pinctrl-cells = <1>; ... pin@foo { reg = <0xf00 MUX_MODE0>; label = "foo_pin"; }; #pinctrl-cells = <2>; ... pin@foo { reg = <0xf00 PIN_INPUT_PULLUP MUX_MODE3>; }; #pinctrl-cells = <2>; ... pin@f00 { reg = <0xf00 DELAY_PS(0) DELAY_PS(0)>; }; #pinctrl-cells = <3>; ... pin@f00 { reg = <0xf00 MUX_MODE3 PULL_UP_STRENGTH(36) PULL_DOWN_STRENGTH(20)>; }; Then let's attempt to use just standard numbers and defines for the values where possible. Then a group of pins is just a list of the pin phandles in the devicetree. Regards, Tony
On Thu, Nov 25, 2021 at 11:49 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Thu, Nov 25, 2021 at 1:04 AM Rafał Miłecki <zajec5@gmail.com> wrote: > Have you thought about making ops structure (or ops inside existing > structure) and all above will be something like > > stuct ops *... = ...->ops; > > if (ops && ops->METHOD) > return ops->METHOD(...); > > return -ERRNO; Forgot to add that ops assignment should happen at the pin control enumeration or something like that. Quite early and quite globally.
On 25.11.2021 09:49, Tony Lindgren wrote: > * Rafał Miłecki <zajec5@gmail.com> [211124 23:05]: >> --- a/Documentation/devicetree/bindings/pinctrl/pinctrl.yaml >> +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl.yaml >> @@ -42,4 +42,44 @@ properties: >> This property can be set either globally for the pin controller or in >> child nodes for individual pin group control. >> >> + pins: >> + type: object >> + >> + patternProperties: >> + "^.*$": >> + type: object >> + >> + properties: >> + number: >> + description: Pin number >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + >> + additionalProperties: false > > Please don't introduce Linux kernel internal numbering here. It's > like bringing back the interrupt numbers again. This is a new bit to me and the reason why I got this binding that way. I had no idea pin numbering is system specific thing. I always thought pin numbers are present in every chip datasheets and that is just a part of hardware. Now I'm reading https://www.kernel.org/doc/Documentation/pinctrl.txt again it indeed seems to mention that numbering is handled in a way not related to specs: "I enumerated the pins from 0 in the upper left corner to 63 in the lower right corner.". Sorry for that, I hopefully understand your point correctly now. > Just make this into > a proper hardware offset from the controller base, so a reg property. > Sure in some cases the reg property is just an index depending on > the controller, we don't really care from the binding point of view. > > We already have #pinctrl-cells, so plase do something like the four > ximaginary examples below: > > #pinctrl-cells = <1>; > ... > pin@foo { > reg = <0xf00 MUX_MODE0>; > label = "foo_pin"; > }; > > > #pinctrl-cells = <2>; > ... > pin@foo { > reg = <0xf00 PIN_INPUT_PULLUP MUX_MODE3>; > }; > > > #pinctrl-cells = <2>; > ... > pin@f00 { > reg = <0xf00 DELAY_PS(0) DELAY_PS(0)>; > }; > > > #pinctrl-cells = <3>; > ... > pin@f00 { > reg = <0xf00 MUX_MODE3 PULL_UP_STRENGTH(36) PULL_DOWN_STRENGTH(20)>; > }; > > > Then let's attempt to use just standard numbers and defines for the > values where possible. Then a group of pins is just a list of the pin > phandles in the devicetree. I need to ask for help on understanding that reg = <...> syntax. (Why) do we need to put that extra info in a "reg" property? That seems like either: 1. Pin specific info or 2. Phandle arguments In the first case, instead of: pin@f00 { reg = <0xf00 MUX_MODE3 PULL_UP_STRENGTH(36) PULL_DOWN_STRENGTH(20)>; }; I'd rather use: pin@f00 { reg = <0xf00>; mux_mode3; pull_up_strength = <36>; pull_down_strength = <20>; }; In the second case, shouldn't that be something like: pins { bar: pin@f00 { reg = <0xf00>; #pinctrl-cells = <3>; }; }; groups { qux { pins = <&bar MUX_MODE3 PULL_UP_STRENGTH(36) PULL_DOWN_STRENGTH(20)>; } };
From: Rafał Miłecki <rafal@milecki.pl> Two weeks ago I sent [PATCH RFC] dt-bindings: pinctrl: support specifying pins https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20211110231436.8866-1-zajec5@gmail.com/ and week ago I sent [PATCH 0/5] pinctrl: allow storing pins, groups & functions in DT https://patchwork.ozlabs.org/project/linux-gpio/list/?series=272685 Initially I planned to allow putting some pinctrl hw details in DT and later that evolved into a slightly more generic API. Again: Please note it's about describing hardware elements and not actual programming way. It may be used with pinctrl-single.c one day but it's designed as a generic solution for data. Patches 1-5 are for linux-pinctrl.git. Patch 6 I found worth including as DT big example. It can go through Linus with Florian's Ack or I can send it to Florian later. Rafał Miłecki (6): dt-bindings: pinctrl: support specifying pins, groups & functions dt-bindings: pinctrl: brcm,ns-pinmux: extend example pinctrl: prepare API for reading pins, groups & functions pinctrl: support reading pins, groups & functions from DT pinctrl: bcm: pinctrl-ns: supoprt DT specified pins, groups & functions ARM: dts: BCM5301X: add pinctrl pins, groups & functions .../bindings/pinctrl/brcm,ns-pinmux.yaml | 24 +++- .../devicetree/bindings/pinctrl/pinctrl.yaml | 40 ++++++ arch/arm/boot/dts/bcm4709.dtsi | 67 +++++++++ arch/arm/boot/dts/bcm47094.dtsi | 11 +- arch/arm/boot/dts/bcm5301x.dtsi | 109 +++++++++++++++ drivers/pinctrl/bcm/pinctrl-ns.c | 90 ++++++++---- drivers/pinctrl/core.c | 18 +++ drivers/pinctrl/core.h | 4 + drivers/pinctrl/devicetree.c | 131 ++++++++++++++++++ drivers/pinctrl/devicetree.h | 29 ++++ drivers/pinctrl/pinmux.c | 10 ++ drivers/pinctrl/pinmux.h | 2 + 12 files changed, 494 insertions(+), 41 deletions(-)