Re: [PATCH 2/3] gpio: davinci: Redesign driver to accommodate ngpios in one gpio chip

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On Friday 13 January 2017 12:36 AM, Grygorii Strashko wrote:


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@xxxxxx>
---

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;

Understood your comment now thanks for clarifying. I will do like you have suggested.


     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);


Okay.

+        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);


I will post the new set with the above changes.


[...]

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux SPI]     [Linux Kernel]     [Linux ARM (vger)]     [Linux ARM MSM]     [Linux Omap]     [Linux Arm]     [Linux Tegra]     [Fedora ARM]     [Linux for Samsung SOC]     [eCos]     [Linux Fastboot]     [Gcc Help]     [Git]     [DCCP]     [IETF Announce]     [Security]     [Linux MIPS]     [Yosemite Campsites]

  Powered by Linux