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

[Keerthy]

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.

> [...]