From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michael Haas Subject: Re: [PATCH 1/4] power: Add an axp20x-ac-power driver Date: Tue, 5 Apr 2016 07:11:02 +0200 Message-ID: <570348E6.5060107@computerlinguist.org> References: <1459689307-8501-1-git-send-email-haas@computerlinguist.org> <20160404213851.GQ4227@lukather> Reply-To: haas-bdq14YP6qtSV9CzYT+GlPGD2FQJk+8+b@public.gmane.org Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="B7K558WWe7e2eP66ci2df62gvgh0TGAkm" Return-path: Sender: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org In-Reply-To: <20160404213851.GQ4227@lukather> List-Post: , List-Help: , List-Archive: , List-Unsubscribe: , To: Maxime Ripard Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, pawel.moll-5wv7dgnIgG8@public.gmane.org, mark.rutland-5wv7dgnIgG8@public.gmane.org, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org, galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org, wens-jdAy2FN1RRM@public.gmane.org, lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org, sre-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, dbaryshkov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org, dwmw2-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org, linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-pm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, bonbons-ud5FBsm0p/xEiooADzr8i9i2O/JbrIOy@public.gmane.org, hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org List-Id: devicetree@vger.kernel.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --B7K558WWe7e2eP66ci2df62gvgh0TGAkm Content-Type: multipart/mixed; boundary="BkrpIVgbtj2qcX675pamwE7coReK5e8Jh" From: Michael Haas To: Maxime Ripard Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, pawel.moll-5wv7dgnIgG8@public.gmane.org, mark.rutland-5wv7dgnIgG8@public.gmane.org, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org, galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org, wens-jdAy2FN1RRM@public.gmane.org, lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org, sre-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, dbaryshkov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org, dwmw2-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org, linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-pm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, bonbons-ud5FBsm0p/xEiooADzr8i9i2O/JbrIOy@public.gmane.org, hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org Message-ID: <570348E6.5060107-bdq14YP6qtSV9CzYT+GlPGD2FQJk+8+b@public.gmane.org> Subject: Re: [PATCH 1/4] power: Add an axp20x-ac-power driver References: <1459689307-8501-1-git-send-email-haas-bdq14YP6qtSV9CzYT+GlPGD2FQJk+8+b@public.gmane.org> <20160404213851.GQ4227@lukather> In-Reply-To: <20160404213851.GQ4227@lukather> --BkrpIVgbtj2qcX675pamwE7coReK5e8Jh Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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_resource= s[] =3D { >> }, >> }; >> =20 >> +static struct resource axp20x_ac_power_supply_resources[] =3D { >> + 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[] =3D { >> { >> .start =3D AXP288_IRQ_QWBTU, >> @@ -440,6 +446,11 @@ static struct mfd_cell axp20x_cells[] =3D { >> .of_compatible =3D "x-powers,axp202-usb-power-supply", >> .num_resources =3D ARRAY_SIZE(axp20x_usb_power_supply_resources), >> .resources =3D axp20x_usb_power_supply_resources, >> + }, { >> + .name =3D "axp20x-ac-power-supply", >> + .of_compatible =3D "x-powers,axp202-ac-power-supply", >> + .num_resources =3D ARRAY_SIZE(axp20x_ac_power_supply_resources), >> + .resources =3D axp20x_ac_power_supply_resources, >> }, >> }; >> >=20 > These changes should be in a separate patch. Will do! >=20 >> + >> +static irqreturn_t axp20x_irq_ac_removal(int irq, void *devid) >> +{ >> + struct axp20x_ac_power *power =3D devid; >> + >> + dev_info(&power->supply->dev, "IRQ#%d AC disconnected\n", irq); >> + power_supply_changed(power->supply); >> + >> + return IRQ_HANDLED; >> +} >=20 > 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. >=20 > Removing that handler, you can register the same interrupt handler on > all the interrupts. >=20 Oops. Yes, I will fix that! >> +static int axp20x_ac_power_probe(struct platform_device *pdev) >> +{ >> + struct axp20x_dev *axp20x =3D dev_get_drvdata(pdev->dev.parent); >> + struct power_supply_config psy_cfg =3D {}; >> + struct axp20x_ac_power *power; >> + static const char * const irq_names[] =3D { "ACIN_PLUGIN", >> + "ACIN_REMOVAL", "ACIN_OVER_V" }; >> + irqreturn_t (*irq_funcs[])(int, void *) =3D { 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; >=20 > That's useless. If the device is not available, you're not going to be > probed in the first place. Ok. >=20 >> + power =3D devm_kzalloc(&pdev->dev, sizeof(*power), GFP_KERNEL); >> + if (!power) >> + return -ENOMEM; >> + >> + power->regmap =3D axp20x->regmap; >> + >> + r =3D 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-su= pply driver instead!"); >> + return -ENODEV; >> + } >=20 > Can't that change over time? Can't we support both drivers at the same ti= me? 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. >=20 >> + /* Enable ac voltage and current measurement */ >> + r =3D 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 =3D pdev->dev.of_node; >> + psy_cfg.drv_data =3D power; >> + >> + power->supply =3D 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 =3D 0; i < ARRAY_SIZE(irq_names); i++) { >> + irq =3D 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 =3D regmap_irq_get_virq(axp20x->regmap_irqc, irq); >> + r =3D 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[] =3D { >> + { .compatible =3D "x-powers,axp202-ac-power-supply" }, >> + { } >> +}; >> +MODULE_DEVICE_TABLE(of, axp20x_ac_power_match); >> + >> +static struct platform_driver axp20x_ac_power_driver =3D { >> + .probe =3D axp20x_ac_power_probe, >> + .driver =3D { >> + .name =3D DRVNAME, >> + .of_match_table =3D axp20x_ac_power_match, >> + }, >> +}; >> + >> +module_platform_driver(axp20x_ac_power_driver); >> + >> +MODULE_AUTHOR("Bruno Pr=C3=A9mont "); >> +MODULE_DESCRIPTION("AXP20x PMIC AC power supply status driver"); >> +MODULE_LICENSE("GPL"); >> --=20 >> 2.8.0 >=20 > Thanks! > Maxime >=20 --=20 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 e= mail to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org For more options, visit https://groups.google.com/d/optout. --BkrpIVgbtj2qcX675pamwE7coReK5e8Jh-- --B7K558WWe7e2eP66ci2df62gvgh0TGAkm Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJXA0jsAAoJEARK++Ppe5yKzFAH/0vfvkd/6sEvirjsAdSY7q50 EFPZ6/Nb/w7mNKj2UFT58r9ntGBkYUNyOByWHKQrph2G1tTKdMHxrDdHfE6vvQES qGMjpTMIyReaqGP9AiIhw6ebdc6PSgff0eSgpskzAzlp2LT6rHp6O5FM5gx0Ro8J U1ENF3WJznvKePOzd4NOSsWhpZOBRmmSvti3m6Demxc85KhLoKEWt0tjibukzRez GD8/JgWQaL6tpNH8F5/s+lxLBnDsixo2W9rps1KYNF95ed7j1C9/Z5VOqlX8/Rzi 5LvHBJrUGoOE2Dmm7WsbFfgBLhaAX2YvQKejavnCKTJyt49sAuoztpnmCThU2VQ= =B8PD -----END PGP SIGNATURE----- --B7K558WWe7e2eP66ci2df62gvgh0TGAkm--