Thanks, my comments below. On Tue, Jul 21, 2020 at 1:41 PM Abanoub Sameh <abanoubsameh8@xxxxxxxxx> wrote: Commit message is missed on why you did all these changes. ... > if (pin <= 255) { > char ev_name[5]; > + > sprintf(ev_name, "_%c%02hhX", > agpio->triggering == ACPI_EDGE_SENSITIVE ? 'E' : 'L', > pin); Should be a separate patch. ... > dev_err(chip->parent, > - "Failed to install GPIO OpRegion handler\n"); > + "Failed to install GPIO OpRegion handler\n"); Make it simple in one line. Also a separate patch. ... > @@ -316,9 +316,8 @@ struct gpio_desc *gpiod_get_from_of_node(struct device_node *node, > desc = of_get_named_gpiod_flags(node, propname, > index, &flags); > > - if (!desc || IS_ERR(desc)) { > + if (!desc || IS_ERR(desc)) > return desc; > - } Also a separate patch. ... > @@ -974,6 +974,7 @@ static irqreturn_t lineevent_irq_thread(int irq, void *p) > if (le->eflags & GPIOEVENT_REQUEST_RISING_EDGE > && le->eflags & GPIOEVENT_REQUEST_FALLING_EDGE) { > int level = gpiod_get_value_cansleep(le->desc); > + > if (level) > /* Emit low-to-high event */ > ge.id = GPIOEVENT_EVENT_RISING_EDGE; This can be unified with a patch against gpiolib-acpi. ... > - if (is_of_node(fwspec->fwnode) && fwspec->param_count == 2) { > + if (is_of_node(fwspec->fwnode) && fwspec->param_count == 2) > return irq_domain_translate_twocell(d, fwspec, hwirq, type); > - } Unify with the patch above removing {}. > /* If a parent irqdomain is provided, let's build a hierarchy */ > if (gpiochip_hierarchy_is_hierarchical(gc)) { > int ret = gpiochip_hierarchy_add_domain(gc); > + > if (ret) > return ret; Similar as above. And so on... -- With Best Regards, Andy Shevchenko