From: Grygorii Strashko <grygorii.strashko@ti.com> To: Keerthy <j-keerthy@ti.com>, linus.walleij@linaro.org, t-kristo@ti.com Cc: linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org, gnurou@gmail.com Subject: Re: [PATCH 2/3] gpio: davinci: Redesign driver to accommodate ngpios in one gpio chip Date: Thu, 12 Jan 2017 13:06:01 -0600 [thread overview] Message-ID: <fd6a1bc4-bcb1-a2ed-4058-35514d114211@ti.com> (raw) In-Reply-To: <3a017847-abad-f215-30d9-764a0cb07041@ti.com> On 01/11/2017 08:00 PM, Keerthy wrote: > > > On Wednesday 11 January 2017 11:23 PM, Grygorii Strashko wrote: >> >> >> On 01/10/2017 11:00 PM, Keerthy wrote: >>> The Davinci GPIO driver is implemented to work with one monolithic >>> Davinci GPIO platform device which may have up to Y(144) gpios. >>> The Davinci GPIO driver instantiates number of GPIO chips with >>> max 32 gpio pins per each during initialization and one IRQ domain. >>> So, the current GPIO's opjects structure is: >>> >>> <platform device> Davinci GPIO controller >>> |- <gpio0_chip0> ------| >>> ... |--- irq_domain (hwirq [0..143]) >>> |- <gpio0_chipN> ------| >>> >>> Current driver creates one chip for every 32 GPIOs in a controller. >>> This was a limitation earlier now there is no need for that. Hence >>> redesigning the driver to create one gpio chip for all the ngpio >>> in the controller. >>> >>> |- <gpio0_chip0> ------|--- irq_domain (hwirq [0..143]). >>> >>> The previous discussion on this can be found here: >>> https://www.spinics.net/lists/linux-omap/msg132869.html >> >> nice rework. > > Thanks! > >> >>> >>> Signed-off-by: Keerthy <j-keerthy@ti.com> >>> --- >>> >>> Boot tested on Davinci platform. >>> >>> drivers/gpio/gpio-davinci.c | 127 >>> +++++++++++++++++------------ [...] >>> >>> #ifdef CONFIG_OF_GPIO >>> - chips[i].chip.of_gpio_n_cells = 2; >>> - chips[i].chip.of_xlate = davinci_gpio_of_xlate; >>> - chips[i].chip.parent = dev; >>> - chips[i].chip.of_node = dev->of_node; >>> + chips->chip.of_gpio_n_cells = 2; >>> + chips->chip.of_xlate = davinci_gpio_of_xlate; >> >> I think It's not necessary to have custom .xlate() and >> it can be removed, then gpiolib will assign default one >> of_gpio_simple_xlate(). > > Okay. Can i do that as a separate patch? I think it's ok. > >> >>> + chips->chip.parent = dev; >>> + chips->chip.of_node = dev->of_node; >>> #endif >>> - spin_lock_init(&chips[i].lock); >>> - [...] >>> >>> irq_set_chip_and_handler_name(irq, &gpio_irqchip, >>> handle_simple_irq, >>> "davinci_gpio"); >>> @@ -459,6 +468,7 @@ static int davinci_gpio_irq_setup(struct >>> platform_device *pdev) >>> struct irq_domain *irq_domain = NULL; >>> const struct of_device_id *match; >>> struct irq_chip *irq_chip; >>> + struct davinci_gpio_irq_data *irqdata[MAX_BANKED_IRQS]; >> >> You declare irqdata as array here but it has not been used anywhere >> except for assignment. Could you remove this array and MAX_BANKED_IRQS >> define? > > irq_set_chained_handler_and_data(bank_irq, gpio_irq_handler, > &chips[gpio / 32]); > irqdata[bank]); > > That is hooked as data for each bank. As there is only one controller > now and the differentiating parameters per bank is the irqdata data > structure with the registers pointer and the bank number. > This structure makes it very easy in the irq handler to identify the > register sets that need to be modified and the bank irqs. That I understood, but why do you need array here? > >> >> Seems you can just use struct davinci_gpio_irq_data *irqdata; why can't you use (see below): struct davinci_gpio_irq_data *irqdata; >> >>> gpio_get_irq_chip_cb_t gpio_get_irq_chip; >>> >>> /* >>> @@ -514,10 +524,8 @@ static int davinci_gpio_irq_setup(struct >>> platform_device *pdev) >>> * IRQs, while the others use banked IRQs, would need some setup >>> * tweaks to recognize hardware which can do that. >>> */ [...] >>> >>> @@ -567,8 +575,19 @@ static int davinci_gpio_irq_setup(struct >>> platform_device *pdev) >>> * gpio irqs. Pass the irq bank's corresponding controller to >>> * the chained irq handler. >>> */ >>> + irqdata[bank] = devm_kzalloc(&pdev->dev, >>> + sizeof(struct >>> + davinci_gpio_irq_data), >>> + GFP_KERNEL); irqdata = devm_kzalloc(&pdev->dev, sizeof(struct davinci_gpio_irq_data), GFP_KERNEL); >>> + if (!irqdata[bank]) >>> + return -ENOMEM; >>> + >>> + irqdata[bank]->regs = g; >>> + irqdata[bank]->bank_num = bank; >>> + irqdata[bank]->chip = chips; >>> + >>> irq_set_chained_handler_and_data(bank_irq, gpio_irq_handler, >>> - &chips[gpio / 32]); >>> + irqdata[bank]); irq_set_chained_handler_and_data(bank_irq, gpio_irq_handler, irqdata); [...] -- regards, -grygorii
WARNING: multiple messages have this Message-ID (diff)
From: Grygorii Strashko <grygorii.strashko@ti.com> To: Keerthy <j-keerthy@ti.com>, <linus.walleij@linaro.org>, <t-kristo@ti.com> Cc: <linux-gpio@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <gnurou@gmail.com> Subject: Re: [PATCH 2/3] gpio: davinci: Redesign driver to accommodate ngpios in one gpio chip Date: Thu, 12 Jan 2017 13:06:01 -0600 [thread overview] Message-ID: <fd6a1bc4-bcb1-a2ed-4058-35514d114211@ti.com> (raw) In-Reply-To: <3a017847-abad-f215-30d9-764a0cb07041@ti.com> On 01/11/2017 08:00 PM, Keerthy wrote: > > > On Wednesday 11 January 2017 11:23 PM, Grygorii Strashko wrote: >> >> >> On 01/10/2017 11:00 PM, Keerthy wrote: >>> The Davinci GPIO driver is implemented to work with one monolithic >>> Davinci GPIO platform device which may have up to Y(144) gpios. >>> The Davinci GPIO driver instantiates number of GPIO chips with >>> max 32 gpio pins per each during initialization and one IRQ domain. >>> So, the current GPIO's opjects structure is: >>> >>> <platform device> Davinci GPIO controller >>> |- <gpio0_chip0> ------| >>> ... |--- irq_domain (hwirq [0..143]) >>> |- <gpio0_chipN> ------| >>> >>> Current driver creates one chip for every 32 GPIOs in a controller. >>> This was a limitation earlier now there is no need for that. Hence >>> redesigning the driver to create one gpio chip for all the ngpio >>> in the controller. >>> >>> |- <gpio0_chip0> ------|--- irq_domain (hwirq [0..143]). >>> >>> The previous discussion on this can be found here: >>> https://www.spinics.net/lists/linux-omap/msg132869.html >> >> nice rework. > > Thanks! > >> >>> >>> Signed-off-by: Keerthy <j-keerthy@ti.com> >>> --- >>> >>> Boot tested on Davinci platform. >>> >>> drivers/gpio/gpio-davinci.c | 127 >>> +++++++++++++++++------------ [...] >>> >>> #ifdef CONFIG_OF_GPIO >>> - chips[i].chip.of_gpio_n_cells = 2; >>> - chips[i].chip.of_xlate = davinci_gpio_of_xlate; >>> - chips[i].chip.parent = dev; >>> - chips[i].chip.of_node = dev->of_node; >>> + chips->chip.of_gpio_n_cells = 2; >>> + chips->chip.of_xlate = davinci_gpio_of_xlate; >> >> I think It's not necessary to have custom .xlate() and >> it can be removed, then gpiolib will assign default one >> of_gpio_simple_xlate(). > > Okay. Can i do that as a separate patch? I think it's ok. > >> >>> + chips->chip.parent = dev; >>> + chips->chip.of_node = dev->of_node; >>> #endif >>> - spin_lock_init(&chips[i].lock); >>> - [...] >>> >>> irq_set_chip_and_handler_name(irq, &gpio_irqchip, >>> handle_simple_irq, >>> "davinci_gpio"); >>> @@ -459,6 +468,7 @@ static int davinci_gpio_irq_setup(struct >>> platform_device *pdev) >>> struct irq_domain *irq_domain = NULL; >>> const struct of_device_id *match; >>> struct irq_chip *irq_chip; >>> + struct davinci_gpio_irq_data *irqdata[MAX_BANKED_IRQS]; >> >> You declare irqdata as array here but it has not been used anywhere >> except for assignment. Could you remove this array and MAX_BANKED_IRQS >> define? > > irq_set_chained_handler_and_data(bank_irq, gpio_irq_handler, > &chips[gpio / 32]); > irqdata[bank]); > > That is hooked as data for each bank. As there is only one controller > now and the differentiating parameters per bank is the irqdata data > structure with the registers pointer and the bank number. > This structure makes it very easy in the irq handler to identify the > register sets that need to be modified and the bank irqs. That I understood, but why do you need array here? > >> >> Seems you can just use struct davinci_gpio_irq_data *irqdata; why can't you use (see below): struct davinci_gpio_irq_data *irqdata; >> >>> gpio_get_irq_chip_cb_t gpio_get_irq_chip; >>> >>> /* >>> @@ -514,10 +524,8 @@ static int davinci_gpio_irq_setup(struct >>> platform_device *pdev) >>> * IRQs, while the others use banked IRQs, would need some setup >>> * tweaks to recognize hardware which can do that. >>> */ [...] >>> >>> @@ -567,8 +575,19 @@ static int davinci_gpio_irq_setup(struct >>> platform_device *pdev) >>> * gpio irqs. Pass the irq bank's corresponding controller to >>> * the chained irq handler. >>> */ >>> + irqdata[bank] = devm_kzalloc(&pdev->dev, >>> + sizeof(struct >>> + davinci_gpio_irq_data), >>> + GFP_KERNEL); irqdata = devm_kzalloc(&pdev->dev, sizeof(struct davinci_gpio_irq_data), GFP_KERNEL); >>> + if (!irqdata[bank]) >>> + return -ENOMEM; >>> + >>> + irqdata[bank]->regs = g; >>> + irqdata[bank]->bank_num = bank; >>> + irqdata[bank]->chip = chips; >>> + >>> irq_set_chained_handler_and_data(bank_irq, gpio_irq_handler, >>> - &chips[gpio / 32]); >>> + irqdata[bank]); irq_set_chained_handler_and_data(bank_irq, gpio_irq_handler, irqdata); [...] -- regards, -grygorii
next prev parent reply other threads:[~2017-01-12 19:06 UTC|newest] Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-01-11 5:00 [PATCH 0/3] gpio: davinci: Redesign driver to accommodate ngpios in one gpio chip Keerthy 2017-01-11 5:00 ` Keerthy 2017-01-11 5:00 ` [PATCH 1/3] gpio: davinci: Remove unwanted blank line Keerthy 2017-01-11 5:00 ` Keerthy 2017-01-11 17:37 ` Grygorii Strashko 2017-01-11 17:37 ` Grygorii Strashko 2017-01-12 1:53 ` Keerthy 2017-01-12 1:53 ` Keerthy 2017-01-11 5:00 ` [PATCH 2/3] gpio: davinci: Redesign driver to accommodate ngpios in one gpio chip Keerthy 2017-01-11 5:00 ` Keerthy 2017-01-11 17:53 ` Grygorii Strashko 2017-01-11 17:53 ` Grygorii Strashko 2017-01-12 2:00 ` Keerthy 2017-01-12 2:00 ` Keerthy 2017-01-12 19:06 ` Grygorii Strashko [this message] 2017-01-12 19:06 ` Grygorii Strashko 2017-01-13 3:42 ` Keerthy 2017-01-13 3:42 ` Keerthy 2017-01-11 5:00 ` [PATCH 3/3] gpio: davinci: Add support for multiple GPIO controllers Keerthy 2017-01-11 5:00 ` Keerthy 2017-01-17 14:52 ` [PATCH 0/3] gpio: davinci: Redesign driver to accommodate ngpios in one gpio chip Linus Walleij 2017-01-17 16:14 ` Keerthy
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=fd6a1bc4-bcb1-a2ed-4058-35514d114211@ti.com \ --to=grygorii.strashko@ti.com \ --cc=gnurou@gmail.com \ --cc=j-keerthy@ti.com \ --cc=linus.walleij@linaro.org \ --cc=linux-gpio@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=t-kristo@ti.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.