Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1560225 > unrolled thread

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

Started bySebastian Reichel <sre@kernel.org>
First post2017-01-17 04:10 +0100
Last post2017-01-29 16:30 +0100
Articles 5 — 4 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 08/22] power: supply: add AC power supply driver for  AXP20X and AXP22X PMICs Sebastian Reichel <sre@kernel.org> - 2017-01-17 04:10 +0100
    Re: [PATCH 08/22] power: supply: add AC power supply driver for  AXP20X and AXP22X PMICs Quentin Schulz <quentin.schulz@free-electrons.com> - 2017-01-26 14:40 +0100
      Re: [PATCH 08/22] power: supply: add AC power supply driver for  AXP20X and AXP22X PMICs Maxime Ripard <maxime.ripard@free-electrons.com> - 2017-01-27 09:40 +0100
        Re: [PATCH 08/22] power: supply: add AC power supply driver for  AXP20X and AXP22X PMICs Jonathan Cameron <jic23@kernel.org> - 2017-01-28 15:40 +0100
          Re: [PATCH 08/22] power: supply: add AC power supply driver for  AXP20X and AXP22X PMICs Sebastian Reichel <sre@kernel.org> - 2017-01-29 16:30 +0100

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

FromSebastian Reichel <sre@kernel.org>
Date2017-01-17 04:10 +0100
SubjectRe: [PATCH 08/22] power: supply: add AC power supply driver for AXP20X and AXP22X PMICs
Message-ID<t0sjM-1mg-7@gated-at.bofh.it>

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [next] | [standalone]

#1567435

FromQuentin Schulz <quentin.schulz@free-electrons.com>
Date2017-01-26 14:40 +0100
Message-ID<t3Sro-7Yz-19@gated-at.bofh.it>
In reply to#1560225
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

[toc] | [prev] | [next] | [standalone]

#1567952

FromMaxime Ripard <maxime.ripard@free-electrons.com>
Date2017-01-27 09:40 +0100
Message-ID<t4aeB-1P5-11@gated-at.bofh.it>
In reply to#1567435

[Multipart message — attachments visible in raw view] — view raw

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

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

[toc] | [prev] | [next] | [standalone]

#1568941

FromJonathan Cameron <jic23@kernel.org>
Date2017-01-28 15:40 +0100
Message-ID<t4Cky-2CT-25@gated-at.bofh.it>
In reply to#1567952
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
>

[toc] | [prev] | [next] | [standalone]

#1569217

FromSebastian Reichel <sre@kernel.org>
Date2017-01-29 16:30 +0100
Message-ID<t4ZAu-ge-25@gated-at.bofh.it>
In reply to#1568941

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web