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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
