lkml.org 
[lkml]   [2017]   [Jan]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 2/3] gpio: davinci: Redesign driver to accommodate ngpios in one gpio chip
    From
    Date


    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.

    >
    > Signed-off-by: Keerthy <j-keerthy@ti.com>
    > ---
    >
    > Boot tested on Davinci platform.
    >
    > drivers/gpio/gpio-davinci.c | 127 +++++++++++++++++------------
    > include/linux/platform_data/gpio-davinci.h | 13 ++-
    > 2 files changed, 84 insertions(+), 56 deletions(-)
    >
    > diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
    > index 26b874a..6c1c00a 100644
    > --- a/drivers/gpio/gpio-davinci.c
    > +++ b/drivers/gpio/gpio-davinci.c
    > @@ -63,11 +63,13 @@ static inline int __davinci_direction(struct gpio_chip *chip,
    > unsigned offset, bool out, int value)
    > {
    > struct davinci_gpio_controller *d = gpiochip_get_data(chip);
    > - struct davinci_gpio_regs __iomem *g = d->regs;
    > + struct davinci_gpio_regs __iomem *g;
    > unsigned long flags;
    > u32 temp;
    > - u32 mask = 1 << offset;
    > + int bank = offset / 32;
    > + u32 mask = 1 << (offset % 32);
    >
    > + g = d->regs[bank];
    > spin_lock_irqsave(&d->lock, flags);
    > temp = readl_relaxed(&g->dir);
    > if (out) {
    > @@ -103,9 +105,12 @@ static int davinci_direction_in(struct gpio_chip *chip, unsigned offset)
    > static int davinci_gpio_get(struct gpio_chip *chip, unsigned offset)
    > {
    > struct davinci_gpio_controller *d = gpiochip_get_data(chip);
    > - struct davinci_gpio_regs __iomem *g = d->regs;
    > + struct davinci_gpio_regs __iomem *g;
    > + int bank = offset / 32;
    >
    > - return !!((1 << offset) & readl_relaxed(&g->in_data));
    > + g = d->regs[bank];
    > +
    > + return !!((1 << (offset % 32)) & readl_relaxed(&g->in_data));

    (1 << (offset % 32) is __gpio_mask()

    > }
    >
    > /*
    > @@ -115,9 +120,13 @@ static int davinci_gpio_get(struct gpio_chip *chip, unsigned offset)
    > davinci_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
    > {
    > struct davinci_gpio_controller *d = gpiochip_get_data(chip);
    > - struct davinci_gpio_regs __iomem *g = d->regs;
    > + struct davinci_gpio_regs __iomem *g;
    > + int bank = offset / 32;
    >
    > - writel_relaxed((1 << offset), value ? &g->set_data : &g->clr_data);
    > + g = d->regs[bank];
    > +
    > + writel_relaxed((1 << (offset % 32)),
    > + value ? &g->set_data : &g->clr_data);
    > }
    >
    > static struct davinci_gpio_platform_data *
    > @@ -165,7 +174,7 @@ static int davinci_gpio_of_xlate(struct gpio_chip *gc,
    > if (gpiospec->args[0] > pdata->ngpio)
    > return -EINVAL;
    >
    > - if (gc != &chips[gpiospec->args[0] / 32].chip)
    > + if (gc != &chips->chip)
    > return -EINVAL;
    >
    > if (flags)
    > @@ -177,11 +186,11 @@ static int davinci_gpio_of_xlate(struct gpio_chip *gc,
    >
    > static int davinci_gpio_probe(struct platform_device *pdev)
    > {
    > - int i, base;
    > + static int ctrl_num;
    > + int gpio, bank;
    > unsigned ngpio, nbank;
    > struct davinci_gpio_controller *chips;
    > struct davinci_gpio_platform_data *pdata;
    > - struct davinci_gpio_regs __iomem *regs;
    > struct device *dev = &pdev->dev;
    > struct resource *res;
    > char label[MAX_LABEL_SIZE];
    > @@ -220,38 +229,30 @@ static int davinci_gpio_probe(struct platform_device *pdev)
    > if (IS_ERR(gpio_base))
    > return PTR_ERR(gpio_base);
    >
    > - for (i = 0, base = 0; base < ngpio; i++, base += 32) {
    > - snprintf(label, MAX_LABEL_SIZE, "davinci_gpio.%d", i);
    > - chips[i].chip.label = devm_kstrdup(dev, label, GFP_KERNEL);
    > - if (!chips[i].chip.label)
    > + snprintf(label, MAX_LABEL_SIZE, "davinci_gpio.%d", ctrl_num++);
    > + chips->chip.label = devm_kstrdup(dev, label, GFP_KERNEL);
    > + if (!chips->chip.label)
    > return -ENOMEM;
    >
    > - chips[i].chip.direction_input = davinci_direction_in;
    > - chips[i].chip.get = davinci_gpio_get;
    > - chips[i].chip.direction_output = davinci_direction_out;
    > - chips[i].chip.set = davinci_gpio_set;
    > + chips->chip.direction_input = davinci_direction_in;
    > + chips->chip.get = davinci_gpio_get;
    > + chips->chip.direction_output = davinci_direction_out;
    > + chips->chip.set = davinci_gpio_set;
    >
    > - chips[i].chip.base = base;
    > - chips[i].chip.ngpio = ngpio - base;
    > - if (chips[i].chip.ngpio > 32)
    > - chips[i].chip.ngpio = 32;
    > + chips->chip.ngpio = ngpio;
    >
    > #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().

    > + chips->chip.parent = dev;
    > + chips->chip.of_node = dev->of_node;
    > #endif
    > - spin_lock_init(&chips[i].lock);
    > -
    > - regs = gpio_base + offset_array[i];
    > - if (!regs)
    > - return -ENXIO;
    > - chips[i].regs = regs;
    > + spin_lock_init(&chips->lock);
    >
    > - gpiochip_add_data(&chips[i].chip, &chips[i]);
    > - }
    > + for (gpio = 0, bank = 0; gpio < ngpio; gpio += 32, bank++)
    > + chips->regs[bank] = gpio_base + offset_array[bank];
    >
    > + gpiochip_add_data(&chips->chip, chips);
    > platform_set_drvdata(pdev, chips);
    > davinci_gpio_irq_setup(pdev);
    > return 0;
    > @@ -312,16 +313,19 @@ static int gpio_irq_type(struct irq_data *d, unsigned trigger)
    >
    > static void gpio_irq_handler(struct irq_desc *desc)
    > {
    > - unsigned int irq = irq_desc_get_irq(desc);
    > struct davinci_gpio_regs __iomem *g;
    > u32 mask = 0xffff;
    > + int bank_num;
    > struct davinci_gpio_controller *d;
    > + struct davinci_gpio_irq_data *irqdata;
    >
    > - d = (struct davinci_gpio_controller *)irq_desc_get_handler_data(desc);
    > - g = (struct davinci_gpio_regs __iomem *)d->regs;
    > + irqdata = (struct davinci_gpio_irq_data *)irq_desc_get_handler_data(desc);
    > + bank_num = irqdata->bank_num;
    > + g = irqdata->regs;
    > + d = irqdata->chip;
    >
    > /* we only care about one bank */
    > - if (irq & 1)
    > + if ((bank_num % 2) == 1)
    > mask <<= 16;
    >
    > /* temporarily mask (level sensitive) parent IRQ */
    > @@ -329,6 +333,7 @@ static void gpio_irq_handler(struct irq_desc *desc)
    > while (1) {
    > u32 status;
    > int bit;
    > + irq_hw_number_t hw_irq;
    >
    > /* ack any irqs */
    > status = readl_relaxed(&g->intstat) & mask;
    > @@ -341,9 +346,13 @@ static void gpio_irq_handler(struct irq_desc *desc)
    > while (status) {
    > bit = __ffs(status);
    > status &= ~BIT(bit);
    > + /* Max number of gpios per controller is 144 so
    > + * hw_irq will be in [0..143]
    > + */
    > + hw_irq = (bank_num / 2) * 32 + bit;
    > +
    > generic_handle_irq(
    > - irq_find_mapping(d->irq_domain,
    > - d->chip.base + bit));
    > + irq_find_mapping(d->irq_domain, hw_irq));
    > }
    > }
    > chained_irq_exit(irq_desc_get_chip(desc), desc);
    > @@ -355,7 +364,7 @@ static int gpio_to_irq_banked(struct gpio_chip *chip, unsigned offset)
    > struct davinci_gpio_controller *d = gpiochip_get_data(chip);
    >
    > if (d->irq_domain)
    > - return irq_create_mapping(d->irq_domain, d->chip.base + offset);
    > + return irq_create_mapping(d->irq_domain, offset);
    > else
    > return -ENXIO;
    > }
    > @@ -369,7 +378,7 @@ static int gpio_to_irq_unbanked(struct gpio_chip *chip, unsigned offset)
    > * can provide direct-mapped IRQs to AINTC (up to 32 GPIOs).
    > */
    > if (offset < d->gpio_unbanked)
    > - return d->gpio_irq + offset;
    > + return d->base_irq + offset;
    > else
    > return -ENODEV;
    > }
    > @@ -382,7 +391,7 @@ static int gpio_irq_type_unbanked(struct irq_data *data, unsigned trigger)
    >
    > d = (struct davinci_gpio_controller *)irq_data_get_irq_handler_data(data);
    > g = (struct davinci_gpio_regs __iomem *)d->regs;
    > - mask = __gpio_mask(data->irq - d->gpio_irq);
    > + mask = __gpio_mask(data->irq - d->base_irq);
    >
    > if (trigger & ~(IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING))
    > return -EINVAL;
    > @@ -401,7 +410,7 @@ static int gpio_irq_type_unbanked(struct irq_data *data, unsigned trigger)
    > {
    > struct davinci_gpio_controller *chips =
    > (struct davinci_gpio_controller *)d->host_data;
    > - struct davinci_gpio_regs __iomem *g = chips[hw / 32].regs;
    > + struct davinci_gpio_regs __iomem *g = chips->regs[hw / 32];
    >
    > 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?

    Seems you can just use 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.
    > */
    > - for (gpio = 0, bank = 0; gpio < ngpio; bank++, gpio += 32) {
    > - chips[bank].chip.to_irq = gpio_to_irq_banked;
    > - chips[bank].irq_domain = irq_domain;
    > - }
    > + chips->chip.to_irq = gpio_to_irq_banked;
    > + chips->irq_domain = irq_domain;
    >
    > /*
    > * AINTC can handle direct/unbanked IRQs for GPIOs, with the GPIO
    > @@ -526,9 +534,9 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev)
    > */
    > if (pdata->gpio_unbanked) {
    > /* pass "bank 0" GPIO IRQs to AINTC */
    > - chips[0].chip.to_irq = gpio_to_irq_unbanked;
    > - chips[0].gpio_irq = bank_irq;
    > - chips[0].gpio_unbanked = pdata->gpio_unbanked;
    > + chips->chip.to_irq = gpio_to_irq_unbanked;
    > + chips->base_irq = bank_irq;
    > + chips->gpio_unbanked = pdata->gpio_unbanked;
    > binten = GENMASK(pdata->gpio_unbanked / 16, 0);
    >
    > /* AINTC handles mask/unmask; GPIO handles triggering */
    > @@ -538,14 +546,14 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev)
    > irq_chip->irq_set_type = gpio_irq_type_unbanked;
    >
    > /* default trigger: both edges */
    > - g = chips[0].regs;
    > + g = chips->regs[0];
    > writel_relaxed(~0, &g->set_falling);
    > writel_relaxed(~0, &g->set_rising);
    >
    > /* set the direct IRQs up to use that irqchip */
    > for (gpio = 0; gpio < pdata->gpio_unbanked; gpio++, irq++) {
    > irq_set_chip(irq, irq_chip);
    > - irq_set_handler_data(irq, &chips[gpio / 32]);
    > + irq_set_handler_data(irq, chips);
    > irq_set_status_flags(irq, IRQ_TYPE_EDGE_BOTH);
    > }
    >
    > @@ -558,7 +566,7 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev)
    > */
    > for (gpio = 0, bank = 0; gpio < ngpio; bank++, bank_irq++, gpio += 16) {
    > /* disabled by default, enabled only as needed */
    > - g = chips[bank / 2].regs;
    > + g = chips->regs[bank / 2];
    > writel_relaxed(~0, &g->clr_falling);
    > writel_relaxed(~0, &g->clr_rising);
    >
    > @@ -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);
    > + 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]);
    >
    > binten |= BIT(bank);
    > }
    > diff --git a/include/linux/platform_data/gpio-davinci.h b/include/linux/platform_data/gpio-davinci.h
    > index 18127c4..ca09686 100644
    > --- a/include/linux/platform_data/gpio-davinci.h
    > +++ b/include/linux/platform_data/gpio-davinci.h
    > @@ -21,19 +21,28 @@
    >
    > #include <asm-generic/gpio.h>
    >
    > +#define MAX_BANKS 5

    probably MAX_REGS_BANKS will be better, as it defines
    max number of idnetical registers sets and not number of gpio banks.

    > +#define MAX_BANKED_IRQS 9
    > +
    > struct davinci_gpio_platform_data {
    > u32 ngpio;
    > u32 gpio_unbanked;
    > };
    >
    > +struct davinci_gpio_irq_data {
    > + void __iomem *regs;
    > + struct davinci_gpio_controller *chip;
    > + int bank_num;
    > +};
    > +
    > struct davinci_gpio_controller {
    > struct gpio_chip chip;
    > struct irq_domain *irq_domain;
    > /* Serialize access to GPIO registers */
    > spinlock_t lock;
    > - void __iomem *regs;
    > + void __iomem *regs[MAX_BANKS];
    > int gpio_unbanked;
    > - unsigned gpio_irq;
    > + unsigned int base_irq;
    > };
    >
    > /*
    >

    --
    regards,
    -grygorii

    \
     
     \ /
      Last update: 2017-01-11 18:56    [W:0.051 / U:6.572 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site