Hi,
On Mon, Apr 4, 2016 at 10:11 PM, Michael Haas <haas@xxxxxxxxxxxxxxxxxxxx> wrote:
> 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.
If they are short circuited, does that mean the PMIC will only
draw power from one source? If both are still used, then the
current and voltage readings still make sense. Then it makes
sense to have both drivers enabled, no?
Or would something bad happen?
ChenYu
>>
>>> + /* 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@xxxxxxxxxxxxxxxxx>");
>>> +MODULE_DESCRIPTION("AXP20x PMIC AC power supply status driver");
>>> +MODULE_LICENSE("GPL");
>>> --
>>> 2.8.0
>>
>> Thanks!
>> Maxime
>>
>
>
--
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
