Hi Quentin, The driver looks mostly fine. I do have a two comments, though. On Mon, Jan 02, 2017 at 05:37:08PM +0100, Quentin Schulz wrote: > [...] > > +static int axp20x_ac_power_probe(struct platform_device *pdev) > +{ > + static const char * const axp20x_irq_names[] = { "ACIN_PLUGIN", > + "ACIN_REMOVAL", NULL }; > > + static const char * const *irq_names; > + const struct power_supply_desc *ac_power_desc; > + int i, irq, ret; > + > + if (!of_device_is_available(pdev->dev.of_node)) > + return -ENODEV; > + > + if (!axp20x) { > + dev_err(&pdev->dev, "Parent drvdata not set\n"); > + return -EINVAL; > + } axp20x will no longer be needed after implementing below comments. > [...] > + irq_names = axp20x_irq_names; Just rename axp20x_irq_names into irq_names, since its only used here. > [...] > > + power->np = pdev->dev.of_node; This can be dropped, it's not used at all. > + power->regmap = axp20x->regmap; power->regmap = dev_get_regmap(pdev->dev.parent, NULL); > [...] > + /* Request irqs after registering, as irqs may trigger immediately */ > + for (i = 0; irq_names[i]; i++) { > + irq = platform_get_irq_byname(pdev, irq_names[i]); > + if (irq < 0) { > + dev_warn(&pdev->dev, "No IRQ for %s: %d\n", > + irq_names[i], irq); > + continue; > + } > + irq = regmap_irq_get_virq(axp20x->regmap_irqc, irq); The mapping should actually happen in the mfd driver, so that the platform resource contains a valid irq. > + ret = devm_request_any_context_irq(&pdev->dev, irq, > + axp20x_ac_power_irq, 0, > + DRVNAME, power); > + if (ret < 0) > + dev_warn(&pdev->dev, "Error requesting %s IRQ: %d\n", > + irq_names[i], ret); > + } -- Sebastian