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
Attachment:
signature.asc
Description: PGP signature
