| Message ID | 1838530.usypLq5u4X@debian64 |
|---|---|
| State | New |
| Headers | show |
On Wed, Apr 27, 2016 at 5:15 AM, Christian Lamparter <chunkeey@googlemail.com> wrote: > On Tuesday, April 26, 2016 09:06:14 PM Rob Herring wrote: >> On Tue, Apr 26, 2016 at 5:51 PM, Christian Lamparter >> <chunkeey@googlemail.com> wrote: >> > From: Álvaro Fernández Rojas <noltari@gmail.com> >> > >> > This patch adds the device tree bindings for the basic-mmio-gpio. >> > The basic-mmio-gpio is already part of a the GPIO generic library >> > and shares its compatible with the platform device. >> >> These things always start out "simple", "basic" or "generic". Then >> people extend them a property or 2 at a time until they are no longer >> basic. Make a bunch of GPIO drivers use a generic driver then maybe >> I'll be convinced this is a good idea. > Well, I mentioned brcm63xx (MIPS) and MyBook Live (PPC). But OK, if you > want me to look at the existing code in the kernel. I found the following > candidates. What I meant was convert the drivers, not the dts files to a generic driver. There is no benefit here from a dts perspective. It is driver consolidation that would make all this worthwhile. > 1. MOXA ART: "moxa,moxart-gpio" > http://lxr.free-electrons.com/source/drivers/gpio/gpio-moxart.c > This driver is used by the: > arch/arm/boot/dts/moxart.dtsi > > --- > diff --git a/arch/arm/boot/dts/moxart.dtsi b/arch/arm/boot/dts/moxart.dtsi > index 1fd27ed..26707d0 100644 > --- a/arch/arm/boot/dts/moxart.dtsi > +++ b/arch/arm/boot/dts/moxart.dtsi > @@ -66,8 +66,11 @@ > gpio: gpio@98700000 { > gpio-controller; > #gpio-cells = <2>; > - compatible = "moxa,moxart-gpio"; > - reg = <0x98700000 0xC>; > + compatible = "basic-mmio-gpio"; > + reg = <0x98700000 0x4 > + 0x98700004 0x4 > + 0x98700008 0x4>; > + reg-names = "set dat dirout"; This is not a compatible change. You can't remove the old drivers as they are needed to work with old dtbs, so there is no gain. You would need to match on existing compatibles such as moxa,moxart-gpio and provide a match data struct that has all the info you are adding here (e.g. data register offset). Then additionally you could add "basic-mmio-gpio" (I would drop "basic" part) and the additional data associated with it. But it has to be new properties, not changing properties. Changing the reg values doesn't work. > 2. GE FPGA based GPIO: "ge,imp3a-gpio", "gef,sbc310-gpio", "gef,sbc610-gpio" > 3. CLPS711X GPIO driver: "cirrus,clps711x-gpio" > 4. GPIO driver for the TS-4800 board: "technologic,ts4800-gpio" > > What do you think. There's a total of six devices that can make use > of the "basic-mmio-gpio". If this gives me your go-ahead, I'll include > them in the series as proper patches. Those are a good start. My other worry about this is next someone will want to add interrupt registers which is probably a majority of GPIO controllers. Of course, I'm sure Linus has an opinion on all of this. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 28, 2016 at 12:08 AM, Rob Herring <robh+dt@kernel.org> wrote: > On Wed, Apr 27, 2016 at 5:15 AM, Christian Lamparter > <chunkeey@googlemail.com> wrote: >> On Tuesday, April 26, 2016 09:06:14 PM Rob Herring wrote: >>> On Tue, Apr 26, 2016 at 5:51 PM, Christian Lamparter >>> <chunkeey@googlemail.com> wrote: >>> > From: Álvaro Fernández Rojas <noltari@gmail.com> >>> > >>> > This patch adds the device tree bindings for the basic-mmio-gpio. >>> > The basic-mmio-gpio is already part of a the GPIO generic library >>> > and shares its compatible with the platform device. >>> >>> These things always start out "simple", "basic" or "generic". Then >>> people extend them a property or 2 at a time until they are no longer >>> basic. Make a bunch of GPIO drivers use a generic driver then maybe >>> I'll be convinced this is a good idea. >> Well, I mentioned brcm63xx (MIPS) and MyBook Live (PPC). But OK, if you >> want me to look at the existing code in the kernel. I found the following >> candidates. > > What I meant was convert the drivers, not the dts files to a generic > driver. There is no benefit here from a dts perspective. It is driver > consolidation that would make all this worthwhile. > >> 1. MOXA ART: "moxa,moxart-gpio" >> http://lxr.free-electrons.com/source/drivers/gpio/gpio-moxart.c >> This driver is used by the: >> arch/arm/boot/dts/moxart.dtsi >> >> --- >> diff --git a/arch/arm/boot/dts/moxart.dtsi b/arch/arm/boot/dts/moxart.dtsi >> index 1fd27ed..26707d0 100644 >> --- a/arch/arm/boot/dts/moxart.dtsi >> +++ b/arch/arm/boot/dts/moxart.dtsi >> @@ -66,8 +66,11 @@ >> gpio: gpio@98700000 { >> gpio-controller; >> #gpio-cells = <2>; >> - compatible = "moxa,moxart-gpio"; >> - reg = <0x98700000 0xC>; >> + compatible = "basic-mmio-gpio"; >> + reg = <0x98700000 0x4 >> + 0x98700004 0x4 >> + 0x98700008 0x4>; >> + reg-names = "set dat dirout"; > > This is not a compatible change. You can't remove the old drivers as > they are needed to work with old dtbs, so there is no gain. > > You would need to match on existing compatibles such as > moxa,moxart-gpio and provide a match data struct that has all the info > you are adding here (e.g. data register offset). Then additionally you > could add "basic-mmio-gpio" (I would drop "basic" part) and the > additional data associated with it. But it has to be new properties, > not changing properties. Changing the reg values doesn't work. > > >> 2. GE FPGA based GPIO: "ge,imp3a-gpio", "gef,sbc310-gpio", "gef,sbc610-gpio" >> 3. CLPS711X GPIO driver: "cirrus,clps711x-gpio" >> 4. GPIO driver for the TS-4800 board: "technologic,ts4800-gpio" >> >> What do you think. There's a total of six devices that can make use >> of the "basic-mmio-gpio". If this gives me your go-ahead, I'll include >> them in the series as proper patches. > > Those are a good start. My other worry about this is next someone will > want to add interrupt registers which is probably a majority of GPIO > controllers. > > Of course, I'm sure Linus has an opinion on all of this. I'm not Linus, but after a quick look at the drivers mentioned by Christian I see little room for improvement (but still some - see below). Like Rob mentioned we want the DT's compatible property to describe the hardware as precisely as possible. "basic-mmio-gpio" is not precise and not backward-compatible. The examples listed by Christian are already all using the generic MMIO driver and are thus already quite short. However, they all repeat the same basic sequence to initialize the generic IOMMU driver, and maybe this could be consolidated in a single file, with each matching chip having the arguments passed to bgpio_init() encoded as data (a bit like the simple-panel driver does - see the platform_of_match member of drivers/gpu/drm/panel/panel-simple.c) This would reduce the amount of code that basically does the same thing, and remove a few files from drivers/gpio. Even there I see that gpio-clps711x.c has some specifics compared to the other drivers, but maybe Christian can iron them out. So without touching the DT I think consolidating the bgpio users in a fashion similar to what simple-panel does would be worthwhile. -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday, April 28, 2016 03:20:39 PM Alexandre Courbot wrote: > On Thu, Apr 28, 2016 at 12:08 AM, Rob Herring <robh+dt@kernel.org> wrote: > > On Wed, Apr 27, 2016 at 5:15 AM, Christian Lamparter > > What I meant was convert the drivers, not the dts files to a generic > > driver. There is no benefit here from a dts perspective. It is driver > > consolidation that would make all this worthwhile. > > [...] > > My other worry about this is next someone will want to add interrupt > > registers which is probably a majority of GPIO controllers. > > > > Of course, I'm sure Linus has an opinion on all of this. > > I'm not Linus, but after a quick look at the drivers mentioned by > Christian I see little room for improvement (but still some - see > below). Like Rob mentioned we want the DT's compatible property to > describe the hardware as precisely as possible. "basic-mmio-gpio" is > not precise and not backward-compatible. The examples listed by > Christian are already all using the generic MMIO driver and are thus > already quite short. > [...] > So without touching the DT I think consolidating the bgpio users in a > fashion similar to what simple-panel does would be worthwhile. I'm getting ready to update my series with a patch like that. I would like to consolidate the gpio drivers into a single file. But for the v4 series at least the parsers are still residing in their original files (I hoped this would make the transition from the _probe to the parser less painless but the diff came out pretty much unreadable). I think this would be just a matter of what's preferred. -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm/boot/dts/moxart.dtsi b/arch/arm/boot/dts/moxart.dtsi index 1fd27ed..26707d0 100644 --- a/arch/arm/boot/dts/moxart.dtsi +++ b/arch/arm/boot/dts/moxart.dtsi @@ -66,8 +66,11 @@ gpio: gpio@98700000 { gpio-controller; #gpio-cells = <2>; - compatible = "moxa,moxart-gpio"; - reg = <0x98700000 0xC>; + compatible = "basic-mmio-gpio"; + reg = <0x98700000 0x4 + 0x98700004 0x4 + 0x98700008 0x4>; + reg-names = "set dat dirout"; }; rtc: rtc { --- 2. GE FPGA based GPIO: "ge,imp3a-gpio", "gef,sbc310-gpio", "gef,sbc610-gpio" http://lxr.free-electrons.com/source/drivers/gpio/gpio-ge.c This driver is used by the: arch/powerpc/boot/dts/fsl/ge_imp3a.dts arch/powerpc/boot/dts/fsl/gef_ppc9a.dts arch/powerpc/boot/dts/fsl/gef_sbc610.dts arch/powerpc/boot/dts/fsl/gef_sbc310.dts --- diff --git a/arch/powerpc/boot/dts/fsl/gef_ppc9a.dts b/arch/powerpc/boot/dts/fsl/gef_ppc9a.dts index 0424fc2..20dd084 100644 --- a/arch/powerpc/boot/dts/fsl/gef_ppc9a.dts +++ b/arch/powerpc/boot/dts/fsl/gef_ppc9a.dts @@ -112,9 +112,14 @@ }; gef_gpio: gpio@7,14000 { #gpio-cells = <2>; - compatible = "gef,ppc9a-gpio", "gef,sbc610-gpio"; - reg = <0x7 0x14000 0x24>; + compatible = "basic-mmio-gpio"; + reg = <0x7 0x14000 0x4 + 0x7 0x14004 0x4 + 0x7 0x14008 0x4>; + reg-names = "dirout dat set"; + ngpio = <19>; gpio-controller; + big-endian; }; }; diff --git a/arch/powerpc/boot/dts/fsl/ge_imp3a.dts b/arch/powerpc/boot/dts/fsl/ge_imp3a.dts index a2bb47f..ac5855a 100644 --- a/arch/powerpc/boot/dts/fsl/ge_imp3a.dts +++ b/arch/powerpc/boot/dts/fsl/ge_imp3a.dts @@ -93,9 +93,13 @@ gef_gpio: gpio@4,400 { #gpio-cells = <2>; - compatible = "ge,imp3a-gpio"; - reg = <0x4 0x400 0x24>; + compatible = "basic-mmio-gpio"; + reg = <0x4 0x400 0x02 + 0x4 0x404 0x02 + 0x4 0x408 0x02>; + reg-names = "dirin dat set"; gpio-controller; + big-endian; }; wdt@4,800 { diff --git a/arch/powerpc/boot/dts/fsl/gef_sbc310.dts b/arch/powerpc/boot/dts/fsl/gef_sbc310.dts index 84b3d38..f09528f 100644 --- a/arch/powerpc/boot/dts/fsl/gef_sbc310.dts +++ b/arch/powerpc/boot/dts/fsl/gef_sbc310.dts @@ -113,9 +113,14 @@ }; gef_gpio: gpio@4,8000 { #gpio-cells = <2>; - compatible = "gef,sbc310-gpio"; - reg = <0x4 0x8000 0x24>; + compatible = "basic-mmio-gpio"; + reg = <0x4 0x8000 0x4 + 0x4 0x8004 0x4 + 0x4 0x8008 0x4>; + reg-names = "dirout dat set"; + ngpio = <6>; gpio-controller; + big-endian; }; }; diff --git a/arch/powerpc/boot/dts/fsl/gef_sbc610.dts b/arch/powerpc/boot/dts/fsl/gef_sbc610.dts index 974446a..a9d740a 100644 --- a/arch/powerpc/boot/dts/fsl/gef_sbc610.dts +++ b/arch/powerpc/boot/dts/fsl/gef_sbc610.dts @@ -110,9 +110,14 @@ }; gef_gpio: gpio@7,14000 { #gpio-cells = <2>; - compatible = "gef,sbc610-gpio"; - reg = <0x7 0x14000 0x24>; + compatible = "basic-mmio-gpio"; + reg = <0x7 0x14000 0x4 + 0x7 0x14004 0x4 + 0x7 0x14008 0x4>; + reg-names = "dirout dat set"; + ngpio = <19>; gpio-controller; + big-endian; }; }; ---