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

[Grygorii Strashko]

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