| Message ID | 83915224c24e43224272b1bf570cddb9545279a6.1309840042.git.nsekhar@ti.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On 05/07/11 15:10, Sekhar Nori wrote: > Some GPIO controllers have an enable register > which needs to be written to before a GPIO > can be used. > > Add support for enabling the GPIO. At this > time inverted logic for enabling the GPIO > is not supported. This can be done by adding > a disable register as and when a controller > with this comes along. > > Signed-off-by: Sekhar Nori<nsekhar@ti.com> > --- > <snip> > static int bgpio_setup_io(struct bgpio_chip *bgc, > void __iomem *dat, > @@ -369,6 +401,7 @@ int __devinit bgpio_init(struct bgpio_chip *bgc, > void __iomem *clr, > void __iomem *dirout, > void __iomem *dirin, > + void __iomem *en, > bool big_endian) The arguments to this function are getting a bit unwieldy :-). Maybe we need to introduce something like: struct bgpio_chip_info { struct device *dev; unsigned long sz; void __iomem *dat; void __iomem *set; void __iomem *clr; void __iomem *dirout; void __iomem *dirin; void __iomem *en; bool big_endian; }; and pass that to bgpio_init instead. It would have the added benefits of making the drivers more readable and that bgpio_chip_info structs in the drivers can probably be marked __initdata also. Since you are already having to touch all of the call sites for bgpio_init this could be done as a separate patch along with this series. ~Ryan
Hi Ryan, On Tue, Jul 05, 2011 at 11:45:44, Ryan Mallon wrote: > On 05/07/11 15:10, Sekhar Nori wrote: > > Some GPIO controllers have an enable register > > which needs to be written to before a GPIO > > can be used. > > > > Add support for enabling the GPIO. At this > > time inverted logic for enabling the GPIO > > is not supported. This can be done by adding > > a disable register as and when a controller > > with this comes along. > > > > Signed-off-by: Sekhar Nori<nsekhar@ti.com> > > --- > > > <snip> > > > static int bgpio_setup_io(struct bgpio_chip *bgc, > > void __iomem *dat, > > @@ -369,6 +401,7 @@ int __devinit bgpio_init(struct bgpio_chip *bgc, > > void __iomem *clr, > > void __iomem *dirout, > > void __iomem *dirin, > > + void __iomem *en, > > bool big_endian) > > The arguments to this function are getting a bit unwieldy :-). Maybe we > need to introduce something like: > > struct bgpio_chip_info { > struct device *dev; > unsigned long sz; > void __iomem *dat; > void __iomem *set; > void __iomem *clr; > void __iomem *dirout; > void __iomem *dirin; > void __iomem *en; > bool big_endian; > }; > > and pass that to bgpio_init instead. It would have the added benefits of > making the drivers more readable and that bgpio_chip_info structs in the > drivers can probably be marked __initdata also. > > Since you are already having to touch all of the call sites for > bgpio_init this could be done as a separate patch along with this series. Agreed. I can do this if Grant too thinks this is a pre-requisite to adding new functionality. Thanks, Sekhar
On Tue, Jul 05, 2011 at 04:15:44PM +1000, Ryan Mallon wrote: > On 05/07/11 15:10, Sekhar Nori wrote: > >Some GPIO controllers have an enable register > >which needs to be written to before a GPIO > >can be used. > > > >Add support for enabling the GPIO. At this > >time inverted logic for enabling the GPIO > >is not supported. This can be done by adding > >a disable register as and when a controller > >with this comes along. > > > >Signed-off-by: Sekhar Nori<nsekhar@ti.com> > >--- > > > <snip> > > >static int bgpio_setup_io(struct bgpio_chip *bgc, > > void __iomem *dat, > >@@ -369,6 +401,7 @@ int __devinit bgpio_init(struct bgpio_chip *bgc, > > void __iomem *clr, > > void __iomem *dirout, > > void __iomem *dirin, > >+ void __iomem *en, > > bool big_endian) > > The arguments to this function are getting a bit unwieldy :-). Maybe > we need to introduce something like: > > struct bgpio_chip_info { > struct device *dev; > unsigned long sz; > void __iomem *dat; > void __iomem *set; > void __iomem *clr; > void __iomem *dirout; > void __iomem *dirin; > void __iomem *en; > bool big_endian; > }; > > and pass that to bgpio_init instead. It would have the added > benefits of making the drivers more readable and that > bgpio_chip_info structs in the drivers can probably be marked > __initdata also. Or, what may be better is to make callers directly update the bgpio_chip structure.
Hi Grant, On Thu, Jul 07, 2011 at 02:40:54, Grant Likely wrote: > On Tue, Jul 05, 2011 at 04:15:44PM +1000, Ryan Mallon wrote: > > On 05/07/11 15:10, Sekhar Nori wrote: > > >Some GPIO controllers have an enable register > > >which needs to be written to before a GPIO > > >can be used. > > > > > >Add support for enabling the GPIO. At this > > >time inverted logic for enabling the GPIO > > >is not supported. This can be done by adding > > >a disable register as and when a controller > > >with this comes along. > > > > > >Signed-off-by: Sekhar Nori<nsekhar@ti.com> > > >--- > > > > > <snip> > > > > >static int bgpio_setup_io(struct bgpio_chip *bgc, > > > void __iomem *dat, > > >@@ -369,6 +401,7 @@ int __devinit bgpio_init(struct bgpio_chip *bgc, > > > void __iomem *clr, > > > void __iomem *dirout, > > > void __iomem *dirin, > > >+ void __iomem *en, > > > bool big_endian) > > > > The arguments to this function are getting a bit unwieldy :-). Maybe > > we need to introduce something like: > > > > struct bgpio_chip_info { > > struct device *dev; > > unsigned long sz; > > void __iomem *dat; > > void __iomem *set; > > void __iomem *clr; > > void __iomem *dirout; > > void __iomem *dirin; > > void __iomem *en; > > bool big_endian; > > }; > > > > and pass that to bgpio_init instead. It would have the added > > benefits of making the drivers more readable and that > > bgpio_chip_info structs in the drivers can probably be marked > > __initdata also. > > Or, what may be better is to make callers directly update the > bgpio_chip structure. I started implementing it this way, but felt that the bgpio_chip structure also has many internal members (like the spinlock) and this method will require users to spend quite a bit of time figuring out which structure members they should initialize and which to leave for bgpio_init() to do for them. There is also the case of direction register which is set from either dirout or dirin depending upon whether the logic is inverted or not. The bgpio_chip however needs to deal with a single direction register offset. Considering these, I am getting inclined towards Ryan's suggestion. Thoughts? Thanks, Sekhar
On Thu, Jul 07, 2011 at 10:15:31PM +0530, Nori, Sekhar wrote: > Hi Grant, > > On Thu, Jul 07, 2011 at 02:40:54, Grant Likely wrote: > > On Tue, Jul 05, 2011 at 04:15:44PM +1000, Ryan Mallon wrote: > > > On 05/07/11 15:10, Sekhar Nori wrote: > > > >Some GPIO controllers have an enable register > > > >which needs to be written to before a GPIO > > > >can be used. > > > > > > > >Add support for enabling the GPIO. At this > > > >time inverted logic for enabling the GPIO > > > >is not supported. This can be done by adding > > > >a disable register as and when a controller > > > >with this comes along. > > > > > > > >Signed-off-by: Sekhar Nori<nsekhar@ti.com> > > > >--- > > > > > > > <snip> > > > > > > >static int bgpio_setup_io(struct bgpio_chip *bgc, > > > > void __iomem *dat, > > > >@@ -369,6 +401,7 @@ int __devinit bgpio_init(struct bgpio_chip *bgc, > > > > void __iomem *clr, > > > > void __iomem *dirout, > > > > void __iomem *dirin, > > > >+ void __iomem *en, > > > > bool big_endian) > > > > > > The arguments to this function are getting a bit unwieldy :-). Maybe > > > we need to introduce something like: > > > > > > struct bgpio_chip_info { > > > struct device *dev; > > > unsigned long sz; > > > void __iomem *dat; > > > void __iomem *set; > > > void __iomem *clr; > > > void __iomem *dirout; > > > void __iomem *dirin; > > > void __iomem *en; > > > bool big_endian; > > > }; > > > > > > and pass that to bgpio_init instead. It would have the added > > > benefits of making the drivers more readable and that > > > bgpio_chip_info structs in the drivers can probably be marked > > > __initdata also. > > > > Or, what may be better is to make callers directly update the > > bgpio_chip structure. > > I started implementing it this way, but felt that the bgpio_chip > structure also has many internal members (like the spinlock) and > this method will require users to spend quite a bit of time figuring > out which structure members they should initialize and which to leave > for bgpio_init() to do for them. > > There is also the case of direction register which is set from > either dirout or dirin depending upon whether the logic is inverted > or not. The bgpio_chip however needs to deal with a single direction > register offset. We *absolutely* still need the helper function for the complex settings, but for the non-complex ones, I'd rather just directly access the structure. The kerneldoc documentation of the structure can and should be explicit about what the caller is allowed to do. g.
On 08/07/11 04:37, Grant Likely wrote: > On Thu, Jul 07, 2011 at 10:15:31PM +0530, Nori, Sekhar wrote: >> Hi Grant, >> >> On Thu, Jul 07, 2011 at 02:40:54, Grant Likely wrote: >>> On Tue, Jul 05, 2011 at 04:15:44PM +1000, Ryan Mallon wrote: >>>> On 05/07/11 15:10, Sekhar Nori wrote: >>>>> Some GPIO controllers have an enable register >>>>> which needs to be written to before a GPIO >>>>> can be used. >>>>> >>>>> Add support for enabling the GPIO. At this >>>>> time inverted logic for enabling the GPIO >>>>> is not supported. This can be done by adding >>>>> a disable register as and when a controller >>>>> with this comes along. >>>>> >>>>> Signed-off-by: Sekhar Nori<nsekhar@ti.com> >>>>> --- >>>>> >>>> <snip> >>>> >>>>> static int bgpio_setup_io(struct bgpio_chip *bgc, >>>>> void __iomem *dat, >>>>> @@ -369,6 +401,7 @@ int __devinit bgpio_init(struct bgpio_chip *bgc, >>>>> void __iomem *clr, >>>>> void __iomem *dirout, >>>>> void __iomem *dirin, >>>>> + void __iomem *en, >>>>> bool big_endian) >>>> The arguments to this function are getting a bit unwieldy :-). Maybe >>>> we need to introduce something like: >>>> >>>> struct bgpio_chip_info { >>>> struct device *dev; >>>> unsigned long sz; >>>> void __iomem *dat; >>>> void __iomem *set; >>>> void __iomem *clr; >>>> void __iomem *dirout; >>>> void __iomem *dirin; >>>> void __iomem *en; >>>> bool big_endian; >>>> }; >>>> >>>> and pass that to bgpio_init instead. It would have the added >>>> benefits of making the drivers more readable and that >>>> bgpio_chip_info structs in the drivers can probably be marked >>>> __initdata also. >>> Or, what may be better is to make callers directly update the >>> bgpio_chip structure. >> I started implementing it this way, but felt that the bgpio_chip >> structure also has many internal members (like the spinlock) and >> this method will require users to spend quite a bit of time figuring >> out which structure members they should initialize and which to leave >> for bgpio_init() to do for them. >> >> There is also the case of direction register which is set from >> either dirout or dirin depending upon whether the logic is inverted >> or not. The bgpio_chip however needs to deal with a single direction >> register offset. > We *absolutely* still need the helper function for the complex > settings, but for the non-complex ones, I'd rather just directly > access the structure. The kerneldoc documentation of the structure > can and should be explicit about what the caller is allowed to do. > You could pull out all of the user accessible parts the bgpio_chip structure in to a structure called bgpio_chip_info (or whatever) that the drivers fill in and pass to bgpio_init, which then gets assigned as a member of bgpio_chip. Not sure what you mean about the helper function. If bgpio_init takes a structure with all of the information about the particular chip then initialisation of either a simple or complex gpio chip is done using the same function. Only the number of fields in the bgpio_chip_info structure which need to be filled in should change. ~Ryan
diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c index 3bfd341..8ed498a 100644 --- a/drivers/gpio/gpio-ep93xx.c +++ b/drivers/gpio/gpio-ep93xx.c @@ -314,7 +314,7 @@ static int ep93xx_gpio_add_bank(struct bgpio_chip *bgc, struct device *dev, void __iomem *dir = mmio_base + bank->dir; int err; - err = bgpio_init(bgc, dev, 1, data, NULL, NULL, dir, NULL, false); + err = bgpio_init(bgc, dev, 1, data, NULL, NULL, dir, NULL, NULL, false); if (err) return err; diff --git a/drivers/gpio/gpio-generic.c b/drivers/gpio/gpio-generic.c index 231714d..cf7d596 100644 --- a/drivers/gpio/gpio-generic.c +++ b/drivers/gpio/gpio-generic.c @@ -247,6 +247,34 @@ static int bgpio_dir_out_inv(struct gpio_chip *gc, unsigned int gpio, int val) return 0; } +static int bgpio_request(struct gpio_chip *gc, unsigned int gpio) +{ + struct bgpio_chip *bgc = to_bgpio_chip(gc); + unsigned long flags; + + spin_lock_irqsave(&bgc->lock, flags); + + bgc->en |= bgc->pin2mask(bgc, gpio); + bgc->write_reg(bgc->reg_en, bgc->en); + + spin_unlock_irqrestore(&bgc->lock, flags); + + return 0; +} + +static void bgpio_free(struct gpio_chip *gc, unsigned int gpio) +{ + struct bgpio_chip *bgc = to_bgpio_chip(gc); + unsigned long flags; + + spin_lock_irqsave(&bgc->lock, flags); + + bgc->en &= ~bgc->pin2mask(bgc, gpio); + bgc->write_reg(bgc->reg_en, bgc->en); + + spin_unlock_irqrestore(&bgc->lock, flags); +} + static int bgpio_setup_accessors(struct device *dev, struct bgpio_chip *bgc, bool be) @@ -302,6 +330,10 @@ static int bgpio_setup_accessors(struct device *dev, * indicates the GPIO is an output. * - an input direction register (named "dirin") where a 1 bit indicates * the GPIO is an input. + * + * To enable and disable a GPIO at the time of requesting it there is a + * a simple enable register supported where a 1 bit indicates that the GPIO + * is enabled. */ static int bgpio_setup_io(struct bgpio_chip *bgc, void __iomem *dat, @@ -369,6 +401,7 @@ int __devinit bgpio_init(struct bgpio_chip *bgc, void __iomem *clr, void __iomem *dirout, void __iomem *dirin, + void __iomem *en, bool big_endian) { int ret; @@ -398,6 +431,11 @@ int __devinit bgpio_init(struct bgpio_chip *bgc, if (ret) return ret; + if (en) { + bgc->gc.request = bgpio_request; + bgc->gc.free = bgpio_free; + } + bgc->data = bgc->read_reg(bgc->reg_dat); return ret; @@ -453,6 +491,7 @@ static int __devinit bgpio_pdev_probe(struct platform_device *pdev) void __iomem *clr; void __iomem *dirout; void __iomem *dirin; + void __iomem *en; unsigned long sz; bool be; int err; @@ -485,13 +524,17 @@ static int __devinit bgpio_pdev_probe(struct platform_device *pdev) if (err) return err; + en = bgpio_map(pdev, "en", sz, &err); + if (err) + return err; + be = !strcmp(platform_get_device_id(pdev)->name, "basic-mmio-gpio-be"); bgc = devm_kzalloc(&pdev->dev, sizeof(*bgc), GFP_KERNEL); if (!bgc) return -ENOMEM; - err = bgpio_init(bgc, dev, sz, dat, set, clr, dirout, dirin, be); + err = bgpio_init(bgc, dev, sz, dat, set, clr, dirout, dirin, en, be); if (err) return err; diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c index 2f6a81b..5ce98c6 100644 --- a/drivers/gpio/gpio-mxc.c +++ b/drivers/gpio/gpio-mxc.c @@ -300,7 +300,7 @@ static int __devinit mxc_gpio_probe(struct platform_device *pdev) err = bgpio_init(&port->bgc, &pdev->dev, 4, port->base + GPIO_PSR, port->base + GPIO_DR, NULL, - port->base + GPIO_GDIR, NULL, false); + port->base + GPIO_GDIR, NULL, NULL, false); if (err) goto out_iounmap; diff --git a/drivers/gpio/gpio-mxs.c b/drivers/gpio/gpio-mxs.c index d8cafba..f3b78bf 100644 --- a/drivers/gpio/gpio-mxs.c +++ b/drivers/gpio/gpio-mxs.c @@ -241,7 +241,7 @@ static int __devinit mxs_gpio_probe(struct platform_device *pdev) err = bgpio_init(&port->bgc, &pdev->dev, 4, port->base + PINCTRL_DIN(port->id), port->base + PINCTRL_DOUT(port->id), NULL, - port->base + PINCTRL_DOE(port->id), NULL, false); + port->base + PINCTRL_DOE(port->id), NULL, NULL, false); if (err) goto out_iounmap; diff --git a/include/linux/basic_mmio_gpio.h b/include/linux/basic_mmio_gpio.h index 98999cf..fc2e1cc 100644 --- a/include/linux/basic_mmio_gpio.h +++ b/include/linux/basic_mmio_gpio.h @@ -35,6 +35,7 @@ struct bgpio_chip { void __iomem *reg_set; void __iomem *reg_clr; void __iomem *reg_dir; + void __iomem *reg_en; /* Number of bits (GPIOs): <register width> * 8. */ int bits; @@ -56,6 +57,9 @@ struct bgpio_chip { /* Shadowed direction registers to clear/set direction safely. */ unsigned long dir; + + /* Shadowed enable register to enable/disable safely. */ + unsigned long en; }; static inline struct bgpio_chip *to_bgpio_chip(struct gpio_chip *gc) @@ -72,6 +76,7 @@ int __devinit bgpio_init(struct bgpio_chip *bgc, void __iomem *clr, void __iomem *dirout, void __iomem *dirin, + void __iomem *en, bool big_endian); #endif /* __BASIC_MMIO_GPIO_H */
Some GPIO controllers have an enable register which needs to be written to before a GPIO can be used. Add support for enabling the GPIO. At this time inverted logic for enabling the GPIO is not supported. This can be done by adding a disable register as and when a controller with this comes along. Signed-off-by: Sekhar Nori <nsekhar@ti.com> --- drivers/gpio/gpio-ep93xx.c | 2 +- drivers/gpio/gpio-generic.c | 45 ++++++++++++++++++++++++++++++++++++++- drivers/gpio/gpio-mxc.c | 2 +- drivers/gpio/gpio-mxs.c | 2 +- include/linux/basic_mmio_gpio.h | 5 ++++ 5 files changed, 52 insertions(+), 4 deletions(-)