Hello,
On Wednesday, February 23, 2011 11:15 AM Kukjin Kim wrote:
> Marek Szyprowski wrote:
> >
> > This patch performs a global cleanup in s5p gpio interrupt support code.
> > The code is prepared for upcoming support for gpio interrupts on S5PC210
> > platform, which has 2 gpio banks (regions) instead of one (like on
> > S5PC110 and S5PC100).
> >
> > Signed-off-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
> > Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
> > ---
> > arch/arm/plat-s5p/irq-gpioint.c | 106
> +++++++++++++++++--------------------
> > --
> > 1 files changed, 46 insertions(+), 60 deletions(-)
> >
> > diff --git a/arch/arm/plat-s5p/irq-gpioint.c b/arch/arm/plat-s5p/irq-
> > gpioint.c
> > index 3b6bf89..af10328 100644
> > --- a/arch/arm/plat-s5p/irq-gpioint.c
> > +++ b/arch/arm/plat-s5p/irq-gpioint.c
> > @@ -22,77 +22,64 @@
> > #include <plat/gpio-core.h>
> > #include <plat/gpio-cfg.h>
> >
> > -#define S5P_GPIOREG(x) (S5P_VA_GPIO + (x))
> > +#define GPIO_BASE(chip) (((unsigned long)(chip)->base) &
> > ~(SZ_4K - 1))
> >
>
> Need SZ_4K here instead of 0xFFFFF000?
No problem, I can change it to 0xFFFFF000
>
> > -#define GPIOINT_CON_OFFSET 0x700
> > -#define GPIOINT_MASK_OFFSET 0x900
> > -#define GPIOINT_PEND_OFFSET 0xA00
> > +#define CON_OFFSET 0x700
> > +#define MASK_OFFSET 0x900
> > +#define PEND_OFFSET 0xA00
>
> I don't know why need to change above definitions...
I've shortened them to make the code the uses them to fit 80 characters
per line... They are just a local defines that imho don't need to be
prefixed with GPIOINT_
> > +#define REG_OFFSET(x) ((x) << 2)
> >
> Actually, this is used instead of "group << 2" in this file.
> So how about "GPIOINT_REG_OFFSET(x)" like others?
Ok.
> > static void s5p_gpioint_ack(struct irq_data *data)
> > {
> > + struct s3c_gpio_chip *chip = irq_data_get_irq_data(data);
> > int group, offset, pend_offset;
> > unsigned int value;
> >
> > - group = s5p_gpioint_get_group(data);
> > + group = chip->group;
> > offset = s5p_gpioint_get_offset(data);
> > - pend_offset = group << 2;
> > + pend_offset = REG_OFFSET(group);
> >
> > - value = __raw_readl(S5P_GPIOREG(GPIOINT_PEND_OFFSET) + pend_offset);
> > - value |= 1 << offset;
> > - __raw_writel(value, S5P_GPIOREG(GPIOINT_PEND_OFFSET) + pend_offset);
> > + value = __raw_readl(GPIO_BASE(chip) + PEND_OFFSET + pend_offset);
> > + value |= BIT(offset);
>
> No need inclusion <linux/bitops.h>?
It has been included indirectly, because the code compiled fine.
snip
> > static void s5p_gpioint_handler(unsigned int irq, struct irq_desc *desc)
> > {
> > - int group, offset, pend_offset, mask_offset;
> > - int real_irq;
> > + int group, pend_offset, mask_offset;
> > unsigned int pend, mask;
> >
> > for (group = 0; group < S5P_GPIOINT_GROUP_MAXNR; group++) {
> > - pend_offset = group << 2;
> > - pend = __raw_readl(S5P_GPIOREG(GPIOINT_PEND_OFFSET) +
> > - pend_offset);
> > + struct s3c_gpio_chip *chip = irq_chips[group];
> > + if (!chip)
> > + continue;
> > +
> > + pend_offset = REG_OFFSET(group);
> > + pend = __raw_readl(GPIO_BASE(chip) + PEND_OFFSET +
> > pend_offset);
> > if (!pend)
> > continue;
> >
> > - mask_offset = group << 2;
> > - mask = __raw_readl(S5P_GPIOREG(GPIOINT_MASK_OFFSET) +
> > - mask_offset);
> > + mask_offset = REG_OFFSET(group);
> > + mask = __raw_readl(GPIO_BASE(chip) + MASK_OFFSET +
> > mask_offset);
> > pend &= ~mask;
> >
> > - for (offset = 0; offset < 8; offset++) {
> > - if (pend & (1 << offset)) {
> > - struct s3c_gpio_chip *chip =
> irq_chips[group];
> > - if (chip) {
> > - real_irq = chip->irq_base + offset;
> > - generic_handle_irq(real_irq);
> > - }
> > - }
> > + while (pend) {
> > + int offset = fls(pend) - 1;
>
> __ffs?
I don't see much difference between ffs and fls here...
> And hmm...do we really need while loop here?
Yes, because more than one gpio pin in a group can issue an interrupt at the
same time. The previous version used for() loop here.
>
> > + int real_irq = chip->irq_base + offset;
> > + generic_handle_irq(real_irq);
> > + pend &= ~BIT(offset);
> > }
> > }
> > }
> > @@ -202,7 +188,7 @@ static __init int s5p_gpioint_add(struct s3c_gpio_chip
> > *chip)
> > for (i = 0; i < chip->chip.ngpio; i++) {
> > irq = chip->irq_base + i;
> > set_irq_chip(irq, &s5p_gpioint);
> > - set_irq_data(irq, &chip->chip);
> > + set_irq_data(irq, chip);
>
> ?
This simplifies all the functions that use get_irq_data. Now they get s3c_gpio_chip
directly and don't need to extract it with contrainer_of().
Best regards
--
Marek Szyprowski
Samsung Poland R&D Center
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
