lkml.org 
[lkml]   [2011]   [Nov]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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


    \
     
     \ /
      Last update: 2011-11-21 05:23    [W:2.805 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site