diff mbox

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

Message ID 6a3edb64-8ea9-cb14-b826-d6e8e2c4c4b7@free-electrons.com (mailing list archive)
State New, archived
Headers show

Commit Message

Quentin Schulz Jan. 26, 2017, 1:32 p.m. UTC 
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:

------------------------------------------------------------------------
------------------------------------------------------------------------

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

Comments

Maxime Ripard Jan. 27, 2017, 8:20 a.m. UTC | #1 
On Thu, Jan 26, 2017 at 02:32:21PM +0100, Quentin Schulz wrote:
> 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?

Yes. The current behaviour goes against what everyone is expecting
from the API.

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

However, I think that can be fixed in a later, independant serie. This
serie is quite big already and this has been long overdue, so I'd
really like not to delay it once again because of a dependency on a
cross-tree cleanup.

Maxime
Jonathan Cameron Jan. 28, 2017, 2:30 p.m. UTC | #2 
On 27/01/17 08:20, Maxime Ripard wrote:
> On Thu, Jan 26, 2017 at 02:32:21PM +0100, Quentin Schulz wrote:
>> 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?
> 
> Yes. The current behaviour goes against what everyone is expecting
> from the API.
> 
>> 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.
> 
> However, I think that can be fixed in a later, independant serie. This
> serie is quite big already and this has been long overdue, so I'd
> really like not to delay it once again because of a dependency on a
> cross-tree cleanup.
It's not that cross tree really. Lining up this level of change to
go through an immutable branch pulled into each of the relevant trees
isn't too hard to arrange.

Jonathan
> 
> Maxime
>
Sebastian Reichel Jan. 29, 2017, 3:16 p.m. UTC | #3 
Hi,

On Sat, Jan 28, 2017 at 02:30:22PM +0000, Jonathan Cameron wrote:
> On 27/01/17 08:20, Maxime Ripard wrote:
> > On Thu, Jan 26, 2017 at 02:32:21PM +0100, Quentin Schulz wrote:
> >> 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

So mostly power-supply, which is affected by this series anyways.
Only input & extcon are added.

> >> Is it really worth to do such a cleanup?
> > 
> > Yes. The current behaviour goes against what everyone is expecting
> > from the API.
> >
> >> 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.
> > 
> > However, I think that can be fixed in a later, independant serie. This
> > serie is quite big already and this has been long overdue, so I'd
> > really like not to delay it once again because of a dependency on a
> > cross-tree cleanup.
>
> It's not that cross tree really. Lining up this level of change to
> go through an immutable branch pulled into each of the relevant trees
> isn't too hard to arrange.

I'm fine with doing this as a separate series, if it follows
directly behind this one. Mainlined drivers tend to be used as
template for new ones and this invalid IRQ resources are a bad
example.

-- Sebastian
diff mbox

Patch

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