lkml.org 
[lkml]   [2018]   [Feb]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v2 2/2] gpio: Add GPIO driver for Spreadtrum SC9860 platform
    On 1 February 2018 at 23:31, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
    > On Thu, Feb 1, 2018 at 5:04 AM, Baolin Wang <baolin.wang@linaro.org> wrote:
    >> The Spreadtrum SC9860 platform GPIO controller contains 16 groups and
    >> each group contains 16 GPIOs. Each GPIO can set input/output and has
    >> the interrupt capability.
    >
    > Just noticed couple of more improvements you can do.
    >
    >> + case IRQ_TYPE_EDGE_RISING:
    >> + sprd_gpio_update(chip, offset, SPRD_GPIO_IS, 0);
    >> + sprd_gpio_update(chip, offset, SPRD_GPIO_IBE, 0);
    >> + sprd_gpio_update(chip, offset, SPRD_GPIO_IEV, 1);
    >> + irq_set_handler_locked(data, handle_edge_irq);
    >> + break;
    >> + case IRQ_TYPE_EDGE_FALLING:
    >> + sprd_gpio_update(chip, offset, SPRD_GPIO_IS, 0);
    >> + sprd_gpio_update(chip, offset, SPRD_GPIO_IBE, 0);
    >> + sprd_gpio_update(chip, offset, SPRD_GPIO_IEV, 0);
    >> + irq_set_handler_locked(data, handle_edge_irq);
    >> + break;
    >> + case IRQ_TYPE_EDGE_BOTH:
    >> + sprd_gpio_update(chip, offset, SPRD_GPIO_IS, 0);
    >> + sprd_gpio_update(chip, offset, SPRD_GPIO_IBE, 1);
    >> + irq_set_handler_locked(data, handle_edge_irq);
    >> + break;
    >> + case IRQ_TYPE_LEVEL_HIGH:
    >> + sprd_gpio_update(chip, offset, SPRD_GPIO_IS, 1);
    >> + sprd_gpio_update(chip, offset, SPRD_GPIO_IBE, 0);
    >> + sprd_gpio_update(chip, offset, SPRD_GPIO_IEV, 1);
    >> + irq_set_handler_locked(data, handle_level_irq);
    >> + break;
    >> + case IRQ_TYPE_LEVEL_LOW:
    >> + sprd_gpio_update(chip, offset, SPRD_GPIO_IS, 1);
    >> + sprd_gpio_update(chip, offset, SPRD_GPIO_IBE, 0);
    >> + sprd_gpio_update(chip, offset, SPRD_GPIO_IEV, 0);
    >> + irq_set_handler_locked(data, handle_level_irq);
    >> + break;
    >> + default:
    >> + return -EINVAL;
    >
    > I guess you can use fallthrough and reduce some lines, but I have no
    > strong opinion which will look better.

    Um, we need keep the sequence of updating registers and each irq type
    has different register updating. So I think current code can make
    things clear.

    >
    >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
    >> + sprd_gpio->base = devm_ioremap_nocache(&pdev->dev, res->start,
    >> + resource_size(res));
    >
    > Didn't notice before, why not to simple call devm_ioremap_resource() ?

    Ah, you are correct, I missed devm_ioremap_resource(). Will change in
    next version. Thanks.

    --
    Baolin.wang
    Best Regards

    \
     
     \ /
      Last update: 2018-02-02 03:33    [W:2.454 / U:0.112 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site