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

[Keerthy <j-keerthy@ti.com>]

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

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.

>
> [...]
>