| Message ID | 1369206634-6778-4-git-send-email-avinashphilip@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, May 22, 2013 at 9:10 AM, Philip Avinash <avinashphilip@ti.com> wrote:
Why not squash patch 1 into this patch?
Anyway, this bring us closer to the model of other drivers so:
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
On 5/22/2013 12:40 PM, Philip Avinash wrote: > From: KV Sujith <sujithkv@ti.com> > > Modify GPIO Davinci driver to be compliant to standard platform drivers. > The driver did not have platform driver structure or a probe. Instead, > had a davinci_gpio_setup() function which is called in the pure_init > sequence. The function also had dependency on davinci_soc_info structure > of the corresponding platform. For Device Tree(DT) implementation, we > need to get rid of the dependency on the davinci_soc_info structure. > Hence as a first stage of DT conversion, we implement a probe. Future > commits shall modify the probe to read platform related data from DT. > > - Add platform_driver structure and driver register function for davinci > GPIO driver. The driver registration is made to happen in > postcore_initcall. This is required since machine init functions like > da850_lcd_hw_init() make use of GPIO. > - Convert the davinci_gpio_setup() to davinci_gpio_probe(). > - Remove access of members in soc_info structure. Instead, relevant data > are taken from davinci_gpio_platform_data structure pointed by > pdev->dev.platform_data. > - Change clk_get() to devm_clk_get() as devm_clk_get() is a device > managed function and makes error handling simpler. > - Change pr_err to dev_err for ngpio error reporting. > - Arrange include files and variables in alphabetical order > > Signed-off-by: KV Sujith <sujithkv@ti.com> > [avinashphilip@ti.com: Move global definition for "struct > davinci_gpio_controller" variable to local in probe and set it as driver > data.] > Signed-off-by: Philip Avinash <avinashphilip@ti.com> > --- > arch/arm/mach-davinci/include/mach/gpio-davinci.h | 2 + > drivers/gpio/gpio-davinci.c | 121 +++++++++++++++------ > 2 files changed, 87 insertions(+), 36 deletions(-) > > diff --git a/arch/arm/mach-davinci/include/mach/gpio-davinci.h b/arch/arm/mach-davinci/include/mach/gpio-davinci.h > index 1fdd1fd..b325a1d 100644 > --- a/arch/arm/mach-davinci/include/mach/gpio-davinci.h > +++ b/arch/arm/mach-davinci/include/mach/gpio-davinci.h > @@ -60,6 +60,8 @@ struct davinci_gpio_controller { > void __iomem *set_data; > void __iomem *clr_data; > void __iomem *in_data; > + int gpio_unbanked; > + unsigned gpio_irq; > }; > > /* The __gpio_to_controller() and __gpio_mask() functions inline to constants > diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c > index d308955..08830aa 100644 > --- a/drivers/gpio/gpio-davinci.c > +++ b/drivers/gpio/gpio-davinci.c > @@ -11,10 +11,17 @@ > */ > > #include <linux/clk.h> > +#include <linux/device.h> > #include <linux/errno.h> > #include <linux/gpio.h> > #include <linux/io.h> > +#include <linux/interrupt.h> > +#include <linux/irqdomain.h> > #include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/platform_data/gpio-davinci.h> > +#include <mach/gpio-davinci.h> This include seems unnecessary. > > #include <asm/mach/irq.h> While at it, you can get rid of this include and use <linux/irq.h> instead? > > @@ -35,10 +42,9 @@ struct davinci_gpio_regs { > #define chip2controller(chip) \ > container_of(chip, struct davinci_gpio_controller, chip) > > -static struct davinci_gpio_controller ctlrs[DIV_ROUND_UP(DAVINCI_N_GPIO, 32)]; > static void __iomem *gpio_base; > > -static struct davinci_gpio_regs __iomem __init *gpio2regs(unsigned gpio) > +static struct davinci_gpio_regs __iomem *gpio2regs(unsigned gpio) > { > void __iomem *ptr; > > @@ -64,7 +70,7 @@ static inline struct davinci_gpio_regs __iomem *irq2regs(int irq) > irq_get_chip_data(irq); > } > > -static int __init davinci_gpio_irq_setup(void); > +static int davinci_gpio_irq_setup(struct platform_device *pdev); > > static inline int __davinci_direction(struct gpio_chip *chip, > unsigned offset, bool out, int value) > @@ -127,33 +133,52 @@ static void davinci_gpio_set(struct gpio_chip *chip, unsigned offset, > __raw_writel((1 << offset), value ? ®s->set_data : ®s->clr_data); > } > > -static int __init davinci_gpio_setup(void) > +static int davinci_gpio_probe(struct platform_device *pdev) > { > int i, base; > unsigned ngpio; > - struct davinci_soc_info *soc_info = &davinci_soc_info; > + struct davinci_gpio_controller *ctlrs; > + struct davinci_gpio_platform_data *pdata; > struct davinci_gpio_regs *regs; > + struct device *dev = &pdev->dev; > + struct resource *res; > > - if (soc_info->gpio_type != GPIO_TYPE_DAVINCI) > - return 0; > + pdata = dev->platform_data; > + if (!pdata) { > + dev_err(dev, "GPIO: No Platform Data Supplied\n"); dev_err should already tell that the error is coming from davinci-gpio so no need to prefix GPIO: again. > + return -EINVAL; > + } > > /* > * The gpio banks conceptually expose a segmented bitmap, > * and "ngpio" is one more than the largest zero-based > * bit index that's valid. > */ > - ngpio = soc_info->gpio_num; > + ngpio = pdata->ngpio; > if (ngpio == 0) { > - pr_err("GPIO setup: how many GPIOs?\n"); > + dev_err(dev, "GPIO Probe: how many GPIOs?\n"); > return -EINVAL; > } > > if (WARN_ON(DAVINCI_N_GPIO < ngpio)) > ngpio = DAVINCI_N_GPIO; > > - gpio_base = ioremap(soc_info->gpio_base, SZ_4K); > - if (WARN_ON(!gpio_base)) > + ctlrs = devm_kzalloc(dev, > + ngpio * sizeof(struct davinci_gpio_controller), GFP_KERNEL); Line break alignment needs fixing. > + if (!ctlrs) { > + dev_err(dev, "Memory alloc failed\n"); > return -ENOMEM; > + } > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (unlikely(!res)) { > + dev_err(dev, "Invalid mem resource\n"); > + return -ENODEV; -EBUSY is better if you cannot get the resource. > + } > + > + gpio_base = devm_ioremap_resource(dev, res); > + if (!gpio_base) > + return -EADDRNOTAVAIL; devm_ioremap_resource gives an error encoder pointer if it fails so please use that instead of masking it. > > for (i = 0, base = 0; base < ngpio; i++, base += 32) { > ctlrs[i].chip.label = "DaVinci"; > @@ -179,13 +204,10 @@ static int __init davinci_gpio_setup(void) > gpiochip_add(&ctlrs[i].chip); > } > > - soc_info->gpio_ctlrs = ctlrs; > - soc_info->gpio_ctlrs_num = DIV_ROUND_UP(ngpio, 32); You drop setting gpio_ctlrs_num here and don't introduce it anywhere else in the patchset so in effect you render the inline gpio get/set API useless. Looks like this initialization should be moved to platform code? > - > - davinci_gpio_irq_setup(); > + platform_set_drvdata(pdev, ctlrs); > + davinci_gpio_irq_setup(pdev); > return 0; > } > -pure_initcall(davinci_gpio_setup); > > /* > * We expect irqs will normally be set up as input pins, but they can also be > @@ -297,14 +319,14 @@ static int gpio_to_irq_banked(struct gpio_chip *chip, unsigned offset) > > static int gpio_to_irq_unbanked(struct gpio_chip *chip, unsigned offset) > { > - struct davinci_soc_info *soc_info = &davinci_soc_info; > + struct davinci_gpio_controller *ctlr = chip2controller(chip); > > /* > * NOTE: we assume for now that only irqs in the first gpio_chip > * can provide direct-mapped IRQs to AINTC (up to 32 GPIOs). > */ > - if (offset < soc_info->gpio_unbanked) > - return soc_info->gpio_irq + offset; > + if (offset < ctlr->irq_base) > + return ctlr->gpio_irq + offset; > else > return -ENODEV; > } > @@ -313,12 +335,11 @@ static int gpio_irq_type_unbanked(struct irq_data *data, unsigned trigger) > { > struct davinci_gpio_controller *ctlr; > struct davinci_gpio_regs __iomem *regs; > - struct davinci_soc_info *soc_info = &davinci_soc_info; > u32 mask; > > ctlr = (struct davinci_gpio_controller *)data->handler_data; > regs = (struct davinci_gpio_regs __iomem *)ctlr->regs; > - mask = __gpio_mask(data->irq - soc_info->gpio_irq); > + mask = __gpio_mask(data->irq - ctlr->gpio_irq); > > if (trigger & ~(IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING)) > return -EINVAL; > @@ -339,23 +360,33 @@ static int gpio_irq_type_unbanked(struct irq_data *data, unsigned trigger) > * (dm6446) can be set appropriately for GPIOV33 pins. > */ > > -static int __init davinci_gpio_irq_setup(void) > +static int davinci_gpio_irq_setup(struct platform_device *pdev) > { > - u32 binten = 0; > - unsigned gpio, irq, bank, ngpio, bank_irq; > - struct clk *clk; > + u32 binten = 0; > + unsigned gpio, irq, bank, ngpio, bank_irq; > + struct clk *clk; > + struct davinci_gpio_controller *ctlrs = platform_get_drvdata(pdev); > + struct davinci_gpio_platform_data *pdata; > struct davinci_gpio_regs __iomem *regs; > - struct davinci_soc_info *soc_info = &davinci_soc_info; > + struct device *dev = &pdev->dev; > + struct resource *res; > + > + pdata = dev->platform_data; > + ngpio = pdata->ngpio; > + res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > + if (unlikely(!res)) { > + dev_err(dev, "Invalid IRQ resource\n"); > + return -ENODEV; -EBUSY again? Thanks, Sekhar
On Tue, Jun 11, 2013 at 17:26:06, Nori, Sekhar wrote: > > > On 5/22/2013 12:40 PM, Philip Avinash wrote: > > From: KV Sujith <sujithkv@ti.com> > > > > Modify GPIO Davinci driver to be compliant to standard platform drivers. > > The driver did not have platform driver structure or a probe. Instead, > > had a davinci_gpio_setup() function which is called in the pure_init > > sequence. The function also had dependency on davinci_soc_info structure > > of the corresponding platform. For Device Tree(DT) implementation, we > > need to get rid of the dependency on the davinci_soc_info structure. > > Hence as a first stage of DT conversion, we implement a probe. Future > > commits shall modify the probe to read platform related data from DT. > > > > - Add platform_driver structure and driver register function for davinci > > GPIO driver. The driver registration is made to happen in > > postcore_initcall. This is required since machine init functions like > > da850_lcd_hw_init() make use of GPIO. > > - Convert the davinci_gpio_setup() to davinci_gpio_probe(). > > - Remove access of members in soc_info structure. Instead, relevant data > > are taken from davinci_gpio_platform_data structure pointed by > > pdev->dev.platform_data. > > - Change clk_get() to devm_clk_get() as devm_clk_get() is a device > > managed function and makes error handling simpler. > > - Change pr_err to dev_err for ngpio error reporting. > > - Arrange include files and variables in alphabetical order > > > > Signed-off-by: KV Sujith <sujithkv@ti.com> > > [avinashphilip@ti.com: Move global definition for "struct > > davinci_gpio_controller" variable to local in probe and set it as driver > > data.] > > Signed-off-by: Philip Avinash <avinashphilip@ti.com> > > --- > > +#include <linux/module.h> > > +#include <linux/platform_device.h> > > +#include <linux/platform_data/gpio-davinci.h> > > > +#include <mach/gpio-davinci.h> > > This include seems unnecessary. This include is not required. > > > > > #include <asm/mach/irq.h> > > While at it, you can get rid of this include and use <linux/irq.h> instead? Ok > > > > > + pdata = dev->platform_data; > > + if (!pdata) { > > + dev_err(dev, "GPIO: No Platform Data Supplied\n"); > > dev_err should already tell that the error is coming from davinci-gpio > so no need to prefix GPIO: again. Ok > > > + return -EINVAL; > > + } > > - if (WARN_ON(!gpio_base)) > > + ctlrs = devm_kzalloc(dev, > > + ngpio * sizeof(struct davinci_gpio_controller), GFP_KERNEL); > > Line break alignment needs fixing. Ok > > > + if (!ctlrs) { > > + dev_err(dev, "Memory alloc failed\n"); > > return -ENOMEM; > > + } > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + if (unlikely(!res)) { > > + dev_err(dev, "Invalid mem resource\n"); > > + return -ENODEV; > > -EBUSY is better if you cannot get the resource. Ok > > > + } > > + > > + gpio_base = devm_ioremap_resource(dev, res); > > + if (!gpio_base) > > + return -EADDRNOTAVAIL; > > devm_ioremap_resource gives an error encoder pointer if it fails so > please use that instead of masking it. Ok > > > > > for (i = 0, base = 0; base < ngpio; i++, base += 32) { > > ctlrs[i].chip.label = "DaVinci"; > > @@ -179,13 +204,10 @@ static int __init davinci_gpio_setup(void) > > gpiochip_add(&ctlrs[i].chip); > > } > > > > - soc_info->gpio_ctlrs = ctlrs; > > > - soc_info->gpio_ctlrs_num = DIV_ROUND_UP(ngpio, 32); > > You drop setting gpio_ctlrs_num here and don't introduce it anywhere > else in the patchset so in effect you render the inline gpio get/set API > useless. Looks like this initialization should be moved to platform code? With [PATCH 08/11] ARM: davinci: start using gpiolib support gpio get/set API Has no more dependency on soc_info->gpio_ctlrs_num. I can merge [PATCH 08/11] ARM: davinci: start using gpiolib support to [PATCH 03/11] gpio: davinci: Modify to platform driver > > > - > > + pdata = dev->platform_data; > > + ngpio = pdata->ngpio; > > + res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > > + if (unlikely(!res)) { > > + dev_err(dev, "Invalid IRQ resource\n"); > > + return -ENODEV; > > -EBUSY again? Ok Thanks Avinash > > Thanks, > Sekhar >
On 6/11/2013 6:25 PM, Philip, Avinash wrote: > On Tue, Jun 11, 2013 at 17:26:06, Nori, Sekhar wrote: >> On 5/22/2013 12:40 PM, Philip Avinash wrote: >>> @@ -179,13 +204,10 @@ static int __init davinci_gpio_setup(void) >>> gpiochip_add(&ctlrs[i].chip); >>> } >>> >>> - soc_info->gpio_ctlrs = ctlrs; >> >>> - soc_info->gpio_ctlrs_num = DIV_ROUND_UP(ngpio, 32); >> >> You drop setting gpio_ctlrs_num here and don't introduce it anywhere >> else in the patchset so in effect you render the inline gpio get/set API >> useless. Looks like this initialization should be moved to platform code? > > With [PATCH 08/11] ARM: davinci: start using gpiolib support gpio get/set API > Has no more dependency on soc_info->gpio_ctlrs_num. With this series, you have removed support for inline gpio get/set API. I see that the inline functions are still available for use on tnetv107x. I wonder why it is important to keep these for tnetv107x when not necessary for other DaVinci devices? When you are removing this feature, please note it prominently in your cover letter and patch description. Also, please provide some data on how the latency now compares to that of inline access earlier. This is important especially for the read. For the writes, gpio clock will mostly determine how soon the value changes on the pin. I am okay with removing the inline access feature. It helps simplify code and most arm machines don't use them. I would just like to see some data for justification as this can be seen as feature regression. Also, if we are removing it, its better to also remove it completely and get the LOC savings instead of just stopping its usage. Thanks, Sekhar
On Wed, Jun 12, 2013 at 13:13:59, Nori, Sekhar wrote: > On 6/11/2013 6:25 PM, Philip, Avinash wrote: > > > On Tue, Jun 11, 2013 at 17:26:06, Nori, Sekhar wrote: > > >> On 5/22/2013 12:40 PM, Philip Avinash wrote: > > >>> @@ -179,13 +204,10 @@ static int __init davinci_gpio_setup(void) > >>> gpiochip_add(&ctlrs[i].chip); > >>> } > >>> > >>> - soc_info->gpio_ctlrs = ctlrs; > >> > >>> - soc_info->gpio_ctlrs_num = DIV_ROUND_UP(ngpio, 32); > >> > >> You drop setting gpio_ctlrs_num here and don't introduce it anywhere > >> else in the patchset so in effect you render the inline gpio get/set API > >> useless. Looks like this initialization should be moved to platform code? > > > > With [PATCH 08/11] ARM: davinci: start using gpiolib support gpio get/set API > > Has no more dependency on soc_info->gpio_ctlrs_num. > > With this series, you have removed support for inline gpio get/set API. > I see that the inline functions are still available for use on > tnetv107x. I wonder why it is important to keep these for tnetv107x when > not necessary for other DaVinci devices? To support DT boot in da850, gpio davinci has to be converted to a driver and remove references to davinci_soc_info from driver. But tnetv107x has separate GPIO driver and reference to davinci_soc_info can also be removed. But I didn't found defconfig support for tnetv107x platforms and left untouched. As I will not be able to build and test on tnetv107x, I prefer to not touch it. > > When you are removing this feature, please note it prominently in your > cover letter and patch description. Ok > Also, please provide some data on > how the latency now compares to that of inline access earlier. This is > important especially for the read. I am not sure whether I understood correctly or not? Meanwhile I had done an experiment by reading printk timing before and after gpio_get_value from a test module. I think this will help in software latency for inline access over gpiolib specific access. gpio_get_value latency testing code + + local_irq_disable(); + pr_emerg("%d %x\n", __LINE__, jiffies); + gpio_get_value(gpio_num); + pr_emerg("%d %x\n", __LINE__, jiffies); + local_irq_enable(); inline gpio functions with interrupt disabled [ 29.734337] 81 ffff966c [ 29.736847] 83 ffff966c Time diff = 0.00251 gpio library with interrupt disabled [ 272.876763] 81 fffff567 [ 272.879291] 83 fffff567 Time diff = 0.002528 Inline function takes less time as expected. > For the writes, gpio clock will > mostly determine how soon the value changes on the pin. > > I am okay with removing the inline access feature. It helps simplify > code and most arm machines don't use them. I would just like to see some > data for justification as this can be seen as feature regression. Also, > if we are removing it, its better to also remove it completely and get > the LOC savings instead of just stopping its usage. I see build failure with below patch for tnetv107x [v6,6/6] Davinci: tnetv107x default configuration So I prefer to leave tnetv107x platform for now. Thanks Avinash >
On 6/12/2013 5:40 PM, Philip, Avinash wrote: > On Wed, Jun 12, 2013 at 13:13:59, Nori, Sekhar wrote: >> On 6/11/2013 6:25 PM, Philip, Avinash wrote: >> >>> On Tue, Jun 11, 2013 at 17:26:06, Nori, Sekhar wrote: >> >>>> On 5/22/2013 12:40 PM, Philip Avinash wrote: >> >>>>> @@ -179,13 +204,10 @@ static int __init davinci_gpio_setup(void) >>>>> gpiochip_add(&ctlrs[i].chip); >>>>> } >>>>> >>>>> - soc_info->gpio_ctlrs = ctlrs; >>>> >>>>> - soc_info->gpio_ctlrs_num = DIV_ROUND_UP(ngpio, 32); >>>> >>>> You drop setting gpio_ctlrs_num here and don't introduce it anywhere >>>> else in the patchset so in effect you render the inline gpio get/set API >>>> useless. Looks like this initialization should be moved to platform code? >>> >>> With [PATCH 08/11] ARM: davinci: start using gpiolib support gpio get/set API >>> Has no more dependency on soc_info->gpio_ctlrs_num. >> >> With this series, you have removed support for inline gpio get/set API. >> I see that the inline functions are still available for use on >> tnetv107x. I wonder why it is important to keep these for tnetv107x when >> not necessary for other DaVinci devices? > > To support DT boot in da850, gpio davinci has to be converted to a driver and > remove references to davinci_soc_info from driver. But tnetv107x has > separate GPIO driver and reference to davinci_soc_info can also be removed. > But I didn't found defconfig support for tnetv107x platforms and left untouched. > As I will not be able to build and test on tnetv107x, I prefer to not touch it. You can always build it by enabling it in menuconfig. Its an ARMv6 platform so you will have to disable other ARMv5 based platforms from while enabling it. ARMv5 and ARMv6 cannot co-exist in the same image. > >> >> When you are removing this feature, please note it prominently in your >> cover letter and patch description. > > Ok > >> Also, please provide some data on >> how the latency now compares to that of inline access earlier. This is >> important especially for the read. > > I am not sure whether I understood correctly or not? Meanwhile I had done > an experiment by reading printk timing before and after gpio_get_value from > a test module. I think this will help in software latency for inline access over > gpiolib specific access. > > gpio_get_value latency testing code > > + > + local_irq_disable(); > + pr_emerg("%d %x\n", __LINE__, jiffies); > + gpio_get_value(gpio_num); > + pr_emerg("%d %x\n", __LINE__, jiffies); > + local_irq_enable(); > > inline gpio functions with interrupt disabled > [ 29.734337] 81 ffff966c > [ 29.736847] 83 ffff966c > > Time diff = 0.00251 > > gpio library with interrupt disabled > > [ 272.876763] 81 fffff567 > [ 272.879291] 83 fffff567 > > Time diff = 0.002528 > > Inline function takes less time as expected. Okay, please note these experiments in your cover letter. Its an 18usec difference. I have no reference to say if that will affect any application, but it will at least serve as information for someone who may get affected by it. > >> For the writes, gpio clock will >> mostly determine how soon the value changes on the pin. >> >> I am okay with removing the inline access feature. It helps simplify >> code and most arm machines don't use them. I would just like to see some >> data for justification as this can be seen as feature regression. Also, >> if we are removing it, its better to also remove it completely and get >> the LOC savings instead of just stopping its usage. > > I see build failure with below patch for tnetv107x > [v6,6/6] Davinci: tnetv107x default configuration Where is this patch? What is the commit-id if it is in mainline? Where is the failure log? > > So I prefer to leave tnetv107x platform for now. I don't think that's acceptable. At least by me. Thanks, Sekhar
diff --git a/arch/arm/mach-davinci/include/mach/gpio-davinci.h b/arch/arm/mach-davinci/include/mach/gpio-davinci.h index 1fdd1fd..b325a1d 100644 --- a/arch/arm/mach-davinci/include/mach/gpio-davinci.h +++ b/arch/arm/mach-davinci/include/mach/gpio-davinci.h @@ -60,6 +60,8 @@ struct davinci_gpio_controller { void __iomem *set_data; void __iomem *clr_data; void __iomem *in_data; + int gpio_unbanked; + unsigned gpio_irq; }; /* The __gpio_to_controller() and __gpio_mask() functions inline to constants diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c index d308955..08830aa 100644 --- a/drivers/gpio/gpio-davinci.c +++ b/drivers/gpio/gpio-davinci.c @@ -11,10 +11,17 @@ */ #include <linux/clk.h> +#include <linux/device.h> #include <linux/errno.h> #include <linux/gpio.h> #include <linux/io.h> +#include <linux/interrupt.h> +#include <linux/irqdomain.h> #include <linux/kernel.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/platform_data/gpio-davinci.h> +#include <mach/gpio-davinci.h> #include <asm/mach/irq.h> @@ -35,10 +42,9 @@ struct davinci_gpio_regs { #define chip2controller(chip) \ container_of(chip, struct davinci_gpio_controller, chip) -static struct davinci_gpio_controller ctlrs[DIV_ROUND_UP(DAVINCI_N_GPIO, 32)]; static void __iomem *gpio_base; -static struct davinci_gpio_regs __iomem __init *gpio2regs(unsigned gpio) +static struct davinci_gpio_regs __iomem *gpio2regs(unsigned gpio) { void __iomem *ptr; @@ -64,7 +70,7 @@ static inline struct davinci_gpio_regs __iomem *irq2regs(int irq) irq_get_chip_data(irq); } -static int __init davinci_gpio_irq_setup(void); +static int davinci_gpio_irq_setup(struct platform_device *pdev); static inline int __davinci_direction(struct gpio_chip *chip, unsigned offset, bool out, int value) @@ -127,33 +133,52 @@ static void davinci_gpio_set(struct gpio_chip *chip, unsigned offset, __raw_writel((1 << offset), value ? ®s->set_data : ®s->clr_data); } -static int __init davinci_gpio_setup(void) +static int davinci_gpio_probe(struct platform_device *pdev) { int i, base; unsigned ngpio; - struct davinci_soc_info *soc_info = &davinci_soc_info; + struct davinci_gpio_controller *ctlrs; + struct davinci_gpio_platform_data *pdata; struct davinci_gpio_regs *regs; + struct device *dev = &pdev->dev; + struct resource *res; - if (soc_info->gpio_type != GPIO_TYPE_DAVINCI) - return 0; + pdata = dev->platform_data; + if (!pdata) { + dev_err(dev, "GPIO: No Platform Data Supplied\n"); + return -EINVAL; + } /* * The gpio banks conceptually expose a segmented bitmap, * and "ngpio" is one more than the largest zero-based * bit index that's valid. */ - ngpio = soc_info->gpio_num; + ngpio = pdata->ngpio; if (ngpio == 0) { - pr_err("GPIO setup: how many GPIOs?\n"); + dev_err(dev, "GPIO Probe: how many GPIOs?\n"); return -EINVAL; } if (WARN_ON(DAVINCI_N_GPIO < ngpio)) ngpio = DAVINCI_N_GPIO; - gpio_base = ioremap(soc_info->gpio_base, SZ_4K); - if (WARN_ON(!gpio_base)) + ctlrs = devm_kzalloc(dev, + ngpio * sizeof(struct davinci_gpio_controller), GFP_KERNEL); + if (!ctlrs) { + dev_err(dev, "Memory alloc failed\n"); return -ENOMEM; + } + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (unlikely(!res)) { + dev_err(dev, "Invalid mem resource\n"); + return -ENODEV; + } + + gpio_base = devm_ioremap_resource(dev, res); + if (!gpio_base) + return -EADDRNOTAVAIL; for (i = 0, base = 0; base < ngpio; i++, base += 32) { ctlrs[i].chip.label = "DaVinci"; @@ -179,13 +204,10 @@ static int __init davinci_gpio_setup(void) gpiochip_add(&ctlrs[i].chip); } - soc_info->gpio_ctlrs = ctlrs; - soc_info->gpio_ctlrs_num = DIV_ROUND_UP(ngpio, 32); - - davinci_gpio_irq_setup(); + platform_set_drvdata(pdev, ctlrs); + davinci_gpio_irq_setup(pdev); return 0; } -pure_initcall(davinci_gpio_setup); /* * We expect irqs will normally be set up as input pins, but they can also be @@ -297,14 +319,14 @@ static int gpio_to_irq_banked(struct gpio_chip *chip, unsigned offset) static int gpio_to_irq_unbanked(struct gpio_chip *chip, unsigned offset) { - struct davinci_soc_info *soc_info = &davinci_soc_info; + struct davinci_gpio_controller *ctlr = chip2controller(chip); /* * NOTE: we assume for now that only irqs in the first gpio_chip * can provide direct-mapped IRQs to AINTC (up to 32 GPIOs). */ - if (offset < soc_info->gpio_unbanked) - return soc_info->gpio_irq + offset; + if (offset < ctlr->irq_base) + return ctlr->gpio_irq + offset; else return -ENODEV; } @@ -313,12 +335,11 @@ static int gpio_irq_type_unbanked(struct irq_data *data, unsigned trigger) { struct davinci_gpio_controller *ctlr; struct davinci_gpio_regs __iomem *regs; - struct davinci_soc_info *soc_info = &davinci_soc_info; u32 mask; ctlr = (struct davinci_gpio_controller *)data->handler_data; regs = (struct davinci_gpio_regs __iomem *)ctlr->regs; - mask = __gpio_mask(data->irq - soc_info->gpio_irq); + mask = __gpio_mask(data->irq - ctlr->gpio_irq); if (trigger & ~(IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING)) return -EINVAL; @@ -339,23 +360,33 @@ static int gpio_irq_type_unbanked(struct irq_data *data, unsigned trigger) * (dm6446) can be set appropriately for GPIOV33 pins. */ -static int __init davinci_gpio_irq_setup(void) +static int davinci_gpio_irq_setup(struct platform_device *pdev) { - u32 binten = 0; - unsigned gpio, irq, bank, ngpio, bank_irq; - struct clk *clk; + u32 binten = 0; + unsigned gpio, irq, bank, ngpio, bank_irq; + struct clk *clk; + struct davinci_gpio_controller *ctlrs = platform_get_drvdata(pdev); + struct davinci_gpio_platform_data *pdata; struct davinci_gpio_regs __iomem *regs; - struct davinci_soc_info *soc_info = &davinci_soc_info; + struct device *dev = &pdev->dev; + struct resource *res; + + pdata = dev->platform_data; + ngpio = pdata->ngpio; + res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); + if (unlikely(!res)) { + dev_err(dev, "Invalid IRQ resource\n"); + return -ENODEV; + } - ngpio = soc_info->gpio_num; + bank_irq = res->start; - bank_irq = soc_info->gpio_irq; - if (bank_irq == 0) { - printk(KERN_ERR "Don't know first GPIO bank IRQ.\n"); - return -EINVAL; + if (unlikely(!bank_irq)) { + dev_err(dev, "Invalid IRQ resource\n"); + return -ENODEV; } - clk = clk_get(NULL, "gpio"); + clk = devm_clk_get(dev, "gpio"); if (IS_ERR(clk)) { printk(KERN_ERR "Error %ld getting gpio clock?\n", PTR_ERR(clk)); @@ -371,8 +402,8 @@ static int __init davinci_gpio_irq_setup(void) */ for (gpio = 0, bank = 0; gpio < ngpio; bank++, gpio += 32) { ctlrs[bank].chip.to_irq = gpio_to_irq_banked; - ctlrs[bank].irq_base = soc_info->gpio_unbanked ? - -EINVAL : (soc_info->intc_irq_num + gpio); + ctlrs[bank].irq_base = pdata->gpio_unbanked ? + -EINVAL : (pdata->intc_irq_num + gpio); } /* @@ -380,7 +411,7 @@ static int __init davinci_gpio_irq_setup(void) * controller only handling trigger modes. We currently assume no * IRQ mux conflicts; gpio_irq_type_unbanked() is only for GPIOs. */ - if (soc_info->gpio_unbanked) { + if (pdata->gpio_unbanked) { static struct irq_chip_type gpio_unbanked; /* pass "bank 0" GPIO IRQs to AINTC */ @@ -400,7 +431,7 @@ static int __init davinci_gpio_irq_setup(void) __raw_writel(~0, ®s->set_rising); /* set the direct IRQs up to use that irqchip */ - for (gpio = 0; gpio < soc_info->gpio_unbanked; gpio++, irq++) { + for (gpio = 0; gpio < pdata->gpio_unbanked; gpio++, irq++) { irq_set_chip(irq, &gpio_unbanked.chip); irq_set_handler_data(irq, &ctlrs[gpio / 32]); irq_set_status_flags(irq, IRQ_TYPE_EDGE_BOTH); @@ -455,3 +486,21 @@ done: return 0; } + +static struct platform_driver davinci_gpio_driver = { + .probe = davinci_gpio_probe, + .driver = { + .name = "davinci_gpio", + .owner = THIS_MODULE, + }, +}; + +/** + * gpio driver register needs to be done before machine_init functions + * access gpio APIs. Hence davinci_gpio_drv_reg() is a postcore_initcall. + */ +static int __init davinci_gpio_drv_reg(void) +{ + return platform_driver_register(&davinci_gpio_driver); +} +postcore_initcall(davinci_gpio_drv_reg);