| Message ID | 20180618143548.29900-1-vkoul@kernel.org |
|---|---|
| Headers | show |
| Series | : input: pm8941-pwrkey: Add support for reboot reason | expand |
On Mon, Jun 18, 2018 at 08:05:47PM +0530, Vinod Koul wrote: > In order to support resin thru the pwrkey driver (they are very > similar in nature) we need to abstract the handling in this driver. > > First we abstract pull_up_bit and status_bit along in driver data. > The event code sent for key events is quiried from DT. > > Since the device can be child of pon lookup regmap and reg from > parent if lookup fails (we are child). > > Signed-off-by: Vinod Koul <vkoul@kernel.org> > --- > drivers/input/misc/pm8941-pwrkey.c | 56 +++++++++++++++++++++++++++++++------- > 1 file changed, 46 insertions(+), 10 deletions(-) > > diff --git a/drivers/input/misc/pm8941-pwrkey.c b/drivers/input/misc/pm8941-pwrkey.c > index 18ad956454f1..aedb6ea2b50a 100644 > --- a/drivers/input/misc/pm8941-pwrkey.c > +++ b/drivers/input/misc/pm8941-pwrkey.c > @@ -20,6 +20,7 @@ > #include <linux/log2.h> > #include <linux/module.h> > #include <linux/of.h> > +#include <linux/of_device.h> > #include <linux/platform_device.h> > #include <linux/reboot.h> > #include <linux/regmap.h> > @@ -42,6 +43,10 @@ > #define PON_DBC_CTL 0x71 > #define PON_DBC_DELAY_MASK 0x7 > > +struct pm8941_data { > + unsigned int pull_up_bit; > + unsigned int status_bit; > +}; > > struct pm8941_pwrkey { > struct device *dev; > @@ -52,6 +57,9 @@ struct pm8941_pwrkey { > > unsigned int revision; > struct notifier_block reboot_notifier; > + > + unsigned int code; > + const struct pm8941_data *data; > }; > > static int pm8941_reboot_notify(struct notifier_block *nb, > @@ -124,7 +132,8 @@ static irqreturn_t pm8941_pwrkey_irq(int irq, void *_data) > if (error) > return IRQ_HANDLED; > > - input_report_key(pwrkey->input, KEY_POWER, !!(sts & PON_KPDPWR_N_SET)); > + input_report_key(pwrkey->input, pwrkey->code, > + !!(sts & pwrkey->data->status_bit)); Since we are changing this: input core will normalize the value for you, no need for doing double negation here. > input_sync(pwrkey->input); > > return IRQ_HANDLED; > @@ -157,6 +166,7 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev) > { > struct pm8941_pwrkey *pwrkey; > bool pull_up; > + struct device *parent; > u32 req_delay; > int error; > > @@ -175,11 +185,20 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev) > return -ENOMEM; > > pwrkey->dev = &pdev->dev; > + pwrkey->data = of_device_get_match_data(&pdev->dev); > > - pwrkey->regmap = dev_get_regmap(pdev->dev.parent, NULL); > + parent = pdev->dev.parent; > + pwrkey->regmap = dev_get_regmap(parent, NULL); > if (!pwrkey->regmap) { > - dev_err(&pdev->dev, "failed to locate regmap\n"); > - return -ENODEV; > + /* > + * we failed to get regmap for parent, check if > + * parent->parent has it (device would be child of pon) > + */ > + pwrkey->regmap = dev_get_regmap(parent->parent, NULL); > + if (!pwrkey->regmap) { > + dev_err(&pdev->dev, "failed to locate regmap\n"); > + return -ENODEV; > + } > } > > pwrkey->irq = platform_get_irq(pdev, 0); > @@ -190,8 +209,13 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev) > > error = of_property_read_u32(pdev->dev.of_node, "reg", > &pwrkey->baseaddr); > - if (error) > - return error; > + if (error) { > + /* check if parent has reg before bailing */ > + error = of_property_read_u32(pdev->dev.parent->of_node, > + "reg", &pwrkey->baseaddr); > + if (error) > + return error; I do not want us blindly look up into parent for individual properties. We need to decide early on if we are dealing with subnode or older binding and stick to it. > + } > > error = regmap_read(pwrkey->regmap, pwrkey->baseaddr + PON_REV2, > &pwrkey->revision); > @@ -200,13 +224,20 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev) > return error; > } > > + error = of_property_read_u32(pdev->dev.of_node, "linux,code", > + &pwrkey->code); > + if (error) { > + dev_err(&pdev->dev, "no linux,code assuming power%d\n", error); This is not an error. dev_dbg or dev_info at most. > + pwrkey->code = KEY_POWER; > + } > + > pwrkey->input = devm_input_allocate_device(&pdev->dev); > if (!pwrkey->input) { > dev_dbg(&pdev->dev, "unable to allocate input device\n"); > return -ENOMEM; > } > > - input_set_capability(pwrkey->input, EV_KEY, KEY_POWER); > + input_set_capability(pwrkey->input, EV_KEY, pwrkey->code); > > pwrkey->input->name = "pm8941_pwrkey"; > pwrkey->input->phys = "pm8941_pwrkey/input0"; > @@ -225,8 +256,8 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev) > > error = regmap_update_bits(pwrkey->regmap, > pwrkey->baseaddr + PON_PULL_CTL, > - PON_KPDPWR_PULL_UP, > - pull_up ? PON_KPDPWR_PULL_UP : 0); > + pwrkey->data->pull_up_bit, > + pull_up ? pwrkey->data->pull_up_bit : 0); > if (error) { > dev_err(&pdev->dev, "failed to set pull: %d\n", error); > return error; > @@ -271,8 +302,13 @@ static int pm8941_pwrkey_remove(struct platform_device *pdev) > return 0; > } > > +static const struct pm8941_data pwrkey_data = { > + .pull_up_bit = PON_KPDPWR_PULL_UP, > + .status_bit = PON_KPDPWR_N_SET, > +}; > + > static const struct of_device_id pm8941_pwr_key_id_table[] = { > - { .compatible = "qcom,pm8941-pwrkey" }, > + { .compatible = "qcom,pm8941-pwrkey", .data = &pwrkey_data }, > { } > }; > MODULE_DEVICE_TABLE(of, pm8941_pwr_key_id_table); > -- > 2.14.4 > Thanks.
On Mon, Jun 18, 2018 at 08:05:43PM +0530, Vinod Koul wrote: > To add support for reboot reason there have been some attempts [1], [2] > in past. Based on these discussions we added a new pon driver and made > pwrkey and resin as child nodes to pon. > > Since the pwrkey and resin are similar, abstract pwrkey driver > and then add support for resin in the same driver. > > Since the patches touch reset and input subsystems, it would be nice if > folks can ACK and we carry thru one subsystem, I would prefer that to be > input, other way would work fine too. This looks reasonable to me, let's add Bjorn. > > [1]: https://patchwork.kernel.org/patch/9751627/ > [2]: https://patchwork.kernel.org/patch/10381801/ > > Vinod Koul (5): > dt-bindings: power: reset: Add qcom pon binding > power: reset: qcom-pon: Add Qcom PON driver > dt-bindings: Input: Add additional property to qcom pwrkey > input: pm8941-pwrkey: Abstract register offsets and event code > input: pm8941-pwrkey: Add resin entry > > .../bindings/input/qcom,pm8941-pwrkey.txt | 9 +++ > .../devicetree/bindings/power/reset/qcom,pon.txt | 47 +++++++++++ > drivers/input/misc/pm8941-pwrkey.c | 64 ++++++++++++--- > drivers/power/reset/Kconfig | 8 ++ > drivers/power/reset/Makefile | 1 + > drivers/power/reset/qcom-pon.c | 91 ++++++++++++++++++++++ > 6 files changed, 210 insertions(+), 10 deletions(-) > create mode 100644 Documentation/devicetree/bindings/power/reset/qcom,pon.txt > create mode 100644 drivers/power/reset/qcom-pon.c > > -- > 2.14.4 >
On Mon 18 Jun 07:35 PDT 2018, Vinod Koul wrote: > Since handling is abstracted in this driver, we need to add resin entry > in id table along with pwrkey_data. > > Signed-off-by: Vinod Koul <vkoul@kernel.org> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> Regards, Bjorn > --- > drivers/input/misc/pm8941-pwrkey.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/input/misc/pm8941-pwrkey.c b/drivers/input/misc/pm8941-pwrkey.c > index aedb6ea2b50a..913405404a92 100644 > --- a/drivers/input/misc/pm8941-pwrkey.c > +++ b/drivers/input/misc/pm8941-pwrkey.c > @@ -29,6 +29,7 @@ > > #define PON_RT_STS 0x10 > #define PON_KPDPWR_N_SET BIT(0) > +#define PON_RESIN_N_SET BIT(1) > > #define PON_PS_HOLD_RST_CTL 0x5a > #define PON_PS_HOLD_RST_CTL2 0x5b > @@ -39,6 +40,7 @@ > > #define PON_PULL_CTL 0x70 > #define PON_KPDPWR_PULL_UP BIT(1) > +#define PON_RESIN_PULL_UP BIT(0) > > #define PON_DBC_CTL 0x71 > #define PON_DBC_DELAY_MASK 0x7 > @@ -307,8 +309,14 @@ static const struct pm8941_data pwrkey_data = { > .status_bit = PON_KPDPWR_N_SET, > }; > > +static const struct pm8941_data resin_data = { > + .pull_up_bit = PON_RESIN_PULL_UP, > + .status_bit = PON_RESIN_N_SET, > +}; > + > static const struct of_device_id pm8941_pwr_key_id_table[] = { > { .compatible = "qcom,pm8941-pwrkey", .data = &pwrkey_data }, > + { .compatible = "qcom,pm8941-resin", .data = &resin_data }, > { } > }; > MODULE_DEVICE_TABLE(of, pm8941_pwr_key_id_table); > -- > 2.14.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon 18 Jun 07:35 PDT 2018, Vinod Koul wrote: > diff --git a/drivers/input/misc/pm8941-pwrkey.c b/drivers/input/misc/pm8941-pwrkey.c [..] > struct pm8941_pwrkey { > struct device *dev; > @@ -52,6 +57,9 @@ struct pm8941_pwrkey { > > unsigned int revision; > struct notifier_block reboot_notifier; > + > + unsigned int code; pwrkey->code is passed into of_property_read_u32(), so make it u32 here instead. > + const struct pm8941_data *data; > }; > [..] > @@ -175,11 +185,20 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev) > return -ENOMEM; > > pwrkey->dev = &pdev->dev; > + pwrkey->data = of_device_get_match_data(&pdev->dev); > > - pwrkey->regmap = dev_get_regmap(pdev->dev.parent, NULL); > + parent = pdev->dev.parent; > + pwrkey->regmap = dev_get_regmap(parent, NULL); > if (!pwrkey->regmap) { > - dev_err(&pdev->dev, "failed to locate regmap\n"); > - return -ENODEV; > + /* > + * we failed to get regmap for parent, check if > + * parent->parent has it (device would be child of pon) > + */ > + pwrkey->regmap = dev_get_regmap(parent->parent, NULL); > + if (!pwrkey->regmap) { > + dev_err(&pdev->dev, "failed to locate regmap\n"); > + return -ENODEV; > + } > } > > pwrkey->irq = platform_get_irq(pdev, 0); > @@ -190,8 +209,13 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev) > > error = of_property_read_u32(pdev->dev.of_node, "reg", > &pwrkey->baseaddr); > - if (error) > - return error; > + if (error) { I agree with Dmitry's comment here. The order of these operations are not significant, so you can easily move this into above "fallback" and read "reg" from the dev itself in and else block. > + /* check if parent has reg before bailing */ > + error = of_property_read_u32(pdev->dev.parent->of_node, > + "reg", &pwrkey->baseaddr); > + if (error) > + return error; > + } Apart from this the patch looks good. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 22-06-18, 11:52, Dmitry Torokhov wrote: > > static int pm8941_reboot_notify(struct notifier_block *nb, > > @@ -124,7 +132,8 @@ static irqreturn_t pm8941_pwrkey_irq(int irq, void *_data) > > if (error) > > return IRQ_HANDLED; > > > > - input_report_key(pwrkey->input, KEY_POWER, !!(sts & PON_KPDPWR_N_SET)); > > + input_report_key(pwrkey->input, pwrkey->code, > > + !!(sts & pwrkey->data->status_bit)); > > Since we are changing this: input core will normalize the value for you, > no need for doing double negation here. Okay will do, since this "should" be explained that input core handles it, do you recommend and separate patch for that? > > @@ -190,8 +209,13 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev) > > > > error = of_property_read_u32(pdev->dev.of_node, "reg", > > &pwrkey->baseaddr); > > - if (error) > > - return error; > > + if (error) { > > + /* check if parent has reg before bailing */ > > + error = of_property_read_u32(pdev->dev.parent->of_node, > > + "reg", &pwrkey->baseaddr); > > + if (error) > > + return error; > > I do not want us blindly look up into parent for individual properties. > We need to decide early on if we are dealing with subnode or older > binding and stick to it. okay will do. > > error = regmap_read(pwrkey->regmap, pwrkey->baseaddr + PON_REV2, > > &pwrkey->revision); > > @@ -200,13 +224,20 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev) > > return error; > > } > > > > + error = of_property_read_u32(pdev->dev.of_node, "linux,code", > > + &pwrkey->code); > > + if (error) { > > + dev_err(&pdev->dev, "no linux,code assuming power%d\n", error); > > This is not an error. dev_dbg or dev_info at most. okay will change
On 22-06-18, 14:10, Bjorn Andersson wrote: > On Mon 18 Jun 07:35 PDT 2018, Vinod Koul wrote: > > diff --git a/drivers/input/misc/pm8941-pwrkey.c b/drivers/input/misc/pm8941-pwrkey.c > [..] > > struct pm8941_pwrkey { > > struct device *dev; > > @@ -52,6 +57,9 @@ struct pm8941_pwrkey { > > > > unsigned int revision; > > struct notifier_block reboot_notifier; > > + > > + unsigned int code; > > pwrkey->code is passed into of_property_read_u32(), so make it u32 here > instead. yes will do > > pwrkey->irq = platform_get_irq(pdev, 0); > > @@ -190,8 +209,13 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev) > > > > error = of_property_read_u32(pdev->dev.of_node, "reg", > > &pwrkey->baseaddr); > > - if (error) > > - return error; > > + if (error) { > > I agree with Dmitry's comment here. > > The order of these operations are not significant, so you can easily > move this into above "fallback" and read "reg" from the dev itself in > and else block. Agreed we can do in a single block and check only once > > > + /* check if parent has reg before bailing */ > > + error = of_property_read_u32(pdev->dev.parent->of_node, > > + "reg", &pwrkey->baseaddr); > > + if (error) > > + return error; > > + } > > Apart from this the patch looks good. Thanks