Messages in this thread | | | Date | Mon, 21 Nov 2011 09:50:05 +0530 | From | Viresh Kumar <> | Subject | Re: [PATCH V2 5/5] gpio/gpio-stmpe: ADD support for stmpe variant 801 |
| |
On 11/18/2011 5:29 PM, Rabin Vincent wrote: > On Thu, Nov 17, 2011 at 11:02, Viresh Kumar <viresh.kumar@st.com> wrote: >> diff --git a/drivers/gpio/gpio-stmpe.c b/drivers/gpio/gpio-stmpe.c >> index 4c980b5..000b019 100644 >> --- a/drivers/gpio/gpio-stmpe.c >> +++ b/drivers/gpio/gpio-stmpe.c >> @@ -65,7 +65,15 @@ static void stmpe_gpio_set(struct gpio_chip *chip, unsigned offset, int val) >> + if (!val && (stmpe->regs[STMPE_IDX_GPSR_LSB] == >> + stmpe->regs[STMPE_IDX_GPCR_LSB])) >> + stmpe_set_bits(stmpe, reg, mask, ~mask); >> + else >> + stmpe_set_bits(stmpe, reg, mask, mask); >> } > > This code, > > (1) for 801, when clearing one GPIO, sets all the others.
I assumed stmpe_set_bits will only affect bits which are 1 in mask and i was wrong. :(
> (2) for other devices, adds an an unnecessary read (within stmpe_set_bits()), > which wasn't there before. >
Correct.
> Please rework to something like: > > if (stmpe->regs[...) > stmpe_set_bits(stmpe, reg, mask, val ? mask : 0); > else > stmpe_reg_write(stmpe, reg, mask); >
Sure.
>> >> static int stmpe_gpio_direction_output(struct gpio_chip *chip, >> @@ -125,10 +133,19 @@ static struct gpio_chip template_chip = { >> static int stmpe_gpio_irq_set_type(struct irq_data *d, unsigned int type) >> { >> struct stmpe_gpio *stmpe_gpio = irq_data_get_irq_chip_data(d); >> + struct stmpe *stmpe = stmpe_gpio->stmpe; >> int offset = d->irq - stmpe_gpio->irq_base; >> int regoffset = offset / 8; >> int mask = 1 << (offset % 8); >> >> + /* STMPE801 doesn't have RE and FE registers */ >> + if (stmpe->partnum == STMPE801) { >> + if (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_LEVEL_HIGH) >> + return 0; > > This looks wrong. From the datasheet I see that it supports edges only, > so perhaps you meant to say IRQ_TYPE_EDGE_* above. >
I meant to say IRQ_TYPE_LEVEL* only, but i was wrong. I didn't read the manual correctly. :(
> In that case please reorganize this to add the return 0 after the > existing check which excludes levels (below). >
Sure
-- viresh
| |