Message ID | 1542701330-23466-2-git-send-email-andrei.stefanescu@microchip.com |
---|---|
State | New |
Headers | show |
Series | add SAMA5D2 PIOBU GPIO driver | expand |
Hello Rob, Could you please tell me if there are any improvements to be made to the patch? Best regards, Andrei On 20.11.2018 10:08, Andrei Stefanescu - M50506 wrote: > This patch describes the compatible and the device tree > bindings necessary for the SAMA5D2 PIOBU GPIO. > > Signed-off-by: Andrei Stefanescu <andrei.stefanescu@microchip.com> > --- > .../bindings/gpio/gpio-sama5d2-piobu.txt | 31 ++++++++++++++++++++++ > 1 file changed, 31 insertions(+) > create mode 100644 Documentation/devicetree/bindings/gpio/gpio-sama5d2-piobu.txt > > diff --git a/Documentation/devicetree/bindings/gpio/gpio-sama5d2-piobu.txt b/Documentation/devicetree/bindings/gpio/gpio-sama5d2-piobu.txt > new file mode 100644 > index 0000000..2e260e1 > --- /dev/null > +++ b/Documentation/devicetree/bindings/gpio/gpio-sama5d2-piobu.txt > @@ -0,0 +1,31 @@ > +GPIO controller for SAMA5D2 PIOBU pins. > + > +These pins have the property of not losing their voltage > +during Backup/Self-refresh mode. > + > +These bindings should be set to a node in the dtsi file. > + > +Required properties: > +- compatible: "syscon", "microchip,sama5d2-piobu" > +- #gpio-cells: There are 2. The pin number is the > + first, the second represents additional > + parameters such as GPIO_ACTIVE_HIGH/LOW. > +- gpio-controller: Marks the port as GPIO controller. > + > +Note that the driver uses syscon and should be the child of > +the syscon node. > + > +Example: > + > + secumod@fc040000 { > + compatible = "atmel,sama5d2-secumod", "syscon", "simple-mfd"; > + status = "okay"; > + reg = <0xfc040000 0x100>; > + > + pioBU: piobu { > + status = "okay"; > + compatible = "microchip,sama5d2-piobu"; > + gpio-controller; > + #gpio-cells = <2>; > + }; > + };
On Tue, Nov 20, 2018 at 08:08:36AM +0000, Andrei.Stefanescu@microchip.com wrote: > This patch describes the compatible and the device tree > bindings necessary for the SAMA5D2 PIOBU GPIO. > > Signed-off-by: Andrei Stefanescu <andrei.stefanescu@microchip.com> > --- > .../bindings/gpio/gpio-sama5d2-piobu.txt | 31 ++++++++++++++++++++++ > 1 file changed, 31 insertions(+) > create mode 100644 Documentation/devicetree/bindings/gpio/gpio-sama5d2-piobu.txt > > diff --git a/Documentation/devicetree/bindings/gpio/gpio-sama5d2-piobu.txt b/Documentation/devicetree/bindings/gpio/gpio-sama5d2-piobu.txt > new file mode 100644 > index 0000000..2e260e1 > --- /dev/null > +++ b/Documentation/devicetree/bindings/gpio/gpio-sama5d2-piobu.txt microchip,sama5d2-piobu.txt for the file name. > @@ -0,0 +1,31 @@ > +GPIO controller for SAMA5D2 PIOBU pins. > + > +These pins have the property of not losing their voltage > +during Backup/Self-refresh mode. > + > +These bindings should be set to a node in the dtsi file. > + > +Required properties: > +- compatible: "syscon", "microchip,sama5d2-piobu" syscon should be removed. > +- #gpio-cells: There are 2. The pin number is the > + first, the second represents additional > + parameters such as GPIO_ACTIVE_HIGH/LOW. > +- gpio-controller: Marks the port as GPIO controller. > + > +Note that the driver uses syscon and should be the child of > +the syscon node. child of the "atmel,sama5d2-secumod" node to be more specific. But why do you need a child node? The parent can be a gpio provider. What other nodes does this need? > + > +Example: > + > + secumod@fc040000 { > + compatible = "atmel,sama5d2-secumod", "syscon", "simple-mfd"; This is not documented as being a simple-mfd. > + status = "okay"; > + reg = <0xfc040000 0x100>; > + > + pioBU: piobu { gpio { Is there not a register range you can put here? > + status = "okay"; > + compatible = "microchip,sama5d2-piobu"; > + gpio-controller; > + #gpio-cells = <2>; > + }; > + }; > -- > 2.7.4 >
Hello Rob, Thank you for your feedback. I will add a bit of context regarding the secumod. The "atmel,sama5d2-secumod" compatible string is not used for loading a driver. It is used by atmel securam driver (drivers/misc/sram.c) which has access to secumod's registers via: syscon_regmap_lookup_by_compatible("atmel,sama5d2-secumod") So the secumod exports multiple hardware functions, eg: the securam, the PIOBU pins which can be used as GPIO pins. My initial patch had the "microchip,sama5d2-piobu" compatible appended to the secumod's compatible e.g.: secumod@fc040000 { compatible = "syscon", "microchip,sama5d2-piobu"; ... Taking into consideration Linus Walleij's advice I arrived to the current version. I thought this was a good idea because it separates the secumod node from the GPIO controller node. Please notice that securam node is already separated from secumod node (it is a separate node in the device tree). I have a few questions because I am not sure of the best approach: 1. Is it ok to have the GPIO controller as a child node? 2. I used simple-mfd because it was the only way I could think of in order to get the driver probed. 3. Should I add a register range? I thought that because the driver uses syscon it is not necessary to add the register range. Also, the register range would have been a subset of the secumod's register range.
On Wed, Dec 5, 2018 at 5:06 AM <Andrei.Stefanescu@microchip.com> wrote: > > Hello Rob, > > Thank you for your feedback. > > I will add a bit of context regarding the secumod. The > "atmel,sama5d2-secumod" > compatible string is not used for loading a driver. It is used by atmel > securam > driver (drivers/misc/sram.c) which has access to secumod's registers via: > > syscon_regmap_lookup_by_compatible("atmel,sama5d2-secumod") > > So the secumod exports multiple hardware functions, eg: the securam, the > PIOBU > pins which can be used as GPIO pins. Yes, I understand why you want to design it this way, but don't design your bindings around how 1 OS happens to currently work. > > My initial patch had the "microchip,sama5d2-piobu" compatible appended > to the > secumod's compatible e.g.: > > secumod@fc040000 { > compatible = "syscon", "microchip,sama5d2-piobu"; That doesn't look appended to me. The documentation says it should look like this: compatible = "atmel,sama5d2-secumod", "syscon"; > ... > > Taking into consideration Linus Walleij's advice I arrived to the current > version. I thought this was a good idea because it separates the secumod > node > from the GPIO controller node. Please notice that securam node is already > separated from secumod node (it is a separate node in the device tree). > > I have a few questions because I am not sure of the best approach: > > 1. Is it ok to have the GPIO controller as a child node? Is it a separate h/w block and do you have other child nodes that need their own resources in DT (interrupts, clocks, etc.)? If these aren't the case, then no you shouldn't. As it stands, you don't have any other child nodes, so no you shouldn't have a child node here. Just add to properties to the existing binding: secumod@fc040000 { compatible = "atmel,sama5d2-secumod", "syscon"; reg = <0xfc040000 0x100>; #gpio-cells = <2>; gpio-controller; }; Now perhaps you have 10 other functions in this block like pinctrl, clocks, phys, power domains, etc. Then child nodes would be warranted. But if that's the case, then please write a complete binding for the secumod block. > 2. I used simple-mfd because it was the only way I could think of in > order to > get the driver probed. Sounds like an OS problem that has little to do with the binding. In any case, if you change a binding which you have, then it needs to be documented. > 3. Should I add a register range? I thought that because the driver uses > syscon > it is not necessary to add the register range. Also, the register > range would > have been a subset of the secumod's register range. If it has one, then yes you should. If it doesn't then it is probably not a separate h/w block and shouldn't have a child node to begin with. Rob
diff --git a/Documentation/devicetree/bindings/gpio/gpio-sama5d2-piobu.txt b/Documentation/devicetree/bindings/gpio/gpio-sama5d2-piobu.txt new file mode 100644 index 0000000..2e260e1 --- /dev/null +++ b/Documentation/devicetree/bindings/gpio/gpio-sama5d2-piobu.txt @@ -0,0 +1,31 @@ +GPIO controller for SAMA5D2 PIOBU pins. + +These pins have the property of not losing their voltage +during Backup/Self-refresh mode. + +These bindings should be set to a node in the dtsi file. + +Required properties: +- compatible: "syscon", "microchip,sama5d2-piobu" +- #gpio-cells: There are 2. The pin number is the + first, the second represents additional + parameters such as GPIO_ACTIVE_HIGH/LOW. +- gpio-controller: Marks the port as GPIO controller. + +Note that the driver uses syscon and should be the child of +the syscon node. + +Example: + + secumod@fc040000 { + compatible = "atmel,sama5d2-secumod", "syscon", "simple-mfd"; + status = "okay"; + reg = <0xfc040000 0x100>; + + pioBU: piobu { + status = "okay"; + compatible = "microchip,sama5d2-piobu"; + gpio-controller; + #gpio-cells = <2>; + }; + };
This patch describes the compatible and the device tree bindings necessary for the SAMA5D2 PIOBU GPIO. Signed-off-by: Andrei Stefanescu <andrei.stefanescu@microchip.com> --- .../bindings/gpio/gpio-sama5d2-piobu.txt | 31 ++++++++++++++++++++++ 1 file changed, 31 insertions(+) create mode 100644 Documentation/devicetree/bindings/gpio/gpio-sama5d2-piobu.txt