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
--
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
