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

[Rohit Vaswani]

On 6/5/2013 2:38 PM, Grant Likely wrote:
> On Sun, 02 Jun 2013 19:31:33 -0700, Rohit Vaswani <rvaswani@xxxxxxxxxxxxxx> wrote:
> > On 6/1/2013 1:09 AM, Grant Likely wrote:
> > > On Sat, Jun 1, 2013 at 1:22 AM, Rohit Vaswani <rvaswani@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> > > > @@ -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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/