Re: [PATCH 1/4] power: Add an axp20x-ac-power driver

[Michael Haas <haas-bdq14YP6qtSV9CzYT+GlPGD2FQJk+8+b@public.gmane.org>]

[-- Attachment #1.1: Type: text/plain, Size: 6394 bytes --]

Hi Maxime,

thanks for taking the time to review this.

On 04/04/2016 11:38 PM, Maxime Ripard wrote:
> Hi,
>>
>> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
>> index a57d6e9..9351c0e 100644
>> --- a/drivers/mfd/axp20x.c
>> +++ b/drivers/mfd/axp20x.c
>> @@ -178,6 +178,12 @@ static struct resource axp288_power_button_resources[] = {
>>  	},
>>  };
>>  
>> +static struct resource axp20x_ac_power_supply_resources[] = {
>> +	DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_ACIN_PLUGIN, "ACIN_PLUGIN"),
>> +	DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_ACIN_REMOVAL, "ACIN_REMOVAL"),
>> +	DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_ACIN_OVER_V, "ACIN_OVER_V"),
>> +};
>> +
>>  static struct resource axp288_fuel_gauge_resources[] = {
>>  	{
>>  		.start = AXP288_IRQ_QWBTU,
>> @@ -440,6 +446,11 @@ static struct mfd_cell axp20x_cells[] = {
>>  		.of_compatible	= "x-powers,axp202-usb-power-supply",
>>  		.num_resources	= ARRAY_SIZE(axp20x_usb_power_supply_resources),
>>  		.resources	= axp20x_usb_power_supply_resources,
>> +	}, {
>> +		.name		= "axp20x-ac-power-supply",
>> +		.of_compatible	= "x-powers,axp202-ac-power-supply",
>> +		.num_resources	= ARRAY_SIZE(axp20x_ac_power_supply_resources),
>> +		.resources	= axp20x_ac_power_supply_resources,
>>  	},
>>  };
>>
> 
> These changes should be in a separate patch.

Will do!
> 

>> +
>> +static irqreturn_t axp20x_irq_ac_removal(int irq, void *devid)
>> +{
>> +	struct axp20x_ac_power *power = devid;
>> +
>> +	dev_info(&power->supply->dev, "IRQ#%d AC disconnected\n", irq);
>> +	power_supply_changed(power->supply);
>> +
>> +	return IRQ_HANDLED;
>> +}
> 
> Logging in the interrupt handler is usually a bad idea, for several
> reasons:
>    - If you have a console, it's going to be output on the console,
>      which might take quite some time. And you don't want to take
>      quite some time in the interrupt handler.
>    - printk might not even work in the interrupt context in some
>      scenarios.
> 
> Removing that handler, you can register the same interrupt handler on
> all the interrupts.
> 

Oops. Yes, I will fix that!

>> +static int axp20x_ac_power_probe(struct platform_device *pdev)
>> +{
>> +	struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
>> +	struct power_supply_config psy_cfg = {};
>> +	struct axp20x_ac_power *power;
>> +	static const char * const irq_names[] = { "ACIN_PLUGIN",
>> +		"ACIN_REMOVAL", "ACIN_OVER_V" };
>> +	irqreturn_t (*irq_funcs[])(int, void *) = { axp20x_irq_ac_plugin,
>> +		axp20x_irq_ac_removal, axp20x_irq_ac_over_v };
>> +	int i, irq, r, input;
>> +
>> +	if (!of_device_is_available(pdev->dev.of_node))
>> +		return -ENODEV;
> 
> That's useless. If the device is not available, you're not going to be
> probed in the first place.

Ok.

> 
>> +	power = devm_kzalloc(&pdev->dev, sizeof(*power), GFP_KERNEL);
>> +	if (!power)
>> +		return -ENOMEM;
>> +
>> +	power->regmap = axp20x->regmap;
>> +
>> +	r = regmap_read(power->regmap, AXP20X_PWR_INPUT_STATUS, &input);
>> +	if (r < 0)
>> +		return r;
>> +
>> +	if (!!(input & AXP20X_PWR_STATUS_AC_VBUS_SHORT)) {
>> +		dev_err(&pdev->dev, "AC is connected to VBUS. Use axp20x_usb_power-supply driver instead!");
>> +		return -ENODEV;
>> +	}
> 
> Can't that change over time? Can't we support both drivers at the same time?

Both drivers are supported at the same time. I did even try with both
AC and USB connected and both return meaningful values, at least for
voltage. The AXP20x can drawn power from both sources at the same time.

The check above fires in a specific scenario where ACIN and VBUS are
physically connected on the PCB. The Datasheet states for that
particular register:

"Indicates whether ACIN/VBUS short circuits on PCB or not"

So, assuming the chip detects the condition correctly, this does not
change over that. But you can switch from ACIN to VBUS and vice-versa
just fine if they are not short-circuited. If they are connected
together, I'd prefer the DTS to only enable the usb driver. The check
above is a last resort there.

Then again, with the quality of those data sheets, one never knows.

> 
>> +	/* Enable ac voltage and current measurement */
>> +	r = regmap_update_bits(power->regmap, AXP20X_ADC_EN1,
>> +			AXP20X_ADC_EN1_ACIN_CURR | AXP20X_ADC_EN1_ACIN_VOLT,
>> +			AXP20X_ADC_EN1_ACIN_CURR | AXP20X_ADC_EN1_ACIN_VOLT);
>> +	if (r)
>> +		return r;
>> +
>> +	psy_cfg.of_node = pdev->dev.of_node;
>> +	psy_cfg.drv_data = power;
>> +
>> +	power->supply = devm_power_supply_register(&pdev->dev,
>> +					&axp20x_ac_power_desc, &psy_cfg);
>> +	if (IS_ERR(power->supply))
>> +		return PTR_ERR(power->supply);
>> +
>> +	/* Request irqs after registering, as irqs may trigger immediately */
>> +	for (i = 0; i < ARRAY_SIZE(irq_names); 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);
>> +		r = devm_request_any_context_irq(&pdev->dev, irq,
>> +				irq_funcs[i], 0, DRVNAME, power);
>> +		if (r < 0)
>> +			dev_warn(&pdev->dev, "Error requesting %s IRQ: %d\n",
>> +				 irq_names[i], r);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id axp20x_ac_power_match[] = {
>> +	{ .compatible = "x-powers,axp202-ac-power-supply" },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, axp20x_ac_power_match);
>> +
>> +static struct platform_driver axp20x_ac_power_driver = {
>> +	.probe = axp20x_ac_power_probe,
>> +	.driver = {
>> +		.name = DRVNAME,
>> +		.of_match_table = axp20x_ac_power_match,
>> +	},
>> +};
>> +
>> +module_platform_driver(axp20x_ac_power_driver);
>> +
>> +MODULE_AUTHOR("Bruno Prémont <bonbons-ud5FBsm0p/xEiooADzr8i9i2O/JbrIOy@public.gmane.org>");
>> +MODULE_DESCRIPTION("AXP20x PMIC AC power supply status driver");
>> +MODULE_LICENSE("GPL");
>> -- 
>> 2.8.0
> 
> Thanks!
> Maxime
> 


-- 
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]