Re: [PATCHv4 3/3] gpio: msm: Add device tree and irqdomain support for gpio-msm-v2

[Rohit Vaswani <rvaswani@codeaurora.org>]

On 6/5/2013 2:38 PM, Grant Likely wrote:
> On Sun, 02 Jun 2013 19:31:33 -0700, Rohit Vaswani <rvaswani@codeaurora.org> wrote:
>> On 6/1/2013 1:09 AM, Grant Likely wrote:
>>> On Sat, Jun 1, 2013 at 1:22 AM, Rohit Vaswani <rvaswani@codeaurora.org> wrote:
>>>> This cleans up the gpio-msm-v2 driver of all the global define usage.
>>>> The number of gpios are now defined in the device tree. This enables
>>>> adding irqdomain support as well.
>>>>
>>>> Signed-off-by: Rohit Vaswani <rvaswani@codeaurora.org>
>>>> @@ -101,11 +96,27 @@ enum {
>>>>     */
>>>>    struct msm_gpio_dev {
>>>>           struct gpio_chip gpio_chip;
>>>> -       DECLARE_BITMAP(enabled_irqs, NR_GPIO_IRQS);
>>>> -       DECLARE_BITMAP(wake_irqs, NR_GPIO_IRQS);
>>>> -       DECLARE_BITMAP(dual_edge_irqs, NR_GPIO_IRQS);
>>>> +       unsigned long *enabled_irqs;
>>>> +       unsigned long *wake_irqs;
>>>> +       unsigned long *dual_edge_irqs;
>>> Was there a reason you ignored the comment to leave these bitmaps as
>>> statically allocated?
>>>
>>> [...]
>>>
>>>> +       msm_gpio.enabled_irqs = devm_kzalloc(&pdev->dev,
>>>> +                                               sizeof(unsigned long) *
>>>> +                                               BITS_TO_LONGS(ngpio) * 3,
>>>> +                                               GFP_KERNEL);
>>>> +       msm_gpio.wake_irqs = &msm_gpio.enabled_irqs[BITS_TO_LONGS(ngpio)];
>>>> +       msm_gpio.dual_edge_irqs =
>>>> +                       &msm_gpio.enabled_irqs[BITS_TO_LONGS(ngpio * 2)];
>>> I should have just deleted my comment about doing it this way. I was
>>> making the point that one allocation is better that three; but then I
>>> also said that it was better to not allocate at all. Go back to the
>>> statically allocated bitmap array. It is far better than this.
>>>
>>> g.
>> I agree that DECLARE_BITMAP is the most efficient way, but
>> DECLARE_BITMAP takes a statically defined number of gpios as an
>> argument. Since we get the ngpio from device tree, these had to go as
>> well. Under this scheme, 1 allocation was better than 3 and went ahead
>> with your suggestion.
> Think about it for a moment; You *know* all the devices that are going
> to use the driver. The largest size for ngpios that I see is 173 which
> is a mere 18 long words for 3 bitmaps. Even if you were to quadruple
> that amount the total for all the bitmasks is 72 long words. You're
> optimizing at the wrong place. Changing it to dynamic allocation makes
> the code slower, more complicated, and probably doesn't do anything to
> save on real memory consumption.
>
> The solution is simple; choose a maximum value of ngpios that the driver
> will likely need to work support. If a device appears with more GPIOs,
> then the driver should throw a warning and work with the maximum it can
> support. Part of enablement for the new device will be increasing the
> driver to handle more gpios.
>
> g.

Got it.

Thanks,
Rohit Vaswani

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation