| Message ID | 20201008130515.2385825-1-lars.povlsen@microchip.com |
|---|---|
| Headers | show |
| Series | pinctrl: Adding support for Microchip/Microsemi serial GPIO controller | expand |
Hi Lars! This is overall looking fine. Except for the 3 cell business. I just can't wrap my head around why that is needed. On Thu, Oct 8, 2020 at 3:05 PM Lars Povlsen <lars.povlsen@microchip.com> wrote: > + '#gpio-cells': > + const: 3 So at the very least needs a description making it crystal clear why each cell is needed, and used for since the standard bindings are not used. + sgpio_in2: gpio@0 { + reg = <0>; + compatible = "microchip,sparx5-sgpio-bank"; + gpio-controller; + #gpio-cells = <3>; + ngpios = <96>; + }; So here reg = 0 and the out port has reg 1. Isn't that what you also put in the second cell of the GPIO phandle? Then why? The driver can very well just parse its own reg property and fill that in. When you obtain a phandle like that: gpios = <&sgpio_in2 11 0 GPIO_OUT_LOW>; Isn't that 0 just duplicating the "reg"? Just parse reg when you set up your driver state and put it as variable in the state container for your driver state for this particular gpio_chip. No need to get it from the phandle. Yours, Linus Walleij
Linus Walleij writes: > Hi Lars! > > This is overall looking fine. Except for the 3 cell business. I just can't > wrap my head around why that is needed. > > On Thu, Oct 8, 2020 at 3:05 PM Lars Povlsen <lars.povlsen@microchip.com> wrote: > >> + '#gpio-cells': >> + const: 3 > > So at the very least needs a description making it crystal clear why each > cell is needed, and used for since the standard bindings are not used. > > + sgpio_in2: gpio@0 { > + reg = <0>; > + compatible = "microchip,sparx5-sgpio-bank"; > + gpio-controller; > + #gpio-cells = <3>; > + ngpios = <96>; > + }; > > So here reg = 0 and the out port has reg 1. Isn't that what you also put > in the second cell of the GPIO phandle? Then why? The driver > can very well just parse its own reg property and fill that in. Linus, NO! The second cell is the second dimension - NOT the direction. As I wrote previously, the direction is now inherent from the handle, ie. the "reg" value of the handle. The hardware describe a "port" and a "bit index" addressing, where the second cell in gpios = <&sgpio_in2 11 0 GPIO_OUT_LOW>; is the "bit index" - not the "reg" from the phandle. In the example above, note ngpios = <96>; As the "port" is [0; 31], this defines "bit index" to be [0; 2], so the (input) GPIO cells will be: p0b0, p0b1, p0b2 ... p31b0, p31b1, p31b2 being identical to <&sgpio_inX 0 0 GPIO_OUT_LOW> <&sgpio_inX 0 1 GPIO_OUT_LOW> <&sgpio_inX 0 2 GPIO_OUT_LOW> ... <&sgpio_inX 31 0 GPIO_OUT_LOW> <&sgpio_inX 31 1 GPIO_OUT_LOW> <&sgpio_inX 31 2 GPIO_OUT_LOW> ('X' being the SGPIO controller instance). So no, there *really* is a need for a 3-cell GPIO specifier (or whatever its called). Hope this is clearer now... ---Lars > > When you obtain a phandle like that: > > gpios = <&sgpio_in2 11 0 GPIO_OUT_LOW>; > > Isn't that 0 just duplicating the "reg"? Just parse reg when you set up > your driver state and put it as variable in the state container for your > driver state for this particular gpio_chip. No need to get it from > the phandle. > > Yours, > Linus Walleij
On Fri, Oct 9, 2020 at 12:00 PM Lars Povlsen <lars.povlsen@microchip.com> wrote: > > So here reg = 0 and the out port has reg 1. Isn't that what you also put > > in the second cell of the GPIO phandle? Then why? The driver > > can very well just parse its own reg property and fill that in. > > NO! The second cell is the second dimension - NOT the direction. As I > wrote previously, the direction is now inherent from the handle, ie. the > "reg" value of the handle. OK I get it ... I think :) > The hardware describe a "port" and a "bit index" addressing, where the > second cell in > > gpios = <&sgpio_in2 11 0 GPIO_OUT_LOW>; > > is the "bit index" - not the "reg" from the phandle. As long as the bindings specify exactly what is meant by bit index and the tupe (port, bit_index) is what uniquely addresses a certain GPIO line then it is fine I suppose. > In the example above, note > > ngpios = <96>; > > As the "port" is [0; 31], this defines "bit index" to be [0; 2], so the > (input) GPIO cells will be: > > p0b0, p0b1, p0b2 > ... > p31b0, p31b1, p31b2 > > being identical to > > <&sgpio_inX 0 0 GPIO_OUT_LOW> > <&sgpio_inX 0 1 GPIO_OUT_LOW> > <&sgpio_inX 0 2 GPIO_OUT_LOW> > ... > <&sgpio_inX 31 0 GPIO_OUT_LOW> > <&sgpio_inX 31 1 GPIO_OUT_LOW> > <&sgpio_inX 31 2 GPIO_OUT_LOW> > > ('X' being the SGPIO controller instance). So 32 possible ports with 3 possible bit indexes on each? This constraint should go into the bindings as well so it becomes impossible to put in illegal port numbers or bit indices. (Use the YAML min/max constraints, I suppose?) > So no, there *really* is a need for a 3-cell GPIO specifier (or whatever > its called). If that is the natural way to address the hardware lines and what is used in the documentation then it's fine, it's just so unorthodox that I have to push back on it a bit you know. Yours, Linus Walleij
Linus Walleij writes: > On Fri, Oct 9, 2020 at 12:00 PM Lars Povlsen <lars.povlsen@microchip.com> wrote: > >> > So here reg = 0 and the out port has reg 1. Isn't that what you also put >> > in the second cell of the GPIO phandle? Then why? The driver >> > can very well just parse its own reg property and fill that in. >> >> NO! The second cell is the second dimension - NOT the direction. As I >> wrote previously, the direction is now inherent from the handle, ie. the >> "reg" value of the handle. > > OK I get it ... I think :) Great! > >> The hardware describe a "port" and a "bit index" addressing, where the >> second cell in >> >> gpios = <&sgpio_in2 11 0 GPIO_OUT_LOW>; >> >> is the "bit index" - not the "reg" from the phandle. > > As long as the bindings specify exactly what is meant by bit index > and the tupe (port, bit_index) is what uniquely addresses a certain > GPIO line then it is fine I suppose. > Yes, that is confirmed. >> In the example above, note >> >> ngpios = <96>; >> >> As the "port" is [0; 31], this defines "bit index" to be [0; 2], so the >> (input) GPIO cells will be: >> >> p0b0, p0b1, p0b2 >> ... >> p31b0, p31b1, p31b2 >> >> being identical to >> >> <&sgpio_inX 0 0 GPIO_OUT_LOW> >> <&sgpio_inX 0 1 GPIO_OUT_LOW> >> <&sgpio_inX 0 2 GPIO_OUT_LOW> >> ... >> <&sgpio_inX 31 0 GPIO_OUT_LOW> >> <&sgpio_inX 31 1 GPIO_OUT_LOW> >> <&sgpio_inX 31 2 GPIO_OUT_LOW> >> >> ('X' being the SGPIO controller instance). > > So 32 possible ports with 3 possible bit indexes on each? > This constraint should go into the bindings as well so it becomes > impossible to put in illegal port numbers or bit indices. > > (Use the YAML min/max constraints, I suppose?) > Yes, I will to see if constraints in the GPIO args is possible. >> So no, there *really* is a need for a 3-cell GPIO specifier (or whatever >> its called). > > If that is the natural way to address the hardware lines > and what is used in the documentation then it's fine, it's just so > unorthodox that I have to push back on it a bit you know. > Yes, this piece of hw is certainly not a stock GPIO controller, so that was kinda expected. But I think we ended up with an abstraction that fits as good as possible. I will send a new (last?) revision that includes the suggestions from Rob tomorrow. Thank you for your time and comments (also Rob!) ---Lars
The series add support for the serial GPIO controller used by Microchip Sparx5, as well as (MSCC) ocelot/jaguar2 SoCs. v5 changes (driver comments from Linus): - Collect bank data in sgpio_bank struct - Add is_input boolean to sgpio_bank struct - Use single-bit bitmasks in sgpio_output_set() and sgpio_output_get() - Eliminate superfluous struct pinctrl_dev *pctl_dev in bank data - Fix wrong ngpio consistency check v4 changes (binding comments from Rob): - microchip,sgpio-port-ranges changed to uint32-matrix so tuples can be represented properly. - gpio controller node name changed to "gpio@[0-1]" - whitespace fixes - DT files updated as per schema changes v3 changes: - Renamed all usage of "mchp" abbrevation with "microchip". - Split the in/output directions into (two) separate banks. - Eliminated the bindings include file (from above) - Changed SPDX license to "GPL-2.0-or-later" - Change -ENOTSUPP to -EOPNOTSUPP - Minor type/symbol naming changes v2 changes: - Adds both in and output modes. - Use direct adressing of the individual banks (#gpio-cells = <4>), also osoleting need for addressing macros in bindings include file. - Property 'microchip,sgpio-ports' (uint32, bitmask) replaced by proper range set (array of [start,end]) 'microchip,sgpio-port-ranges'. - Fixes whitespace issues in Kconfig file Lars Povlsen (3): dt-bindings: pinctrl: Add bindings for pinctrl-microchip-sgpio driver pinctrl: pinctrl-microchip-sgpio: Add pinctrl driver for Microsemi Serial GPIO arm64: dts: sparx5: Add SGPIO devices .../pinctrl/microchip,sparx5-sgpio.yaml | 140 ++++ MAINTAINERS | 1 + arch/arm64/boot/dts/microchip/sparx5.dtsi | 91 +++ .../boot/dts/microchip/sparx5_pcb125.dts | 5 + .../dts/microchip/sparx5_pcb134_board.dtsi | 258 +++++++ .../dts/microchip/sparx5_pcb135_board.dtsi | 55 ++ drivers/pinctrl/Kconfig | 18 + drivers/pinctrl/Makefile | 1 + drivers/pinctrl/pinctrl-microchip-sgpio.c | 665 ++++++++++++++++++ 9 files changed, 1234 insertions(+) create mode 100644 Documentation/devicetree/bindings/pinctrl/microchip,sparx5-sgpio.yaml create mode 100644 drivers/pinctrl/pinctrl-microchip-sgpio.c -- 2.25.1