Re: [PATCH v2 2/2] gpio: Add GPIO driver for Spreadtrum SC9860 platform

[Baolin Wang]

On 1 February 2018 at 23:31, Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
> On Thu, Feb 1, 2018 at 5:04 AM, Baolin Wang <baolin.wang@xxxxxxxxxx> 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