Re: [PATCH 08/22] power: supply: add AC power supply driver for AXP20X and AXP22X PMICs

[Quentin Schulz <quentin.schulz@free-electrons.com>]

Hi Sebastian,

On 17/01/2017 04:00, Sebastian Reichel wrote:
> 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);
> 

ACK on everything above.

>> [...]
>> +	/* 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.
> 

I've come with this solution:

------------------------------------------------------------------------
diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
index 012c064..117eacb 100644
--- a/drivers/mfd/axp20x.c
+++ b/drivers/mfd/axp20x.c
@@ -882,7 +882,7 @@ EXPORT_SYMBOL(axp20x_match_device);

 int axp20x_device_probe(struct axp20x_dev *axp20x)
 {
-	int ret;
+	int ret, irq_base;

 	ret = regmap_add_irq_chip(axp20x->regmap, axp20x->irq,
 				  IRQF_ONESHOT | IRQF_SHARED, -1,
@@ -893,8 +893,9 @@ int axp20x_device_probe(struct axp20x_dev *axp20x)
 		return ret;
 	}

+	irq_base = regmap_irq_chip_get_base(axp20x->regmap_irqc);
 	ret = mfd_add_devices(axp20x->dev, -1, axp20x->cells,
-			      axp20x->nr_cells, NULL, 0, NULL);
+			      axp20x->nr_cells, NULL, irq_base, NULL);

 	if (ret) {
 		dev_err(axp20x->dev, "failed to add MFD devices: %d\n", ret);
------------------------------------------------------------------------

However, this implies that all cells added by the mfd driver which are
requesting irqs will need to be changed in the same commit to remove the
regmap_irq_get_virq calls. If we don't modify the drivers, they will
purely fail to request the irqs.

The impacted drivers are the following:

 - drivers/extcon/extcon-axp288.c
 - drivers/input/misc/axp20x-pek.c
 - drivers/power/supply/axp20x_usb_power.c
 - drivers/power/supply/axp288_charger.c
 - drivers/power/supply/axp288_fuel_gauge.c

Is it really worth to do such a cleanup? I'm assuming that impacting
four different subsystems at the same time might require a bit of time
to make the patch into the kernel. I don't see also another way than
doing one single patch for all changes since the changes in the mfd
driver will break all aforementioned drivers.

Thanks,
Quentin

-- 
Quentin Schulz, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com