| Message ID | 20231106-b4-qcom-dt-compat-v1-2-0ccbb7841241@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Qualcomm PMIC fixes | expand |
On Mon, Nov 06, 2023 at 08:57:30PM +0000, Caleb Connolly wrote: > The power and resin keys were implemented as GPIOs here, but their only > use would be as buttons. Avoid the additional layer of introspection and > rework this driver into a button driver. > > While we're here, replace the "qcom,pm8998-pwrkey" compatible with > "qcom,pm8941-pwrkey" to match upstream (Linux). > > The dragonboard410c and 820c boards are adjusted to benefit from this > change too, simplify their custom board init code. > > Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> Thanks a lot for working on bringing the Qualcomm DTs in U-Boot closer to Linux upstream! I agree that modelling the pwr/resin keys is better than exposing tham as GPIOs. I'm a bit confused about the actual diff in this patch series though. Did you perhaps forget to make some changes you had planned or sent the wrong version? In particular: - You talk about replacing the custom "qcom,pm8998-pwrkey" compatible with "qcom,pm8941-pwrkey" to match upstream, but don't seem to adjust the users (sdm845.dtsi)? - sdm845.dtsi also uses GPIOs for PON, but you only update DB410c and DB820c. Isn't SDM845 the platform you're testing on? Some more comments below. > --- > arch/arm/dts/dragonboard410c-uboot.dtsi | 11 +- > arch/arm/dts/dragonboard820c-uboot.dtsi | 9 +- > arch/arm/dts/dragonboard820c.dts | 3 - > board/qualcomm/dragonboard410c/dragonboard410c.c | 29 ++-- > board/qualcomm/dragonboard820c/dragonboard820c.c | 29 ++-- > drivers/gpio/Kconfig | 3 +- > drivers/gpio/qcom_pmic_gpio.c | 161 +++++++++++++++-------- > 7 files changed, 139 insertions(+), 106 deletions(-) > > diff --git a/arch/arm/dts/dragonboard410c-uboot.dtsi b/arch/arm/dts/dragonboard410c-uboot.dtsi > index 3b0bd0ed0a1b..c96f1fcc8930 100644 > --- a/arch/arm/dts/dragonboard410c-uboot.dtsi > +++ b/arch/arm/dts/dragonboard410c-uboot.dtsi > @@ -5,6 +5,9 @@ > * (C) Copyright 2015 Mateusz Kulikowski <mateusz.kulikowski@gmail.com> > */ > > +#include <dt-bindings/input/linux-event-codes.h> > +#include <dt-bindings/interrupt-controller/irq.h> > + > / { > > smem { > @@ -46,10 +49,14 @@ > > &pm8916_pon { > key_vol_down { > - gpios = <&pm8916_pon 1 0>; > + interrupts = <0x0 0x8 1 IRQ_TYPE_EDGE_BOTH>; > + linux,code = <KEY_DOWN>; > + label = "key_vol_down"; > }; > > key_power { > - gpios = <&pm8916_pon 0 0>; > + interrupts = <0x0 0x8 0 IRQ_TYPE_EDGE_BOTH>; > + linux,code = <KEY_ENTER>; > + label = "key_power"; > }; > }; The upstream Linux DT looks like this: pon@800 { compatible = "qcom,pm8916-pon"; reg = <0x800>; mode-bootloader = <0x2>; mode-recovery = <0x1>; pwrkey { compatible = "qcom,pm8941-pwrkey"; interrupts = <0x0 0x8 0 IRQ_TYPE_EDGE_BOTH>; debounce = <15625>; bias-pull-up; linux,code = <KEY_POWER>; }; pm8916_resin: resin { compatible = "qcom,pm8941-resin"; interrupts = <0x0 0x8 1 IRQ_TYPE_EDGE_BOTH>; debounce = <15625>; bias-pull-up; linux,code = <KEY_VOLUME_DOWN>; }; }; The new version you add is closer to upstream, but you also add a new custom property called "label". You could just derive a unique label from the node name ("pwrkey" vs "resin"). For looking up the buttons in the DB410c/DB820c couldn't you just loop over all buttons and find a suitable one based on button_get_code()? I think having different *linux*,code values (KEY_POWER vs KEY_ENTER and KEY_DOWN vs KEY_VOLUME_DOWN) is also a bit strange. If U-Boot wants different key codes it's kind of not the Linux code anymore, might as well call it "u-boot,code" then. :-) If KEY_POWER => KEY_ENTER and KEY_VOLUME_DOWN => KEY_DOWN is more useful for U-Boot maybe that mapping could be done automatically in the code, without having to change the real hardware description in the DT. > diff --git a/arch/arm/dts/dragonboard820c.dts b/arch/arm/dts/dragonboard820c.dts > index ad201d48749c..7db0cc9d64cc 100644 > --- a/arch/arm/dts/dragonboard820c.dts > +++ b/arch/arm/dts/dragonboard820c.dts > @@ -112,9 +112,6 @@ > pm8994_pon: pm8994_pon@800 { > compatible = "qcom,pm8994-pwrkey"; > reg = <0x800 0x96>; > - #gpio-cells = <2>; > - gpio-controller; > - gpio-bank-name="pm8994_key."; > }; Shouldn't we do the same change for pm8916_pon in db410c.dts? > diff --git a/drivers/gpio/qcom_pmic_gpio.c b/drivers/gpio/qcom_pmic_gpio.c > index e5841f502953..3dbc02d83198 100644 > --- a/drivers/gpio/qcom_pmic_gpio.c > +++ b/drivers/gpio/qcom_pmic_gpio.c > @@ -5,8 +5,10 @@ > * (C) Copyright 2015 Mateusz Kulikowski <mateusz.kulikowski@gmail.com> > */ > > +#include <button.h> > #include <common.h> > #include <dm.h> > +#include <dm/lists.h> > #include <log.h> > #include <power/pmic.h> > #include <spmi/spmi.h> > @@ -275,107 +277,150 @@ U_BOOT_DRIVER(qcom_pmic_gpio) = { > .priv_auto = sizeof(struct qcom_gpio_bank), > }; > > +struct qcom_pmic_btn_priv { > + u32 base; > + u32 status_bit; > + int code; > + struct udevice *pmic; > +}; > > /* Add pmic buttons as GPIO as well - there is no generic way for now */ > #define PON_INT_RT_STS 0x10 > #define KPDPWR_ON_INT_BIT 0 > #define RESIN_ON_INT_BIT 1 > > -static int qcom_pwrkey_get_function(struct udevice *dev, unsigned offset) > +static enum button_state_t qcom_pwrkey_get_state(struct udevice *dev) > { > - return GPIOF_INPUT; > -} > + struct qcom_pmic_btn_priv *priv = dev_get_priv(dev); > > -static int qcom_pwrkey_get_value(struct udevice *dev, unsigned offset) > -{ > - struct qcom_gpio_bank *priv = dev_get_priv(dev); > - > - int reg = pmic_reg_read(dev->parent, priv->pid + PON_INT_RT_STS); > + int reg = pmic_reg_read(priv->pmic, priv->base + PON_INT_RT_STS); > > if (reg < 0) > return 0; > > - switch (offset) { > - case 0: /* Power button */ > - return (reg & BIT(KPDPWR_ON_INT_BIT)) != 0; > - break; > - case 1: /* Reset button */ > - default: > - return (reg & BIT(RESIN_ON_INT_BIT)) != 0; > - break; > - } > + return (reg & BIT(priv->status_bit)) != 0; > } > > -/* > - * Since pmic buttons modelled as GPIO, we need empty direction functions > - * to trick u-boot button driver > - */ > -static int qcom_pwrkey_direction_input(struct udevice *dev, unsigned int offset) > +static int qcom_pwrkey_get_code(struct udevice *dev) > { > - return 0; > -} > + struct qcom_pmic_btn_priv *priv = dev_get_priv(dev); > > -static int qcom_pwrkey_direction_output(struct udevice *dev, unsigned int offset, int value) > -{ > - return -EOPNOTSUPP; > + return priv->code; > } > > -static const struct dm_gpio_ops qcom_pwrkey_ops = { > - .get_value = qcom_pwrkey_get_value, > - .get_function = qcom_pwrkey_get_function, > - .direction_input = qcom_pwrkey_direction_input, > - .direction_output = qcom_pwrkey_direction_output, > -}; > - > static int qcom_pwrkey_probe(struct udevice *dev) > { > - struct qcom_gpio_bank *priv = dev_get_priv(dev); > - int reg; > - u64 pid; > + struct button_uc_plat *uc_plat = dev_get_uclass_plat(dev); > + struct qcom_pmic_btn_priv *priv = dev_get_priv(dev); > + int ret; > + u64 base; > > - pid = dev_read_addr(dev); > - if (pid == FDT_ADDR_T_NONE) > - return log_msg_ret("bad address", -EINVAL); > + /* Ignore the top-level button node */ > + if (!uc_plat->label) > + return 0; > > - priv->pid = pid; > + /* the pwrkey and resin nodes are children of the "pon" node, get the > + * PMIC device to use in pmic_reg_* calls. > + */ > + priv->pmic = dev->parent->parent; > + > + base = dev_read_addr(dev); > + if (!base || base == FDT_ADDR_T_NONE) { > + /* Linux devicetrees don't specify an address in the pwrkey node */ > + base = dev_read_addr(dev->parent); > + if (base == FDT_ADDR_T_NONE) { > + printf("%s: Can't find address\n", dev->name); > + return -EINVAL; > + } > + } Is it worth introducing new code that supports non-standard Linux DTs? Or do we need to stay compatible with old U-Boot DTs too? Would expect those are always bundled together with U-Boot. > + > + priv->base = base; > > /* Do a sanity check */ > - reg = pmic_reg_read(dev->parent, priv->pid + REG_TYPE); > - if (reg != 0x1) > - return log_msg_ret("bad type", -ENXIO); > + ret = pmic_reg_read(priv->pmic, priv->base + REG_TYPE); > + if (ret != 0x1 && ret != 0xb) { > + printf("%s: unexpected PMIC function type %d\n", dev->name, ret); > + return -ENXIO; > + } > > - reg = pmic_reg_read(dev->parent, priv->pid + REG_SUBTYPE); > - if ((reg & 0x5) == 0) > - return log_msg_ret("bad subtype", -ENXIO); > + ret = pmic_reg_read(priv->pmic, priv->base + REG_SUBTYPE); > + if ((ret & 0x7) == 0) { > + printf("%s: unexpected PMCI function subtype %d\n", dev->name, ret); > + //return -ENXIO; I guess this shouldn't be commented out? :D > + } > + > + /* Bit of a hack, we use the interrupt number to derive if this is the pwrkey or resin > + * node, it just so happens to line up with the bit numbers in the interrupt status register > + */ > + ret = ofnode_read_u32_index(dev_ofnode(dev), "interrupts", 2, &priv->status_bit); > + if (ret < 0) { > + printf("%s: Couldn't read interrupts: %d\n", __func__, ret); > + return ret; > + } > + > + ret = ofnode_read_u32(dev_ofnode(dev), "linux,code", &priv->code); > + if (ret < 0) { > + printf("%s: Couldn't read interrupts: %d\n", __func__, ret); > + return ret; > + } > > return 0; > } > > -static int qcom_pwrkey_of_to_plat(struct udevice *dev) > +static int button_qcom_pmic_bind(struct udevice *parent) > { > - struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev); > + struct udevice *dev; > + ofnode node; > + int ret; > > - uc_priv->gpio_count = 2; > - uc_priv->bank_name = dev_read_string(dev, "gpio-bank-name"); > - if (uc_priv->bank_name == NULL) > - uc_priv->bank_name = "pwkey_qcom"; > + dev_for_each_subnode(node, parent) { > + struct button_uc_plat *uc_plat; > + const char *label; > + > + if (!ofnode_is_enabled(node)) > + continue; > + > + label = ofnode_read_string(node, "label"); > + if (!label) { > + printf("%s: node %s has no label\n", __func__, > + ofnode_get_name(node)); > + /* Don't break booting, just print a warning and continue */ > + continue; > + } > + /* We need the PMIC device to be the parent, so flatten it out here */ > + ret = device_bind_driver_to_node(parent, "pwrkey_qcom", > + ofnode_get_name(node), > + node, &dev); > + if (ret) { > + printf("Failed to bind %s! %d\n", label, ret); > + return ret; > + } > + uc_plat = dev_get_uclass_plat(dev); > + uc_plat->label = label; > + } > > return 0; > } > > +static const struct button_ops button_qcom_pmic_ops = { > + .get_state = qcom_pwrkey_get_state, > + .get_code = qcom_pwrkey_get_code, > +}; > + > static const struct udevice_id qcom_pwrkey_ids[] = { > { .compatible = "qcom,pm8916-pwrkey" }, > { .compatible = "qcom,pm8994-pwrkey" }, These are also qcom,pm8941-pwrkey upstream. > - { .compatible = "qcom,pm8998-pwrkey" }, > + { .compatible = "qcom,pm8941-pwrkey" }, > + { .compatible = "qcom,pm8998-pon" }, "qcom,pm8998-pon" is the outer container node for pwrkey+resin, while "qcom,pm8941-pwrkey" is the actual power button. Why are both here next to each other? > { } > }; > > U_BOOT_DRIVER(pwrkey_qcom) = { > .name = "pwrkey_qcom", > - .id = UCLASS_GPIO, > + .id = UCLASS_BUTTON, > .of_match = qcom_pwrkey_ids, > - .of_to_plat = qcom_pwrkey_of_to_plat, > + .bind = button_qcom_pmic_bind, > .probe = qcom_pwrkey_probe, > - .ops = &qcom_pwrkey_ops, > - .priv_auto = sizeof(struct qcom_gpio_bank), > + .ops = &button_qcom_pmic_ops, > + .priv_auto = sizeof(struct qcom_pmic_btn_priv), > }; > Can we move this out of the drivers/gpio into drivers/button? Seems like there are now two quite different drivers in the same file. :-) Thanks, Stephan
On 07/11/2023 13:27, Stephan Gerhold wrote: > On Mon, Nov 06, 2023 at 08:57:30PM +0000, Caleb Connolly wrote: >> The power and resin keys were implemented as GPIOs here, but their only >> use would be as buttons. Avoid the additional layer of introspection and >> rework this driver into a button driver. >> >> While we're here, replace the "qcom,pm8998-pwrkey" compatible with >> "qcom,pm8941-pwrkey" to match upstream (Linux). >> >> The dragonboard410c and 820c boards are adjusted to benefit from this >> change too, simplify their custom board init code. >> >> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> > > Thanks a lot for working on bringing the Qualcomm DTs in U-Boot closer > to Linux upstream! I agree that modelling the pwr/resin keys is better > than exposing tham as GPIOs. > > I'm a bit confused about the actual diff in this patch series though. > Did you perhaps forget to make some changes you had planned or sent the > wrong version? > > In particular: > > - You talk about replacing the custom "qcom,pm8998-pwrkey" compatible > with "qcom,pm8941-pwrkey" to match upstream, but don't seem to adjust > the users (sdm845.dtsi)? > > - sdm845.dtsi also uses GPIOs for PON, but you only update DB410c and > DB820c. Isn't SDM845 the platform you're testing on? Argh, yeah you're totally right, I have been testing in the context of upstream sdm845 DT and I forgot to pull in the individual changes for this series. > > Some more comments below. > >> --- >> arch/arm/dts/dragonboard410c-uboot.dtsi | 11 +- >> arch/arm/dts/dragonboard820c-uboot.dtsi | 9 +- >> arch/arm/dts/dragonboard820c.dts | 3 - >> board/qualcomm/dragonboard410c/dragonboard410c.c | 29 ++-- >> board/qualcomm/dragonboard820c/dragonboard820c.c | 29 ++-- >> drivers/gpio/Kconfig | 3 +- >> drivers/gpio/qcom_pmic_gpio.c | 161 +++++++++++++++-------- >> 7 files changed, 139 insertions(+), 106 deletions(-) >> >> diff --git a/arch/arm/dts/dragonboard410c-uboot.dtsi b/arch/arm/dts/dragonboard410c-uboot.dtsi >> index 3b0bd0ed0a1b..c96f1fcc8930 100644 >> --- a/arch/arm/dts/dragonboard410c-uboot.dtsi >> +++ b/arch/arm/dts/dragonboard410c-uboot.dtsi >> @@ -5,6 +5,9 @@ >> * (C) Copyright 2015 Mateusz Kulikowski <mateusz.kulikowski@gmail.com> >> */ >> >> +#include <dt-bindings/input/linux-event-codes.h> >> +#include <dt-bindings/interrupt-controller/irq.h> >> + >> / { >> >> smem { >> @@ -46,10 +49,14 @@ >> >> &pm8916_pon { >> key_vol_down { >> - gpios = <&pm8916_pon 1 0>; >> + interrupts = <0x0 0x8 1 IRQ_TYPE_EDGE_BOTH>; >> + linux,code = <KEY_DOWN>; >> + label = "key_vol_down"; >> }; >> >> key_power { >> - gpios = <&pm8916_pon 0 0>; >> + interrupts = <0x0 0x8 0 IRQ_TYPE_EDGE_BOTH>; >> + linux,code = <KEY_ENTER>; >> + label = "key_power"; >> }; >> }; > > The upstream Linux DT looks like this: > > pon@800 { > compatible = "qcom,pm8916-pon"; > reg = <0x800>; > mode-bootloader = <0x2>; > mode-recovery = <0x1>; > > pwrkey { > compatible = "qcom,pm8941-pwrkey"; > interrupts = <0x0 0x8 0 IRQ_TYPE_EDGE_BOTH>; > debounce = <15625>; > bias-pull-up; > linux,code = <KEY_POWER>; > }; > > pm8916_resin: resin { > compatible = "qcom,pm8941-resin"; > interrupts = <0x0 0x8 1 IRQ_TYPE_EDGE_BOTH>; > debounce = <15625>; > bias-pull-up; > linux,code = <KEY_VOLUME_DOWN>; > }; > }; > > The new version you add is closer to upstream, but you also add a new > custom property called "label". You could just derive a unique label > from the node name ("pwrkey" vs "resin"). I just kinda followed what gpio-button does. I agree though this should be dropped and we can just use the node name. > > For looking up the buttons in the DB410c/DB820c couldn't you just loop > over all buttons and find a suitable one based on button_get_code()? > > I think having different *linux*,code values (KEY_POWER vs KEY_ENTER and > KEY_DOWN vs KEY_VOLUME_DOWN) is also a bit strange. If U-Boot wants > different key codes it's kind of not the Linux code anymore, might as > well call it "u-boot,code" then. :-) Yeah, I don't really know what the right solution is here, in U-Boot we want to use these buttons as controls - like you would in ABL fastboot. > > If KEY_POWER => KEY_ENTER and KEY_VOLUME_DOWN => KEY_DOWN is more useful > for U-Boot maybe that mapping could be done automatically in the code, > without having to change the real hardware description in the DT. That sounds reasonable. > >> diff --git a/arch/arm/dts/dragonboard820c.dts b/arch/arm/dts/dragonboard820c.dts >> index ad201d48749c..7db0cc9d64cc 100644 >> --- a/arch/arm/dts/dragonboard820c.dts >> +++ b/arch/arm/dts/dragonboard820c.dts >> @@ -112,9 +112,6 @@ >> pm8994_pon: pm8994_pon@800 { >> compatible = "qcom,pm8994-pwrkey"; >> reg = <0x800 0x96>; >> - #gpio-cells = <2>; >> - gpio-controller; >> - gpio-bank-name="pm8994_key."; >> }; > > Shouldn't we do the same change for pm8916_pon in db410c.dts? Yes > >> diff --git a/drivers/gpio/qcom_pmic_gpio.c b/drivers/gpio/qcom_pmic_gpio.c >> index e5841f502953..3dbc02d83198 100644 >> --- a/drivers/gpio/qcom_pmic_gpio.c >> +++ b/drivers/gpio/qcom_pmic_gpio.c >> @@ -5,8 +5,10 @@ >> * (C) Copyright 2015 Mateusz Kulikowski <mateusz.kulikowski@gmail.com> >> */ >> >> +#include <button.h> >> #include <common.h> >> #include <dm.h> >> +#include <dm/lists.h> >> #include <log.h> >> #include <power/pmic.h> >> #include <spmi/spmi.h> >> @@ -275,107 +277,150 @@ U_BOOT_DRIVER(qcom_pmic_gpio) = { >> .priv_auto = sizeof(struct qcom_gpio_bank), >> }; >> >> +struct qcom_pmic_btn_priv { >> + u32 base; >> + u32 status_bit; >> + int code; >> + struct udevice *pmic; >> +}; >> >> /* Add pmic buttons as GPIO as well - there is no generic way for now */ >> #define PON_INT_RT_STS 0x10 >> #define KPDPWR_ON_INT_BIT 0 >> #define RESIN_ON_INT_BIT 1 >> >> -static int qcom_pwrkey_get_function(struct udevice *dev, unsigned offset) >> +static enum button_state_t qcom_pwrkey_get_state(struct udevice *dev) >> { >> - return GPIOF_INPUT; >> -} >> + struct qcom_pmic_btn_priv *priv = dev_get_priv(dev); >> >> -static int qcom_pwrkey_get_value(struct udevice *dev, unsigned offset) >> -{ >> - struct qcom_gpio_bank *priv = dev_get_priv(dev); >> - >> - int reg = pmic_reg_read(dev->parent, priv->pid + PON_INT_RT_STS); >> + int reg = pmic_reg_read(priv->pmic, priv->base + PON_INT_RT_STS); >> >> if (reg < 0) >> return 0; >> >> - switch (offset) { >> - case 0: /* Power button */ >> - return (reg & BIT(KPDPWR_ON_INT_BIT)) != 0; >> - break; >> - case 1: /* Reset button */ >> - default: >> - return (reg & BIT(RESIN_ON_INT_BIT)) != 0; >> - break; >> - } >> + return (reg & BIT(priv->status_bit)) != 0; >> } >> >> -/* >> - * Since pmic buttons modelled as GPIO, we need empty direction functions >> - * to trick u-boot button driver >> - */ >> -static int qcom_pwrkey_direction_input(struct udevice *dev, unsigned int offset) >> +static int qcom_pwrkey_get_code(struct udevice *dev) >> { >> - return 0; >> -} >> + struct qcom_pmic_btn_priv *priv = dev_get_priv(dev); >> >> -static int qcom_pwrkey_direction_output(struct udevice *dev, unsigned int offset, int value) >> -{ >> - return -EOPNOTSUPP; >> + return priv->code; >> } >> >> -static const struct dm_gpio_ops qcom_pwrkey_ops = { >> - .get_value = qcom_pwrkey_get_value, >> - .get_function = qcom_pwrkey_get_function, >> - .direction_input = qcom_pwrkey_direction_input, >> - .direction_output = qcom_pwrkey_direction_output, >> -}; >> - >> static int qcom_pwrkey_probe(struct udevice *dev) >> { >> - struct qcom_gpio_bank *priv = dev_get_priv(dev); >> - int reg; >> - u64 pid; >> + struct button_uc_plat *uc_plat = dev_get_uclass_plat(dev); >> + struct qcom_pmic_btn_priv *priv = dev_get_priv(dev); >> + int ret; >> + u64 base; >> >> - pid = dev_read_addr(dev); >> - if (pid == FDT_ADDR_T_NONE) >> - return log_msg_ret("bad address", -EINVAL); >> + /* Ignore the top-level button node */ >> + if (!uc_plat->label) >> + return 0; >> >> - priv->pid = pid; >> + /* the pwrkey and resin nodes are children of the "pon" node, get the >> + * PMIC device to use in pmic_reg_* calls. >> + */ >> + priv->pmic = dev->parent->parent; >> + >> + base = dev_read_addr(dev); >> + if (!base || base == FDT_ADDR_T_NONE) { >> + /* Linux devicetrees don't specify an address in the pwrkey node */ >> + base = dev_read_addr(dev->parent); >> + if (base == FDT_ADDR_T_NONE) { >> + printf("%s: Can't find address\n", dev->name); >> + return -EINVAL; >> + } >> + } > > Is it worth introducing new code that supports non-standard Linux DTs? > Or do we need to stay compatible with old U-Boot DTs too? Would expect > those are always bundled together with U-Boot. I've been aiming to minimise the number of changes made to db410c and db820c as I can't easily test on these boards... I don't think we need to worry about backwards compatibility - the DTB is usually built-in to the U-Boot image. > >> + >> + priv->base = base; >> >> /* Do a sanity check */ >> - reg = pmic_reg_read(dev->parent, priv->pid + REG_TYPE); >> - if (reg != 0x1) >> - return log_msg_ret("bad type", -ENXIO); >> + ret = pmic_reg_read(priv->pmic, priv->base + REG_TYPE); >> + if (ret != 0x1 && ret != 0xb) { >> + printf("%s: unexpected PMIC function type %d\n", dev->name, ret); >> + return -ENXIO; >> + } >> >> - reg = pmic_reg_read(dev->parent, priv->pid + REG_SUBTYPE); >> - if ((reg & 0x5) == 0) >> - return log_msg_ret("bad subtype", -ENXIO); >> + ret = pmic_reg_read(priv->pmic, priv->base + REG_SUBTYPE); >> + if ((ret & 0x7) == 0) { >> + printf("%s: unexpected PMCI function subtype %d\n", dev->name, ret); >> + //return -ENXIO; > > I guess this shouldn't be commented out? :D Nope! Thanks for catching that > >> + } >> + >> + /* Bit of a hack, we use the interrupt number to derive if this is the pwrkey or resin >> + * node, it just so happens to line up with the bit numbers in the interrupt status register >> + */ >> + ret = ofnode_read_u32_index(dev_ofnode(dev), "interrupts", 2, &priv->status_bit); >> + if (ret < 0) { >> + printf("%s: Couldn't read interrupts: %d\n", __func__, ret); >> + return ret; >> + } >> + >> + ret = ofnode_read_u32(dev_ofnode(dev), "linux,code", &priv->code); >> + if (ret < 0) { >> + printf("%s: Couldn't read interrupts: %d\n", __func__, ret); >> + return ret; >> + } >> >> return 0; >> } >> >> -static int qcom_pwrkey_of_to_plat(struct udevice *dev) >> +static int button_qcom_pmic_bind(struct udevice *parent) >> { >> - struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev); >> + struct udevice *dev; >> + ofnode node; >> + int ret; >> >> - uc_priv->gpio_count = 2; >> - uc_priv->bank_name = dev_read_string(dev, "gpio-bank-name"); >> - if (uc_priv->bank_name == NULL) >> - uc_priv->bank_name = "pwkey_qcom"; >> + dev_for_each_subnode(node, parent) { >> + struct button_uc_plat *uc_plat; >> + const char *label; >> + >> + if (!ofnode_is_enabled(node)) >> + continue; >> + >> + label = ofnode_read_string(node, "label"); >> + if (!label) { >> + printf("%s: node %s has no label\n", __func__, >> + ofnode_get_name(node)); >> + /* Don't break booting, just print a warning and continue */ >> + continue; >> + } >> + /* We need the PMIC device to be the parent, so flatten it out here */ >> + ret = device_bind_driver_to_node(parent, "pwrkey_qcom", >> + ofnode_get_name(node), >> + node, &dev); >> + if (ret) { >> + printf("Failed to bind %s! %d\n", label, ret); >> + return ret; >> + } >> + uc_plat = dev_get_uclass_plat(dev); >> + uc_plat->label = label; >> + } >> >> return 0; >> } >> >> +static const struct button_ops button_qcom_pmic_ops = { >> + .get_state = qcom_pwrkey_get_state, >> + .get_code = qcom_pwrkey_get_code, >> +}; >> + >> static const struct udevice_id qcom_pwrkey_ids[] = { >> { .compatible = "qcom,pm8916-pwrkey" }, >> { .compatible = "qcom,pm8994-pwrkey" }, > > These are also qcom,pm8941-pwrkey upstream. > >> - { .compatible = "qcom,pm8998-pwrkey" }, >> + { .compatible = "qcom,pm8941-pwrkey" }, >> + { .compatible = "qcom,pm8998-pon" }, > > "qcom,pm8998-pon" is the outer container node for pwrkey+resin, while > "qcom,pm8941-pwrkey" is the actual power button. Why are both here next > to each other? This is a bit of a U-Boot trick, if you look in the probe function you'll see the comment "Ignore the top-level button node". Basically the same Driver is used to handle the NOP parent node and the children. Otherwise I would have to add a new driver with UCLASS_NOP just to handle the pon node and bind the children. > >> { } >> }; >> >> U_BOOT_DRIVER(pwrkey_qcom) = { >> .name = "pwrkey_qcom", >> - .id = UCLASS_GPIO, >> + .id = UCLASS_BUTTON, >> .of_match = qcom_pwrkey_ids, >> - .of_to_plat = qcom_pwrkey_of_to_plat, >> + .bind = button_qcom_pmic_bind, >> .probe = qcom_pwrkey_probe, >> - .ops = &qcom_pwrkey_ops, >> - .priv_auto = sizeof(struct qcom_gpio_bank), >> + .ops = &button_qcom_pmic_ops, >> + .priv_auto = sizeof(struct qcom_pmic_btn_priv), >> }; >> > > Can we move this out of the drivers/gpio into drivers/button? Seems like > there are now two quite different drivers in the same file. :-) Yeah, fair enough. Will do. > > Thanks, > Stephan
diff --git a/arch/arm/dts/dragonboard410c-uboot.dtsi b/arch/arm/dts/dragonboard410c-uboot.dtsi index 3b0bd0ed0a1b..c96f1fcc8930 100644 --- a/arch/arm/dts/dragonboard410c-uboot.dtsi +++ b/arch/arm/dts/dragonboard410c-uboot.dtsi @@ -5,6 +5,9 @@ * (C) Copyright 2015 Mateusz Kulikowski <mateusz.kulikowski@gmail.com> */ +#include <dt-bindings/input/linux-event-codes.h> +#include <dt-bindings/interrupt-controller/irq.h> + / { smem { @@ -46,10 +49,14 @@ &pm8916_pon { key_vol_down { - gpios = <&pm8916_pon 1 0>; + interrupts = <0x0 0x8 1 IRQ_TYPE_EDGE_BOTH>; + linux,code = <KEY_DOWN>; + label = "key_vol_down"; }; key_power { - gpios = <&pm8916_pon 0 0>; + interrupts = <0x0 0x8 0 IRQ_TYPE_EDGE_BOTH>; + linux,code = <KEY_ENTER>; + label = "key_power"; }; }; diff --git a/arch/arm/dts/dragonboard820c-uboot.dtsi b/arch/arm/dts/dragonboard820c-uboot.dtsi index 457728a43ecb..ed8ac0e5cf1a 100644 --- a/arch/arm/dts/dragonboard820c-uboot.dtsi +++ b/arch/arm/dts/dragonboard820c-uboot.dtsi @@ -5,6 +5,9 @@ * (C) Copyright 2017 Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org> */ +#include <dt-bindings/input/linux-event-codes.h> +#include <dt-bindings/interrupt-controller/irq.h> + / { smem { bootph-all; @@ -33,12 +36,14 @@ &pm8994_pon { key_vol_down { - gpios = <&pm8994_pon 1 0>; + interrupts = <0x0 0x8 1 IRQ_TYPE_EDGE_BOTH>; + linux,code = <KEY_DOWN>; label = "key_vol_down"; }; key_power { - gpios = <&pm8994_pon 0 0>; + interrupts = <0x0 0x8 0 IRQ_TYPE_EDGE_BOTH>; + linux,code = <KEY_ENTER>; label = "key_power"; }; }; diff --git a/arch/arm/dts/dragonboard820c.dts b/arch/arm/dts/dragonboard820c.dts index ad201d48749c..7db0cc9d64cc 100644 --- a/arch/arm/dts/dragonboard820c.dts +++ b/arch/arm/dts/dragonboard820c.dts @@ -112,9 +112,6 @@ pm8994_pon: pm8994_pon@800 { compatible = "qcom,pm8994-pwrkey"; reg = <0x800 0x96>; - #gpio-cells = <2>; - gpio-controller; - gpio-bank-name="pm8994_key."; }; pm8994_gpios: pm8994_gpios@c000 { diff --git a/board/qualcomm/dragonboard410c/dragonboard410c.c b/board/qualcomm/dragonboard410c/dragonboard410c.c index 371b3262f8c5..1d6cabfb9c41 100644 --- a/board/qualcomm/dragonboard410c/dragonboard410c.c +++ b/board/qualcomm/dragonboard410c/dragonboard410c.c @@ -5,6 +5,7 @@ * (C) Copyright 2015 Mateusz Kulikowski <mateusz.kulikowski@gmail.com> */ +#include <button.h> #include <common.h> #include <cpu_func.h> #include <dm.h> @@ -108,30 +109,18 @@ int board_usb_init(int index, enum usb_init_type init) /* Check for vol- button - if pressed - stop autoboot */ int misc_init_r(void) { - struct udevice *pon; - struct gpio_desc resin; - int node, ret; + struct udevice *btn; + int ret; + enum button_state_t state; - ret = uclass_get_device_by_name(UCLASS_GPIO, "pm8916_pon@800", &pon); + ret = button_get_by_label("key_vol_down", &btn); if (ret < 0) { - printf("Failed to find PMIC pon node. Check device tree\n"); - return 0; + printf("Couldn't find power button!\n"); + return ret; } - node = fdt_subnode_offset(gd->fdt_blob, dev_of_offset(pon), - "key_vol_down"); - if (node < 0) { - printf("Failed to find key_vol_down node. Check device tree\n"); - return 0; - } - - if (gpio_request_by_name_nodev(offset_to_ofnode(node), "gpios", 0, - &resin, 0)) { - printf("Failed to request key_vol_down button.\n"); - return 0; - } - - if (dm_gpio_get_value(&resin)) { + state = button_get_state(btn); + if (state == BUTTON_ON) { env_set("preboot", "setenv preboot; fastboot 0"); printf("key_vol_down pressed - Starting fastboot.\n"); } diff --git a/board/qualcomm/dragonboard820c/dragonboard820c.c b/board/qualcomm/dragonboard820c/dragonboard820c.c index 6785bf58e949..789b17a48636 100644 --- a/board/qualcomm/dragonboard820c/dragonboard820c.c +++ b/board/qualcomm/dragonboard820c/dragonboard820c.c @@ -5,6 +5,7 @@ * (C) Copyright 2017 Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org> */ +#include <button.h> #include <cpu_func.h> #include <init.h> #include <env.h> @@ -139,30 +140,18 @@ void reset_cpu(void) /* Check for vol- button - if pressed - stop autoboot */ int misc_init_r(void) { - struct udevice *pon; - struct gpio_desc resin; - int node, ret; + struct udevice *btn; + int ret; + enum button_state_t state; - ret = uclass_get_device_by_name(UCLASS_GPIO, "pm8994_pon@800", &pon); + ret = button_get_by_label("key_power", &btn); if (ret < 0) { - printf("Failed to find PMIC pon node. Check device tree\n"); - return 0; + printf("Couldn't find power button!\n"); + return ret; } - node = fdt_subnode_offset(gd->fdt_blob, dev_of_offset(pon), - "key_vol_down"); - if (node < 0) { - printf("Failed to find key_vol_down node. Check device tree\n"); - return 0; - } - - if (gpio_request_by_name_nodev(offset_to_ofnode(node), "gpios", 0, - &resin, 0)) { - printf("Failed to request key_vol_down button.\n"); - return 0; - } - - if (dm_gpio_get_value(&resin)) { + state = button_get_state(btn); + if (state == BUTTON_ON) { env_set("bootdelay", "-1"); printf("Power button pressed - dropping to console.\n"); } diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index ba42b0768e12..fbf77673c5e0 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -309,12 +309,13 @@ config CMD_PCA953X config QCOM_PMIC_GPIO bool "Qualcomm generic PMIC GPIO/keypad driver" depends on DM_GPIO && PMIC_QCOM + select BUTTON help Support for GPIO pins and power/reset buttons found on Qualcomm SoCs PMIC. Default name for GPIO bank is "pm8916". Power and reset buttons are placed in "pwkey_qcom" bank and - have gpio numbers 0 and 1 respectively. + have gpio numbers 0 and 1 respectively. config PCF8575_GPIO bool "PCF8575 I2C GPIO Expander driver" diff --git a/drivers/gpio/qcom_pmic_gpio.c b/drivers/gpio/qcom_pmic_gpio.c index e5841f502953..3dbc02d83198 100644 --- a/drivers/gpio/qcom_pmic_gpio.c +++ b/drivers/gpio/qcom_pmic_gpio.c @@ -5,8 +5,10 @@ * (C) Copyright 2015 Mateusz Kulikowski <mateusz.kulikowski@gmail.com> */ +#include <button.h> #include <common.h> #include <dm.h> +#include <dm/lists.h> #include <log.h> #include <power/pmic.h> #include <spmi/spmi.h> @@ -275,107 +277,150 @@ U_BOOT_DRIVER(qcom_pmic_gpio) = { .priv_auto = sizeof(struct qcom_gpio_bank), }; +struct qcom_pmic_btn_priv { + u32 base; + u32 status_bit; + int code; + struct udevice *pmic; +}; /* Add pmic buttons as GPIO as well - there is no generic way for now */ #define PON_INT_RT_STS 0x10 #define KPDPWR_ON_INT_BIT 0 #define RESIN_ON_INT_BIT 1 -static int qcom_pwrkey_get_function(struct udevice *dev, unsigned offset) +static enum button_state_t qcom_pwrkey_get_state(struct udevice *dev) { - return GPIOF_INPUT; -} + struct qcom_pmic_btn_priv *priv = dev_get_priv(dev); -static int qcom_pwrkey_get_value(struct udevice *dev, unsigned offset) -{ - struct qcom_gpio_bank *priv = dev_get_priv(dev); - - int reg = pmic_reg_read(dev->parent, priv->pid + PON_INT_RT_STS); + int reg = pmic_reg_read(priv->pmic, priv->base + PON_INT_RT_STS); if (reg < 0) return 0; - switch (offset) { - case 0: /* Power button */ - return (reg & BIT(KPDPWR_ON_INT_BIT)) != 0; - break; - case 1: /* Reset button */ - default: - return (reg & BIT(RESIN_ON_INT_BIT)) != 0; - break; - } + return (reg & BIT(priv->status_bit)) != 0; } -/* - * Since pmic buttons modelled as GPIO, we need empty direction functions - * to trick u-boot button driver - */ -static int qcom_pwrkey_direction_input(struct udevice *dev, unsigned int offset) +static int qcom_pwrkey_get_code(struct udevice *dev) { - return 0; -} + struct qcom_pmic_btn_priv *priv = dev_get_priv(dev); -static int qcom_pwrkey_direction_output(struct udevice *dev, unsigned int offset, int value) -{ - return -EOPNOTSUPP; + return priv->code; } -static const struct dm_gpio_ops qcom_pwrkey_ops = { - .get_value = qcom_pwrkey_get_value, - .get_function = qcom_pwrkey_get_function, - .direction_input = qcom_pwrkey_direction_input, - .direction_output = qcom_pwrkey_direction_output, -}; - static int qcom_pwrkey_probe(struct udevice *dev) { - struct qcom_gpio_bank *priv = dev_get_priv(dev); - int reg; - u64 pid; + struct button_uc_plat *uc_plat = dev_get_uclass_plat(dev); + struct qcom_pmic_btn_priv *priv = dev_get_priv(dev); + int ret; + u64 base; - pid = dev_read_addr(dev); - if (pid == FDT_ADDR_T_NONE) - return log_msg_ret("bad address", -EINVAL); + /* Ignore the top-level button node */ + if (!uc_plat->label) + return 0; - priv->pid = pid; + /* the pwrkey and resin nodes are children of the "pon" node, get the + * PMIC device to use in pmic_reg_* calls. + */ + priv->pmic = dev->parent->parent; + + base = dev_read_addr(dev); + if (!base || base == FDT_ADDR_T_NONE) { + /* Linux devicetrees don't specify an address in the pwrkey node */ + base = dev_read_addr(dev->parent); + if (base == FDT_ADDR_T_NONE) { + printf("%s: Can't find address\n", dev->name); + return -EINVAL; + } + } + + priv->base = base; /* Do a sanity check */ - reg = pmic_reg_read(dev->parent, priv->pid + REG_TYPE); - if (reg != 0x1) - return log_msg_ret("bad type", -ENXIO); + ret = pmic_reg_read(priv->pmic, priv->base + REG_TYPE); + if (ret != 0x1 && ret != 0xb) { + printf("%s: unexpected PMIC function type %d\n", dev->name, ret); + return -ENXIO; + } - reg = pmic_reg_read(dev->parent, priv->pid + REG_SUBTYPE); - if ((reg & 0x5) == 0) - return log_msg_ret("bad subtype", -ENXIO); + ret = pmic_reg_read(priv->pmic, priv->base + REG_SUBTYPE); + if ((ret & 0x7) == 0) { + printf("%s: unexpected PMCI function subtype %d\n", dev->name, ret); + //return -ENXIO; + } + + /* Bit of a hack, we use the interrupt number to derive if this is the pwrkey or resin + * node, it just so happens to line up with the bit numbers in the interrupt status register + */ + ret = ofnode_read_u32_index(dev_ofnode(dev), "interrupts", 2, &priv->status_bit); + if (ret < 0) { + printf("%s: Couldn't read interrupts: %d\n", __func__, ret); + return ret; + } + + ret = ofnode_read_u32(dev_ofnode(dev), "linux,code", &priv->code); + if (ret < 0) { + printf("%s: Couldn't read interrupts: %d\n", __func__, ret); + return ret; + } return 0; } -static int qcom_pwrkey_of_to_plat(struct udevice *dev) +static int button_qcom_pmic_bind(struct udevice *parent) { - struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev); + struct udevice *dev; + ofnode node; + int ret; - uc_priv->gpio_count = 2; - uc_priv->bank_name = dev_read_string(dev, "gpio-bank-name"); - if (uc_priv->bank_name == NULL) - uc_priv->bank_name = "pwkey_qcom"; + dev_for_each_subnode(node, parent) { + struct button_uc_plat *uc_plat; + const char *label; + + if (!ofnode_is_enabled(node)) + continue; + + label = ofnode_read_string(node, "label"); + if (!label) { + printf("%s: node %s has no label\n", __func__, + ofnode_get_name(node)); + /* Don't break booting, just print a warning and continue */ + continue; + } + /* We need the PMIC device to be the parent, so flatten it out here */ + ret = device_bind_driver_to_node(parent, "pwrkey_qcom", + ofnode_get_name(node), + node, &dev); + if (ret) { + printf("Failed to bind %s! %d\n", label, ret); + return ret; + } + uc_plat = dev_get_uclass_plat(dev); + uc_plat->label = label; + } return 0; } +static const struct button_ops button_qcom_pmic_ops = { + .get_state = qcom_pwrkey_get_state, + .get_code = qcom_pwrkey_get_code, +}; + static const struct udevice_id qcom_pwrkey_ids[] = { { .compatible = "qcom,pm8916-pwrkey" }, { .compatible = "qcom,pm8994-pwrkey" }, - { .compatible = "qcom,pm8998-pwrkey" }, + { .compatible = "qcom,pm8941-pwrkey" }, + { .compatible = "qcom,pm8998-pon" }, { } }; U_BOOT_DRIVER(pwrkey_qcom) = { .name = "pwrkey_qcom", - .id = UCLASS_GPIO, + .id = UCLASS_BUTTON, .of_match = qcom_pwrkey_ids, - .of_to_plat = qcom_pwrkey_of_to_plat, + .bind = button_qcom_pmic_bind, .probe = qcom_pwrkey_probe, - .ops = &qcom_pwrkey_ops, - .priv_auto = sizeof(struct qcom_gpio_bank), + .ops = &button_qcom_pmic_ops, + .priv_auto = sizeof(struct qcom_pmic_btn_priv), };
The power and resin keys were implemented as GPIOs here, but their only use would be as buttons. Avoid the additional layer of introspection and rework this driver into a button driver. While we're here, replace the "qcom,pm8998-pwrkey" compatible with "qcom,pm8941-pwrkey" to match upstream (Linux). The dragonboard410c and 820c boards are adjusted to benefit from this change too, simplify their custom board init code. Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> --- arch/arm/dts/dragonboard410c-uboot.dtsi | 11 +- arch/arm/dts/dragonboard820c-uboot.dtsi | 9 +- arch/arm/dts/dragonboard820c.dts | 3 - board/qualcomm/dragonboard410c/dragonboard410c.c | 29 ++-- board/qualcomm/dragonboard820c/dragonboard820c.c | 29 ++-- drivers/gpio/Kconfig | 3 +- drivers/gpio/qcom_pmic_gpio.c | 161 +++++++++++++++-------- 7 files changed, 139 insertions(+), 106 deletions(-)